Bug 1927944: pkg/start: Fix shutdown deadlock when die before getting a leader lock #519
Conversation
a9e075a (pkg/cvo/cvo: Guard Operator.Run goroutine handling from early cancels, 2021-01-28, openshift#508) made us more robust to situations where we are canceled after acquiring the leader lock but before we got into Operator.Run's UntilWithContext. However, there was still a bug from cc1921d (pkg/start: Release leader lease on graceful shutdown, 2020-08-03, openshift#424) where we had not acquired the leader lock [1]. postMainContext is used for metrics, informers, and the leader election loop. We used to only call postMainCancel after reaping the main goroutine, and obviously that will only work if we've launched the main goroutine. This commit adds a new launchedMain to track that. If launchedMain is true, we get the old handling. If launchedMain is still false when runContext.Done, we now call postMainCancel without waiting to reap a non-existent main goroutine. There's also a new postMainCancel when the shutdown timer expires. I don't expect us to ever need that, but it protects us from future bugs like this one. I've added launchedMain without guarding it behind a lock, and it is touched by both the main Options.run goroutine and the leader-election callback. So there's a racy chance of: 1. Options.run goroutine: runContext canceled, so runContext.Done() matches 2. Leader-election goroutine: Leader lock acquired 3. Options.run goroutine: !launchedMain, so we call postMainCancel() 4. Leader-election goroutine: launchedMain set true 5. Leader-election goroutine: launches the main goroutine via CVO.Run(runContext, ...) I'm trusting Operator.Run to respect runContext there and not do anything significant, so the fact that we are already tearing down all the post-main stuff won't cause problems. Previous fixes like a9e075a will help with that. But there could still be bugs in Operator.Run. A lock around launchedMain that avoided calling Operator.Run when runContext was already done would protect against that, but seems like overkill in an already complicated goroutine tangle. Without the lock, we just have to field and fix any future Operator.Run runContext issues as we find them. [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1927944
|
@wking: This pull request references Bugzilla bug 1927944, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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/test-infra repository. |
openshift-ci-robot
added
bugzilla/severity-high
openshift-ci-robot
added
the
approved
wking
force-pushed
the
post-main-when-main-never-launched
branch
from
2cf4082 to
7ae934a
Compare
|
I wouldn't consider adding a lock around |
|
/lgtm Discussed my above comment with Trevor on slack and we agreed that if |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: All pull requests linked via external trackers have merged: Bugzilla bug 1927944 has been moved to the MODIFIED state. 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/test-infra repository. |
5 participants
a9e075a (#508) made us more robust to situations where we are canceled after acquiring the leader lock but before we got into
Operator.Run'sUntilWithContext. However, there was still a bug from cc1921d (#424) where we had not acquired the leader lock.postMainContextis used for metrics, informers, and the leader election loop. We used to only callpostMainCancelafter reaping the main goroutine, and obviously that will only work if we've launched the main goroutine. This commit adds a newlaunchedMainto track that. IflaunchedMainis true, we get the old handling. IflaunchedMainis still false whenrunContext.Done, we now callpostMainCancelwithout waiting to reap a non-existent main goroutine.There's also a new
postMainCancelwhen the shutdown timer expires. I don't expect us to ever need that, but it protects us from future bugs like this one.I've added
launchedMainwithout guarding it behind a lock, and it is touched by both the mainOptions.rungoroutine and the leader-election callback. So there's a racy chance of:Options.rungoroutine:runContextcanceled, sorunContext.Done()Options.rungoroutine:!launchedMain, so we callpostMainCancel()launchedMainset trueCVO.Run(runContext, ...)I'm trusting
Operator.Runto respectrunContextthere and not do anything significant, so the fact that we are already tearing down allthe post-main stuff won't cause problems. Previous fixes like a9e075a will help with that. But there could still be bugs in
Operator.Run. A lock aroundlaunchedMainthat avoided callingOperator.RunwhenrunContextwas already done would protect against that, but seems like overkill in an already complicated goroutine tangle. Without the lock, we just have to field and fix any futureOperator.RunrunContextissues as we find them.