Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a toggle for Multi-cluster Pod-to-Pod connectivity #4605

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

hjiajing
Copy link
Contributor

@hjiajing hjiajing commented Feb 7, 2023

Add a toggle for Multi-cluster Pod-to-Pod connectivity in antrea-agent configuration. The Pod-to-Pod connectivity feature will be enabled when the toggle is set true and Pod CIDRs are provided in the Antrea Multi-cluster configuration.

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Merging #4605 (4b57014) into main (9df5a81) will increase coverage by 1.03%.
The diff coverage is 90.00%.

❗ Current head 4b57014 differs from pull request most recent head 7cf1a3f. Consider uploading reports for the commit 7cf1a3f to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4605      +/-   ##
==========================================
+ Coverage   68.67%   69.70%   +1.03%     
==========================================
  Files         400      412      +12     
  Lines       59450    58332    -1118     
==========================================
- Hits        40828    40662     -166     
+ Misses      15804    14893     -911     
+ Partials     2818     2777      -41     
Flag Coverage Δ *Carryforward flag
e2e-tests 62.01% <ø> (+23.67%) ⬆️
integration-tests 34.61% <ø> (+0.08%) ⬆️ Carriedforward from 7cf1a3f
kind-e2e-tests 47.42% <0.00%> (+6.43%) ⬆️ Carriedforward from 7cf1a3f
unit-tests 59.74% <90.00%> (-0.03%) ⬇️ Carriedforward from 7cf1a3f

*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%> (ø)
pkg/agent/util/net.go 58.17% <ø> (+10.06%) ⬆️
pkg/apiserver/handlers/webhook/convert_crd.go 3.97% <ø> (+0.05%) ⬆️
pkg/ovs/ovsconfig/default_windows.go 100.00% <ø> (ø)
pkg/ovs/ovsconfig/ovs_client.go 64.64% <ø> (-2.07%) ⬇️
pkg/ovs/ovsctl/ovsctl_others.go 52.63% <ø> (-1.22%) ⬇️
pkg/agent/multicluster/mc_route_controller.go 56.31% <100.00%> (+0.75%) ⬆️
...g/agent/apiserver/handlers/addressgroup/handler.go 5.00% <0.00%> (-35.00%) ⬇️
...gent/controller/noderoute/node_route_controller.go 56.79% <0.00%> (-11.65%) ⬇️
pkg/agent/controller/egress/egress_controller.go 75.27% <0.00%> (-9.96%) ⬇️
... and 93 more

@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Feb 7, 2023
@luolanzone
Copy link
Contributor

@hjiajing please fix github check failure.

@hjiajing
Copy link
Contributor Author

hjiajing commented Feb 7, 2023

@luolanzone Thanks for the reminder. The Go / Verify docs and spelling failed because I didn't generate a new helm chart README.md. It works well now.

I think another PR will face the same problem, I'll fix it, too.

ConfigMap `antrea-mc-controller-config` of each member cluster like the example
below. Note, **Pod CIDRs must not overlap among clusters to enable cross-cluster
within a ClusterSet. To enable this feature, the cluster's Pod CIDRs must be set
in ConfigMap `antrea-mc-controller-config` of each member cluster and make sure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably use passive voice for both to consistent: "and multicluster.enablePodToPodConnectivity must be set to true in the `antrea-agent' configuration"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to passive voice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should remove "make sure" too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

docs/multicluster/user-guide.md Outdated Show resolved Hide resolved
build/charts/antrea/README.md Outdated Show resolved Hide resolved
pkg/agent/multicluster/mc_route_controller.go Show resolved Hide resolved
below. Note, **Pod CIDRs must not overlap among clusters to enable cross-cluster
within a ClusterSet. To enable this feature, the cluster's Pod CIDRs must be set
in ConfigMap `antrea-mc-controller-config` of each member cluster and make sure
that `enablePodToPodConnectivity` is set to `true` in antrea-agent ConfigMap.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
that `enablePodToPodConnectivity` is set to `true` in antrea-agent ConfigMap.
that `enablePodToPodConnectivity` is set to `true` in ConfigMap `antrea-config`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use "in the antrea-agent configuration". I know we usually say ConfigMap for MC Controller configuration in this doc, but I think we usually do not say Antrea ConfigMap for agent/controller configuration in other docs. Technically, we can pass configuration to controllers/agent without a ConfigMap.

