Refine Endpoint selection for Multi-cluster Service #4693

luolanzone · 2023-03-09T03:46:40Z

In order to decouple Multi-cluster with Antrea Proxy and refine Endpoint selection for Multi-cluster Service, following changes are included in this PR:

Revert previous changes for local Endpoint selection.
Add a new flow for the Service's ClusterIP in the EndpointDNAT table with
group action. When an Endpoint of a Multi-cluster Service is a local Service
ClusterIP and being selected, it will go to the corresponding exported Service's
group to select the final Endpoint. This can avoid that the traffic goes out of the
OVS bridge from antrea-gw0 (and handled by kube-proxy when it is running) and
comes back again.

The proposal details can be found in the comment:
#4508 (comment)

luolanzone · 2023-03-09T03:50:16Z

@jianjuns @tnqn could you take a look at this PR, I am still working on unit test. thanks.
Btw, should I merge the reverted commit and new commit into one? or keep the reverted commit standalone?

pkg/agent/openflow/client.go

tnqn · 2023-03-09T04:00:44Z

pkg/agent/openflow/client.go

 	// UninstallServiceFlows removes flows installed by InstallServiceFlows.
 	UninstallServiceFlows(svcIP net.IP, svcPort uint16, protocol binding.Protocol) error

+	// InstallServiceClusterIPFlows install flows for accessing Endpoints which is Service's ClusterIP, and has an action
+	// to Service's corresponding group.
+	InstallServiceClusterIPFlows(svcIP net.IP, svcPort uint16, protocol binding.Protocol, groupID binding.GroupIDType) error


Thought it over again, perhaps it's simpler to just generate one more flow in InstallServiceFlows directly.

func InstallServiceFlows() { ... if svcType == v1.ServiceTypeClusterIP && !nested { flows = append(flows, c.featureService. endpointRedirectFlowForServiceIP(svcIP, svcPort, protocol, groupID)) } ... }

Make sense, I will update it.

pkg/agent/openflow/fields.go

pkg/agent/proxy/proxier.go

pkg/agent/openflow/fields.go

pkg/agent/openflow/pipeline.go

pkg/agent/proxy/proxier.go

pkg/agent/proxy/topology.go

pkg/agent/openflow/pipeline.go

pkg/agent/openflow/client.go

hongliangl · 2023-03-09T08:49:47Z

pkg/agent/proxy/proxier.go

 			// Install ClusterIP flows for the Service.
 			groupID := p.groupCounter.AllocateIfNotExist(svcPortName, internalPolicyLocal)
