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.

Sign up for GitHub

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

Jump to bottom

Fix data race in serviceexport_controller #4305

Merged
merged 1 commit into from
Oct 18, 2022
Merged

Fix data race in serviceexport_controller #4305

merged 1 commit into from
Oct 18, 2022

Conversation

Dyanngg
Copy link
Contributor

@Dyanngg Dyanngg commented Oct 17, 2022

Data race can occur if multiple workers read or write r.localClusterID at the same time.
Refactored the Reconcile loop so that the localClusterID is only set when it is missing.

Signed-off-by: Dyanngg [email protected]

@Dyanngg Dyanngg added kind/bug Categorizes issue or PR as related to a bug. area/multi-cluster Issues or PRs related to multi cluster. labels Oct 17, 2022
@Dyanngg Dyanngg added this to the Antrea v1.9 release milestone Oct 17, 2022
@Dyanngg Dyanngg mentioned this pull request Oct 17, 2022
Merged
@Dyanngg
Fix data race in serviceexport_controller
d35e828 
Data race can occur if multiple workers read or write r.localClusterID
at the same time. Refactored the Reconcile loop so that the
localClusterID is only set when it is missing.

Signed-off-by: Dyanngg <[email protected]>
@codecov
Copy link

codecov bot commented Oct 17, 2022 •
edited
Loading

Codecov Report

Merging #4305 (233fcc3) into main (cd4ccdd) will decrease coverage by 1.58%.
The diff coverage is 84.00%.

❗ Current head 233fcc3 differs from pull request most recent head d35e828. Consider uploading reports for the commit d35e828 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4305      +/-   ##
==========================================
- Coverage   60.49%   58.90%   -1.59%     
==========================================
  Files         378      400      +22     
  Lines       54670    55332     +662     
==========================================
- Hits        33070    32595     -475     
- Misses      19085    20286    +1201     
+ Partials     2515     2451      -64
Flag Coverage Δ *Carryforward flag
e2e-tests 61.29% <76.19%> (?)
integration-tests 34.43% <69.56%> (-0.02%) ⬇️ Carriedforward from 6a26bf7
kind-e2e-tests 31.74% <ø> (-16.21%) ⬇️ Carriedforward from 6a26bf7
unit-tests 45.49% <69.56%> (+0.67%) ⬆️ Carriedforward from 6a26bf7

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

Impacted Files Coverage Δ
...ntrollers/multicluster/serviceexport_controller.go 84.74% <82.60%> (+12.79%) ⬆️
...ter/controllers/multicluster/gateway_controller.go 72.05% <100.00%> (+70.58%) ⬆️
pkg/agent/proxy/endpointslicecache.go 0.00% <0.00%> (-83.60%) ⬇️
...g/agent/apiserver/handlers/featuregates/handler.go 4.54% <0.00%> (-77.28%) ⬇️
...agent/controller/traceflow/traceflow_controller.go 14.03% <0.00%> (-58.93%) ⬇️
pkg/agent/nodeportlocal/npl_agent_init.go 0.00% <0.00%> (-57.15%) ⬇️
pkg/agent/nodeportlocal/rules/iptable_rule.go 0.00% <0.00%> (-55.79%) ⬇️
...kg/apiserver/registry/system/supportbundle/rest.go 22.17% <0.00%> (-50.44%) ⬇️
pkg/agent/controller/traceflow/packetin.go 12.81% <0.00%> (-50.00%) ⬇️
pkg/support/dump.go 7.90% <0.00%> (-49.16%) ⬇️
... and 135 more

@Dyanngg
Copy link
Contributor Author

Dyanngg commented Oct 17, 2022

/skip-all /test-multicluster-e2e

luolanzone
luolanzone approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@luolanzone luolanzone added the action/backport Indicates a PR that requires backports. label Oct 18, 2022
@tnqn tnqn merged commit 0a7fd95 into antrea-io:main Oct 18, 2022
@luolanzone luolanzone mentioned this pull request Oct 19, 2022
Merged
@Dyanngg Dyanngg deleted the fix-serviceexport-race branch February 23, 2023 20:10
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
@Dyanngg @heanlan
Fix data race in serviceexport_controller (antrea-io#4305)
07dae1c 
Data race can occur if multiple workers read or write r.localClusterID
at the same time. Refactored the Reconcile loop so that the
localClusterID is only set when it is missing.

Signed-off-by: Dyanngg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luolanzone luolanzone luolanzone approved these changes

@tnqn tnqn Awaiting requested review from tnqn

Assignees
No one assigned
Labels
action/backport Indicates a PR that requires backports. area/multi-cluster Issues or PRs related to multi cluster. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
Antrea v1.9 release
Development

Successfully merging this pull request may close these issues.
3 participants
@Dyanngg @luolanzone @tnqn