Fix unexpected reporting of empty MulticastGroupInfo #4240

ceclinux · 2022-09-20T14:28:07Z

The empty MulticastGroupInfo will be reported unexpectedly during a single report session when:

Statistics of NP, ANP, or ANCP are getting updated.
MulticastGroupInfo is not refreshed. Fix: Always report updated MulticastGroupInfo when reporting is necessary.

Signed-off-by: ceclinux [email protected]

codecov · 2022-09-20T14:43:26Z

Codecov Report

Merging #4240 (2ea1010) into main (b2ba248) will increase coverage by 2.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4240      +/-   ##
==========================================
+ Coverage   61.30%   63.32%   +2.02%     
==========================================
  Files         388      388              
  Lines       55145    55154       +9     
==========================================
+ Hits        33806    34929    +1123     
+ Misses      18799    17671    -1128     
- Partials     2540     2554      +14

Flag	Coverage Δ
e2e-tests	`39.56% <34.09%> (?)`
integration-tests	`34.52% <ø> (+0.11%)`	⬆️
kind-e2e-tests	`47.83% <34.09%> (+5.18%)`	⬆️
unit-tests	`45.13% <84.09%> (+0.09%)`	⬆️

Impacted Files	Coverage Δ
pkg/agent/stats/collector.go	`90.69% <100.00%> (+16.40%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`71.53% <0.00%> (-10.15%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`75.25% <0.00%> (-3.02%)`	⬇️
pkg/ipam/ipallocator/allocator.go	`88.14% <0.00%> (-1.55%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`73.57% <0.00%> (-0.24%)`	⬇️
pkg/controller/grouping/controller.go	`67.10% <0.00%> (ø)`
...g/controller/networkpolicy/clusternetworkpolicy.go	`74.37% <0.00%> (+0.22%)`	⬆️
pkg/agent/secondarynetwork/podwatch/controller.go	`74.82% <0.00%> (+0.35%)`	⬆️
pkg/controller/ipam/validate.go	`80.32% <0.00%> (+0.54%)`	⬆️
pkg/agent/openflow/network_policy.go	`79.82% <0.00%> (+0.65%)`	⬆️
... and 47 more

pkg/agent/stats/collector.go

ceclinux · 2022-09-23T02:21:09Z

Update: Made a minor refactor based on yesterday's discussion @wenyingd

wenyingd · 2022-09-23T02:36:42Z

pkg/agent/stats/collector.go

-	npStats := calculateDiff(curStatsCollection.networkPolicyStats, m.lastStatsCollection.networkPolicyStats)
-	acnpStats := calculateRuleDiff(curStatsCollection.antreaClusterNetworkPolicyStats, m.lastStatsCollection.antreaClusterNetworkPolicyStats)
-	anpStats := calculateRuleDiff(curStatsCollection.antreaNetworkPolicyStats, m.lastStatsCollection.antreaNetworkPolicyStats)
+func (m *Collector) calculateStats(curStatsCollection *statsCollection) (npStats, acnpStats, anpStats []cpv1beta.NetworkPolicyStats) {


Suggested change

func (m *Collector) calculateStats(curStatsCollection *statsCollection) (npStats, acnpStats, anpStats []cpv1beta.NetworkPolicyStats) {

func (m *Collector) calculateNPStats(curStatsCollection *statsCollection) (npStats, acnpStats, anpStats []cpv1beta.NetworkPolicyStats) {

wenyingd · 2022-09-23T02:40:18Z

pkg/agent/stats/collector.go

 	var multicastGroups []cpv1beta.MulticastGroupInfo
-	multicastGroupsUpdated := false
+	npStats, acnpStats, anpStats := m.calculateStats(curStatsCollection)
 	if m.multicastEnabled {


Still feel better to use a flag multicastGroupsUpdated and set its value with function isIdenticalMulticastGroupMap inside "if m.multicastEnabled", and then consume the flag in line 222

wenyingd

LGTM

tnqn

LGTM

tnqn · 2022-10-12T13:51:12Z

I assume the issue is that user will see empty/incomplete multicast group members wrongly in some cases?
For future PRs, please create an issue or describe the consequence in the PR so others can better understand the problem and can point to the fix when same issue is encountered.

tnqn · 2022-10-12T13:52:08Z

If my understanding is correct, I think the patch should be backported.

tnqn · 2022-10-12T13:53:52Z

DCO check failed.
So do codecov/patch. Since you refactored the code, they will be counted as new code, you need to add unit test for them.

ceclinux · 2022-10-13T09:38:52Z

DCO check failed. So do codecov/patch. Since you refactored the code, they will be counted as new code, you need to add unit test for them.

@tnqn Your understanding is correct. I will add more tests later to cover the refactored code later in this PR. Thanks.

The empty MulticastGroupInfo will be reported unexpectedly during a single report session when: 1. Statistics of NP, ANP, or ANCP are getting updated. 2. MulticastGroupInfo is not refreshed. Fix: Always report updated MulticastGroupInfo when reporting is necessary. Also, add a minor refactor for the collector report function. Signed-off-by: ceclinux <[email protected]>

ceclinux · 2022-10-16T22:30:43Z

/test-all

ceclinux · 2022-10-17T01:52:36Z

/test-multicast-e2e

ceclinux · 2022-10-17T03:19:21Z

DCO check failed. So do codecov/patch. Since you refactored the code, they will be counted as new code, you need to add unit test for them.

Tests added. DCO and codecov passed. @tnqn

tnqn

LGTM

The empty MulticastGroupInfo will be reported unexpectedly during a single report session when: 1. Statistics of NP, ANP, or ANCP are getting updated. 2. MulticastGroupInfo is not refreshed. Fix: Always report updated MulticastGroupInfo when reporting is necessary. Also, add a minor refactor for the collector report function. Signed-off-by: ceclinux <[email protected]>

ceclinux force-pushed the fix-multicastgroups-report branch 6 times, most recently from be19a86 to 2081a30 Compare September 21, 2022 17:59

liu4480 reviewed Sep 22, 2022

View reviewed changes

pkg/agent/stats/collector.go Outdated Show resolved Hide resolved

liu4480 reviewed Sep 22, 2022

View reviewed changes

pkg/agent/stats/collector.go Outdated Show resolved Hide resolved

wenyingd reviewed Sep 22, 2022

View reviewed changes

pkg/agent/stats/collector.go Outdated Show resolved Hide resolved

ceclinux force-pushed the fix-multicastgroups-report branch from 2081a30 to 1dd5a3c Compare September 23, 2022 01:29

wenyingd reviewed Sep 23, 2022

View reviewed changes

ceclinux force-pushed the fix-multicastgroups-report branch from 1dd5a3c to 95d0aaa Compare September 23, 2022 09:19

ceclinux requested a review from tnqn September 28, 2022 07:50

ceclinux force-pushed the fix-multicastgroups-report branch from 95d0aaa to a928e26 Compare October 8, 2022 04:07

wenyingd previously approved these changes Oct 10, 2022

View reviewed changes

tnqn added this to the Antrea v1.9 release milestone Oct 12, 2022

tnqn previously approved these changes Oct 12, 2022

View reviewed changes

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Oct 12, 2022

ceclinux dismissed stale reviews from tnqn and wenyingd via 7c983d6 October 15, 2022 19:59

ceclinux force-pushed the fix-multicastgroups-report branch 3 times, most recently from fd79c92 to eea7dcd Compare October 16, 2022 10:48

ceclinux force-pushed the fix-multicastgroups-report branch from eea7dcd to 7a1d29b Compare October 16, 2022 14:34

ceclinux force-pushed the fix-multicastgroups-report branch from 7a1d29b to 2ea1010 Compare October 16, 2022 15:31

ceclinux requested a review from tnqn October 17, 2022 03:45

tnqn approved these changes Oct 17, 2022

View reviewed changes

tnqn merged commit 03b2948 into antrea-io:main Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unexpected reporting of empty MulticastGroupInfo #4240

Fix unexpected reporting of empty MulticastGroupInfo #4240

ceclinux commented Sep 20, 2022

codecov bot commented Sep 20, 2022 •

edited

Loading

ceclinux commented Sep 23, 2022

wenyingd Sep 23, 2022

ceclinux Sep 23, 2022

wenyingd Sep 23, 2022

ceclinux Sep 23, 2022

wenyingd left a comment

tnqn left a comment

tnqn commented Oct 12, 2022

tnqn commented Oct 12, 2022

tnqn commented Oct 12, 2022

ceclinux commented Oct 13, 2022 •

edited

Loading

ceclinux commented Oct 16, 2022

ceclinux commented Oct 17, 2022

ceclinux commented Oct 17, 2022

tnqn left a comment

	func (m Collector) calculateStats(curStatsCollection statsCollection) (npStats, acnpStats, anpStats []cpv1beta.NetworkPolicyStats) {
	func (m Collector) calculateNPStats(curStatsCollection statsCollection) (npStats, acnpStats, anpStats []cpv1beta.NetworkPolicyStats) {

Fix unexpected reporting of empty MulticastGroupInfo #4240

Fix unexpected reporting of empty MulticastGroupInfo #4240

Conversation

ceclinux commented Sep 20, 2022

codecov bot commented Sep 20, 2022 • edited Loading

Codecov Report

ceclinux commented Sep 23, 2022

wenyingd Sep 23, 2022

Choose a reason for hiding this comment

ceclinux Sep 23, 2022

Choose a reason for hiding this comment

wenyingd Sep 23, 2022

Choose a reason for hiding this comment

ceclinux Sep 23, 2022

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Oct 12, 2022

tnqn commented Oct 12, 2022

tnqn commented Oct 12, 2022

ceclinux commented Oct 13, 2022 • edited Loading

ceclinux commented Oct 16, 2022

ceclinux commented Oct 17, 2022

ceclinux commented Oct 17, 2022

tnqn left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2022 •

edited

Loading

ceclinux commented Oct 13, 2022 •

edited

Loading