Retrieve featuregate status in e2e tests #1387

weiqiangt · 2020-10-15T03:56:26Z

Read feature gate status instead of indirect checking to
determine whether a feature is enabled.

Signed-off-by: Weiqiang Tang weiqiangt@vmware.com

antrea-bot · 2020-10-15T03:56:42Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-10-15T04:00:48Z

Codecov Report

Merging #1387 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
- Coverage   64.41%   64.31%   -0.10%     
==========================================
  Files         159      159              
  Lines       12664    12664              
==========================================
- Hits         8157     8145      -12     
- Misses       3650     3658       +8     
- Partials      857      861       +4

Flag	Coverage Δ
#integration-tests	`44.95% <ø> (-0.08%)`	⬇️
#kind-e2e-tests	`50.25% <ø> (-0.23%)`	⬇️
#unit-tests	`42.07% <ø> (-0.04%)`	⬇️

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

Impacted Files	Coverage Δ
...ver/registry/controlplane/nodestatssummary/rest.go	`50.00% <0.00%> (-50.00%)`	⬇️
pkg/agent/stats/collector.go	`91.95% <0.00%> (-5.75%)`	⬇️
pkg/ovs/openflow/ofctrl_bridge.go	`70.35% <0.00%> (-1.59%)`	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	`80.56% <0.00%> (-0.27%)`	⬇️
...kg/controller/networkpolicy/store/networkpolicy.go	`81.35% <0.00%> (+3.38%)`	⬆️

weiqiangt · 2020-10-15T11:07:18Z

/test-all

antoninbas

LGTM

antoninbas · 2020-10-15T19:27:16Z

test/e2e/framework.go

+	var agentCfg interface{}
+	if err := yaml.Unmarshal([]byte(cfgMap.Data[confName]), &agentCfg); err != nil {
+		return nil, err
+	}
+	rawFeatureGateMap, ok := agentCfg.(map[interface{}]interface{})["featureGates"]
+	if !ok || rawFeatureGateMap == nil {
+		return featureGate, err
+	}
+	featureGateMap := make(map[string]bool)
+	for k, v := range rawFeatureGateMap.(map[interface{}]interface{}) {
+		featureGateMap[k.(string)] = v.(bool)
+	}


Too bad there isn't a more direct way to do that... at least not without being able to import the AgentConfig struct

Maybe this can be alleviated once #723 is fixed?

One good thing is that I found the variable name is not good in this block. Let me fix it first.

Maybe this can be alleviated once #723 is fixed?

The configmap update will take 1min to take effect on the mounted file, I think this behavior will slow tests down.

weiqiangt · 2020-10-16T03:42:49Z

/test-all

weiqiangt · 2020-10-19T02:45:17Z

/test-networkpolicy

tnqn · 2020-10-19T02:58:34Z

test/e2e/framework.go

@@ -1142,6 +1148,41 @@ func (data *TestData) GetEncapMode() (config.TrafficEncapModeType, error) {
 	return config.TrafficEncapModeInvalid, fmt.Errorf("antrea-conf config map is not found")
 }

+func (data *TestData) getFeatures(confName string, antreaNamespace string) (featuregate.FeatureGate, error) {
+	featureGate := featuregate.NewFeatureGate()
+	if err := featureGate.Add(features.DefaultAntreaFeatureGates); err != nil {


Why not using DefaultMutableFeatureGate which is for mutating FeatureGate? I think DefaultAntreaFeatureGates doesn't need to be public in that way.

I thought it may be not good to reuse a global variable. I also have an idea that uses a function to copy the defaultAntreaFeatureGates without returning it directly to keep it read-only.

Thanks for the explanation. Then perhaps you could just use DefaultMutableFeatureGate.DeepCopy() to avoid constructing the featuregate from scratch again.

Thanks, updated.

tnqn

LGTM, one nit-picking.

tnqn · 2020-10-19T12:20:51Z

test/e2e/framework.go

+	}
+	rawFeatureGateMap, ok := cfg.(map[interface{}]interface{})["featureGates"]
+	if !ok || rawFeatureGateMap == nil {
+		return featureGate, err


nit: err is from yaml.Unmarshal and might be confusing here, it should return nil explicitly?
and do we need to check both of ok and rawFeatureGateMap to go this branch?

Updated.
If no feature is enabled, the map would be nil and then we don't need further application. And the ok only indicates whether the key exists or not.

Read featuregate status instead of indirect checking to determine whether a feature is enabled. Signed-off-by: Weiqiang Tang <weiqiangt@vmware.com>

tnqn

LGTM

weiqiangt · 2020-10-19T14:52:46Z

/skip-all

weiqiangt · 2020-10-19T14:52:56Z

/test-e2e

weiqiangt requested review from antoninbas, tnqn and Dyanngg October 15, 2020 03:56

vmwclabot added the cla-not-required label Oct 15, 2020

antoninbas previously approved these changes Oct 15, 2020

View reviewed changes

Dyanngg previously approved these changes Oct 15, 2020

View reviewed changes

weiqiangt dismissed stale reviews from Dyanngg and antoninbas via 85574e6 October 16, 2020 03:42

weiqiangt force-pushed the e2e-featuregate branch from a1e55ed to 85574e6 Compare October 16, 2020 03:42

weiqiangt requested review from Dyanngg and antoninbas October 19, 2020 02:20

tnqn reviewed Oct 19, 2020

View reviewed changes

weiqiangt force-pushed the e2e-featuregate branch from 85574e6 to 3ba1d58 Compare October 19, 2020 10:43

tnqn reviewed Oct 19, 2020

View reviewed changes

Retrieve featuregate status in e2e tests

a8c43db

Read featuregate status instead of indirect checking to determine whether a feature is enabled. Signed-off-by: Weiqiang Tang <weiqiangt@vmware.com>

weiqiangt force-pushed the e2e-featuregate branch from 3ba1d58 to a8c43db Compare October 19, 2020 14:44

tnqn approved these changes Oct 19, 2020

View reviewed changes

weiqiangt merged commit b189ee6 into antrea-io:master Oct 20, 2020

weiqiangt deleted the e2e-featuregate branch October 20, 2020 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve featuregate status in e2e tests #1387

Retrieve featuregate status in e2e tests #1387

weiqiangt commented Oct 15, 2020

antrea-bot commented Oct 15, 2020

codecov-io commented Oct 15, 2020 •

edited

Loading

weiqiangt commented Oct 15, 2020

antoninbas left a comment

antoninbas Oct 15, 2020

Dyanngg Oct 15, 2020

weiqiangt Oct 16, 2020

weiqiangt Oct 16, 2020

weiqiangt commented Oct 16, 2020

weiqiangt commented Oct 19, 2020

tnqn Oct 19, 2020

weiqiangt Oct 19, 2020

tnqn Oct 19, 2020

weiqiangt Oct 19, 2020

tnqn left a comment

tnqn Oct 19, 2020

weiqiangt Oct 19, 2020

tnqn left a comment

weiqiangt commented Oct 19, 2020

weiqiangt commented Oct 19, 2020

Retrieve featuregate status in e2e tests #1387

Retrieve featuregate status in e2e tests #1387

Conversation

weiqiangt commented Oct 15, 2020

antrea-bot commented Oct 15, 2020

codecov-io commented Oct 15, 2020 • edited Loading

Codecov Report

weiqiangt commented Oct 15, 2020

antoninbas left a 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

weiqiangt commented Oct 16, 2020

weiqiangt commented Oct 19, 2020

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

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

weiqiangt commented Oct 19, 2020

weiqiangt commented Oct 19, 2020

codecov-io commented Oct 15, 2020 •

edited

Loading