Support more traffic modes for Multi-cluster Gateway #4407

luolanzone · 2022-11-23T06:47:33Z

In order to support multi-cluster traffic when the member cluster is deployed with networkPolicyOnly, noEncap and hybrid mode, antrea-agent will be responsible for the following:

Create tunnel interface antrea-tun0 for cross-cluster traffic
Watch all Pods on the Gateway and set up one rule per Pod in L3Fowarding table as long as the Pod is running on a regular Node instead of the Gateway.
Update container interface's MTU with the tunnel header size deducted.

For #4383
The change is based on PR #4508, please review the top commit, thanks.

Signed-off-by: Lan Luo luola@vmware.com

codecov · 2022-11-23T08:18:57Z

Codecov Report

Merging #4407 (a26a3b8) into main (be49b4a) will decrease coverage by 1.81%.
The diff coverage is 67.04%.

@@            Coverage Diff             @@
##             main    #4407      +/-   ##
==========================================
- Coverage   69.83%   68.03%   -1.81%     
==========================================
  Files         401      404       +3     
  Lines       59529    60278     +749     
==========================================
- Hits        41575    41013     -562     
- Misses      15142    16453    +1311     
  Partials     2812     2812

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.40% <ø> (+0.02%)`	⬆️	Carriedforward from 8641721
integration-tests	`34.29% <5.66%> (-0.16%)`	⬇️
kind-e2e-tests	`38.17% <10.10%> (-9.11%)`	⬇️
unit-tests	`59.95% <66.36%> (+0.19%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
...agent/cniserver/interface_configuration_windows.go	`64.56% <0.00%> (-0.40%)`	⬇️
pkg/agent/openflow/multicluster.go	`85.50% <0.00%> (-9.66%)`	⬇️
pkg/agent/cniserver/server.go	`80.30% <18.18%> (-2.00%)`	⬇️
pkg/agent/openflow/client.go	`87.00% <36.84%> (-1.67%)`	⬇️
...s/multicluster/leader/resourceexport_controller.go	`78.07% <50.00%> (-1.61%)`	⬇️
pkg/agent/multicluster/mc_route_controller.go	`60.00% <71.87%> (+3.87%)`	⬆️
pkg/agent/multicluster/pod_route_controller.go	`72.97% <72.97%> (ø)`
pkg/agent/agent.go	`56.32% <85.71%> (-0.56%)`	⬇️
pkg/agent/proxy/proxier.go	`68.17% <85.71%> (-7.91%)`	⬇️
... and 82 more

pkg/agent/cniserver/interface_configuration_windows.go

multicluster/controllers/multicluster/resourceexport_controller.go

pkg/agent/cniserver/interface_configuration_windows.go

pkg/agent/cniserver/server.go

pkg/agent/cniserver/pod_configuration.go

pkg/agent/multicluster/policy_only_route_controller.go

luolanzone · 2022-12-06T02:34:32Z

Will improve the unit test after cniserver package's UT #4348 is merged

jianjuns · 2022-12-08T18:43:35Z

In commit message:

responsible to do the following things -> responsible for the following

in a general Node -> on a regular Node

to minus the tunnel overhead -> with the tunnel header size deducted.

cmd/antrea-agent/agent.go

cmd/antrea-agent/options.go

multicluster/controllers/multicluster/resourceexport_controller.go

pkg/agent/cniserver/interface_configuration_windows.go

pkg/agent/multicluster/policy_only_route_controller.go

cmd/antrea-agent/options.go

pkg/agent/cniserver/server.go

pkg/agent/multicluster/pod_route_controller.go

pkg/agent/cniserver/server.go

jianjuns · 2023-01-05T04:35:08Z

@luolanzone : my last commit changed the validation logic to ignore Multi-cluster options if the Multicluster feature gate is disabled, to be consistent with other features. You need to rebase and change the validation and agent init code too.

luolanzone · 2023-01-05T04:36:24Z

@jianjuns sure, I will re-base first, thanks for the reminder.

cmd/antrea-agent/options.go

cmd/antrea-agent/agent.go

pkg/agent/agent.go

cmd/antrea-agent/agent.go

pkg/agent/cniserver/pod_configuration.go

pkg/agent/multicluster/pod_route_controller.go

luolanzone · 2023-03-14T09:47:21Z

/test-multicluster-e2e

cmd/antrea-agent/agent.go

tnqn · 2023-03-14T13:26:22Z

cmd/antrea-agent/agent.go

 			mcClient,
-			gwInformer,
-			ciImportInformer,
+			mcInformerFactoryWithOption,


Why change it? I think the previous code that injects gwInformer and ciImportInformer explicitly is better:

It's clear the controller has dependencies on Gateway and ClusterInfoImport.

It follows the principle of least privilege.

The gatewayInformer can be shared with mcPodRouteController.

I was feeling the parameters are too long, and trying to reduce the number. Added them back.

tnqn · 2023-03-14T13:37:30Z

cmd/antrea-agent/agent.go

 			ofClient,
 			ovsBridgeClient,
 			ifaceStore,
 			nodeConfig,
-			mcNamespace,
+			o.config.Multicluster.Namespace,


why it still needs to pass the namespace when the lister only contains objects in this namespace?

cmd/antrea-agent/agent.go

pkg/agent/multicluster/pod_route_controller.go

pkg/agent/openflow/client.go

pkg/agent/openflow/multicluster.go

pkg/agent/openflow/client.go

cmd/antrea-agent/agent.go

tnqn · 2023-03-15T10:52:05Z

pkg/agent/multicluster/mc_route_controller.go

-	}
-
-	if ciImp.Namespace != c.namespace {
+		klog.ErrorS(errors.New("received unexpected object"), "enqueueClusterInfoImport can't process event", "obj", obj)


not resolved

tnqn · 2023-03-15T10:58:35Z

pkg/agent/multicluster/pod_route_controller.go

+
+	podIP := pod.Status.PodIP
+	podName := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}
+	cachedPodIP, exists := c.getPodNameIPCache(podName)


