add ipvs metaproxier endpointslice support #84089

khenidak · 2019-10-18T16:47:09Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds endpointSlice support metaproxier (ipv6 dualstack).

Which issue(s) this PR fixes:
n/a

Special notes for your reviewer:
n/a

Does this PR introduce a user-facing change?:

EndpointSlice and ipv6 dualstack (ipvs) are no longer mutually exclusive features. They can be enabled concurrently on a cluster

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]:  https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20180612-ipv4-ipv6-dual-stack.md
- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190603-EndpointSlice-API.md

@robscott FYI..

robscott · 2019-10-18T17:14:12Z

@khenidak Thanks for taking this on! Could you add some tests that cover the combination of ipvs, dualstack, and endpointslices?

khenidak · 2019-10-18T18:57:49Z

@robscott the metaproxy is just a thin shell over actual proxy. there is no dualstackness per se, it calls what actual proxies do. A test covering meta proxy itself specially the part it selects which proxy to call might be a good idea, but beyond that is really up to proxy implementation, and is tested separately

andrewsykim · 2019-10-21T20:24:28Z

pkg/proxy/ipvs/meta_proxier.go

@@ -153,25 +153,30 @@ func (proxier *metaProxier) OnEndpointsSynced() {
 // OnEndpointSliceAdd is called whenever creation of a new endpoint slice object
 // is observed.
 func (proxier *metaProxier) OnEndpointSliceAdd(endpointSlice *discovery.EndpointSlice) {
-	// noop
+	proxier.ipv4Proxier.OnEndpointSliceAdd(endpointSlice)


Does the IP family need to be checked first similar to the OnEndpoints* methods?

Even if EndpointSlice can contain address in both families should we still check if it contains only v4 or only v6 first before calling both event handlers??

Would metaProxier be used if the dual stack feature gate hadn't been enabled? If not, it seems likely that EndpointSlices would consistently contain both v4 and v6 addresses consistently. Are there other use cases where there might only be v4 or v6 addresses in an EndpointSlice?

I think this might be over optimization, giving that a slice can grow to a 1000 entry we may actually get opposite results. I understand that you are trying to avoid the call to syncProxyRules() Let us talk tom, i want to understand more.

andrewsykim · 2019-10-21T20:24:53Z

/assign

k8s-ci-robot · 2019-11-05T17:46:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: khenidak
To complete the pull request process, please assign andrewsykim
You can assign the PR to them by writing /assign @andrewsykim in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/proxy/ipvs/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

khenidak · 2019-11-05T17:47:20Z

@andrewsykim the filtering is now done at the poxy level. it should work, though i am not totally comfortable with this change.

andrewsykim · 2019-11-05T19:34:12Z

I thought we were goign to check if an EndpointSlice is v4 or v6 (by checking it's endpoints) and then call the appropriate proxier from there. That can happen at the meta proxier level.

robscott · 2019-11-05T19:40:21Z

@andrewsykim EndpointSlices can contain both v4 and v6 addresses in the addresses field for each Endpoint. This is different than the approach with Endpoints where addresses are separated in different v4 and v6 resources.

andrewsykim · 2019-11-05T19:48:57Z

Gotcha, thanks for clarifying. The problem is that syncProxyRules is not cheap even if we are skipping individual addresses not in the same IP family. Wondering if the logic can be something like this instead:

func (proxier *metaProxier) OnEndpointSliceUpdate(old, endpointSlice *discovery.EndpointSlice) {
       hasIPv4, hasIPv6 := getIPFamilies(endpointSlice)
       if hasIPv4{
           proxier.ipv4Proxier.OnEndpointSliceUpdate(old, endpointSlice)
      }

      if hasIPv6{
          proxier.ipv6Proxier.OnEndpointSliceUpdate(old, endpointSlice)
      }
}

khenidak · 2019-11-05T19:52:21Z

Gotcha, thanks for clarifying. The problem is that syncProxyRules is not cheap even if we are skipping addresses not in the same IP family. Wondering if the logic can be something like this instead:

func (proxier *metaProxier) OnEndpointSliceUpdate(old, endpointSlice *discovery.EndpointSlice) {
       hasIPv4, hasIPv6 := getIPFamilies(endpointSlice)
       if hasIPv4{
           proxier.ipv4Proxier.OnEndpointSliceUpdate(old, endpointSlice)
      }

      if hasIPv6{
          proxier.ipv6Proxier.OnEndpointSliceUpdate(old, endpointSlice)
      }
}

Wouldn't that still require skipping in syncProxyRules()

andrewsykim · 2019-11-05T19:58:22Z

Skipping syncProxyRules() (when IP family doesn't match) is what we want though? What we should avoid is the IPv6 proxier running syncProxyRules() when an EndpointSlice is all v4 (and vice versa).

robscott · 2019-11-05T19:51:47Z

pkg/proxy/ipvs/proxier.go

+	// (khenidak) the previous assumption is valid only for single stack
+	// for dual stack, we have no way for endpointslice (or general endpoint processing)
+	// to know if this endpoint is being processed for an IPv6 or IPv4 proxier
+	// TODO: Find a way where the endpoints/slice can be loaded with knowledge
+	// of proxy/ipfamily and perform the filtering there.


Maybe our caching structures (endpointsMap and/or endpointSliceCache?) could be updated in the future to make this easier?

We need a strategy covering the both proxies and endpoint processing. But that strategy can not be implemented now it needs a deeper look at the stack. For example, there is a proxy under development that does not care about ipfamily (nftables iirc). That strategy also needs to be ready to handle dualstack services (as in dual cluster IPs). that said, this strategy is way beyond the scope of this PR.

What i am trying to say this PR is a stop gap until this strategy is made, and applied.

robscott · 2019-11-05T19:55:19Z

pkg/proxy/ipvs/meta_proxier.go

@@ -153,25 +153,30 @@ func (proxier *metaProxier) OnEndpointsSynced() {
 // OnEndpointSliceAdd is called whenever creation of a new endpoint slice object
 // is observed.
 func (proxier *metaProxier) OnEndpointSliceAdd(endpointSlice *discovery.EndpointSlice) {
-	// noop
+	proxier.ipv4Proxier.OnEndpointSliceAdd(endpointSlice)


Would metaProxier be used if the dual stack feature gate hadn't been enabled? If not, it seems likely that EndpointSlices would consistently contain both v4 and v6 addresses consistently. Are there other use cases where there might only be v4 or v6 addresses in an EndpointSlice?

robscott · 2019-11-05T20:01:44Z

pkg/proxy/ipvs/proxier.go

+			// ignore stale services that does not match ipfamily of the proxier
+			isIpv6Svc := utilnet.IsIPv6(svcInfo.ClusterIP())
+			if proxier.isIPv6 != isIpv6Svc {
+				klog.V(10).Infof("ipvs proxier (ipv6:%v) is ignoring  a stale service:%v (ipv6:%v) because it is not of same ipfamily", proxier.isIPv6, svcPortName, isIpv6Svc)
+				continue
+			}
+


Is this something we could do in the caches instead of syncProxyRules? As I understand it, right now a change to a v4 address would be considered a change by the v6 proxier since both addresses would be stored in both caches. If on the other hand we filtered addresses before storing them in the cache, changes would be more accurate by the time they got here.

I think this would help with what @andrewsykim is asking about. I think https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/endpoints.go#L129 and https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/endpoints.go#L175 would be the starting point for that.

khenidak · 2019-11-05T20:10:07Z

skipping/ignoring is what we thought is the best approach the last time we discussed this. Because EndPointSlices carry mixed ip families. I do believe skipping/ignoring is ok not great. but ok.

Please do note pods in a dualstack clusters carry dualstack IPs (v4/v6 or v6/v4). As in, a change in pod (create, update, delete) will always result into a change in two EndPoints. That means max of two EndpointSlices will be affected when a pod ips change (only one EndpointSlice if the two endpoints landed in the same slice, which what the controller always attempt to do). So skipping will only happen in max one extra EndpointSlice.

For single stack, no skipping will happen since all endpoints will carry ips from the same family.

robscott · 2019-11-05T21:24:49Z

Thanks for the explanation @khenidak! Since EndpointSlice endpoints are tied directly to a Pod, a change affecting a Pod should only change 1 EndpointSlice. So unless there's some kind of case where a v4 or v6 address could change independently of each other for the same Pod, this does seem like a good approach.

khenidak · 2019-11-06T02:52:50Z

:-) max of two, if you need to overflow into another endpointslice. I will reword. Thanks.

andrewsykim · 2019-11-06T15:31:35Z

So to clarify - if I set the ipFamily field in Services to IPv4, the cluster IP is IPv4 but it's endpoints set of endpoints can be both IPv4 and IPv6?

khenidak · 2019-11-06T16:19:14Z

So to clarify - if I set the ipFamily field in Services to IPv4, the cluster IP is IPv4 but it's endpoints set of endpoints can be both IPv4 and IPv6?

No. Service.Spec.ipFamily drives the endpoint selection for a service. User will not get a different ip family assigned.

andrewsykim · 2019-11-06T16:31:17Z

Gotcha, so doesn't that mean there are EndpointSlice updates where the updates are exclusively IPv4 or exclusively IPv6 (re: #84089 (comment))? Thinking of a case where pods are dual-stack but some user of the cluster just care about one IP family still. That seems like a pretty likely scenario.

khenidak · 2019-11-06T17:05:54Z

Thanks!

I need to/let us push this forward because EndpointSlice BETA stage has dependency on supporting dualstack.

andrewsykim · 2019-11-06T17:19:13Z

pkg/proxy/ipvs/proxier.go

 	serviceUpdateResult := proxy.UpdateServiceMap(proxier.serviceMap, proxier.serviceChanges)
 	endpointUpdateResult := proxier.endpointsMap.Update(proxier.endpointsChanges)

 	staleServices := serviceUpdateResult.UDPStaleClusterIP
 	// merge stale services gathered from updateEndpointsMap
 	for _, svcPortName := range endpointUpdateResult.StaleServiceNames {
 		if svcInfo, ok := proxier.serviceMap[svcPortName]; ok && svcInfo != nil && svcInfo.Protocol() == v1.ProtocolUDP {
+			// ignore stale services that does not match ipfamily of the proxier
+			isIpv6Svc := utilnet.IsIPv6(svcInfo.ClusterIP())


What about ExternalIP?

A service that has ExternalIPs must have ClusterIP AFAIK. Do you know of any case where a service can carry ExternalIPs but ClusterIP is empty?

Headless Services (ClusterIP: None) with external IPs?

apiVersion: v1 kind: Service metadata: name: nginx labels: app: nginx spec: clusterIP: None externalIPs: - 1.1.1.1 selector: app: nginx ports: - port: 80 protocol: TCP

$ kubectl get svc nginx NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE nginx ClusterIP None 1.1.1.1 80/TCP 62s

andrewsykim · 2019-11-06T17:19:37Z

pkg/proxy/ipvs/proxier.go

@@ -1023,6 +1040,14 @@ func (proxier *Proxier) syncProxyRules() {
 			klog.Errorf("Failed to cast serviceInfo %q", svcName.String())
 			continue
 		}
+
+		// ignore services that does not match proxier ipfamily
+		isIpv6Svc := utilnet.IsIPv6(svcInfo.ClusterIP())


Same here, what about ExternalIP?

andrewsykim · 2019-11-13T18:16:54Z

/milestone v1.17

robscott · 2019-11-13T22:31:00Z

I think @khenidak is pretty busy so I'm going to work on a separate PR that integrates the EndpointSlice address type changes.

/hold

khenidak · 2019-11-14T18:02:06Z

closing in favor of #85246

k8s-ci-robot requested review from johnbelamaric and MrHohn October 18, 2019 16:48

k8s-ci-robot added area/ipvs sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 18, 2019

andrewsykim reviewed Oct 21, 2019

View reviewed changes

k8s-ci-robot assigned andrewsykim Oct 21, 2019

robscott mentioned this pull request Oct 25, 2019

Promoting EndpointSlices to beta #84390

Merged

8 tasks

khenidak force-pushed the dualstack-slice branch from f0da611 to 9b490e1 Compare November 5, 2019 17:45

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2019

add ipvs metaproxier endpointslice support

8788385

khenidak force-pushed the dualstack-slice branch from 9b490e1 to 8788385 Compare November 5, 2019 19:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2019

robscott reviewed Nov 5, 2019

View reviewed changes

andrewsykim reviewed Nov 6, 2019

View reviewed changes

lachie83 mentioned this pull request Nov 6, 2019

Add IPv4/IPv6 dual-stack support kubernetes/enhancements#563

Closed

andrewsykim mentioned this pull request Nov 7, 2019

Service Topology implementation for Kubernetes #72046

Merged

mrbobbytables mentioned this pull request Nov 13, 2019

EndpointSlice API kubernetes/enhancements#752

Closed

k8s-ci-robot added this to the v1.17 milestone Nov 13, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019

robscott mentioned this pull request Nov 13, 2019

Updating kube-proxy to support new EndpointSlice address types #85246

Merged

khenidak closed this Nov 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ipvs metaproxier endpointslice support #84089

add ipvs metaproxier endpointslice support #84089

khenidak commented Oct 18, 2019

robscott commented Oct 18, 2019 •

edited

khenidak commented Oct 18, 2019

andrewsykim Oct 21, 2019

andrewsykim Oct 21, 2019 •

edited

robscott Nov 5, 2019

khenidak Nov 7, 2019

andrewsykim commented Oct 21, 2019

k8s-ci-robot commented Nov 5, 2019

khenidak commented Nov 5, 2019

andrewsykim commented Nov 5, 2019

robscott commented Nov 5, 2019

andrewsykim commented Nov 5, 2019 •

edited

khenidak commented Nov 5, 2019

andrewsykim commented Nov 5, 2019

robscott Nov 5, 2019

khenidak Nov 5, 2019

robscott Nov 5, 2019

robscott Nov 5, 2019

robscott Nov 5, 2019

khenidak commented Nov 5, 2019 •

edited

robscott commented Nov 5, 2019

khenidak commented Nov 6, 2019

andrewsykim commented Nov 6, 2019 •

edited

khenidak commented Nov 6, 2019 •

edited

andrewsykim commented Nov 6, 2019 •

edited

khenidak commented Nov 6, 2019 •

edited

andrewsykim Nov 6, 2019

khenidak Nov 7, 2019

andrewsykim Nov 7, 2019

andrewsykim Nov 7, 2019

andrewsykim Nov 6, 2019

andrewsykim commented Nov 13, 2019

robscott commented Nov 13, 2019

khenidak commented Nov 14, 2019

add ipvs metaproxier endpointslice support #84089

add ipvs metaproxier endpointslice support #84089

Conversation

khenidak commented Oct 18, 2019

robscott commented Oct 18, 2019 • edited

khenidak commented Oct 18, 2019

Choose a reason for hiding this comment

andrewsykim Oct 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Oct 21, 2019

k8s-ci-robot commented Nov 5, 2019

khenidak commented Nov 5, 2019

andrewsykim commented Nov 5, 2019

robscott commented Nov 5, 2019

andrewsykim commented Nov 5, 2019 • edited

khenidak commented Nov 5, 2019

andrewsykim commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khenidak commented Nov 5, 2019 • edited

robscott commented Nov 5, 2019

khenidak commented Nov 6, 2019

andrewsykim commented Nov 6, 2019 • edited

khenidak commented Nov 6, 2019 • edited

andrewsykim commented Nov 6, 2019 • edited

khenidak commented Nov 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Nov 13, 2019

robscott commented Nov 13, 2019

khenidak commented Nov 14, 2019

robscott commented Oct 18, 2019 •

edited

andrewsykim Oct 21, 2019 •

edited

andrewsykim commented Nov 5, 2019 •

edited

khenidak commented Nov 5, 2019 •

edited

andrewsykim commented Nov 6, 2019 •

edited

khenidak commented Nov 6, 2019 •

edited

andrewsykim commented Nov 6, 2019 •

edited

khenidak commented Nov 6, 2019 •

edited