Support Pod to Pod connectivity in Antrea Multi-cluster #4219

hjiajing · 2022-09-09T01:43:48Z

Add a new field PodCIDRs in the ClusterInfo for a member cluster to export its Pod CIDRs to other clusters. antrea-agent adds flows to route traffic to remote cluster Pods through Gateway Nodes.

Signed-off-by: hujiajing [email protected]

codecov · 2022-09-13T07:59:05Z

Codecov Report

Merging #4219 (c588bdd) into main (0f77529) will increase coverage by 0.32%.
The diff coverage is 55.17%.

❗ Current head c588bdd differs from pull request most recent head 3d85046. Consider uploading reports for the commit 3d85046 to get more accurate results

@@            Coverage Diff             @@
##             main    #4219      +/-   ##
==========================================
+ Coverage   62.26%   62.58%   +0.32%     
==========================================
  Files         385      385              
  Lines       54501    55017     +516     
==========================================
+ Hits        33933    34433     +500     
- Misses      18069    18073       +4     
- Partials     2499     2511      +12

Flag	Coverage Δ	*Carryforward flag
e2e-tests	`39.63% <47.36%> (?)`
integration-tests	`34.84% <24.63%> (ø)`	Carriedforward from 0f77529
kind-e2e-tests	`48.16% <20.58%> (ø)`	Carriedforward from 0f77529
unit-tests	`43.86% <21.95%> (ø)`	Carriedforward from 0f77529

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

Impacted Files	Coverage Δ
multicluster/cmd/multicluster-controller/member.go	`0.00% <0.00%> (ø)`
...ulticluster/cmd/multicluster-controller/options.go	`9.30% <0.00%> (ø)`
pkg/ovs/openflow/ofctrl_meter.go	`44.77% <0.00%> (ø)`
pkg/agent/openflow/multicast.go	`18.60% <13.33%> (-2.59%)`	⬇️
pkg/agent/openflow/service.go	`91.57% <40.00%> (+0.27%)`	⬆️
pkg/ovs/openflow/ofctrl_bridge.go	`78.18% <47.36%> (+2.06%)`	⬆️
pkg/agent/openflow/client.go	`73.02% <69.23%> (+1.16%)`	⬆️
pkg/ovs/openflow/ofctrl_group.go	`74.07% <72.72%> (+9.43%)`	⬆️
...ter/controllers/multicluster/gateway_controller.go	`62.40% <75.00%> (ø)`
pkg/agent/multicluster/mc_route_controller.go	`50.18% <100.00%> (-2.10%)`	⬇️
... and 16 more

luolanzone · 2022-09-14T01:37:32Z

Hi @hjiajing your DCO info is missing, and the unit test is failed, please fix them.
The patch test coverage is lower than target, could you check if possible to improve it. thanks.

multicluster/controllers/multicluster/node_controller.go

multicluster/controllers/multicluster/gateway_controller.go

luolanzone · 2022-09-14T02:05:12Z

multicluster/controllers/multicluster/gateway_controller.go

+		return err
+	}
+	for _, node := range nodeList.Items {
+		podCIDRs = append(podCIDRs, node.Spec.PodCIDRs...)


Could you check if this can handle empty PodCIDRs properly?

luolanzone · 2022-09-14T02:11:04Z

multicluster/controllers/multicluster/node_controller.go

@@ -142,7 +143,17 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 			return ctrl.Result{}, err
 		}
 	}
-	return ctrl.Result{}, nil
+
+	// When a Node is added to a member cluster or removed from a member cluster.


I feel this comment should be updated, this block also handles Node update case.
Can we compare existing CIDRs so we can skip refresh Gateway unnecessarily?

The reason I used node list rather than other annotations such as "lastUpdateTime" is that we will compare the node list to skip the unnecessary refresh in the updateActiveGateway method.

If we need to compare the CIDRs, maybe we need to set the store the CIDRs in the Gateway.Spec or Annotations. I'm not sure if it's necessary since we store it in the ClusterInfo.

