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 WARNING: DATA RACE issue when multiple goroutines access the backend #13875

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Apr 2, 2022

Fix issues/13873.

One of the CI failures in pull/13854 was also caused by this issue.

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2022

Codecov Report

Merging #13875 (836bd6b) into main (3d3c437) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #13875      +/-   ##
==========================================
+ Coverage   72.50%   72.55%   +0.04%     
==========================================
  Files         468      468              
  Lines       38307    38315       +8     
==========================================
+ Hits        27776    27799      +23     
+ Misses       8745     8736       -9     
+ Partials     1786     1780       -6     
Flag Coverage Δ
all 72.55% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
server/etcdserver/adapters.go 73.68% <100.00%> (+3.09%) ⬆️
server/etcdserver/server.go 84.52% <100.00%> (+0.16%) ⬆️
server/etcdserver/v3_server.go 79.26% <100.00%> (+0.07%) ⬆️
server/storage/backend/backend.go 83.76% <100.00%> (+0.10%) ⬆️
client/pkg/v3/testutil/leak.go 62.83% <0.00%> (-4.43%) ⬇️
server/proxy/grpcproxy/watch.go 93.64% <0.00%> (-2.90%) ⬇️
client/v3/experimental/recipes/key.go 75.34% <0.00%> (-2.74%) ⬇️
server/storage/mvcc/kvstore_txn.go 75.27% <0.00%> (-1.10%) ⬇️
server/etcdserver/api/v3rpc/watch.go 86.57% <0.00%> (-1.01%) ⬇️
server/embed/serve.go 62.10% <0.00%> (-0.92%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d3c437...836bd6b. Read the comment docs.

@ahrtr
Copy link
Member Author

ahrtr commented Apr 2, 2022

cc @ptabor @serathius @spzala

@@ -237,7 +237,11 @@ func (s *EtcdServer) Compact(ctx context.Context, r *pb.CompactionRequest) (*pb.
// the hash may revert to a hash prior to compaction completing
// if the compaction resumes. Force the finished compaction to
// commit so it won't resume following a crash.
//
// `applySnapshot` sets a new backend instance, so we need to acquire the bemu lock.
s.bemu.RLock()
s.be.ForceCommit()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's interesting whether we have good test-coverage of 'swapping' backends in the middle of compaction.

The scheduledCompaction theoretically might finish on the 'old backends' why the recovered remains not compacted...

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's discuss & fix the race condition between compaction & defragmentation & snapshot and applySnapshot cases in separate issues/PRs. I will drive it.

@ptabor
Copy link
Contributor

ptabor commented Apr 3, 2022

Thank you for fixing this.

@ahrtr
Copy link
Member Author

ahrtr commented Apr 3, 2022

cc @serathius @spzala Please also take a look. Thanks.

The fix in this PR can increase pipeline stability.

@serathius serathius merged commit 9dc8bbb into etcd-io:main Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Flake: Data race to verify
4 participants