I think we don't need this cache. The Pod update event has the old Pod information. The old Pod IP is already known. A single enqueuePod callback is not suitable here.
For add and delete event, the handler should just enqueue the PodIP if it's not empty and not HostNetwork.
For update event, the handler should check if PodIP or HostIP changes. If yes, it should enqueue old PodIP and new PodIP, or just new PodIP if it doesn't change.

Yeah, the only usage seems to check if flows are installed for any Pod to call UninstallMulticlusterPodFlows() in syncGateway(). Maybe that is not necessary, and we can instead optimize a little to first check if the flow cache is empty or not in UninstallMulticlusterPodFlows() or deleteAllFlows.

deleteFlows called by UninstallMulticlusterPodFlows already has this optimization (do nothing if cache is empty)

pkg/agent/multicluster/pod_route_controller.go

tnqn · 2023-03-15T11:04:49Z

pkg/agent/openflow/client.go

+		delAllFlows = append(delAllFlows, delFlows...)
+		cache.Delete(key)


it should be done after the operation succeeds only, otherwise the cache will be inconsistent if it fails

tnqn · 2023-03-15T11:16:41Z

pkg/agent/openflow/multicluster.go

+	// This generates the flow to forward cross-cluster request packets based
+	// on Pod IP.
+	return L3ForwardingTable.ofTable.BuildFlow(priorityNormal).


I was asking why it must use another rule to process this packet instead of the existing rule installed for in-cluster traffic. I now got it's because the VNI. I guess this should still use higher priority, otherwise packets may hit the other rule which doesn't encapsulate packet.

You are right, updated it to priorityHigh

pkg/agent/multicluster/pod_route_controller.go

jianjuns · 2023-03-15T17:12:06Z

pkg/agent/multicluster/pod_route_controller.go

+
+	podIP := pod.Status.PodIP
+	podName := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}
+	cachedPodIP, exists := c.getPodNameIPCache(podName)


Yeah, the only usage seems to check if flows are installed for any Pod to call UninstallMulticlusterPodFlows() in syncGateway(). Maybe that is not necessary, and we can instead optimize a little to first check if the flow cache is empty or not in UninstallMulticlusterPodFlows() or deleteAllFlows.

jianjuns · 2023-03-15T17:56:40Z

pkg/agent/multicluster/pod_route_controller.go

