✨ leader election: Block in OnStartedLeading until the context is cancelled - #3560
✨ leader election: Block in OnStartedLeading until the context is cancelled#3560tchap wants to merge 2 commits into
Conversation
Align with upstream Kubernetes changes to leaderelection.Run() which, when ReleaseOnCancel is true, calls OnStartedLeading synchronously and keeps renewing the lease until it returns before releasing it. When leaderElectionReleaseOnCancel is set, OnStartedLeading now blocks on <-ctx.Done() and then calls StopAndWait on the leader election runnables to ensure all goroutines have terminated before returning. This guarantees the lease is not released while runnables are still active — both in the normal shutdown path (where engageStopProcedure already called StopAndWait, making this a no-op via stopOnce) and in the renewal failure path (where this is the first StopAndWait call).
Add two tests verifying that leader election runnables are stopped before the lease is released when ReleaseOnCancel is true: - Normal shutdown: the runnable asserts the lease is still held while it shuts down, confirming the lease is only released afterward. - Renewal failure: blocking the fake lock simulates renewal failure. The test verifies the runnable finishes before Start returns, which is only possible if OnStartedLeading called StopAndWait. Point k8s.io/* dependencies to github.com/tchap/kubernetes at the fix-leader-election-release-on-cancel branch to pick up the upstream ReleaseOnCancel behavior changes.
kubernetes-prow
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @tchap! |
|
Hi @tchap. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kubernetes-prow
Bot
added
cncf-cla: yes
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
|
@tchap: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
kubernetes-prow
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
This change is necessary to push kubernetes/kubernetes#139991 in upstream.
Basically after the upstream change is merged, when ReleaseOnCancel is set for leader election, Run() calls OnStartedLeading synchronously (it tracks the goroutine) and keeps renewing the lease until it returns, then releases the lease. We must block until ctx is cancelled and all runnables have finished.
This patch aligns controller-runtime to be compatible with that change.
The change should be actually backwards compatible from what I can tell. I added a second commit containing a test, but that points to the upstream PR so that I can test the new behavior. So before merging this, the second commit needs to be dropped.
/hold