New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tr/service_hook: prevent Update from running before Poststart finish #7600
Conversation
Shout out to @42wim for doing the leg work to track down this bug 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one question about managing hook fields.
) | ||
|
||
// Statically assert the stats hook implements the expected interfaces | ||
var _ interfaces.TaskPoststartHook = (*serviceHook)(nil) | ||
var _ interfaces.TaskExitedHook = (*serviceHook)(nil) | ||
var _ interfaces.TaskPreKillHook = (*serviceHook)(nil) | ||
var _ interfaces.TaskUpdateHook = (*serviceHook)(nil) | ||
|
||
func TestUpdate_beforePoststart(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good simple test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - minor suggestions but merge away! Thanks!
@@ -164,6 +173,7 @@ func (h *serviceHook) Exited(context.Context, *interfaces.TaskExitedRequest, *in | |||
h.mu.Lock() | |||
defer h.mu.Unlock() | |||
h.deregister() | |||
h.initialRegistration = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in Stop()
or move to deregister
?
require.NoError(t, hook.Poststart(context.Background(), &interfaces.TaskPoststartRequest{}, &interfaces.TaskPoststartResponse{})) | ||
require.Len(t, c.GetOps(), 5) | ||
require.NoError(t, hook.Update(context.Background(), &interfaces.TaskUpdateRequest{Alloc: alloc}, &interfaces.TaskUpdateResponse{})) | ||
require.Len(t, c.GetOps(), 6) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also test PreKilled then update after the last update?
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. |
The service hook requires fields initialized during Poststart when executing other lifecycle functions. In particular Update can be called at any time including before Poststart. This manifests as an error because the driver network information has not been set yet.
fixes #5767