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

Bugfix: Remove multicast group from cache when group is uninstalled #4176

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

wenyingd
Copy link
Contributor

An issue exists in the code when calling UninstallGroups in multicast
feature, that the group entry in the cache is not removed. This is
because the existing code only remove group in featureService cache.

To resolve the issue, rename the existing API UninstallGroup as
UninstallServiceGroup, and add a new API UninstallMulticastGroup to
delete multicast related groups. The new API also removes the group
entry in the cache.

Fixes #4175

Signed-off-by: wenyingd [email protected]

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #4176 (396ab64) into main (53bb0c3) will increase coverage by 0.25%.
The diff coverage is 66.66%.

❗ Current head 396ab64 differs from pull request most recent head d216562. Consider uploading reports for the commit d216562 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4176      +/-   ##
==========================================
+ Coverage   58.35%   58.60%   +0.25%     
==========================================
  Files         374      374              
  Lines       53893    54085     +192     
==========================================
+ Hits        31447    31695     +248     
+ Misses      20069    20006      -63     
- Partials     2377     2384       +7     
Flag Coverage Δ *Carryforward flag
e2e-tests 39.43% <7.69%> (?)
integration-tests 34.83% <0.00%> (ø) Carriedforward from 53bb0c3
kind-e2e-tests 48.76% <0.00%> (ø) Carriedforward from 53bb0c3
unit-tests 41.99% <64.28%> (ø) Carriedforward from 53bb0c3

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/multicast/mcast_controller.go 53.47% <0.00%> (ø)
pkg/agent/proxy/proxier.go 66.03% <0.00%> (ø)
pkg/agent/openflow/client.go 69.94% <90.90%> (+2.18%) ⬆️
pkg/agent/openflow/pipeline.go 83.47% <0.00%> (+0.19%) ⬆️
pkg/ovs/ovsconfig/ovs_client.go 67.92% <0.00%> (+2.26%) ⬆️
pkg/agent/controller/networkpolicy/fqdn.go 76.30% <0.00%> (+2.51%) ⬆️
...gent/controller/noderoute/node_route_controller.go 61.63% <0.00%> (+4.09%) ⬆️
pkg/agent/agent.go 55.11% <0.00%> (+4.31%) ⬆️
pkg/agent/openflow/meters_linux.go 72.22% <0.00%> (+5.55%) ⬆️

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client_test.go Outdated Show resolved Hide resolved
@@ -58,7 +58,7 @@ function generate_mocks {
"pkg/controller/querier ControllerQuerier testing"
"pkg/flowaggregator/exporter Interface testing"
"pkg/ipfix IPFIXExportingProcess,IPFIXRegistry,IPFIXCollectingProcess,IPFIXAggregationProcess testing"
"pkg/ovs/openflow Bridge,Table,Flow,Action,CTAction,FlowBuilder testing"
"pkg/ovs/openflow Bridge,Table,Flow,Action,CTAction,FlowBuilder,Group,BucketBuilder testing"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any new auto-generated code by this udpate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with this change we could use gomock on Group and BucketBuilder in unit test.

hongliangl
hongliangl previously approved these changes Aug 31, 2022
Copy link
Contributor

@hongliangl hongliangl left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyingd
Copy link
Contributor Author

/test-all
/test-multicast-e2e
/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-e2e
/test-ipv6-e2e

@wenyingd
Copy link
Contributor Author

/test-multicast-e2e

hongliangl
hongliangl previously approved these changes Aug 31, 2022
@wenyingd
Copy link
Contributor Author

/test-multicast-e2e

@wenyingd wenyingd requested a review from tnqn September 1, 2022 05:42
@wenyingd
Copy link
Contributor Author

wenyingd commented Sep 1, 2022

/test-multicluster-e2e

@wenyingd wenyingd changed the title Bugfix: Remove multicast group from cache when group is unintalled Bugfix: Remove multicast group from cache when group is uninstalled Sep 2, 2022
pkg/agent/openflow/client_test.go Outdated Show resolved Hide resolved
pkg/agent/openflow/client_test.go Outdated Show resolved Hide resolved
@tnqn tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Sep 5, 2022
An issue exists in the code when calling UninstallGroups in multicast
feature, that the group entry in the cache is not removed. This is
because the existing code only remove group in featureService cache.

To resolve the issue, rename the existing API UninstallGroup as
UninstallServiceGroup, and add a new API UninstallMulticastGroup to
delete multicast related groups. The new API also removes the group
entry in the cache.

Signed-off-by: wenyingd <[email protected]>
@wenyingd
Copy link
Contributor Author

wenyingd commented Sep 5, 2022

/test-all
/test-multicast-e2e
/test-multicluster-e2e
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

1 similar comment
@XinShuYang
Copy link
Contributor

/test-all
/test-multicast-e2e
/test-multicluster-e2e
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

@wenyingd wenyingd requested a review from tnqn September 5, 2022 11:40
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Sep 5, 2022

/test-all

@tnqn tnqn merged commit e91c77c into antrea-io:main Sep 5, 2022
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
…ntrea-io#4176)

An issue exists in the code when calling UninstallGroups in multicast
feature, that the group entry in the cache is not removed. This is
because the existing code only remove group in featureService cache.

To resolve the issue, rename the existing API UninstallGroup as
UninstallServiceGroup, and add a new API UninstallMulticastGroup to
delete multicast related groups. The new API also removes the group
entry in the cache.

Signed-off-by: wenyingd <[email protected]>
@wenyingd wenyingd deleted the bugfix_uninstallgroup branch May 30, 2023 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Members of group cache for multicast are never deleted
4 participants