Add library for manually syncing EndpointSlices - #180
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
chrisseto
left a comment
There was a problem hiding this comment.
Just did a quick pass but overall LGTM. For an MVP in cloud this looks great. If we want to expose this degree of configuration more generally, the wiring up will likely get pretty nasty. Happy to do a more thorough pass if you'd like.
| // slices. Cleanup only acts when those leftovers are actually present, | ||
| // so a long-migrated Service costs nothing. Set this when something else | ||
| // legitimately manages those objects. | ||
| DisableNativeCleanup bool |
There was a problem hiding this comment.
nit: DisableMigrationCleanup may be more clear
|
|
||
| lookupCtx, cancel := context.WithTimeout(ctx, nodeLookupTimeout) | ||
| var node corev1.Node | ||
| err := r.client.Get(lookupCtx, client.ObjectKey{Name: nodeName}, &node) |
There was a problem hiding this comment.
Wouldn't it be more efficient to perform a List? I can't think of any real benefit to perform name specific look ups.
| // nodeLookupTimeout bounds each Node cache lookup: the first Get lazily | ||
| // starts a Node informer, and without Node RBAC its cache never syncs -- an | ||
| // unbounded Get would wedge the reconcile worker forever instead of | ||
| // degrading. |
There was a problem hiding this comment.
Is this true? I was under the impression that an error would be returned if the underlying cache/informer hadn't yet been started up.
| // One check budget for the whole render: membership probes can block up | ||
| // to their timeout apiece, so fan-out is bounded per reconcile. | ||
| sem := make(chan struct{}, r.cfg.MaxConcurrentChecks) |
There was a problem hiding this comment.
Seems like a perfect use case for errgroup.Group.SetLimit?
| if !sliceChanged(current, want) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Any reason to not rely on SSA to perform this check for you?
|
Ah, last thought: We'll need to test how this performs in each cloud. It would not surprise me if certain configurations bypassed kube-proxy and/or ignored endpoint slices. Huh... I wonder if Cilium would respect this. |
|
@chrisseto yeah -- I've been going back and forth about whether we should actually maintain both EndpointSlices and deprecated Endpoints directly. I recall there being some oddities around DNS resolution in stock CoreDNS for certain configurations where we were trying to populate EndpointSlices without creating the underlying legacy Endpoints as well. Definitely worth trying out/testing in each major cloud just to make sure that we don't have to do a bunch of legacy stuff too. |
|
Before I forget: We should probably respect publishNotReady (I don't think I saw that anywhere) |
This adds a library-style Kubernetes controller that publishes the EndpointSlices backing "colocated"
Services:Serviceswhose ports are served by overlapping-but-distinct subsets of a shared pod pool.It replaces the native
EndpointSlicecontroller forServicesthat deliberately define no selector. Where the nativecontroller aligns
PodstoServiceswithspec.selector, this library aligns them through a configurable label or annotation, and a pluggable membership check decides — per pod, per port — which pods back which ports:Usage is found in the example file included, but roughly corresponds to:
Custom membership mapping functions (with an optional tristate membership-determination interface for ambiguous responses) are supported as well.