Stop leading when onstarted_leading raises - #2670
Conversation
The onstarted_leading callback runs in its own thread, so an exception raised there was swallowed by that thread and never observed by the renew loop. The candidate kept renewing its lease indefinitely while the work the lease was meant to protect was no longer running, and onstopped_leading was never called. Record the failure and check it in the renew loop, so the candidate stops renewing and run() invokes onstopped_leading as it already does when a renewal fails. A callback that returns normally is left alone; only the failure case reported in the issue changes.
kubernetes-prow
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LALITH0110 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 |
kubernetes-prow
Bot
added
the
size/M
|
please investigate the CI test failures |
|
/close |
|
@yliaog: Closed this PR. DetailsIn response to this:
Instructions 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. |
|
/reopen |
|
@yliaog: Reopened this PR. DetailsIn response to this:
Instructions 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
What type of PR is this?
/kind bug
What this PR does / why we need it:
onstarted_leadingis started on its own thread, so when it raises, theexception is swallowed by that thread and the renew loop never learns about it.
The candidate keeps renewing its lease indefinitely even though the work the
lease is meant to protect has stopped, and
onstopped_leadingis never called.Because the lease keeps being renewed, no other candidate can take over either.
This records the failure and checks it at the top of the renew loop, so the
candidate stops renewing and
run()callsonstopped_leading— the same pathalready taken when a renewal fails.
Scope: only the failure case reported in the issue changes. A callback that
returns normally behaves exactly as before, and none of the existing tests
needed to be modified.
Reproduced before fixing, with a lock that never refuses a renewal (so the only
thing that can end the leadership is the callback):
onstopped_leadingcalledWhich issue(s) this PR fixes:
Fixes #2075
Special notes for your reviewer:
MockResourceLockwhoserenew_count_maxissys.maxsize, so the renew loop can only exit because the callback failed —it fails on
master(times out) and passes with this change. It runs theelection on a daemon thread so a regression times out rather than hanging CI.
renew_loopkeeps its old zero-argument form via a default ofNone, so anyexisting caller is unaffected.
BaseExceptionis deliberately not caught, soKeyboardInterruptandSystemExitkeep their current behaviour.passed, 42 skipped, 0 failed; the leaderelection suite goes 4 -> 5 passed.
flake8 --select=E9,F63,F7,F82is clean, andpycodestylereports exactlythe same pre-existing findings before and after this change.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: