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

Fix ClusterGroup realization status logic #3030

Merged
merged 1 commit into from Nov 19, 2021

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Nov 15, 2021

Fixes #3002.

ClusterGroup with child groups should only be considered groupMembersComputed after all its child groups are created and processed.

Signed-off-by: Yang Ding dingyang@vmware.com

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2021

Codecov Report

Merging #3030 (df9dad9) into main (ce63b96) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3030      +/-   ##
==========================================
+ Coverage   60.93%   61.02%   +0.09%     
==========================================
  Files         292      292              
  Lines       24708    24721      +13     
==========================================
+ Hits        15056    15087      +31     
+ Misses       8015     7993      -22     
- Partials     1637     1641       +4     
Flag Coverage Δ
kind-e2e-tests 48.14% <0.00%> (-0.08%) ⬇️
unit-tests 40.16% <100.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
pkg/controller/types/group.go 0.00% <ø> (ø)
pkg/controller/networkpolicy/clustergroup.go 76.99% <100.00%> (+1.37%) ⬆️
...ver/registry/controlplane/nodestatssummary/rest.go 50.00% <0.00%> (-50.00%) ⬇️
pkg/legacyclient/clientset/versioned/clientset.go 25.80% <0.00%> (-12.91%) ⬇️
pkg/apiserver/storage/ram/store.go 78.35% <0.00%> (-1.50%) ⬇️
pkg/agent/openflow/client.go 57.76% <0.00%> (-0.66%) ⬇️
pkg/agent/agent.go 52.15% <0.00%> (-0.40%) ⬇️
pkg/agent/route/route_linux.go 46.38% <0.00%> (-0.30%) ⬇️
pkg/agent/openflow/pipeline.go 75.55% <0.00%> (ø)
pkg/agent/controller/networkpolicy/reconciler.go 77.19% <0.00%> (+0.20%) ⬆️
... and 7 more

@Dyanngg Dyanngg force-pushed the fix-group-member-status branch 2 times, most recently from 4fbf1cf to 1281e30 Compare November 16, 2021 01:19
@@ -202,6 +202,10 @@ type NetworkPolicyController struct {
// concurrent access during updates to the internal NetworkPolicy object.
internalNetworkPolicyMutex sync.RWMutex

// internalGroupMutex protects the internalGroupStore from current access
Copy link
Member

Choose a reason for hiding this comment

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

concurrent.

Why the mutex is needed? internalGroupStore itself is thread-safe. You may want to prevent race condition between the event handlers and syncWorkers but the critical sections in this commit only protects the store operations, which is duplicate with the lock in the store itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. Removed the group mutex

Signed-off-by: Yang Ding <dingyang@vmware.com>
@Dyanngg
Copy link
Contributor Author

Dyanngg commented Nov 18, 2021

/test-all

@Dyanngg Dyanngg requested a review from tnqn November 19, 2021 05:16
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 merged commit 1df8e4a into antrea-io:main Nov 19, 2021
@Dyanngg Dyanngg deleted the fix-group-member-status branch November 19, 2021 19:01
@xliuxu xliuxu added the action/backport Indicates a PR that requires backports. label Mar 15, 2022
antoninbas pushed a commit that referenced this pull request Oct 20, 2022
Signed-off-by: Yang Ding <dingyang@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More Accurate ClusterGroup Realization Tracking
4 participants