garbage collect stale distributed locks #9982
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aanm
added
kind/bug
|
Release note label not set, please set the appropriate release note. |
4 similar comments
|
Release note label not set, please set the appropriate release note. |
|
Release note label not set, please set the appropriate release note. |
|
Release note label not set, please set the appropriate release note. |
|
Release note label not set, please set the appropriate release note. |
aanm
added
the
release-note/bug
|
test-me-please |
aanm
force-pushed
the
pr/issue-9855-kvstore-lock
branch
from
03fce22
to
0305d87
Compare
|
test-me-please |
aanm
force-pushed
the
pr/issue-9855-kvstore-lock
branch
2 times, most recently
from
a26a239
to
37935f8
Compare
|
test-me-please |
aanm
commented
Jan 29, 2020
pkg/kvstore/allocator/allocator.go
Outdated
| if err := kvstore.Delete(ctx, key); err != nil { | ||
| scopedLog.WithError(err). | ||
| Warning("Unable to unlocked distributed lock due client staleness." + | ||
| " Please check the connectivity between the KVStore and the client with that lease ID.") |
There was a problem hiding this comment.
Unfortunately it's tricky... One needs to access each cilium status on each cilium instance to know which lease ID the cilium-agent is using. I thought about this while developing the PR and the easiest would be to add it into the cilium node structure that we have. What do you think? Also, be aware this needs to be backported to 1.5 and 1.6
|
test-me-please |
tgraf
requested changes
Jan 29, 2020
Under special conditions [1], cilium-agent could leak distributed locks into the KVStore making all cilium-agents to deadlock. To avoid this cilium-operator will garbage collect stale distributed locks. If a lock is being held for more than 2x default value of cilium's lock lease TTL, cilium-operator will delete that lock from the KVStore. On a first sight this might not be reasonable but cilium-agents only use the distributed locks to coordinated across each other which one should pick the security identity for a new set of labels. Even if 2 cilium-agents pick 2 different security identities for the same set of labels, only the first one will be able to publish that identity into the KVStore. The 2nd agent will fail to publish the security identity that it picked for the set of labels: [1] 1) connect to a etcd member (https://node-A:2379) to create a lock with a 25 second lease (the default for cilium) 2) drop all peer packets, not client packets, to and from that etcd member (https://node-A:2380). 3) try releasing that lock immediately. The unlock will fail with the error level=error msg="Unable to unlock kvstore lock" error="etcdserver: request timed out". 4) As soon the unlock fails, resume connectivity dropped in 2), this re-connectivity needs to happen before the lease expires, which means the Unlock on step 3) needs to fail in less than 25 seconds, this is what happen for my case. 5) since the client resumed connectivity with the server in less than 25 seconds, the lease didn't expire which means the server still thinks the client is still holding the lock but the client assumes the lock was unlocked. Signed-off-by: André Martins <andre@cilium.io>
aanm
force-pushed
the
pr/issue-9855-kvstore-lock
branch
from
637d2f9
to
36ab0b5
Compare
|
test-me-please |
tgraf
approved these changes
Jan 29, 2020
joestringer
approved these changes
Jan 30, 2020
|
test-me-please |
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.5
in 1.5.12
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.6
in 1.6.6
maintainer-s-little-helper
bot
moved this from Backport pending to v1.5
to Backport done to v1.5
in 1.5.12
maintainer-s-little-helper
bot
moved this from Backport pending to v1.6
to Backport done to v1.6
in 1.6.6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Under special conditions [1], cilium-agent could leak distributed locks
into the KVStore making all cilium-agents to deadlock. To avoid this
cilium-operator will garbage collect stale distributed locks. If a lock
is being held for more than 2x default value of cilium's lock lease TTL,
cilium-operator will delete that lock from the KVStore. On a first sight
this might not be reasonable but cilium-agents only use the distributed
locks to coordinated across each other which one should pick the
security identity for a new set of labels. Even if 2 cilium-agents pick
2 different security identities for the same set of labels, only the
first one will be able to publish that identity into the KVStore. The
2nd agent will fail to publish the security identity that it picked for
the set of labels:
[1]
a 25 second lease (the default for cilium)
member (https://node-A:2380).
error level=error msg="Unable to unlock kvstore lock"
error="etcdserver: request timed out".
re-connectivity needs to happen before the lease expires, which means
the Unlock on step 3) needs to fail in less than 25 seconds, this is
what happen for my case.
seconds, the lease didn't expire which means the server still thinks
the client is still holding the lock but the client assumes the lock was
unlocked.
Signed-off-by: André Martins andre@cilium.io
Fix #9855
Fix #9046
This change is