Do you guys think we can just ask users to provide the PodCIDRs? I want not to add much complexity for auto-discovering Pod CIDRs? @luolanzone

Yes, I think this is also workable. Maybe add a config item clusterCIDRs the same as Antrea agent?

Yeah, maybe still podCIDRs for easier understanding.

@luolanzone @jianjuns Maybe we can add a new field PodCIDRs in struct options inmulticluster/cmd/multicluster-controller/options.go. And the user must edit the antrea-multicluster-member.yml to specify the CIDR.

The CIDR must be the cluster Pod CIDRs or a superset of it, should we add a check about it?

I think no need to check. But wonder if we should check no overlapping among clusters. At least we should check in each cluster that remote cluster CIDRs cannot overlap with the local cluster, and reject that import if it does overlap. We can do a separate PR for that.

@hjiajing Yes, I think a new option field is required for PodCIDRs, and you should also add a config item here https://github.com/antrea-io/antrea/blob/main/multicluster/apis/multicluster/v1alpha1/multiclusterconfig_types.go#L38.

@luolanzone Thanks for the reminder. The item PodCIDRs was added to MulticlusterConfig.

hjiajing · 2022-09-14T03:15:23Z

Hi @hjiajing your DCO info is missing, and the unit test is failed, please fix them. The patch test coverage is lower than target, could you check if possible to improve it. thanks.

Sure. I'll fix it. Thanks

multicluster/apis/multicluster/v1alpha1/gateway_types.go

multicluster/cmd/multicluster-controller/options.go

multicluster/controllers/multicluster/gateway_controller.go

jianjuns · 2022-09-22T20:54:58Z

LGTM

luolanzone · 2022-09-23T07:04:40Z

pkg/agent/multicluster/mc_route_controller.go

 			klog.V(2).InfoS("No difference between new and installed ClusterInfoImports, skip updating", "clusterinfoimport", ciImport.Name)
 			return nil
 		}
 	}

 	klog.InfoS("Adding/updating remote Gateway Node flows for Multi-cluster", "gateway", klog.KObj(activeGW),
 		"node", c.nodeConfig.Name, "peer", tunnelPeerIPToRemoteGW)
-	allCIDRs := []string{ciImport.Spec.ServiceCIDR}
+	allCIDRs := append([]string{ciImport.Spec.ServiceCIDR}, ciImport.Spec.PodCIDRs...)


If you add all Pod CIDRs here like Service CIDR, I think it will also add a SNAT flow to perform SNAT for Pod-to-Pod connectivity, I feel it would be OK to keep the behavior consistent, but seems we didn't discuss this before. @jianjuns it this expected to do SNAT for Pod to Pod access?

I feel we can do SNAT for now. @luolanzone Maybe you can also reach to Tao to understand his requirements?

sure, @hjiajing let's do SNAT for 1.9, I will check with Tao for the details.

multicluster/cmd/multicluster-controller/options.go

luolanzone

LGTM

jianjuns · 2022-09-26T18:40:58Z

/test-all

jianjuns · 2022-09-26T18:41:15Z

/test-multicluster-e2e

jianjuns · 2022-09-26T22:54:17Z

@hjiajing @luolanzone : this PR did not reach the UT coverage target. Could you check any way to improve?

hjiajing · 2022-09-26T23:44:36Z

@jianjuns Sure. I'll check and improve the unit test coverage.

hjiajing · 2022-09-27T01:31:49Z

@jianjuns I added some UT for the options.go. The coverage increased from 3% to 75% when I tested it locally. But the codecov report shows that it decreased by 6% and there are many files I didn't edit. I have reported this to Wenqi (He said he noticed it too)

multicluster/cmd/multicluster-controller/testdata/antrea-mc-config-with-empty-podcidrs.yml

multicluster/cmd/multicluster-controller/testdata/antrea-mc-config-with-invalid-podcidrs.yml

multicluster/cmd/multicluster-controller/testdata/antrea-mc-config-with-valid-podcidrs.yml

