-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
clustermesh: enable endpointslice sync by default on headless services #32021
Conversation
c723f23
to
07c8798
Compare
/test |
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.
I'm not sure if this is sufficient to be enabled by default. AFAIK, Cilium Ingress/GWAPI works without this endpointslice synchronization, so this is only relevant for the people who wants to integrate third-party controllers which is I would say not a regular use case. What do you think @giorio94?
Headless services represent a special case, and they don't currently work as expected if marked as global, because coredns doesn't know about the remote backends (given that the corresponding epslices are not present locally). Given the above, I personally think it is fine to enable endpointslice synchronization by default for them (only if marked as global, obviously), especially because the endpointslice synchronization is already gated by a dedicated feature flag (disabled by default), and thus the change would not affect any existing deployment. Differently, I wouldn't enable endpointslice synchronization by default for all the other service types, as required only by third-party controllers, and potentially associated with scalability concerns. |
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.
Thanks!
07c8798
to
484a1b3
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.
Docs good. Should this change be highlighted in the upgrade notes too?
Hmmm that's a good point thanks, but I think it should not because the whole feature of endpointslice syncing was not available in the previous stable release so there is no existing users of this and it's still guarded by a helm config/feature flag disabled by default. So it changes the default but only if prior to that you enabled the feature flag (which wasn't here in any releases so far). |
/test |
As it's highly likely that for an Headless Service the user wants to sync EndpointSlices we should set it by default in this case and make the behavior opt-out instead of opt-in. It's highly unlikely that this change anything for existing setup as before endpoint slice synchronization were a thing Headless services were ignored. Also EndpointSlice sync is still guarded by a feature flag set in helm/in the configmap. Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
/ci-integration |
/ci-ginkgo |
484a1b3
to
59869af
Compare
(I rebased as an attempt to have less flakes in the CI) |
/test |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
As it's highly likely that for an Headless Service the user wants to sync EndpointSlices we should set it by default in this case and make the behavior opt-out instead of opt-in.
It's highly unlikely that this change anything for existing setup as before endpoint slice synchronization were a thing Headless services were ignored. Also EndpointSlice sync is still guarded by a feature flag set in helm/in the configmap.