Added waitgroups for autoupdate workers to complete before stopping #613
Conversation
arjunrn
force-pushed
the
wait-autoupdate-complete
branch
from
834bd82
to
53edde3
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
arjunrn
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/assign @wking |
| for i := 0; i < workers; i++ { | ||
| // FIXME: actually wait until these complete if the Context is canceled. And possibly add utilruntime.HandleCrash. |
There was a problem hiding this comment.
A runtime.HandleCrash() is not required here because it's already part of the JitterUntil function which is further down the call stack.
There was a problem hiding this comment.
My impression was that HandleCrash() would ideally be called whenever we launch a new goroutine. Walking the stack, I don't see any goroutines being launched before our UntilWithContext call and the underlying JitterUntil, but I dunno if we want to rely on that current lack of intermediate goroutines as part of the library's committed API. Does it hurt to have an extra call here?
There was a problem hiding this comment.
Adding a HandleCrash() here would recover from any panics in the apimachinery code, because the user function is already covered by the inner HandleCrash(). This is an issue because the wait.UntilWithContext would return and the autoupdate worker would terminate. If this happens in all workers then basically autoupdate would stop working. It would instead be better if program panicked so that the pod would restart. We can expect anything in the apimachinery code to be well tested and shouldn't be guarding against it.
There was a problem hiding this comment.
From the docs:
HandleCrash actually crashes, after calling the handlers and logging the panic message.
So I expect we will still exit and get restarted even if we have a HandleCrash in here.
There was a problem hiding this comment.
So then the only thing HandleCrash() does(without any additional handlers) is to klog the stack trace. 🤷🏼
Will add the HandleCrash() here as well.
|
/retest |
arjunrn
force-pushed
the
wait-autoupdate-complete
branch
from
53edde3
to
f15f84e
Compare
pkg/autoupdate/autoupdate.go
Outdated
| defer wg.Done() | ||
| defer utilruntime.HandleCrash() | ||
| wait.UntilWithContext(ctx, ctrl.worker, time.Second) | ||
| }() | ||
| } | ||
|
|
||
| <-ctx.Done() |
There was a problem hiding this comment.
now that we have a wait-group blocking on our child goroutines, we can probably drop this direct block on the passed ctx.
The wait.UntilWithContext will repeatedly run the function passed to it till the context also passed to it is done. At which point it will return. Adding a wait group after the UntilWithContext ensures that the top level goroutine does not return till all the spawned goroutines terminate. Also added `HandleCrash()` to log any panics in inner goroutines.
arjunrn
force-pushed
the
wait-autoupdate-complete
branch
from
f15f84e
to
0fee80b
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arjunrn, wking 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 |
openshift-ci
bot
added
the
approved
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
The
wait.UntilWithContextwill repeatedly run the function passed to ittill the context also passed to it is done. At which point it will
return. Adding a wait group after the UntilWithContext ensures that
the top level goroutine does not return till all the spawned goroutines
terminate.