overlord/hooks: make sure only one hook for given snap is executed at a time. #3235
Conversation
|
Thanks for working on this! The code looks fine. I would appreciate a test (direct or indirect) for this as well. |
|
@mvo5 Ok, added an (indirect) test. I'm not sure if this is the most effective way of testing this, but it fails every time without the setBlocked() change in the code, so I think it's good enough to prove we serialize tasks. I'm not sure though how to test if we allow hooks for different snaps to run concurrently - it's not impossible, but could be flaky. |
| @@ -32,6 +33,9 @@ type MockHandler struct { | |||
| ErrorCalled bool | |||
| ErrorError bool | |||
| Err error | |||
|
|
|||
| Executed int32 | |||
There was a problem hiding this comment.
Agreed, "Executing" sounds good, done.
| @@ -41,6 +45,10 @@ func NewMockHandler() *MockHandler { | |||
|
|
|||
| // Before satisfies hookstate.Handler.Before | |||
| func (h *MockHandler) Before() error { | |||
| executing := atomic.AddInt32(&h.Executing, 1) | |||
| if executing != 1 { | |||
| panic(fmt.Sprintf("More than one handler executed: %d", executing)) | |||
There was a problem hiding this comment.
The logic here doesn't feel entirely sensible because we do allow handlers to execute concurrently as long as they're taking care of different snaps. Your test only passed because there was no second snap on it.
One might also argue that every execution should create a new handler, and that's what we tend to do, but then the logic is bogus for the opposite reason: if every hook has its own handler, this wouldn't catch the problem either.
There was a problem hiding this comment.
This test deliberately reuses single instance of mock handler and relies on its implementation to verify that multiple hooks are not run concurrently and that SetBlocked() lambda does its job. This test will fail every time if SetBlocked implementation is removed.
I agree it would be nice to have a test where there is a second snap, and its hook can be run at the same time and independent of first snap' hook. But I'm not sure how to make such a test right now without introducing flakiness.
There was a problem hiding this comment.
Actually, regarding a test with different snap, two mock handlers where one waits for the other on a channel should do the job... Let me hack on it.
There was a problem hiding this comment.
Added second test, both tests should now cover all cases (making sure we serialize hook tasks for same snap, and making sure we allow hooks from different snaps run concurrently).
There was a problem hiding this comment.
Again, can we please not do that in this type? This is hardcoding an assumption in a publicly exposed test type that is simply not a real constraint and can blow things up without the logic in the test being wrong.
There was a problem hiding this comment.
@niemeyer Ok, I've addressed this by introducing callbacks to the MockHandler as discussed.
overlord/hookstate/hookstate_test.go
Outdated
| }) | ||
|
|
||
| var testSnap1HookCalls, testSnap2HookCalls int | ||
| ch := make(chan struct{}, 0) |
There was a problem hiding this comment.
the ,0 is unsual I think style wise
There was a problem hiding this comment.
Ok, removed. I prefer to make it more explicit what the intent is even if it's the default, but you're right, we don't do that in other places so it's better to be consistent.
There was a problem hiding this comment.
@stolowski thanks, the langref is not super clear about what make(chan T, 0) should do,
https://golang.org/ref/spec#Making_slices_maps_and_channels
it seems you get un unbuffered channel, but a buffered channel where send would always fail would seem not ruled out. it would be bit useless, though that's the behavior of the nil channel
…pdated in callbacks.
| thisSnapName := hooksup.Snap | ||
| // examine all hook tasks, block thisTask if we find any other hook task affecting same snap | ||
| for _, t := range running { | ||
| if t.Kind() != "run-hook" || t.Get("hook-setup", &hooksup) != nil { |
There was a problem hiding this comment.
because Snap is something that is always set in HookSetup this not an actual problem here but in general because json.Unmarshal updates without resetting target values, doing Get on a reused variable can be risky
Make sure only one hook for given snap is executed at a time.