template: protect use of template manager with a lock #15192
Conversation
shoenig
force-pushed
the
b-template-hook-manager-panic
branch
from
2eb3293
to
81e27ae
Compare
shoenig
changed the title
| // just quit if we are being killed anyway | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| default: | ||
| } |
There was a problem hiding this comment.
Would this leak resources like the templateManager? Or is the expectation that the Stop hook is responsible for any clean-up (in which case it may be good to document this expectation in the comment)?
There was a problem hiding this comment.
Hmm yeah, actually this is unrelated to the bug fix at hand so I'll just eliminate it
| h.managerLock.Lock() | ||
| defer h.managerLock.Unlock() | ||
|
|
||
| if req.DriverExec != nil && h.templateManager != nil { |
There was a problem hiding this comment.
It seems like the root cause was the missing lock acquisition?
Update hooks run concurrently with others and so if an update happens while the Poststart hook is running the SetDriverHandle call may happen between these two lines, causing h.templateManager to be nil.
Apart from this I'm not sure how else templateManager could be nil, should we just no-op like in Update?
There was a problem hiding this comment.
should we just no-op like [in Update]
I think so, yeah - as in lets check & exit on nil before entering this if/else.
This PR protects access to `templateHook.templateManager` with its lock. So far we have not been able to reproduce the panic - but it seems either Poststart is running without a Prestart being run first (should be impossible), or the Update hook is running concurrently with Poststart, nil-ing out the templateManager in a race with Poststart. Fixes #15189
shoenig
force-pushed
the
b-template-hook-manager-panic
branch
from
81e27ae
to
97c6f30
Compare
shoenig
added
backport/1.3.x
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR protects access to
templateHook.templateManagerwith its lock. Sofar we have not been able to reproduce the panic - but it seems either Poststart
is running without a Prestart being run first (should be impossible), or the
Update hook is running concurrently with Poststart, nil-ing out the templateManager
in a race with Poststart.
Fixes #15189
Backport only to 1.4.x and 1.3.x