etcd: Fix several etcd related issues#12605
Merged
Conversation
849d7f3 to
e446a2a
Compare
aanm
requested changes
Jul 21, 2020
Contributor
Author
There was a problem hiding this comment.
This is still returning nil after all changes unlike the previous case. The code is choosing to not return an error to not trigger a retry when the controller stops or the etcd client shuts down.
cc462a6 to
fb1bbeb
Compare
Contributor
Author
|
test-me-please |
fb1bbeb to
d58c811
Compare
a7fc092 to
7db5d5b
Compare
Contributor
Author
|
test-me-please |
joestringer
requested changes
Jul 22, 2020
Member
joestringer
left a comment
There was a problem hiding this comment.
One question below on errChan vs. errorChan that needs to be checked.
The rest are misc. nits. Not sure whether we've collectively decided on errors.Wrap() but I know some parts of the code have started using it already.
Contributor
Author
|
retest-4.19 |
christarazi
approved these changes
Jul 22, 2020
joestringer
approved these changes
Jul 22, 2020
Member
joestringer
left a comment
There was a problem hiding this comment.
Questions asked & answered. Minor nits can be addressed now or as followup.
Same as ba9031b ("etcd: Ensure that session renewal controller exit") but for the lock session. Signed-off-by: Thomas Graf <thomas@cilium.io>
The firstSession channel has been used to allow blocking until the initial session has been established. However, because callers have not been handling errors, it was not possible to close the channel if the etcd client was shut down before the session was ever established and thus the channel was leaked. This was even more problematic for etcd operations performed without a context with a timeout as most operations currently block on firstSession or context. Consolidate the errChan and firstSession as they effectively served the same purpose. The only slight change is that the firstSession channel is closed *after* the etcd version has been verified which is the right behavior anyway. Having the error condition returned via the firstSession channel allows to return it to owning controllers as well when renewing sessions for better visibility. Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Thomas Graf <thomas@cilium.io>
Signed-off-by: Thomas Graf <thomas@cilium.io>
The context passed into NewSession() was supposed to enforce a timeout on the NewSession() operation which is triggering a Grant() and KeepAlive() instruction. However, the context passed into NewSession() will also be associated with the resulting lease. As per etcd documentation, this will result in: > If the context is canceled before Close() completes, the session's lease will > be abandoned and left to expire instead of being revoked. Because of this, any session renewal triggering a new session would create a session that is immediately closed again due to the context passed into NewSession being cancelled when the controller run ends successfully. This resulted in any renewed session to have an effective lifetime of 10 milliseconds before requiring renewal again. Fixes: #12619 Fixes: 8524fca ("kvstore: Add session renew backoff") Signed-off-by: Thomas Graf <thomas@cilium.io>
43670ff to
f8e5a5c
Compare
Member
|
test-me-please |
Member
|
Partial backport to v1.6 in #12748. |
Member
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
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.
See individual commits. The most important fix is in the last commit.