Fix AntreaNetworkPolicyStats with drop action #1606

ceclinux · 2020-12-03T05:29:58Z

This PR fixes #1599. The test added would successfully collect dropped packet from AntreaNetworkPolicyStat because the agent is not crashed after the fix

vmwclabot · 2020-12-03T05:30:04Z

@ceclinux, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

antrea-bot · 2020-12-03T05:30:14Z

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-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/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-12-03T06:07:51Z

Codecov Report

Merging #1606 (aa0710d) into master (9d3d10b) will decrease coverage by 1.41%.
The diff coverage is 57.64%.

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   63.31%   61.90%   -1.42%     
==========================================
  Files         170      181      +11     
  Lines       14250    15433    +1183     
==========================================
+ Hits         9023     9554     +531     
- Misses       4292     4856     +564     
- Partials      935     1023      +88

Flag	Coverage Δ
kind-e2e-tests	`53.02% <53.15%> (-2.37%)`	⬇️
unit-tests	`40.53% <24.61%> (-0.75%)`	⬇️

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

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <0.00%> (ø)`
.../agent/apiserver/handlers/networkpolicy/handler.go	`58.33% <ø> (ø)`
...gent/controller/noderoute/node_route_controller.go	`46.06% <ø> (-0.41%)`	⬇️
pkg/agent/stats/collector.go	`97.72% <ø> (ø)`
pkg/antctl/antctl.go	`100.00% <ø> (ø)`
pkg/antctl/transform/addressgroup/transform.go	`0.00% <0.00%> (ø)`
pkg/antctl/transform/appliedtogroup/transform.go	`0.00% <0.00%> (ø)`
pkg/antctl/transform/controllerinfo/transform.go	`0.00% <ø> (ø)`
pkg/antctl/transform/version/transform.go	`0.00% <ø> (ø)`
pkg/controller/networkpolicy/tier.go	`90.00% <ø> (ø)`
... and 70 more

tnqn

thanks @ceclinux, LGTM overall.

test/e2e/antreapolicy_test.go

pkg/agent/openflow/network_policy.go

test/e2e/antreapolicy_test.go

pkg/agent/openflow/network_policy.go

test/e2e/antreapolicy_test.go

test/e2e/infra/vagrant/playbook/roles/common/tasks/docker.yml

antoninbas

some pretty minor comments, otherwise LGTM

pkg/agent/openflow/network_policy.go

test/e2e/antreapolicy_test.go

antoninbas

LGTM

weiqiangt · 2020-12-08T03:18:34Z

/test-all

tnqn

A race condition needs to be fixed

test/e2e/antreapolicy_test.go

test/e2e/networkpolicy_test.go

test/e2e/antreapolicy_test.go

pkg/agent/openflow/network_policy.go

tnqn

Could you also rebase on master and squash the commits when addressing the remaining comments?

ceclinux · 2020-12-09T09:59:00Z

Could you also rebase on master and squash the commits when addressing the remaining comments?

I have rebased and re-organized the commits

tnqn

@ceclinux Typically we take only one commit from a PR unless it's a branch merging so you don't have to split the commits. Of course you could amend your patches when addressing commits for ease of review, and normally we will use the squash button to merge them into one before checking in, but in this PR's case I need to reorganize your commit message since you used 4 patches in this PR. I would appreciate if you could squash it manually and have a single commit message to list what it does.

tnqn · 2020-12-09T12:36:28Z

test/e2e/antreapolicy_test.go

+		if err != nil {
+			return false, err
+		}
+		t.Logf("Got AntreaNetworkPolicy stat: %v", stats)


Suggested change

t.Logf("Got AntreaNetworkPolicy stat: %v", stats)

t.Logf("Got AntreaNetworkPolicy stats: %v", stats)

Very reasonable, I have squashed into single commit.

tnqn · 2020-12-09T13:25:26Z

I think the commit title can just be "Fix ANP with drop action when stats is enabled(#1599)", "by an e2e test" seems not useful here.

And a typo "s/matric/metric"

1. refactor parseMetricFlow to map format 2. add AntreaNetworkPolicyStats test with drop action 3. add extra ansible version check, forgotten by(#1611) 4. change metric to stats in terms of NetworkPolicyStats

tnqn

LGTM

tnqn · 2020-12-09T13:52:03Z

/test-all

1. refactor parseMetricFlow to map format 2. add AntreaNetworkPolicyStats test with drop action 3. add extra ansible version check, forgotten by(#1611) 4. change metric to stats in terms of NetworkPolicyStats

…a-io#1606) 1. refactor parseMetricFlow to map format 2. add AntreaNetworkPolicyStats test with drop action 3. add extra ansible version check, forgotten by(antrea-io#1611) 4. change metric to stats in terms of NetworkPolicyStats

1. refactor parseMetricFlow to map format 2. add AntreaNetworkPolicyStats test with drop action 3. add extra ansible version check, forgotten by(#1611) 4. change metric to stats in terms of NetworkPolicyStats

ceclinux changed the title ~~add AntreaNetworkPolicyStats test with drop action~~ Fix AntreaNetworkPolicyStats with drop action Dec 3, 2020

tnqn reviewed Dec 3, 2020

View reviewed changes

test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved

test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved

antoninbas reviewed Dec 3, 2020

View reviewed changes

weiqiangt reviewed Dec 4, 2020

View reviewed changes

pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved

tnqn mentioned this pull request Dec 4, 2020

Fix metrics flow parsing error caused by traceflow flows #1615

Merged

weiqiangt reviewed Dec 6, 2020

View reviewed changes

pkg/agent/openflow/network_policy.go Outdated Show resolved Hide resolved

test/e2e/antreapolicy_test.go Show resolved Hide resolved

test/e2e/infra/vagrant/playbook/roles/common/tasks/docker.yml Show resolved Hide resolved

antoninbas reviewed Dec 7, 2020

View reviewed changes

pkg/agent/openflow/network_policy.go Show resolved Hide resolved

test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved

test/e2e/antreapolicy_test.go Outdated Show resolved Hide resolved

antoninbas previously approved these changes Dec 8, 2020

View reviewed changes

tnqn requested changes Dec 8, 2020

View reviewed changes

ceclinux dismissed antoninbas’s stale review via c25bf17 December 9, 2020 01:18

tnqn reviewed Dec 9, 2020

View reviewed changes

ceclinux requested a review from tnqn December 9, 2020 10:01

tnqn reviewed Dec 9, 2020

View reviewed changes

Fix ANP with drop action when stats is enabled(#1599)

aa0710d

1. refactor parseMetricFlow to map format 2. add AntreaNetworkPolicyStats test with drop action 3. add extra ansible version check, forgotten by(#1611) 4. change metric to stats in terms of NetworkPolicyStats

tnqn approved these changes Dec 9, 2020

View reviewed changes

tnqn merged commit abe3af2 into antrea-io:master Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AntreaNetworkPolicyStats with drop action #1606

Fix AntreaNetworkPolicyStats with drop action #1606

ceclinux commented Dec 3, 2020

vmwclabot commented Dec 3, 2020

antrea-bot commented Dec 3, 2020

codecov-io commented Dec 3, 2020 •

edited

Loading

tnqn left a comment

antoninbas left a comment

antoninbas left a comment

weiqiangt commented Dec 8, 2020

tnqn left a comment

tnqn left a comment

ceclinux commented Dec 9, 2020

tnqn left a comment

tnqn Dec 9, 2020

ceclinux Dec 9, 2020

tnqn commented Dec 9, 2020

tnqn left a comment

tnqn commented Dec 9, 2020

	t.Logf("Got AntreaNetworkPolicy stat: %v", stats)
	t.Logf("Got AntreaNetworkPolicy stats: %v", stats)

Fix AntreaNetworkPolicyStats with drop action #1606

Fix AntreaNetworkPolicyStats with drop action #1606

Conversation

ceclinux commented Dec 3, 2020

vmwclabot commented Dec 3, 2020

antrea-bot commented Dec 3, 2020

codecov-io commented Dec 3, 2020 • edited Loading

Codecov Report

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

weiqiangt commented Dec 8, 2020

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

ceclinux commented Dec 9, 2020

tnqn left a comment

Choose a reason for hiding this comment

tnqn Dec 9, 2020

Choose a reason for hiding this comment

ceclinux Dec 9, 2020

Choose a reason for hiding this comment

tnqn commented Dec 9, 2020

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Dec 9, 2020

codecov-io commented Dec 3, 2020 •

edited

Loading