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

lease: fix deadlock with Renew lease when the checkpointor is set #10492

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

nolouch
Copy link
Contributor

@nolouch nolouch commented Feb 21, 2019

Signed-off-by: nolouch [email protected]

There is a deadlock issue which block the write. when we renew a lease that have remaining TTL cause by the long TTL setting, it write lease checkpointing entry to raft and wait it apply, but apply this entry also need acquire the same lock which already acquired by renew, and then meet the deadlock until timeout.

@xiang90
Copy link
Contributor

xiang90 commented Feb 21, 2019

/cc @WIZARD-CXY can you take a look?

@WIZARD-CXY
Copy link
Contributor

will do

@codecov-io
Copy link

codecov-io commented Feb 21, 2019

Codecov Report

Merging #10492 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10492      +/-   ##
==========================================
- Coverage   71.83%    71.6%   -0.23%     
==========================================
  Files         392      392              
  Lines       36518    36520       +2     
==========================================
- Hits        26231    26150      -81     
- Misses       8464     8542      +78     
- Partials     1823     1828       +5
Impacted Files Coverage Δ
lease/lessor.go 88.17% <100%> (+0.55%) ⬆️
proxy/grpcproxy/register.go 69.44% <0%> (-11.12%) ⬇️
clientv3/leasing/util.go 91.66% <0%> (-6.67%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
etcdserver/api/v3rpc/watch.go 80.06% <0%> (-3.93%) ⬇️
clientv3/leasing/cache.go 87.77% <0%> (-3.89%) ⬇️
etcdserver/util.go 95% <0%> (-3.75%) ⬇️
pkg/testutil/recorder.go 77.77% <0%> (-3.71%) ⬇️
clientv3/leasing/txn.go 88.09% <0%> (-3.18%) ⬇️
proxy/grpcproxy/watch.go 88.55% <0%> (-3.02%) ⬇️
... 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 8c228d6...e20b9d9. Read the comment docs.

@nolouch
Copy link
Contributor Author

nolouch commented Feb 21, 2019

ci passed. PTAL

lease/lessor.go Show resolved Hide resolved
lease/lessor.go Outdated Show resolved Hide resolved
lease/lessor.go Outdated Show resolved Hide resolved
@WIZARD-CXY
Copy link
Contributor

@xiang90 it is indeed a bug which would cause the deadlock. @nolouch small knits to fix then LGTM.

@jingyih
Copy link
Contributor

jingyih commented Feb 25, 2019

/cc @jpbetz

@WIZARD-CXY
Copy link
Contributor

lgtm cc @xiang90 @gyuho

@xiang90
Copy link
Contributor

xiang90 commented Feb 26, 2019

lgtm. defer to @jpbetz

@nolouch
Copy link
Contributor Author

nolouch commented Mar 5, 2019

PTAL @jpbetz

@xiang90 xiang90 merged commit 4b69cfc into etcd-io:master Mar 6, 2019
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.

6 participants