Run task shutdown_delay regardless of service registration #7663
Conversation
| // before waiting to kill task | ||
| if delay := tr.Task().ShutdownDelay; delay != 0 { | ||
| tr.logger.Debug("waiting before killing task", "shutdown_delay", delay) | ||
| <-time.After(delay) |
There was a problem hiding this comment.
This is the equivalent of time.Sleep(delay).
Also, should we check for tr.shutdownCtx and exit? Depending on long this command is, we may lock up the agent until process is killed before agent exits. This matches current behavior, so not too much of a concern.
There was a problem hiding this comment.
I think shutdownCtx is used to exit task runner without affecting the task state, so handleKill won't be invoked by shutdownCtx, killCtx.Done() case invokes handleKill so I think we are stuck waiting
There was a problem hiding this comment.
Let's manually (automated around timing is hard and false negative/positive prone) test your assertion and leave the result in a comment. I would imagine everyone who comes across this code in the future will wonder why a context isn't also checked.
There was a problem hiding this comment.
-dev agent running a job with shutdown delay of 50s hangs for 50 before finishing shutdown
^C==> Caught signal: interrupt
2020-04-10T11:48:11.367-0400 [DEBUG] http: shutting down http server
2020-04-10T11:48:11.367-0400 [INFO] agent: requesting shutdown
2020-04-10T11:48:11.367-0400 [INFO] client: shutting down
2020-04-10T11:48:11.367-0400 [DEBUG] client.alloc_runner.runner_hook.group_services: waiting before removing group service: alloc_id=160da727-bb2d-5ff1-d101-2bdbf6db2a12 shutdown_delay=50s
same job running on a client kill -9 pid exits immediately with job still running
→ docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
45f5de1841df hashicorpnomad/counter-dashboard:v1 "./dashboard-service" About a minute ago Up About a minute 9002/tcp dashboard-213af190-7ea9-d1ed-e58b-06219d0fdfec
9cb712bf23e5 hashicorpnomad/counter-api:v1 "./counting-service" About a minute ago Up About a minute 9001/tcp web-213af190-7ea9-d1ed-e58b-06219d0fdfec
task shutdown_delay will currently only run if there are registered services for the task. This implementation detail isn't explicity stated anywhere and is defined outside of the service stanza. This change moves shutdown_delay to be evaluated after prekill hooks are run, outside of any task runner hooks. just use time.sleep
drewbailey
force-pushed
the
b-taskrunner-shutdown_delay
branch
from
3c322b8
to
e288658
Compare
|
Sorry I'm a bit late on this PR, just saw it was merged, but question @drewbailey: since this is an additive check, should we expect tasks with both a group |
|
Hey @djenriquez, yes, that is currently the case. I'll create a follow up ticket to ensure its properly called out in the docs. |
|
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. |
task shutdown_delay will currently only run if there are registered
services for the task. This implementation detail isn't explicity stated
anywhere and is defined outside of the service stanza.
This change moves shutdown_delay to be evaluated after prekill hooks are
run, outside of any task runner hooks.
fixes #7271