multicluster/cmd/multicluster-controller/options_test.go

hjiajing · 2022-09-27T05:39:19Z

@jianjuns I added some UT for the options.go. The coverage increased from 3% to 75% when I tested it locally. But the codecov report shows that it decreased by 6% and there are many files I didn't edit. I have reported this to Wenqi (He said he noticed it too)

@jianjuns Discussed with Wenqi. The total coverage decreased because some tests was still running, and some tests wasn't triggered. After more unit tests added, the unit test coverage of this PR is 93% reached the target now (70%)

wenqiq · 2022-09-27T05:43:48Z

@jianjuns I added some UT for the options.go. The coverage increased from 3% to 75% when I tested it locally. But the codecov report shows that it decreased by 6% and there are many files I didn't edit. I have reported this to Wenqi (He said he noticed it too)

**decov/patch ** — 93.75% of diff hit (target 70.00%)

Hi, @hjiajing After checking the Codecov links. I think you should just keep an eye on this patch/status data. And 93.75% is a good coverage rate.
The decreased coverage you see just now is caused by integration or e2e test cases that not ran completely thus the results are not uploaded to the Codecov.

jianjuns · 2022-09-27T17:54:03Z

Yes, the coverage rate is good now.

@luolanzone : do you have extra comments?

jianjuns · 2022-09-27T17:54:20Z

/test-all

luolanzone · 2022-09-28T14:27:24Z

@jianjuns no more comments from me. thanks.

luolanzone · 2022-09-28T14:28:51Z

Seems there are some problems with github checks. re-trigger test-all to see if it helps.
/test-all

luolanzone

LGTM

hjiajing · 2022-09-29T01:16:35Z

/test-all

hjiajing · 2022-09-29T01:24:26Z

/test-all

hjiajing · 2022-09-29T07:03:27Z

/test-all

Signed-off-by: hujiajing <[email protected]>

Each member cluster exports cluster PodCIDRs with ClusterInfo. antrea-agent programs flows to route traffic to remote cluster Pods through the tunnels between Gateway Nodes. Signed-off-by: hujiajing <[email protected]>

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

luolanzone added this to the Antrea v1.9 release milestone Sep 9, 2022

hjiajing force-pushed the pod-pod branch from 47ea1e0 to b3ddd9d Compare September 13, 2022 07:44

hjiajing force-pushed the pod-pod branch 2 times, most recently from 5b336c6 to 0c694e9 Compare September 13, 2022 12:18

luolanzone reviewed Sep 14, 2022

View reviewed changes

luolanzone mentioned this pull request Sep 14, 2022

Allow Pod IP as MC Service Endpoints #4198

Merged

hjiajing force-pushed the pod-pod branch 2 times, most recently from bb5f668 to 51f0332 Compare September 19, 2022 01:09

jianjuns changed the title ~~Multicluster Pod to Pod connectivity~~ Support Pod to Pod connectivity in Antrea Multi-cluster Sep 20, 2022

jianjuns reviewed Sep 20, 2022

View reviewed changes

multicluster/apis/multicluster/v1alpha1/gateway_types.go Outdated Show resolved Hide resolved

hjiajing force-pushed the pod-pod branch 2 times, most recently from 39f7227 to cedc9ee Compare September 21, 2022 15:28

jianjuns reviewed Sep 22, 2022

View reviewed changes

multicluster/cmd/multicluster-controller/options.go Outdated Show resolved Hide resolved

multicluster/controllers/multicluster/gateway_controller.go Show resolved Hide resolved

hjiajing force-pushed the pod-pod branch 2 times, most recently from 5e15dd8 to dfd766e Compare September 22, 2022 16:22

luolanzone reviewed Sep 23, 2022

View reviewed changes

multicluster/cmd/multicluster-controller/options.go Outdated Show resolved Hide resolved

hjiajing force-pushed the pod-pod branch from dfd766e to bc9e290 Compare September 23, 2022 12:47

luolanzone previously approved these changes Sep 26, 2022

