WIP: Support EndpointSlice in sdn and test handling terminating endpoints #271
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |
| "k8s.io/klog/v2" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| discoveryv1beta1 "k8s.io/api/discovery/v1beta1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
| corev1listers "k8s.io/client-go/listers/core/v1" | ||
|
|
@@ -27,11 +28,28 @@ type RunnableProxy interface { | |
| SetSyncRunner(b *async.BoundedFrequencyRunner) | ||
| } | ||
|
|
||
| // NoopEndpointSliceHandler is a noop handler for proxiers that have not yet | ||
| // implemented a full EndpointSliceHandler. | ||
| type NoopEndpointsHandler struct{} | ||
|
|
||
| func (*NoopEndpointsHandler) OnEndpointsAdd(*corev1.Endpoints) {} | ||
|
|
||
| func (*NoopEndpointsHandler) OnEndpointsUpdate(old, new *corev1.Endpoints) { | ||
| } | ||
|
|
||
| func (*NoopEndpointsHandler) OnEndpointsDelete(*corev1.Endpoints) {} | ||
|
|
||
| func (*NoopEndpointsHandler) OnEndpointsSynced() {} | ||
|
|
||
| var _ proxyconfig.EndpointsHandler = &NoopEndpointsHandler{} | ||
|
|
||
| // HybridProxier runs an unidling proxy and a primary proxy at the same time, | ||
| // delegating idled services to the unidling proxy and other services to the | ||
| // primary proxy. | ||
| type HybridProxier struct { | ||
| // TODO implement https://github.com/kubernetes/enhancements/pull/640 | ||
| proxyconfig.NoopNodeHandler | ||
| NoopEndpointsHandler | ||
|
|
||
| mainProxy RunnableProxy | ||
| unidlingProxy RunnableProxy | ||
|
|
@@ -89,22 +107,6 @@ func NewHybridProxier( | |
| return p, nil | ||
| } | ||
|
|
||
| func (p *HybridProxier) OnServiceAdd(service *corev1.Service) { | ||
| svcName := types.NamespacedName{ | ||
| Namespace: service.Namespace, | ||
|
|
@@ -190,11 +192,11 @@ func (p *HybridProxier) OnServiceSynced() { | |
| klog.V(6).Infof("hybrid proxy: services synced") | ||
| } | ||
|
|
||
| // shouldEndpointSliceUseUserspace checks to see if the given endpoints have the correct | ||
| // annotations and size to use the unidling proxy. | ||
| func (p *HybridProxier) shouldEndpointSliceUseUserspace(endpoints *discoveryv1beta1.EndpointSlice) bool { | ||
| hasEndpoints := false | ||
| for _, subset := range endpoints.Endpoints { | ||
| if len(subset.Addresses) > 0 { | ||
| hasEndpoints = true | ||
| break | ||
|
|
@@ -244,11 +246,11 @@ func (p *HybridProxier) switchService(name types.NamespacedName) { | |
| p.switchedToUserspace[name] = p.usingUserspace[name] | ||
| } | ||
|
|
||
| func (p *HybridProxier) OnEndpointSliceAdd(endpoints *discoveryv1beta1.EndpointSlice) { | ||
| // we track all endpoints in the unidling endpoints handler so that we can succesfully | ||
| // detect when a service become unidling | ||
| klog.V(6).Infof("hybrid proxy: (always) add ep %s/%s in unidling proxy", endpoints.Namespace, endpoints.Name) | ||
| p.unidlingProxy.OnEndpointSliceAdd(endpoints) | ||
|
There was a problem hiding this comment. I'm not familiar at all with the sdn/proxy implementation, maybe this information is redundant, but can be multiple slices for the same service, and each slice can have duplicate endpoints, kube-proxy uses a cache There was a problem hiding this comment. the limit of endpoints per slice is 100, so if you have more than 100 endpoints, let's say 110 for service X you'll receive two slices Y1 and Y2, maybe with 100 and 10 endpoints each
Contributor
Author
There was a problem hiding this comment. We really should have an e2e test in upstream that creates a service with > 100 endpoints then to exercise this. Is there one you know of I can crib?
Contributor
Author
There was a problem hiding this comment. Actually, for idling, wondering whether we even need to support > three or four endpoints. The only time the user space proxy should be in play is on an idle service which has no endpoints. There was a problem hiding this comment.
as I said, I'm not familiar with this code, just raising some points that I think may be taking into consideration, if that is the case, it seems we should't worry about this
this is well tested upstream
Contributor
There was a problem hiding this comment. The unidling proxy can just ignore the endpointslices and work with the endpoints like it always did. Endpoints objects always contain the full set of endpoints, even in cases where the EndpointSlice controller would start splitting things up; it just means that code working with the Endpoints objects doesn't get the efficiency wins that code working with the EndpointSlice objects would get. |
||
|
|
||
| p.usingUserspaceLock.Lock() | ||
| defer p.usingUserspaceLock.Unlock() | ||
|
|
@@ -259,11 +261,11 @@ func (p *HybridProxier) OnEndpointsAdd(endpoints *corev1.Endpoints) { | |
| } | ||
|
|
||
| wasUsingUserspace, knownEndpoints := p.usingUserspace[svcName] | ||
| p.usingUserspace[svcName] = p.shouldEndpointSliceUseUserspace(endpoints) | ||
|
|
||
| if !p.usingUserspace[svcName] { | ||
| klog.V(6).Infof("hybrid proxy: add ep %s/%s in main proxy", endpoints.Namespace, endpoints.Name) | ||
| p.mainProxy.OnEndpointSliceAdd(endpoints) | ||
| } | ||
|
|
||
| // a service could appear before endpoints, so we have to treat this as a potential | ||
|
|
@@ -273,11 +275,11 @@ func (p *HybridProxier) OnEndpointsAdd(endpoints *corev1.Endpoints) { | |
| } | ||
| } | ||
|
|
||
| func (p *HybridProxier) OnEndpointSliceUpdate(oldEndpoints, endpoints *discoveryv1beta1.EndpointSlice) { | ||
| // we track all endpoints in the unidling endpoints handler so that we can succesfully | ||
| // detect when a service become unidling | ||
| klog.V(6).Infof("hybrid proxy: (always) update ep %s/%s in unidling proxy", endpoints.Namespace, endpoints.Name) | ||
| p.unidlingProxy.OnEndpointSliceUpdate(oldEndpoints, endpoints) | ||
|
|
||
| p.usingUserspaceLock.Lock() | ||
| defer p.usingUserspaceLock.Unlock() | ||
|
|
@@ -288,7 +290,7 @@ func (p *HybridProxier) OnEndpointsUpdate(oldEndpoints, endpoints *corev1.Endpoi | |
| } | ||
|
|
||
| wasUsingUserspace, knownEndpoints := p.usingUserspace[svcName] | ||
| p.usingUserspace[svcName] = p.shouldEndpointSliceUseUserspace(endpoints) | ||
|
|
||
| if !knownEndpoints { | ||
| utilruntime.HandleError(fmt.Errorf("received update for unknown endpoints %s", svcName.String())) | ||
|
|
@@ -299,26 +301,26 @@ func (p *HybridProxier) OnEndpointsUpdate(oldEndpoints, endpoints *corev1.Endpoi | |
|
|
||
| if !isSwitch && !p.usingUserspace[svcName] { | ||
| klog.V(6).Infof("hybrid proxy: update ep %s/%s in main proxy", endpoints.Namespace, endpoints.Name) | ||
| p.mainProxy.OnEndpointSliceUpdate(oldEndpoints, endpoints) | ||
| return | ||
| } | ||
|
|
||
| if p.usingUserspace[svcName] { | ||
| klog.V(6).Infof("hybrid proxy: del ep %s/%s in main proxy", endpoints.Namespace, endpoints.Name) | ||
| p.mainProxy.OnEndpointSliceDelete(oldEndpoints) | ||
| } else { | ||
| klog.V(6).Infof("hybrid proxy: add ep %s/%s in main proxy", endpoints.Namespace, endpoints.Name) | ||
| p.mainProxy.OnEndpointSliceAdd(endpoints) | ||
| } | ||
|
|
||
| p.switchService(svcName) | ||
| } | ||
|
|
||
| func (p *HybridProxier) OnEndpointSliceDelete(endpoints *discoveryv1beta1.EndpointSlice) { | ||
| // we track all endpoints in the unidling endpoints handler so that we can succesfully | ||
| // detect when a service become unidling | ||
| klog.V(6).Infof("hybrid proxy: (always) del ep %s/%s in unidling proxy", endpoints.Namespace, endpoints.Name) | ||
| p.unidlingProxy.OnEndpointSliceDelete(endpoints) | ||
|
|
||
| // Careful - there is the potential for deadlocks here, | ||
| // except that we always get usingUserspaceLock first, then | ||
|
|
@@ -340,15 +342,15 @@ func (p *HybridProxier) OnEndpointsDelete(endpoints *corev1.Endpoints) { | |
|
|
||
| if !usingUserspace { | ||
| klog.V(6).Infof("hybrid proxy: del ep %s/%s in main proxy", endpoints.Namespace, endpoints.Name) | ||
| p.mainProxy.OnEndpointSliceDelete(endpoints) | ||
| } | ||
|
|
||
| p.cleanupState(svcName) | ||
| } | ||
|
|
||
| func (p *HybridProxier) OnEndpointSlicesSynced() { | ||
| p.unidlingProxy.OnEndpointSlicesSynced() | ||
| p.mainProxy.OnEndpointSlicesSynced() | ||
| klog.V(6).Infof("hybrid proxy: endpoints synced") | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The HybridProxier can't no-op Endpoints handling; it has to pass EndpointSlice events down to the iptables proxier and Endpoints events to the userspace proxier. And since OsdnProxy acts as a filter on top of HybridProxier, it needs to also pass both sets of events down to the proxier it's wrapping.
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 updated userspace proxier to use EndpointSlice, I thought we were already going to have to switch to use Service instead of Endpoints for idling.
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.
Oops, I see what you mean. Why wasn't userspace proxier updated? Just no one signed up for it?
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.
Upstream doesn't care about the userspace proxier any more (Tim would probably have already deleted it if OCP wasn't using it for unidling) and Red Hat had thought we weren't going to have to use EndpointSlice in openshift-sdn, so we didn't care about updating it either.
At any rate, I think we don't actually need to update userspace to use EndpointSlice; we just need to make
HybridProxierandOsdnProxypass both endpoint events and endpointslice events down to their wrapper proxiers, and then eventually the iptables proxy will act on the endpointslice events and the userspace proxy will act on the endpoint events.