+	podIP := pod.Status.PodIP
+	podName := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}
+	cachedPodIP, exists := c.getPodNameIPCache(podName)
+	if !exists && (podIP == "" || pod.Spec.HostNetwork) {


As Quan said you should pass both old and new Pod to this func, and check both (exists && !HostNetwork && podIP != "" && nodeIP != ""), and something like:

!isOldValid && !newValid: return

!isOldValid: Add(newPodIP)

!isNewValid: Add(oldPodIP)

if oldPodIP != newPodIP: Add(newPodIP) Add(newPodIP)

if oldNodeIP != newNodeIP: Add(newPodIP)

luolanzone · 2023-03-16T02:14:03Z

/test-multicluster-e2e

pkg/agent/multicluster/pod_route_controller.go

tnqn · 2023-03-16T04:26:28Z

pkg/agent/multicluster/pod_route_controller.go

+
+	podIP := pod.Status.PodIP
+	podName := types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}
+	cachedPodIP, exists := c.getPodNameIPCache(podName)


deleteFlows called by UninstallMulticlusterPodFlows already has this optimization (do nothing if cache is empty)

tnqn · 2023-03-16T06:53:06Z

pkg/agent/multicluster/pod_route_controller.go

+	mcClient        mcclientset.Interface
+	ovsBridgeClient ovsconfig.OVSBridgeClient
+	ofClient        openflow.Client
+	interfaceStore  interfacestore.InterfaceStore
+	nodeConfig      *config.NodeConfig
+	podQueue        workqueue.RateLimitingInterface
+	gwQueue         workqueue.RateLimitingInterface
+	podInformer     cache.SharedIndexInformer
+	podLister       corelisters.PodLister
+	gwInformer      cache.SharedIndexInformer
+	gwLister        mclisters.GatewayLister
+	// podWorkersStarted is a boolean which tracks if the Pod flow controller has been started.
+	podWorkersStarted      bool
+	podWorkersStartedMutex sync.RWMutex
+	podWorkerStopCh        chan struct{}


There are a few attributes never used, please clean them up and remove from the constructor.

Removed, thanks.

luolanzone · 2023-03-16T07:04:20Z

/test-multicluster-e2e

tnqn · 2023-03-16T13:59:28Z

pkg/agent/multicluster/pod_route_controller.go