View reviewed changes

hjiajing dismissed luolanzone’s stale review via 552e8c8 September 27, 2022 00:12

hjiajing force-pushed the pod-pod branch from bc9e290 to 552e8c8 Compare September 27, 2022 00:12

hjiajing force-pushed the pod-pod branch from 552e8c8 to 2384d6a Compare September 27, 2022 01:11

luolanzone reviewed Sep 27, 2022

View reviewed changes

hjiajing force-pushed the pod-pod branch from 2384d6a to 24d2c07 Compare September 27, 2022 03:18

luolanzone previously approved these changes Sep 28, 2022

View reviewed changes

Support Pod to Pod connectivity

3d85046

Signed-off-by: hujiajing <[email protected]>

hjiajing dismissed luolanzone’s stale review via 3d85046 September 29, 2022 07:18

hjiajing force-pushed the pod-pod branch from 24d2c07 to 3d85046 Compare September 29, 2022 07:18

jianjuns approved these changes Sep 29, 2022

View reviewed changes

jianjuns merged commit c7c70e6 into antrea-io:main Oct 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Pod to Pod connectivity in Antrea Multi-cluster #4219

Support Pod to Pod connectivity in Antrea Multi-cluster #4219

hjiajing commented Sep 9, 2022 •

edited by jianjuns

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

luolanzone commented Sep 14, 2022

luolanzone Sep 14, 2022

luolanzone Sep 14, 2022

hjiajing Sep 19, 2022

jianjuns Sep 20, 2022

luolanzone Sep 20, 2022

jianjuns Sep 20, 2022 •

edited

Loading

hjiajing Sep 21, 2022

jianjuns Sep 22, 2022

luolanzone Sep 22, 2022

hjiajing Sep 22, 2022

hjiajing commented Sep 14, 2022

jianjuns commented Sep 22, 2022

luolanzone Sep 23, 2022

jianjuns Sep 23, 2022

luolanzone Sep 26, 2022

luolanzone left a comment

jianjuns commented Sep 26, 2022

jianjuns commented Sep 26, 2022

jianjuns commented Sep 26, 2022

hjiajing commented Sep 26, 2022

hjiajing commented Sep 27, 2022

hjiajing commented Sep 27, 2022

wenqiq commented Sep 27, 2022

jianjuns commented Sep 27, 2022

jianjuns commented Sep 27, 2022

luolanzone commented Sep 28, 2022

luolanzone commented Sep 28, 2022 •

edited

Loading

luolanzone left a comment

hjiajing commented Sep 29, 2022

hjiajing commented Sep 29, 2022

hjiajing commented Sep 29, 2022

Support Pod to Pod connectivity in Antrea Multi-cluster #4219

Support Pod to Pod connectivity in Antrea Multi-cluster #4219

Conversation

hjiajing commented Sep 9, 2022 • edited by jianjuns Loading

codecov bot commented Sep 13, 2022 • edited Loading

Codecov Report

luolanzone commented Sep 14, 2022

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

jianjuns Sep 20, 2022 • edited Loading

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

hjiajing commented Sep 14, 2022

jianjuns commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

jianjuns commented Sep 26, 2022

jianjuns commented Sep 26, 2022

jianjuns commented Sep 26, 2022

hjiajing commented Sep 26, 2022

hjiajing commented Sep 27, 2022

hjiajing commented Sep 27, 2022

wenqiq commented Sep 27, 2022

jianjuns commented Sep 27, 2022

jianjuns commented Sep 27, 2022

luolanzone commented Sep 28, 2022

luolanzone commented Sep 28, 2022 • edited Loading

luolanzone left a comment

Choose a reason for hiding this comment

hjiajing commented Sep 29, 2022

hjiajing commented Sep 29, 2022

hjiajing commented Sep 29, 2022

hjiajing commented Sep 9, 2022 •

edited by jianjuns

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

jianjuns Sep 20, 2022 •

edited

Loading

luolanzone commented Sep 28, 2022 •

edited

Loading