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

Rename Agent config option multicluster.enable to enableGateway #4533

Merged
merged 1 commit into from Jan 5, 2023

Conversation

jianjuns
Copy link
Contributor

@jianjuns jianjuns commented Jan 4, 2023

To be compatible with earlier version deployment manifests, set multicluster.enableGateway to true if multicluster.enable is set to true.
Also change the validation logic to ignore Multi-cluster options if the Multicluster feature gate is disabled, to be consistent with other features.

Signed-off-by: Jianjun Shen shenj@vmware.com

@jianjuns jianjuns added the area/multi-cluster Issues or PRs related to multi cluster. label Jan 4, 2023
return nil
}

if !features.DefaultFeatureGate.Enabled(features.Multicluster) {
return fmt.Errorf("Multicluster can only be enabled when Multicluster feature gate is enabled")
klog.InfoS("Multicluster feature gate is disabled. Multi-cluster options are ignored")
Copy link
Contributor Author

@jianjuns jianjuns Jan 4, 2023

Choose a reason for hiding this comment

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

@GraysonWu : I found other features (e.g. NodePortLocal) ignore config options when the feature gate is not enabled, which makes sense to me, and so made this change.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #4533 (d8782c5) into main (00f59cd) will increase coverage by 1.40%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4533      +/-   ##
==========================================
+ Coverage   66.80%   68.20%   +1.40%     
==========================================
  Files         404      412       +8     
  Lines       58231    58204      -27     
==========================================
+ Hits        38899    39698     +799     
+ Misses      16552    15722     -830     
- Partials     2780     2784       +4     
Flag Coverage Δ
e2e-tests 61.93% <ø> (?)
integration-tests 34.49% <ø> (-0.07%) ⬇️
kind-e2e-tests 47.03% <ø> (+6.51%) ⬆️
unit-tests 56.96% <30.76%> (+0.15%) ⬆️
Impacted Files Coverage Δ
cmd/antrea-agent/agent.go 0.00% <0.00%> (ø)
cmd/antrea-controller/controller.go 0.00% <0.00%> (ø)
cmd/antrea-controller/options.go 0.00% <0.00%> (ø)
cmd/antrea-agent/options.go 36.47% <100.00%> (+13.26%) ⬆️
pkg/agent/flowexporter/exporter/certificate.go 50.00% <0.00%> (-16.67%) ⬇️
pkg/agent/flowexporter/exporter/exporter.go 71.53% <0.00%> (-7.68%) ⬇️
pkg/controller/labelidentity/controller.go 78.00% <0.00%> (-6.00%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 75.25% <0.00%> (-3.02%) ⬇️
pkg/ipam/ipallocator/allocator.go 88.14% <0.00%> (-1.55%) ⬇️
...kg/apiserver/registry/system/supportbundle/rest.go 83.24% <0.00%> (-1.53%) ⬇️
... and 84 more

luolanzone
luolanzone previously approved these changes Jan 4, 2023
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@luolanzone
Copy link
Contributor

/test-multicluster-e2e

@@ -333,12 +332,13 @@ multicluster:
{{- with .Values.multicluster }}
# Enable Antrea Multi-cluster Gateway to support cross-cluster traffic.
# This feature is supported only with encap mode.
enable: {{ .enable }}
enableGateway: {{ .enableGateway }}
Copy link
Contributor

Choose a reason for hiding this comment

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

The config in test-mc.sh need to be updated as well.
https://github.com/antrea-io/antrea/blob/main/ci/jenkins/test-mc.sh#L255

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated test-mc.sh. Thanks!

To be compatible with earlier version deployment manifests, set
multicluster.enableGateway to true if multicluster.enable is set to
true.
Also change the validation logic to ingore Multi-cluster options if
the Multicluster feature gate is disabled, to be consistent with other
features.

Signed-off-by: Jianjun Shen <shenj@vmware.com>
@jianjuns
Copy link
Contributor Author

jianjuns commented Jan 4, 2023

/test-multicluster-e2e

Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@jianjuns
Copy link
Contributor Author

jianjuns commented Jan 5, 2023

/skip-all

@jianjuns jianjuns merged commit 7e055c6 into antrea-io:main Jan 5, 2023
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Jan 27, 2023
…ea-io#4533)

To be compatible with earlier version deployment manifests, set
multicluster.enableGateway to true if multicluster.enable is set to
true.
Also change the validation logic to ignore Multi-cluster options if
the Multicluster feature gate is disabled, to be consistent with other
features.

Signed-off-by: Jianjun Shen <shenj@vmware.com>
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.

None yet

2 participants