+// traffic will not go through tunnels in those modes.
+type MCPodRouteController struct {
+	k8sClient   kubernetes.Interface
+	mcClient    mcclientset.Interface


this is not used either.

tnqn · 2023-03-16T14:18:41Z

/test-all

jianjuns

LGTM

In order to support multi-cluster traffic when the member cluster is deployed with networkPolicyOnly, noEcap or hybrid mode, antrea-agent will be responsible for the following things: 1. Create tunnel interface `antrea-tun0` for cross-cluster traffic 2. Watch all Pods on the Gateway and set up one rule per Pod in L3Fowarding table as long as the Pod is running on a regular Node instead of the Gateway. 3. Update container interface's MTU with the tunnel header size deducted. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone · 2023-03-17T01:04:18Z

/test-all
/test-multicluster-e2e

tnqn

LGTM

In order to support multi-cluster traffic when the member cluster is deployed with networkPolicyOnly, noEcap or hybrid mode, antrea-agent will be responsible for the following things: 1. Create tunnel interface `antrea-tun0` for cross-cluster traffic 2. Watch all Pods on the Gateway and set up one rule per Pod in L3Fowarding table as long as the Pod is running on a regular Node instead of the Gateway. 3. Update container interface's MTU with the tunnel header size deducted. Signed-off-by: Lan Luo <luola@vmware.com>

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Nov 23, 2022

luolanzone force-pushed the mc-policyonly-routes branch from cbf4317 to 21e451b Compare November 23, 2022 07:43

luolanzone force-pushed the mc-policyonly-routes branch 2 times, most recently from ac9547d to 810bdd6 Compare November 25, 2022 07:40

luolanzone commented Nov 28, 2022

View reviewed changes

pkg/agent/cniserver/interface_configuration_windows.go Outdated Show resolved Hide resolved

luolanzone force-pushed the mc-policyonly-routes branch from 810bdd6 to 120d9e7 Compare November 28, 2022 04:21

wenyingd reviewed Nov 28, 2022

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch 3 times, most recently from d7d6c06 to 08eae28 Compare November 30, 2022 00:53

luolanzone force-pushed the mc-policyonly-routes branch from 08eae28 to b40fd4c Compare December 6, 2022 06:32

luolanzone requested review from jianjuns and tnqn December 8, 2022 06:07

jianjuns reviewed Dec 8, 2022

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch 3 times, most recently from 21ebeda to 234b02e Compare December 12, 2022 09:07

luolanzone changed the title ~~Multi-cluster support with networkPolicyOnly mode~~ Multi-cluster support with more modes Dec 12, 2022

jianjuns reviewed Dec 13, 2022

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch 2 times, most recently from d3e1729 to 6e38b7e Compare December 20, 2022 05:12

luolanzone force-pushed the mc-policyonly-routes branch 2 times, most recently from ed50fad to c0916d9 Compare January 4, 2023 03:14

luolanzone force-pushed the mc-policyonly-routes branch from c0916d9 to b4a2186 Compare January 6, 2023 07:18

jianjuns reviewed Jan 9, 2023

View reviewed changes

jianjuns reviewed Mar 13, 2023

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch from a00a36b to ff78c5b Compare March 14, 2023 09:43

luolanzone force-pushed the mc-policyonly-routes branch from ff78c5b to f15ba00 Compare March 14, 2023 14:18

tnqn reviewed Mar 14, 2023

View reviewed changes

pkg/agent/openflow/client.go Show resolved Hide resolved

luolanzone force-pushed the mc-policyonly-routes branch 2 times, most recently from 1c2a591 to f215a36 Compare March 15, 2023 06:53

tnqn reviewed Mar 15, 2023

View reviewed changes

jianjuns reviewed Mar 15, 2023

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch from f215a36 to 4e43cba Compare March 16, 2023 02:11

jianjuns reviewed Mar 16, 2023

View reviewed changes

pkg/agent/multicluster/pod_route_controller.go Outdated Show resolved Hide resolved

pkg/agent/multicluster/pod_route_controller.go Outdated Show resolved Hide resolved

luolanzone force-pushed the mc-policyonly-routes branch 2 times, most recently from 4e14b49 to ccf17e0 Compare March 16, 2023 03:51

tnqn reviewed Mar 16, 2023

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch from ccf17e0 to a71b642 Compare March 16, 2023 07:03

luolanzone force-pushed the mc-policyonly-routes branch from a71b642 to be7e924 Compare March 16, 2023 07:29

tnqn reviewed Mar 16, 2023

View reviewed changes

jianjuns reviewed Mar 16, 2023

View reviewed changes

luolanzone force-pushed the mc-policyonly-routes branch from be7e924 to cb53264 Compare March 17, 2023 01:03

tnqn approved these changes Mar 17, 2023

View reviewed changes

tnqn merged commit 0391f4d into antrea-io:main Mar 17, 2023

luolanzone mentioned this pull request Mar 17, 2023

Support multi-cluster with networkPolicyOnly mode #4383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support more traffic modes for Multi-cluster Gateway #4407

Support more traffic modes for Multi-cluster Gateway #4407

luolanzone commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading

luolanzone commented Dec 6, 2022

jianjuns commented Dec 8, 2022

jianjuns commented Jan 5, 2023

luolanzone commented Jan 5, 2023

luolanzone commented Mar 14, 2023

tnqn Mar 14, 2023

luolanzone Mar 15, 2023

tnqn Mar 14, 2023

luolanzone Mar 15, 2023

tnqn Mar 15, 2023

tnqn Mar 15, 2023

jianjuns Mar 15, 2023

tnqn Mar 16, 2023

tnqn Mar 15, 2023

luolanzone Mar 16, 2023

tnqn Mar 15, 2023

luolanzone Mar 16, 2023

jianjuns Mar 15, 2023

jianjuns Mar 15, 2023

luolanzone Mar 16, 2023

luolanzone commented Mar 16, 2023

tnqn Mar 16, 2023

tnqn Mar 16, 2023

luolanzone Mar 16, 2023

luolanzone commented Mar 16, 2023

tnqn Mar 16, 2023

luolanzone Mar 17, 2023

tnqn commented Mar 16, 2023

jianjuns left a comment

luolanzone commented Mar 17, 2023

tnqn left a comment

		delAllFlows = append(delAllFlows, delFlows...)
		cache.Delete(key)

Support more traffic modes for Multi-cluster Gateway #4407

Support more traffic modes for Multi-cluster Gateway #4407

Conversation

luolanzone commented Nov 23, 2022 • edited Loading

codecov bot commented Nov 23, 2022 • edited Loading

Codecov Report

luolanzone commented Dec 6, 2022

jianjuns commented Dec 8, 2022

jianjuns commented Jan 5, 2023

luolanzone commented Jan 5, 2023

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

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 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Mar 16, 2023

jianjuns left a comment

Choose a reason for hiding this comment

luolanzone commented Mar 17, 2023

tnqn left a comment

Choose a reason for hiding this comment

luolanzone commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading