Support EndpointSlice in Antrea proxy #1703

hongliangl · 2020-12-24T02:43:06Z

Support EndpointSlice in AntreaProxy, ServiceTopology is not supported in this commit

codecov-io · 2020-12-24T02:46:36Z

Codecov Report

❗ No coverage uploaded for pull request base (main@60d1d28). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1703   +/-   ##
=======================================
  Coverage        ?   52.80%           
=======================================
  Files           ?      188           
  Lines           ?    16347           
  Branches        ?        0           
=======================================
  Hits            ?     8632           
  Misses          ?     6856           
  Partials        ?      859

Flag	Coverage Δ
e2e-tests	`26.78% <0.00%> (?)`
unit-tests	`42.12% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Makefile

ci/check-manifest.sh

cmd/antrea-agent/agent.go

pkg/agent/proxy/endpoints.go

antoninbas

Overall LGTM. What is the behavior if some enabled the Antrea EndpointSlice feature gate in a K8s cluster which does not support the EndpointSlice API?

Makefile

docs/feature-gates.md

hack/generate-manifest.sh

pkg/agent/proxy/endpointslicecache.go

pkg/features/antrea_features.go

.github/workflows/kind.yml

pkg/agent/proxy/proxier.go

docs/feature-gates.md

pkg/agent/proxy/endpoints.go

tnqn · 2021-01-15T12:50:05Z

pkg/agent/proxy/proxier.go

@@ -105,6 +109,7 @@ func (p *proxier) removeStaleServices() {
 			}
 		}
 		delete(p.serviceInstalledMap, svcPortName)
+		delete(p.endpointInstalledMap, svcPortName)


Is this a bug fix for memory leak?

@weiqiangt is it still supposed to remove the endpointInstalledMap here given that you also do it in removeStaleEndpoints?

@hongliangl please reply here so we have the context to discuss.
Moving your reply here:

is it still supposed to remove the endpointInstalledMap here given that you also do it in removeStaleEndpoints?

Here removes all endpoints related with a stale service. Function removeStaleEndpoints removes stale endpoints of a normal service.

@weiqiangt From my understanding of #1815, you need the endpointInstalledMap to be there so that you can identify stale endpoints to delete, right? I think deleting it here cause openflow leak?

OK, I see that in this version, endpoints are still removed in removeStaleServices so it makes sense to delete p.endpointInstalledMap.
There will be a conflict between this PR and #1815. @weiqiangt could you cooperate with @hongliangl to see how to merge these PRs?

is it still supposed to remove the endpointInstalledMap here given that you also do it in removeStaleEndpoints?

Here removes all endpoints related with a stale service. Function removeStaleEndpoints removes stale endpoints of a normal service.

Thanks, @tnqn. I think this PR does not have many changes on the syncProxyRules and its sub-calls. I think we can first merge #1815 and then rebase #1703 onto #1815 without this line.

pkg/agent/proxy/proxier.go

cmd/antrea-agent/agent.go

antoninbas

Code LGTM. I will defer to @tnqn for final approval.
One of my comments has not been addressed: I do not see any mention anywhere of the behavior if 1) AntreaProxy and EndpointSlice feature gates are enabled; 2) the EndpointSlice API is disabled in K8s. IMO, the behavior should at least be mentioned in docs/feature-gates.md and should probably also be included in the commit message.

docs/feature-gates.md

hack/generate-manifest.sh

tnqn

Overall LGTM

.github/workflows/kind.yml

pkg/agent/proxy/endpointslicecache.go

hongliangl · 2021-01-20T07:03:04Z

hack/generate-manifest.sh

@@ -106,6 +108,11 @@ case $key in
    PROXY=false
    shift
    ;;
+    --endpointslice)
+    PROXY=true


@antoninbas AntreaProxy and EndpointSlice can be enabled at the same time. However it's unnecessary to enable both arguments. We can see here if EndpointSlice is enabled, AntreaProxy will also be set as enabled.

Sorry, is this the answer to one of my questions? I'm confused

One of my comments has not been addressed: I do not see any mention anywhere of the behavior if 1) AntreaProxy and EndpointSlice feature gates are enabled; 2) the EndpointSlice API is disabled in K8s. IMO, the behavior should at least be mentioned in docs/feature-gates.md and should probably also be included in the commit message.

@antoninbas This is the answer of question 1).

I am asking about the behavior when EndpointSlice is enabled in Antrea but the EndpointSlice API is disabled / not supported in Kubernetes. Is there a crash? Is there an error log message? Is it a no-op?

@antoninbas The restriction of using EndpointSlice in Antrea should be explained in docs. The explain is that this feature can be set as enabled if only that Kubernetes 1.16 (alpha) or later with EndpointSlice enabled. For users of Antrea, the behavior when EndpointSlice is enabled in Antrea but the EndpointSlice API is disabled / not supported in Kubernetes should be avoid. So far, there is not an exact evaluation about the unwanted behavior.

So far, there is not an exact evaluation about the unwanted behavior.

Could we make a quick test to have a clear idea on that?

I've tested this scenario on a single node minikube cluster with Kubernetes v1.16.1.
The agent was not crash and no services/endpoints could be watched, and thus the agent can not connect to the controller.
There are logs in the agent indicate that the EndpointSlice resource is unavailable.

E0202 04:53:35.852020 1 reflector.go:178] pkg/mod/github.com/tnqn/client-go@v0.18.4-1/tools/cache/reflector.go:125: Failed to list *v1beta1.EndpointSlice: the server could not find the requested resource (get endpointslices.discovery.k8s.io)

I think we can first try to check if the EndpointSlice resource exists before really starting Antrea Proxy with EndpointSlice enabled.

cmd/antrea-agent/agent.go

pkg/agent/proxy/proxier.go

docs/feature-gates.md

build/yamls/base/conf/antrea-agent.conf

cmd/antrea-agent/agent.go

jianjuns · 2021-02-02T23:51:47Z

pkg/agent/proxy/endpointslicecache.go

@@ -0,0 +1,364 @@
+/*


I would agree!

Also thinking should we add comments for the changes we made to the original copied files, e.g. at the function level? Or there might be too many changes to track in this way?

hongliangl · 2021-02-03T07:05:54Z

./skip-all

tnqn · 2021-02-08T10:31:48Z

pkg/agent/proxy/proxier.go

@@ -105,6 +109,7 @@ func (p *proxier) removeStaleServices() {
 			}
 		}
 		delete(p.serviceInstalledMap, svcPortName)
+		delete(p.endpointInstalledMap, svcPortName)


@weiqiangt is it still supposed to remove the endpointInstalledMap here given that you also do it in removeStaleEndpoints?

tnqn · 2021-02-08T10:39:34Z

pkg/agent/proxy/proxier.go

@@ -276,6 +281,9 @@ func (p *proxier) installServices() {
 		}

 		p.serviceInstalledMap[svcPortName] = svcPort
+		for _, endpoint := range endpointUpdateList {


Why is this needed? Haven't L210-L213 done it?

Why is this needed? Haven't L210-L213 done it?

Agreed, it is not needed since endpointUpdateList is not updated after L210-L213.

is it still supposed to remove the endpointInstalledMap here given that you also do it in removeStaleEndpoints?

Here removes all endpoints related with a stale service. Function removeStaleEndpoints removes stale endpoints of a normal service.

The EndpointSlice API version that AntreaProxy supports is v1beta1 for now, and other EndpointSlice API versions are not supported. Endpoint condition Serving,Terminating as well as ServiceTopology is not supported in this commit.

tnqn

Thanks, LGTM

jianjuns

I just have some nits on comments, logs, and documentation. I am fine to merge the PR first, given Hongliang should be taking holiday leave already.

jianjuns · 2021-02-10T19:14:04Z

docs/feature-gates.md

+
+`EndpointSlice` enables Service EndpointSlice support in AntreaProxy. The
+EndpointSlice API was introduced in Kubernetes 1.16 (alpha) and it is enabled
+by default in Kubernetes 1.17 (beta). This flag will take no effect if AntreaProxy


Probably clearer to replace "This flag" with "The EndpointSlice feature gate".

Thanks for advice, I will handle today.

jianjuns · 2021-02-10T19:15:05Z

docs/feature-gates.md

+by default in Kubernetes 1.17 (beta). This flag will take no effect if AntreaProxy
+is not enabled. The endpoint conditions of `Serving` and `Terminating` are not
+supported currently. ServiceTopology is not supported either. Refer to this [link](https://kubernetes.io/docs/tasks/administer-cluster/enabling-endpointslices/)
+for more information. EndpointSlice API version that AntreaProxy supports is v1beta1


The EndpointSlice API version

jianjuns · 2021-02-10T19:17:16Z

docs/feature-gates.md

+for more information. EndpointSlice API version that AntreaProxy supports is v1beta1
+currently, and other EndpointSlice API versions are not supported. If EndpointSlice is
+enabled in AntreaProxy, but EndpointSlice API is disabled in Kubernetes or EndpointSlice
+API version v1beta1 is not supported in Kubernetes, error messages will be logged by


Probably "Antrea Agent will log an error message and ..."

jianjuns · 2021-02-10T19:22:08Z

pkg/agent/proxy/endpoints.go

+	}
+
+	if _, _, err := endpointSliceCacheKeys(endpointSlice); err != nil {
+		klog.Warningf("Error getting endpoint slice cache keys: %v", err)


Better to use the same format for EndpointService (rather than all endpointSlice, EndpointSlice, endpoint slice..)

jianjuns · 2021-02-10T19:23:48Z

pkg/agent/proxy/endpointslicecache.go

+// limitations under the License.
+//
+// Original file https://raw.githubusercontent.com/kubernetes/kubernetes/0c0d4fea8dd6bdcd16b9e1d35da3f7d209341a6f/pkg/proxy/endpointslicecache.go
+// If this file is located in third_party, there will be an import cycle issue when build Antrea as this file import


when build -> when building

import -> imports

antoninbas · 2021-02-10T21:16:46Z

/test-all
/test-ipv6-all
/test-ipv6-only-all

antoninbas · 2021-02-11T00:40:07Z

All the test have passed. @hongliangl let me know if you only make documentation changes and log message changes (please consider pushing a separate commit instead of amending the existing one) and I will not need to run the IPv6 and dual-stack tests again.

Including files: - docs/feature-gates.md - pkg/agent/proxy/endpoints.go - pkg/agent/proxy/endpointslicecache.go

hongliangl · 2021-02-11T01:36:45Z

All the test have passed. @hongliangl let me know if you only make documentation changes and log message changes (please consider pushing a separate commit instead of amending the existing one) and I will not need to run the IPv6 and dual-stack tests again.

I have pushed a separate commit that only includes documentation and log message changes. @antoninbas

jianjuns

Thanks for addressing the comments!

antoninbas · 2021-02-11T01:46:30Z

/skip-all
all the tests passed before the last commit

antoninbas · 2021-02-11T02:12:47Z

Thanks @hongliangl I will merge this - enjoy your holiday!

hongliangl · 2021-02-11T03:22:14Z

Thanks for everyone's help and advice, I've learned a lot.

vmwclabot added the cla-not-required label Dec 24, 2020

hongliangl force-pushed the endpointslice branch 2 times, most recently from b8fbd91 to d0e83a1 Compare December 27, 2020 02:20

hongliangl force-pushed the endpointslice branch 2 times, most recently from 57cbc6c to 3866e27 Compare January 11, 2021 09:44

jianjuns requested a review from weiqiangt January 11, 2021 22:50

weiqiangt reviewed Jan 12, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

ci/check-manifest.sh Outdated Show resolved Hide resolved

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

pkg/agent/proxy/endpoints.go Outdated Show resolved Hide resolved

hongliangl force-pushed the endpointslice branch from 5d1a1ef to 649969e Compare January 12, 2021 05:50

antoninbas reviewed Jan 12, 2021

View reviewed changes

hongliangl force-pushed the endpointslice branch 4 times, most recently from 8dcbf3a to 88af983 Compare January 13, 2021 10:06

hongliangl requested review from antoninbas and weiqiangt January 14, 2021 02:19

tnqn reviewed Jan 15, 2021

View reviewed changes

hongliangl force-pushed the endpointslice branch from 88af983 to 67e5f07 Compare January 18, 2021 07:56

hongliangl requested a review from tnqn January 19, 2021 01:14

antoninbas reviewed Jan 19, 2021

View reviewed changes

docs/feature-gates.md Outdated Show resolved Hide resolved

hack/generate-manifest.sh Outdated Show resolved Hide resolved

tnqn reviewed Jan 20, 2021

View reviewed changes

.github/workflows/kind.yml Outdated Show resolved Hide resolved

pkg/agent/proxy/endpointslicecache.go Outdated Show resolved Hide resolved

hongliangl commented Jan 20, 2021

View reviewed changes

hongliangl force-pushed the endpointslice branch 6 times, most recently from f0204ae to 77304a6 Compare January 20, 2021 16:33

hongliangl requested review from antoninbas and tnqn January 22, 2021 01:21

hongliangl commented Feb 2, 2021

View reviewed changes

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved

pkg/agent/proxy/proxier.go Outdated Show resolved Hide resolved

jianjuns reviewed Feb 2, 2021

View reviewed changes

hongliangl force-pushed the endpointslice branch 3 times, most recently from 1da8bbc to e7b242e Compare February 3, 2021 07:05

hongliangl force-pushed the endpointslice branch 2 times, most recently from 56d7762 to 81639b8 Compare February 5, 2021 06:07

tnqn reviewed Feb 8, 2021

View reviewed changes

hongliangl force-pushed the endpointslice branch 2 times, most recently from 867d6fe to 0ba1d22 Compare February 10, 2021 07:29

Support EndpointSlice in AntreaProxy

8411bca

The EndpointSlice API version that AntreaProxy supports is v1beta1 for now, and other EndpointSlice API versions are not supported. Endpoint condition Serving,Terminating as well as ServiceTopology is not supported in this commit.

hongliangl force-pushed the endpointslice branch from 0ba1d22 to 8411bca Compare February 10, 2021 08:45

tnqn previously approved these changes Feb 10, 2021

View reviewed changes

jianjuns previously approved these changes Feb 10, 2021

View reviewed changes

antoninbas added this to the Antrea v0.13.0 release milestone Feb 10, 2021

hongliangl dismissed stale reviews from jianjuns and tnqn via 57e5e8f February 11, 2021 01:26

Grammar and typos corrections according to Jianjun's comments.

73ee70e

Including files: - docs/feature-gates.md - pkg/agent/proxy/endpoints.go - pkg/agent/proxy/endpointslicecache.go

hongliangl force-pushed the endpointslice branch from 57e5e8f to 73ee70e Compare February 11, 2021 01:30

jianjuns approved these changes Feb 11, 2021

View reviewed changes

antoninbas merged commit 5037859 into antrea-io:main Feb 11, 2021

antoninbas mentioned this pull request Feb 11, 2021

Support EndpointSlices in AntreaProxy #1164

Closed

hongliangl deleted the endpointslice branch April 28, 2022 11:53

Support EndpointSlice in Antrea proxy #1703

Support EndpointSlice in Antrea proxy #1703

Conversation

hongliangl commented Dec 24, 2020 • edited Loading

codecov-io commented Dec 24, 2020 • edited Loading

Codecov Report

antoninbas left a 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

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

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

vicky-liu Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl commented Feb 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongliangl Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

hongliangl Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

jianjuns left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Feb 10, 2021

antoninbas commented Feb 11, 2021

hongliangl commented Feb 11, 2021

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas commented Feb 11, 2021

antoninbas commented Feb 11, 2021

hongliangl commented Feb 11, 2021

hongliangl commented Dec 24, 2020 •

edited

Loading

codecov-io commented Dec 24, 2020 •

edited

Loading

hongliangl Jan 20, 2021 •

edited

Loading

hongliangl Jan 20, 2021 •

edited

Loading

vicky-liu Jan 22, 2021 •

edited

Loading

hongliangl Feb 8, 2021 •

edited

Loading

hongliangl Feb 8, 2021 •

edited

Loading