-
Notifications
You must be signed in to change notification settings - Fork 2k
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
alloc_runner: stop sidecar tasks last #13055
Conversation
15e7d2b
to
55cbbda
Compare
1600da0
to
86cf32f
Compare
86cf32f
to
be7adb9
Compare
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.
Minor suggestion about the log message.
I was also looking at the existing test suite, but it doesn't seem like we have any timing based tests. One thing that I thought was to check the task event order in TestAllocRunner_Lifecycle_Poststart
but I don't know how reliable that would be.
Thanks for catching the test omission. I'll take a pass at adding a specific test for this. |
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. |
Closes #10340
This PR addresses a defect where sidecar tasks are stopped prior to their associated main task being successfully stopped. This can create problems in scenarios where the main task can take a long time to shut down, but still need their sidecars to be operational while they finish their shutdown task (e.g. Consul Connect proxy).
This fix includes the following changes:
IsSidecarTask
helper method.IsSidecarTask
.hasSidecarTasks
method to correctly describe the behavior of the function.allocRunner.killTasks
to skip shutting down sidecar tasks. This allows the existing logic inallocHandler.handleTaskStateUpdates
to shutdown sidecars after the main task has exited as it was designed to do.