🐛 Controller: Wait for all reconciliations before shutting down #1427
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
Currently, the controller will instantly shutdown and return when its context gets cancelled, leaving active reconciliations be. This change makes it wait for those before finishing shutdown.
alvaroaleman
changed the title
| for i := 0; i < c.MaxConcurrentReconciles; i++ { | ||
| go wait.UntilWithContext(ctx, func(ctx context.Context) { |
There was a problem hiding this comment.
From what I can see, the wait.UntilWithContext was harmless, but not useful because it would start the closure every c.JitterPeriod for as long as the ctx is not cancelled. The closure will however stay in the for c.processNextWorkItem(ctx) loop until the workqueue is shutdown and the workqueue is only shutdown when the ctx is cancelled.
I think we still have this because before b4a0212 it was possible for the closure to end.
With the changes in this PR, using wait.UntilWithContext may lead to a deadlock:
- We add the workers to the
WaitGroup - Context gets cancelled before the closure is started
wait.UntilWithContextwill not start the closure so nothing will ever callwg.Done()- The
wg.Waitis deadlocked
There was a problem hiding this comment.
This makes lots of sense, and it'd actually explain why I was still noticing some errors during tests that wouldn't properly clean up or reconcile everything before shutting stuff down (including the event recorder, which panics).
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/milestone v0.9.x |
|
/retest |
1 similar comment
|
/retest |
Currently, the controller will instantly shutdown and return when its
context gets cancelled, leaving active reconciliations be. This change
makes it wait for those before finishing shutdown.
Fixes #1424