-			if err := p.ofClient.InstallServiceFlows(groupID, svcInfo.ClusterIP(), uint16(svcInfo.Port()), svcInfo.OFProtocol, uint16(affinityTimeout), externalPolicyLocal, corev1.ServiceTypeClusterIP); err != nil {
+			if err := p.ofClient.InstallServiceFlows(groupID, svcInfo.ClusterIP(), uint16(svcInfo.Port()), svcInfo.OFProtocol, uint16(affinityTimeout), externalPolicyLocal, corev1.ServiceTypeClusterIP, hasNestedService); err != nil {


I think the new parameter should be a pointer

nil, do nothing in InstallServiceFlows. The Service is not related to nested Service, like NodePort or LoadBalancer.

true, install extra flow in EndpointDNATTable. The Service could be used as Endpoint of MC Service.

false, add extra action NestedServiceRegMark to the flow in ServiceLBTable. The Service is a MC Service.

In addition, this pointer should be always nil when MC is not enabled.

I feel this make the case more complicated, we only need to handle true or false case. There is actually no difference for nil and false since NodePort or LoadBalancer Service also have ClusterIP which can be exported.

I remember that you said at least now only ClusterIP can be used as the Endpoint of MC Service according to Kubernetes KEP-xxxx. If so, I think we should only enhance the flow of pure ClusterIP Service and skip the ClusterIP in NodePort or LoadBalancer.

Yes, I mean ClusterIP, not ClusterIP type of Service, since both NodePort and LoadBalancer have ClusterIP, I mean ClusterIP can be exported instead of NodeIP:NodePort or "LBIP:Port". KEP-1645 also suggested to use ClusterIP of NodePort and LoadBalancer Services.

Could we support that using ClusterIP of NodePort or LoadBalancer as Endpoint of MC-Service currently?

pkg/agent/openflow/pipeline.go

tnqn

LGTM overall, there are integration test failures.

pkg/agent/proxy/proxier.go

pkg/agent/openflow/pipeline.go

tnqn · 2023-03-09T11:57:38Z

pkg/agent/proxy/types/types.go

+	// NestedServiceSupport means the Service has an annotation "multicluster.antrea.io/imported-service"
+	// which means its Endpoints might be another Serivce's ClusterIP.
+	NestedServiceSupport bool


Suggested change

// NestedServiceSupport means the Service has an annotation "multicluster.antrea.io/imported-service"

// which means its Endpoints might be another Serivce's ClusterIP.

NestedServiceSupport bool

// IsNested means the Service's Endpoints could be another Service's ClusterIP.

// Currently it's true for Multicluster Service, determined by whether there is a Multicluster specific annotation.

IsNested bool

hongliangl · 2023-03-09T12:45:19Z

pkg/agent/openflow/client.go

+	if svcType == v1.ServiceTypeClusterIP && !nested {
+		flows = append(flows, c.featureService.endpointRedirectFlowForServiceIP(svcIP, svcPort, protocol, groupID))
+	}


Suggested change

if svcType == v1.ServiceTypeClusterIP && !nested {

flows = append(flows, c.featureService.endpointRedirectFlowForServiceIP(svcIP, svcPort, protocol, groupID))

}

if svcType == v1.ServiceTypeClusterIP && nested != nil && !*nested {

flows = append(flows, c.featureService.endpointRedirectFlowForServiceIP(svcIP, svcPort, protocol, groupID))

}

In this way, I think we can skip the ClusterIP flow of NodePort or LoadBalancer.

As I mentioned in previous comment, I feel there is no need to skip it. They will be be supported via ClusterIP.

I think the only gap here is that we didn't officially announce that we support these two kinds of Service in multi-cluster docs, I probably will update it after double check MC controller codes.

If we don't officially announce and support that , maybe we shouldn't introduce extra flows which are not used totally.

As I mentioned, there are probably just doc updates involved to support NodePort or LoadBalancer with ClusterIP, so I prefer not to skip it, otherwise we may change this part back soon. If you have further concern, we can sync offline, thanks.

If ClusterIP from NodePort or LoadBalancer will be supported eventually, I'm ok with that.

pkg/agent/openflow/pipeline.go

pkg/agent/proxy/proxier.go

luolanzone · 2023-03-09T14:20:21Z

/test-multicluster-e2e

jianjuns · 2023-03-09T19:03:59Z

pkg/agent/proxy/types/types.go

@@ -22,16 +22,25 @@ import (
 	k8sproxy "antrea.io/antrea/third_party/proxy"
 )

+const AntreaMCServiceAnnotation = "multicluster.antrea.io/imported-service"


Can we define it in pkg/agent/multicluster? Even consider defining a func there like IsMulticlusterService.

Done, add a new func.

pkg/agent/openflow/fields.go

luolanzone · 2023-03-10T01:07:16Z

/test-multicluster-e2e

luolanzone · 2023-03-10T01:07:29Z

/test-all

multicluster/controllers/multicluster/common/helper.go

tnqn

LGTM

jianjuns · 2023-03-10T20:18:01Z

@hongliangl : any further comments?
@luolanzone : please resolve the conflict.

…o#4508)" This reverts commit 6cdbca3. Signed-off-by: Lan Luo <luola@vmware.com>

Add a new flow for the Service's ClusterIP in the EndpointDNAT table with group action. When an Endpoint of a Multi-cluster Service is a local Service ClusterIP and being selected, it will go to the corresponding exported Service's group to select the final Endpoint. This can avoid that the traffic goes out of the OVS bridge from antrea-gw0 (and handled by kube-proxy when it is running) and comes back again. The proposal details can be found in the comment: antrea-io#4508 (comment) Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone · 2023-03-13T07:19:31Z

Conflicts resolved.

/test-multicluster-e2e
/test-all

tnqn · 2023-03-13T11:49:57Z

/test-multicluster-e2e

jianjuns · 2023-03-13T17:33:40Z

/test-multicluster-e2e

luolanzone · 2023-03-14T01:06:26Z

Seems the testbed is not stable again, the e2es take much longer time which is 2 times than before, and NP tests failed with timeout, I will check with @hjiajing.

luolanzone · 2023-03-14T02:53:04Z

/test-multicluster-e2e

luolanzone · 2023-03-14T03:31:58Z

/test-multicluster-e2e

luolanzone · 2023-03-14T05:18:00Z

/test-multicluster-e2e

luolanzone · 2023-03-14T08:02:20Z

/test-multicluster-e2e

luolanzone · 2023-03-14T08:55:28Z

/test-multicluster-e2e

luolanzone · 2023-03-14T09:17:42Z

I have tried to trigger MC e2e locally with this branch, the results looks fine, all tests are passed. The latest triggered jenkins job might be back to normal. @tnqn you may either merged this or wait for the latest build result from Jenkins. Thanks.

--- PASS: TestConnectivity (340.06s)
    --- PASS: TestConnectivity/TestMCService (63.24s)
        --- PASS: TestConnectivity/TestMCService/Case=MCServiceConnectivity (0.75s)
        --- PASS: TestConnectivity/TestMCService/Case=ScaleDownMCServiceEndpoints (3.02s)
        --- PASS: TestConnectivity/TestMCService/Case=ANPToServices (12.61s)
        --- PASS: TestConnectivity/TestMCService/Case=StretchedNetworkPolicy (12.62s)
        --- PASS: TestConnectivity/TestMCService/Case=StretchedNetworkPolicyReject (6.66s)
        --- PASS: TestConnectivity/TestMCService/Case=StretchedNetworkPolicyUpdatePod (3.54s)
        --- PASS: TestConnectivity/TestMCService/Case=StretchedNetworkPolicyUpdateNS (6.67s)
        --- PASS: TestConnectivity/TestMCService/Case=StretchedNetworkPolicyUpdatePolicy (3.35s)
    --- PASS: TestConnectivity/TestAntreaPolicy (264.22s)
PASS
ok  	antrea.io/antrea/multicluster/test/e2e	342.245s

* Revert "Refine Endpoint selection for multi-cluster Service (antrea-io#4508)" This reverts commit 6cdbca3. Signed-off-by: Lan Luo <luola@vmware.com> * Refine Endpoint selection for MC Service Add a new flow for the Service's ClusterIP in the EndpointDNAT table with group action. When an Endpoint of a Multi-cluster Service is a local Service ClusterIP and being selected, it will go to the corresponding exported Service's group to select the final Endpoint. This can avoid that the traffic goes out of the OVS bridge from antrea-gw0 (and handled by kube-proxy when it is running) and comes back again. The proposal details can be found in the comment: antrea-io#4508 (comment) Signed-off-by: Lan Luo <luola@vmware.com> --------- Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone requested review from hongliangl, jianjuns and tnqn March 9, 2023 03:52

luolanzone force-pushed the mc-local-ep-new-action branch from 31f9364 to cd7e9e3 Compare March 9, 2023 04:14

tnqn reviewed Mar 9, 2023

View reviewed changes

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

luolanzone force-pushed the mc-local-ep-new-action branch 2 times, most recently from daac839 to 7e43f5c Compare March 9, 2023 05:57

luolanzone changed the title ~~[WIP]Refine Endpoint selection for Multi-cluster Service~~ Refine Endpoint selection for Multi-cluster Service Mar 9, 2023

hongliangl reviewed Mar 9, 2023

View reviewed changes

tnqn reviewed Mar 9, 2023

View reviewed changes

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

luolanzone force-pushed the mc-local-ep-new-action branch 2 times, most recently from 823e868 to 32f91ce Compare March 9, 2023 10:59

tnqn reviewed Mar 9, 2023

View reviewed changes

hongliangl reviewed Mar 9, 2023

View reviewed changes

luolanzone force-pushed the mc-local-ep-new-action branch from 32f91ce to 49ede2d Compare March 9, 2023 13:50

tnqn mentioned this pull request Mar 9, 2023

Keep the cached Endpoints the same as those installed in OVS #4691

Merged

luolanzone force-pushed the mc-local-ep-new-action branch from 49ede2d to 2ebb729 Compare March 9, 2023 14:19

luolanzone force-pushed the mc-local-ep-new-action branch from 2ebb729 to 966807c Compare March 9, 2023 14:23

jianjuns reviewed Mar 9, 2023

View reviewed changes

tnqn added this to the Antrea v1.11 release milestone Mar 10, 2023

luolanzone force-pushed the mc-local-ep-new-action branch from 966807c to c8e693b Compare March 10, 2023 01:06

jianjuns reviewed Mar 10, 2023

View reviewed changes

multicluster/controllers/multicluster/common/helper.go Show resolved Hide resolved

tnqn previously approved these changes Mar 10, 2023

View reviewed changes

Revert "Refine Endpoint selection for multi-cluster Service (antrea-i…

ff9d52d

…o#4508)" This reverts commit 6cdbca3. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone dismissed tnqn’s stale review via b2dc841 March 13, 2023 07:18

luolanzone force-pushed the mc-local-ep-new-action branch from c8e693b to b2dc841 Compare March 13, 2023 07:18

tnqn approved these changes Mar 14, 2023

View reviewed changes

tnqn merged commit 25ece2f into antrea-io:main Mar 14, 2023

Refine Endpoint selection for Multi-cluster Service #4693

Refine Endpoint selection for Multi-cluster Service #4693

Conversation

luolanzone commented Mar 9, 2023

luolanzone commented Mar 9, 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

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

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 Mar 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Mar 10, 2023

luolanzone commented Mar 10, 2023

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Mar 10, 2023

luolanzone commented Mar 13, 2023

tnqn commented Mar 13, 2023

jianjuns commented Mar 13, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023

luolanzone commented Mar 14, 2023