[1.6] Various etcd fixes backports #12748
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
Member
tgraf
commented
Aug 3, 2020
•
tgraf
commented
[ upstream commit bfac77f ] Due to creating a context with a timeout prior to blocking on the session channel to wait for it to end, the timeout of the context will likely already hvae expired by the time the kvstore operation is performed. It is thus cancelled immediately, requiring a subsequent controller call. This prolongs the time the session is renewed and causes unnecessary controller failures which can be mistaken for etcd issues. Also pass the etcd client context to the controller to bind the lifecycle of a controller run to it. This fixes errors like these: ``` kvstore-etcd-lock-session-renew 20s ago 10s ago 1 unable to renew etcd lock session: context deadline exceeded ``` MERGE CONFLICT: The controller layer does not support providing a context so the client context is passed in later. It means that we can't merge the controller and etcd client context. Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit ba9031b ] The controller functions could potentially block forever while waiting on channels to close. Bind waiting on the channel and check of the etcd version to the controller and etcd client context so these operations get cancelled. MERGE CONFLICT: Due to not passing the etcd client context to the controller, the etcd client context is an exit case as well. Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit f6a751f ] Same as ba9031b ("etcd: Ensure that session renewal controller exit") but for the lock session. MERGE CONFLICT: Due to not providing the etcd client context to the controller, the etcd client controller must be checked as well in the select conditionals. Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit 02a1810 ] Signed-off-by: Thomas Graf <thomas@cilium.io>
[ upstream commit 0262854 ] 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>
maintainer-s-little-helper
bot
added
the
kind/backports
|
test-backport-1.6 |
aanm
approved these changes
Aug 3, 2020
qmonnet
approved these changes
Aug 3, 2020
This was referenced Aug 3, 2020
tklauser
approved these changes
Aug 3, 2020
maintainer-s-little-helper
bot
added
the
ready-to-merge
aanm
changed the title
aanm
changed the title
joestringer
reviewed
Aug 3, 2020
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.