-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Derive exported EndpointSlices from K8s EndpointSlices instead of Endpoints #1318
Conversation
Signed-off-by: Tom Pantelis <[email protected]>
Signed-off-by: Tom Pantelis <[email protected]>
🤖 Created branch: z_pr1318/tpantelis/lh_eps_from_svc_eps |
708b185
to
6c6cda6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Reviewed and tested over weekend. Great work with E2Es too +1. Need a tracking issue to update documentation, esp Troubleshooting Doc.
@@ -302,7 +303,7 @@ func newEndpointSlice(namespace, name, clusterID string, ports []mcsv1a1.Service | |||
|
|||
return &discovery.EndpointSlice{ | |||
ObjectMeta: metav1.ObjectMeta{ | |||
Name: name + "-" + namespace + "-" + clusterID, | |||
Name: fmt.Sprintf("%s-%s-%s-%s", name, utilrand.String(5), namespace, clusterID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to release note this. Longer name may cause name length related failures for some deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I thought about that. I think we should use the GenerateName
functionality that K8s uses for EndpointSlices with the service name as the prefix. I'm planning to do that next in a followup PR. It's going to require supporting changes in admiral.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! It can go in as-is, I have a couple of minor tweaks if you re-spin.
6c6cda6
to
64c6b84
Compare
f52fa6c
to
ae12431
Compare
...instead of the service Endpoints. For headless services, we simply mirror each K8s EndpointSlice, substituting Globalnet IPs if enabled. For cluster IP services, we still create a single EndpointSlice containing the service IP's endpoint address. The ready condition is detetermined by listing all the EndpointSlices and checking if any endpoint condition is ready. Signed-off-by: Tom Pantelis <[email protected]>
Signed-off-by: Tom Pantelis <[email protected]>
...to handle multiple headless EndpointSlices. Signed-off-by: Tom Pantelis <[email protected]>
ae12431
to
8252f73
Compare
I've never seen that before. It looks like there was some recent weirdness along these lines: |
For headless services, the controller retrieves all the EndpointSlices and, on add/update, passes them to PutEndpointSlices. On delete, i there's no more PutEndpointSlices, it calls RemoveEndpointSlice, otherwise PutEndpointSlices. Signed-off-by: Tom Pantelis <[email protected]>
8252f73
to
c9f8741
Compare
🤖 Closed branches: [z_pr1318/tpantelis/lh_eps_from_svc_eps] |
See this comment for reasoning.
See individual commits for details.
Fixes #1022
Fixes #755