Support EndpointSlice API for ServiceExport controller #4895

luolanzone · 2023-04-25T08:19:32Z

Process EndpointSlice API by default for exported Service's endpoint change.
Validate endpointIPType to allow 'PodIP' or 'ClusterIP' only.
Fix a typo

For #4730

luolanzone · 2023-04-25T08:19:45Z

/test-multicluster-e2e

luolanzone · 2023-04-26T08:05:45Z

/test-all

luolanzone · 2023-04-28T01:28:35Z

/test-all

luolanzone · 2023-04-28T01:28:45Z

/test-multicluster-e2e

multicluster/controllers/multicluster/member/serviceexport_controller.go

luolanzone · 2023-05-08T08:29:45Z

/test-multicluster-e2e

multicluster/controllers/multicluster/member/serviceexport_controller.go

luolanzone · 2023-05-12T06:32:23Z

/test-multicluster-e2e

multicluster/controllers/multicluster/member/serviceexport_controller.go

jianjuns · 2023-05-12T23:55:41Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

@@ -579,7 +627,9 @@ func (r *ServiceExportReconciler) refreshResourceExport(resName, kind string,
 	case constants.ServiceKind:
 		re.ObjectMeta.Name = resName
 		re.Spec.Service = &mcsv1alpha1.ServiceExport{
-			ServiceSpec: svc.Spec,
+			ServiceSpec: corev1.ServiceSpec{


Maybe we should redefine ResourceExport for a Service, not to use the ServiceExport/Service.Spec, if we just need Ports. We can revisit this later.

Sure, will create a new PR for this.

multicluster/controllers/multicluster/member/serviceexport_controller.go

jianjuns

LGTM. Two nits.

multicluster/controllers/multicluster/member/serviceexport_controller.go

luolanzone · 2023-05-16T01:23:42Z

/test-multicluster-e2e

tnqn · 2023-05-18T09:40:14Z

multicluster/apis/multicluster/v1alpha1/multiclusterconfig_types.go

@@ -57,6 +57,9 @@ type MultiClusterConfig struct {
 	// ClusterSet and allow Antrea-native policies to select peers from other clusters
 	// in a ClusterSet.
 	EnableStretchedNetworkPolicy bool `json:"enableStretchedNetworkPolicy,omitempty"`
+	// Enable EndpointSlice to watch EndpointSlice changes only for exported Service, this is required
+	// when EndpointIPType is PodIP and CNI's EndpointSlice feature is enabled.
+	EnableEndpointSlice bool `json:"enableEndpointSlice,omitempty"`


I wonder why a configuration parameter is needed but not use the feature gate like AntreaProxy? Ideally EndpointSlice should be the default API we use to get Endpoints as it's GA and more efficient. User shouldn't care whether we use which API we are using.

In AntreaProxy, we use EndpointSlice when the feature gate is enabled (the current default value) and will fall back to Endpoint API if the EndpointSlice API is not available (K8s <1.21)

Make sense, we didn't introduce feature gate in MC, but I guess it's better to follow AntreaProxy way to check EndpointSlice API and fall back to Endpoint if it's not available. I will refine this part. thanks.

I addressed the comment in a new commit, since Antrea MC didn't provide feature gate for EndpointSlice, the current behavior is to use EndpointSlice by default as long as the API is available and fall back to Endpoint when it's not available. @jianjuns any thoughts?

Do we have a way to know if Endpoints are also created? In the current implementation, watching/exporting Endpoints is more efficient than exporting EndpointSlice.

EndpointSlice will be created automatically if Endpoint is created, but I doubt Endpoint controller will do the same if EndpointSlice is created for a Service. @tnqn any idea?

Since Endpoints will be mirrored to EndpointSlice and exporting Exporting Endpoints is more efficient in Multicluster, why do we need this new patch? I guess there is no EndpointSlice -> Endpoints mirroring, but you can test it by creating an EndpointSlice.

I tested there is automatic Endpoints -> EndpointSlice mirroring and no EndpointSlice -> Endpoints mirroring. And the official doc about Services without selector guides users to create EndpointSlice manually now. If we want to support Services without selector, the best choice seems listening to EndpointSlice API when it's available because:

If user create EndpointSlice manually, it can be exported.

If user create Endpoints manually, it will be mirrored to EndpointSlice and be exported.

Ok. Got it.

luolanzone · 2023-05-19T02:24:10Z

/test-multicluster-e2e

luolanzone · 2023-05-19T06:13:29Z

/test-multicluster-e2e

luolanzone · 2023-05-19T06:25:13Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

+		newCondition.Reason = getStringPointer("Succeed")
+		newCondition.Message = getStringPointer("The Service is exported successfully")


Note: Reason and Message are required fields in upstream ServiceExport definition, so recover it to fix the bug which is introduced by #4814.

luolanzone · 2023-05-19T07:18:20Z

/test-multicluster-e2e

hjiajing · 2023-05-19T08:28:13Z

/test-multicluster-e2e

luolanzone · 2023-05-23T01:18:35Z

@jianjuns @tnqn could you help to review the PR again? I addressed the comment to watch EndpointSlice API in the top commit. thanks.

tnqn · 2023-05-23T06:55:06Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

+				ports = convertEndpointPorts(eps.Ports)
+			}
+			for _, ep := range eps.Endpoints {
+				if *ep.Conditions.Ready {


The explanation of the field says it could be nil, indicating an unknown state. If it happens, the program would panic.

tnqn · 2023-05-23T07:34:02Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

+					subset := corev1.EndpointSubset{}
+					readyAddresses := ipsToEndpointAddresses(ep.Addresses)
+					subset.Addresses = append(subset.Addresses, readyAddresses...)
+					subset.Ports = ports
+					subsets = append(subsets, subset)


It generates one subset per Endpoint, but I think the relationship between EndpointSlice and Endpoints is one EndpointSlice maps to one subset in Endpoints?

// EndpointSubset is a group of addresses with a common set of ports. The
// expanded set of endpoints is the Cartesian product of Addresses x Ports.

Otherwise the ports field is rather redundant as there is a duplicate in each subset.

multicluster/controllers/multicluster/member/serviceexport_controller.go

tnqn · 2023-05-23T07:55:23Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

+		if r.endpointSliceEnabled {
+			newSubsets = eps.Subsets
+		} else {
+			newSubsets = filterEndpointSubsets(eps.Subsets)


Can we process Endpoints in one place and stick to use newSubsets to cache the new subsets? The current logic is distributed at L242-L268 and here, and somestimes use eps.Subsets to cache the result but sometimes set newSubsets to eps.Subsets.

luolanzone · 2023-05-23T08:32:41Z

/test-multicluster-e2e

tnqn · 2023-05-23T08:44:06Z

multicluster/controllers/multicluster/member/serviceexport_controller.go

 		}
 	} else {
-		if !apierrors.IsNotFound(err) {
+		hasReadyEndpoints, err = r.checkSubsetsFromEndpoint(ctx, req, eps)


why don't return newSubsets from checkSubsetsFromEndpoint and remove filterEndpointSubsets? They seems to do same work.

Good point, refined, thanks.

luolanzone · 2023-05-23T09:30:35Z

/test-multicluster-e2e

luolanzone · 2023-05-23T09:35:03Z

/test-multicluster-e2e

luolanzone · 2023-05-23T10:14:12Z

/test-multicluster-e2e

tnqn

Code LGTM, but the PR description and commit message are out-of-date.

jianjuns · 2023-05-23T20:10:33Z

CONTRIBUTING.md

@@ -188,7 +188,7 @@ Here are the trigger phrases for individual checks:
 Here are the trigger phrases for groups of checks:

 * `/test-all`: Linux IPv4 tests
-* `/test-windows-all`: Windows IPv4 tests, including e2e tests with proxyAll enabled. It also includes all Containderd runtime based Windows tests since 1.10.0.
+* `/test-windows-all`: Windows IPv4 tests, including e2e tests with proxyAll enabled. It also includes all Containerd runtime based Windows tests since 1.10.0.


Containerd -> containerd

1. Process EndpointSlice API by default for exported Service's endpoint change. 2. Validate `endpointIPType` to allow 'PodIP' or 'ClusterIP' only. 3. Fix a typo. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone · 2023-05-24T02:01:04Z

/test-multicluster-e2e

luolanzone · 2023-05-24T02:01:30Z

Updated the messages and squashed them into one commit.

tnqn · 2023-05-24T04:07:24Z

/skip-all

tnqn · 2023-05-24T04:07:49Z

@jianjuns do you have other comments?

jianjuns · 2023-05-24T04:10:10Z

@jianjuns do you have other comments?

No

1. Process EndpointSlice API by default for exported Service's endpoint change. 2. Validate `endpointIPType` to allow 'PodIP' or 'ClusterIP' only. 3. Fix a typo. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Apr 25, 2023

luolanzone force-pushed the mc-endpointslice branch from beacaa3 to e3ff2c8 Compare April 27, 2023 12:54

luolanzone requested review from jianjuns, Dyanngg and hjiajing April 28, 2023 01:29

jianjuns reviewed Apr 28, 2023

View reviewed changes

luolanzone added this to the Antrea v1.12 release milestone May 8, 2023

luolanzone force-pushed the mc-endpointslice branch from e3ff2c8 to 9739a38 Compare May 8, 2023 08:27

jianjuns reviewed May 8, 2023

View reviewed changes

luolanzone force-pushed the mc-endpointslice branch from 9739a38 to 4801b10 Compare May 10, 2023 02:57

jianjuns reviewed May 10, 2023

View reviewed changes

luolanzone force-pushed the mc-endpointslice branch 2 times, most recently from 6ecaaf3 to 005a952 Compare May 12, 2023 06:31

jianjuns reviewed May 13, 2023

View reviewed changes

luolanzone force-pushed the mc-endpointslice branch from 005a952 to 373f898 Compare May 15, 2023 01:55

jianjuns reviewed May 15, 2023

View reviewed changes

multicluster/controllers/multicluster/member/serviceexport_controller.go Outdated Show resolved Hide resolved

multicluster/controllers/multicluster/member/serviceexport_controller.go Outdated Show resolved Hide resolved

luolanzone force-pushed the mc-endpointslice branch from 373f898 to cdcd4f1 Compare May 16, 2023 01:23

luolanzone force-pushed the mc-endpointslice branch from cdcd4f1 to 5d25088 Compare May 16, 2023 02:33

jianjuns previously approved these changes May 16, 2023

View reviewed changes

tnqn reviewed May 18, 2023

View reviewed changes

luolanzone dismissed jianjuns’s stale review via eefe56c May 19, 2023 02:23

luolanzone force-pushed the mc-endpointslice branch from eefe56c to 9d7d401 Compare May 19, 2023 02:25

luolanzone force-pushed the mc-endpointslice branch from ecd3be2 to 424b3be Compare May 19, 2023 06:22

luolanzone commented May 19, 2023

View reviewed changes

tnqn reviewed May 23, 2023

View reviewed changes

luolanzone force-pushed the mc-endpointslice branch from 424b3be to 8654940 Compare May 23, 2023 08:32

tnqn reviewed May 23, 2023

View reviewed changes

luolanzone force-pushed the mc-endpointslice branch from 8654940 to efd172f Compare May 23, 2023 09:30

luolanzone force-pushed the mc-endpointslice branch from efd172f to b233bb0 Compare May 23, 2023 09:33

luolanzone force-pushed the mc-endpointslice branch from b233bb0 to e626140 Compare May 23, 2023 10:13

tnqn previously approved these changes May 23, 2023

View reviewed changes

jianjuns reviewed May 23, 2023

View reviewed changes

luolanzone dismissed tnqn’s stale review via a4759cc May 24, 2023 01:48

luolanzone force-pushed the mc-endpointslice branch from e626140 to a4759cc Compare May 24, 2023 01:48

Support EndpointSlice API for ServiceExport controller

25f3f48

1. Process EndpointSlice API by default for exported Service's endpoint change. 2. Validate `endpointIPType` to allow 'PodIP' or 'ClusterIP' only. 3. Fix a typo. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone force-pushed the mc-endpointslice branch from a4759cc to 25f3f48 Compare May 24, 2023 02:00

tnqn approved these changes May 24, 2023

View reviewed changes

tnqn merged commit 042e772 into antrea-io:main May 24, 2023

		newCondition.Reason = getStringPointer("Succeed")
		newCondition.Message = getStringPointer("The Service is exported successfully")

Support EndpointSlice API for ServiceExport controller #4895

Support EndpointSlice API for ServiceExport controller #4895

Conversation

luolanzone commented Apr 25, 2023 • edited Loading

luolanzone commented Apr 25, 2023

luolanzone commented Apr 26, 2023

luolanzone commented Apr 28, 2023

luolanzone commented Apr 28, 2023

luolanzone commented May 8, 2023

luolanzone commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

luolanzone commented May 16, 2023

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

luolanzone commented May 19, 2023

luolanzone commented May 19, 2023

Choose a reason for hiding this comment

luolanzone commented May 19, 2023

hjiajing commented May 19, 2023

luolanzone commented May 23, 2023

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

luolanzone commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented May 23, 2023

luolanzone commented May 23, 2023

luolanzone commented May 23, 2023

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

luolanzone commented May 24, 2023

luolanzone commented May 24, 2023

tnqn commented May 24, 2023

tnqn commented May 24, 2023

jianjuns commented May 24, 2023

luolanzone commented Apr 25, 2023 •

edited

Loading