If we still want to use "ConfigMap", then we should at least say "ConfigMap in the Antrea deployment manifest" to be clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment on lines 538 to 544
antrea-controller.conf: |
featureGates:
...
Multicluster: true
...
multicluster:
enablePodToPodConnectivity: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format is incorrect, please add necessary empty spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I double-checked then found that once I push code, the IDE will format the Markdown file automatically with a wrong format. I enabled the feature now.

pkg/config/agent/config.go Outdated Show resolved Hide resolved
ConfigMap `antrea-mc-controller-config` of each member cluster like the example
below. Note, **Pod CIDRs must not overlap among clusters to enable cross-cluster
within a ClusterSet. To enable this feature, the cluster's Pod CIDRs must be set
in ConfigMap `antrea-mc-controller-config` of each member cluster and make sure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should remove "make sure" too.

below. Note, **Pod CIDRs must not overlap among clusters to enable cross-cluster
within a ClusterSet. To enable this feature, the cluster's Pod CIDRs must be set
in ConfigMap `antrea-mc-controller-config` of each member cluster and make sure
that `enablePodToPodConnectivity` is set to `true` in antrea-agent ConfigMap.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use "in the antrea-agent configuration". I know we usually say ConfigMap for MC Controller configuration in this doc, but I think we usually do not say Antrea ConfigMap for agent/controller configuration in other docs. Technically, we can pass configuration to controllers/agent without a ConfigMap.

If we still want to use "ConfigMap", then we should at least say "ConfigMap in the Antrea deployment manifest" to be clear.

@@ -292,6 +292,9 @@ type MulticlusterConfig struct {
// Enable Multi-cluster NetworkPolicy which allows Antrea-native policy ingress rules to select peers
// from all clusters in a ClusterSet.
EnableStretchedNetworkPolicy bool `yaml:"enableStretchedNetworkPolicy,omitempty"`
// Enable Multi-cluster Pod to Pod Connectivity which allows one Pod access another Pod in other member
// clusters directly. It also requires the Pod CIDRs in Multi-cluster configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This feature also requires Pod CIDRs to be provided in the Multi-cluster Controller configuration"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

jianjuns
jianjuns previously approved these changes Feb 9, 2023
@luolanzone
Copy link
Contributor

@hjiajing please help to resolve the manifest conflicts. thanks.

below. Note, **Pod CIDRs must not overlap among clusters to enable cross-cluster
within a ClusterSet. To enable this feature, the cluster's Pod CIDRs must be set
in ConfigMap `antrea-mc-controller-config` of each member cluster and
`multicluster.enablePodToPodConnectivity` must be set to `true` in the `antrea-agent`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we should highlight this toggle is introduced from v1.11? @jianjuns any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is not important. But if we want, no harm to add the information either.

@hjiajing
Copy link
Contributor Author

@luolanzone Thanks for the reminder, regenerated the manifest and resolved the conflict now.

@jianjuns
Copy link
Contributor

/test-e2e
/test-networkpolicy

@jianjuns
Copy link
Contributor

@hjiajing : some tests have been pending for long, and some (e.g. multicluster-e2e) are not shown on the PR. Please re-push the commit, and make sure the required tests are passed.

Signed-off-by: hujiajing <hjiajing@vmware.com>
@hjiajing
Copy link
Contributor Author

@jianjuns Thanks for the reminder, pe-pushed now.

@hjiajing
Copy link
Contributor Author

/test-e2e
/test-networkpolicy

@jianjuns
Copy link
Contributor

/test-all

@jianjuns
Copy link
Contributor

/test-e2e
/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/test-multicluster-e2e

@jianjuns jianjuns merged commit cf90cfa into antrea-io:main Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants