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

Set default feature gates of unsupported features to false on Windows #3527

Merged
merged 1 commit into from
Mar 31, 2022

Conversation

jianjuns
Copy link
Contributor

A feature might be enabled by default on Linux, but is not supported on
Windows, so set the default feature gates of such features to false on
Windows.

Signed-off-by: Jianjun Shen [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2022

Codecov Report

Merging #3527 (58c77b3) into main (b9e37a8) will decrease coverage by 3.77%.
The diff coverage is 10.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3527      +/-   ##
==========================================
- Coverage   64.17%   60.39%   -3.78%     
==========================================
  Files         278      278              
  Lines       27825    38376   +10551     
==========================================
+ Hits        17856    23177    +5321     
- Misses       8048    13183    +5135     
- Partials     1921     2016      +95     
Flag Coverage Δ
e2e-tests 49.36% <10.00%> (?)
kind-e2e-tests 51.17% <16.66%> (-0.06%) ⬇️
unit-tests 43.36% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/features/antrea_features.go 11.11% <10.00%> (-5.56%) ⬇️
pkg/agent/cniserver/pod_configuration_linux.go 26.31% <0.00%> (-40.36%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 43.87% <0.00%> (-30.27%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 61.46% <0.00%> (-29.97%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go 57.24% <0.00%> (-25.74%) ⬇️
pkg/util/sets/string.go 75.00% <0.00%> (-25.00%) ⬇️
pkg/controller/ipam/validate.go 57.95% <0.00%> (-24.31%) ⬇️
pkg/controller/networkpolicy/clustergroup.go 54.14% <0.00%> (-24.17%) ⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go 56.66% <0.00%> (-23.34%) ⬇️
pkg/agent/util/iptables/lock.go 60.00% <0.00%> (-21.82%) ⬇️
... and 239 more

A feature might be enabled by default on Linux, but is not supported on
Windows, so set the default feature gates of such features to false on
Windows.

Signed-off-by: Jianjun Shen <[email protected]>
@hongliangl
Copy link
Contributor

hongliangl commented Mar 29, 2022

Maybe in NetworkPolicyOnly mode, feature gate Egress should be also false anyway.

@jianjuns
Copy link
Contributor Author

jianjuns commented Mar 29, 2022

Maybe in NetworkPolicyOnly mode, feature gate Egress should be also false anyway.

Feature gate should be for code isolation, but not for configuring a feature (e.g. when enabling the feature has overhead or leads to unexpected behavior in some configuration). So, we should not auto-disable feature gates based on configuration in my mind. If we believe enabling Egress with NetworkPolicyOnly mode will result in some unexpected issues, we should do internal checks to avoid such issues. @tnqn

@jianjuns jianjuns requested a review from tnqn March 30, 2022 17:17
@tnqn
Copy link
Member

tnqn commented Mar 31, 2022

Maybe in NetworkPolicyOnly mode, feature gate Egress should be also false anyway.

Feature gate should be for code isolation, but not for configuring a feature (e.g. when enabling the feature has overhead or leads to unexpected behavior in some configuration). So, we should not auto-disable feature gates based on configuration in my mind. If we believe enabling Egress with NetworkPolicyOnly mode will result in some unexpected issues, we should do internal checks to avoid such issues. @tnqn

I wonder if we should still use a separate variable to determine whether we should activate a feature. A feature will eventually graduate to GA and be locked to true (or even removed from feature gates?) but it's still possible it shouldn't be activated in some scenario, like Egress in networkPolicyOnly mode.

@jianjuns
Copy link
Contributor Author

I agree we should check internally for features that can lead to cost or unexpected behavior once enabled. On the other hand, I feel this PR has nothing wrong to disable the feature gate on Windows, as it still looks confusing an unsupported feature is enabled (e.g. from antctl output). What you think? @tnqn

@tnqn
Copy link
Member

tnqn commented Mar 31, 2022

I agree we should check internally for features that can lead to cost or unexpected behavior once enabled. On the other hand, I feel this PR has nothing wrong to disable the feature gate on Windows, as it still looks confusing an unsupported feature is enabled (e.g. from antctl output). What you think? @tnqn

Make sense to me.

@jianjuns
Copy link
Contributor Author

/test-all

@jianjuns
Copy link
Contributor Author

/test-windows-e2e
/test-integration

@jianjuns jianjuns merged commit ce0de59 into antrea-io:main Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants