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

Avoid redundant Openflow messages when syncing an updated group to OVS #4160

Merged

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Aug 29, 2022

Fixes: #4159

This PR fixes the issue by appending buckets directly in Done() in
pkg/ovs/openflow/ofctrl_group.go instead of calling AddBuckets method
of Group defined in the ofnet which sends an Openflow message to install
the group.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

Fix antrea-io#4159

This PR fix the issue by appending buckets directly in `Done()` in
pkg/ovs/openflow/ofctrl_group.go instead of calling `AddBuckets` method
of `Group` defined in the ofnet which sends an Openflow message to install
the group.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
@hongliangl hongliangl added the area/proxy Issues or PRs related to proxy functions in Antrea label Aug 29, 2022
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #4160 (11d39fd) into main (0aad945) will increase coverage by 9.37%.
The diff coverage is n/a.

❗ Current head 11d39fd differs from pull request most recent head 9ac0d44. Consider uploading reports for the commit 9ac0d44 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4160      +/-   ##
==========================================
+ Coverage   56.38%   65.75%   +9.37%     
==========================================
  Files         417      303     -114     
  Lines       60811    46531   -14280     
==========================================
- Hits        34286    30596    -3690     
+ Misses      23743    13584   -10159     
+ Partials     2782     2351     -431     
Flag Coverage Δ *Carryforward flag
e2e-tests 39.33% <ø> (?)
integration-tests 35.02% <ø> (-0.21%) ⬇️ Carriedforward from 9ac0d44
kind-e2e-tests 48.86% <ø> (-2.46%) ⬇️ Carriedforward from 9ac0d44
unit-tests 44.97% <ø> (+0.62%) ⬆️ Carriedforward from 9ac0d44

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

Impacted Files Coverage Δ
...md/multicluster-controller/clusterclaim_webhook.go 74.50% <ø> (ø)
.../cmd/multicluster-controller/clusterset_webhook.go 60.41% <ø> (ø)
...icluster/cmd/multicluster-controller/controller.go 7.77% <ø> (+1.32%) ⬆️
multicluster/cmd/multicluster-controller/leader.go 0.00% <ø> (ø)
multicluster/cmd/multicluster-controller/main.go 0.00% <ø> (ø)
multicluster/cmd/multicluster-controller/member.go 0.00% <ø> (ø)
...luster-controller/memberclusterannounce_webhook.go 55.00% <ø> (-0.56%) ⬇️
...ulticluster/cmd/multicluster-controller/options.go 10.52% <ø> (+1.22%) ⬆️
...icluster/controllers/multicluster/common/helper.go 58.00% <ø> (ø)
...uster/commonarea/acnp_resourceimport_controller.go 68.75% <ø> (+4.04%) ⬆️
... and 279 more

Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

/test-all

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 tnqn added the action/release-note Indicates a PR that should be included in release notes. label Aug 29, 2022
@tnqn
Copy link
Member

tnqn commented Aug 29, 2022

Since the original code causes great unnecessary overhead when used in large scale, I would suggest to backport the fix.

@tnqn tnqn added the action/backport Indicates a PR that requires backports. label Aug 29, 2022
@tnqn tnqn merged commit 53b639f into antrea-io:main Aug 29, 2022
@hongliangl
Copy link
Contributor Author

hongliangl commented Aug 29, 2022

Since the original code causes great unnecessary overhead when used in large scale, I would suggest to backport the fix.

Yes, do we need to backport this to earlier versions? Like the versions before v1.0? @tnqn

@tnqn
Copy link
Member

tnqn commented Aug 29, 2022

@hongliangl please backport to release 1.5 and later.

@hongliangl
Copy link
Contributor Author

@hongliangl please backport to release 1.5 and later.

Done.

heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
antrea-io#4160)

Fix antrea-io#4159

This PR fix the issue by appending buckets directly in `Done()` in
pkg/ovs/openflow/ofctrl_group.go instead of calling `AddBuckets` method
of `Group` defined in the ofnet which sends an Openflow message to install
the group.

Signed-off-by: Hongliang Liu <lhongliang@vmware.com>
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. area/proxy Issues or PRs related to proxy functions in Antrea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant Openflow messages are sent when syncing an updated group to OVS
3 participants