Repository navigation
fix(supervisor): bound dedicated SIGTERM shutdown and stop recovering a lost boundary - #5342
Conversation
After SIGTERM the supervisor signals, waits for, and terminates the boundary. When the boundary connection drops during that sequence, each call retried reattach for up to 30s, so the supervisor outlived the runtime's stop timeout and was killed. Once shutdown starts, report a lost connection instead of recovering it. Un-ignore the conformance test that covers slow-TERM deletes. Signed-off-by: divesh <[email protected]>
FrostGod
added
test:e2e
|
Label |
|
/ok to test 235e738 |
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. The Podman numbers (24/24 vs 9/24) are convincing for the case where the boundary dies alongside the supervisor. I'm not convinced this fully fixes the supervisor half of #5025 yet, though.
The Docker E2E failure looks like the same bug class. docker-e2e / E2E (rust-docker) failed in ssh_host_identity_survives_restarts_and_changes_after_recreation. Every other StopSandbox in that run took 142-155 ms, including other sandboxes on the same e2e-python image. The one in this test took 5,167 ms. That's Docker's 5s stop timeout on the supervisor container (stop_container(...t(5)) in the Docker driver). Right after, the reconciler moved the sandbox to Error / ContainerExited and sandbox start failed. On Docker the driver stops the supervisor before the workload container, so the boundary is still alive during shutdown and the new stopping path shouldn't be what's slow. In other words, a supervisor still failed to exit within the runtime's stop timeout on this branch. I don't know yet whether main does the same. Could you get a baseline of that test on main?
Overall shutdown deadline. This fixes one way shutdown can hang (retrying reattach to a dead boundary), but nothing bounds the total time. Signal, the 5s wait, terminate, and TerminateBoundary can each still take up to their own timeouts. Would it be cleaner to put one deadline on the whole SIGTERM path, shorter than the runtime grace periods (Docker 5s, Podman stop timeout, K8s 30s), and abandon whatever is left when it expires? That would cover this case, whatever happened on Docker above, and future causes. The stopping flag could stay as a fast path inside it.
Comments inline on the scope of the flag and on how the supervisor handles the resulting error.
Stopping a dedicated sandbox now finishes within 8 seconds, including the exit report and trace flush, while the workload keeps its 5 second TERM grace. A boundary connection lost while stopping is reported as a terminated workload instead of failing the shutdown. Only a dedicated supervisor's own shutdown skips boundary recovery; a shared supervisor removing one sandbox keeps recovering as before. The Docker driver gave the supervisor 5 seconds, the same as its wait for the workload, so stops ended in SIGKILL. It now allows 10 seconds. Signed-off-by: divesh <[email protected]>
|
Baseline on main: the test passes there too (9/9), but every supervisor stop ended in a Docker SIGKILL on both main and this branch (24/24). The default agent is an interactive bash that ignores SIGTERM. The supervisor waits 5s for it, and Docker only gave the supervisor 5s, so the CI failure was that existing race going the wrong way, not something this PR introduced. Agreed on one deadline. The whole SIGTERM path is now capped at 8s: the agent gets its 5s TERM grace, and the exit report and OTLP flush fit in what's left. I also raised Docker's supervisor stop timeout to 10s so the cap fits under it. Kubernetes (30s) and Podman (45s) already had room. Locally that's 0/17 Docker SIGKILLs, and Docker conformance passes 16/16. The |
|
/ok to test 91b6ccf |
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Thanks, this is much better. The overall deadline is what I was hoping for, and the main-branch baseline clears up the Docker failure. CI is green now too.
A couple of things left inline. The shared-supervisor one is the one I'd want fixed before approving. The exit-code one is small.
One non-code item: the PR description is out of date. It doesn't mention the 8s shutdown cap or the Docker supervisor stop timeout going from 5s to 10s. It's also missing the note you mentioned in the earlier thread about the stopping flag: in dedicated mode, a connection drop during the 5s TERM grace won't reconnect, and that includes the workload's own network traffic. Could you update it so reviewers and the changelog reflect what this actually changes? The title could probably mention the shutdown bound as well.
…s exit Only a dedicated supervisor's own shutdown is bounded by the 8 second deadline and treats a lost boundary as the end of the workload. A shared supervisor stopping one hosted instance keeps the host's removal grace and reports a stop it could not confirm as a failure, as before. When the boundary reports the workload's exit status before it is lost, that status is reported rather than the terminated fallback. Signed-off-by: divesh <[email protected]>
|
/ok to test 6482d53 |
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Thanks, this addresses everything from my last pass. Hosted instances now keep main's unbounded stop and report a stop they couldn't confirm as a failure. The workload's real exit status is kept when the boundary returned one. The README no longer overclaims the bound. Fine with bounding the exporter shutdowns in a follow-up.
The two Podman failures on this commit (podman-e2e / rust-podman-ci and conformance-integration / fedora-podman-rootless) are both persistent_workspace_survives_stop_start going to Error with an empty ContainerExited right after start. That's #5258. The same test failed the same way in the same fedora-podman-rootless lane on #5280, which doesn't touch stop logic, and the fix (#5265) hasn't merged yet. A rerun should clear it, so I'm not holding this on it.
One thing before merging: please update the PR description and title. They still describe only the first commit. It'd be good to cover the 8s shutdown bound for dedicated supervisors, the Docker supervisor stop timeout going from 5s to 10s, hosted instances keeping their existing stop behavior, and the note about the stopping flag: in dedicated mode, a connection drop during the 5s TERM grace isn't recovered, and that includes the workload's own network traffic.
FrostGod
changed the title
|
updated the PR and re-ran the jobs |
… a lost boundary (NVIDIA#5342) * fix(supervisor): stop recovering a lost boundary after SIGTERM After SIGTERM the supervisor signals, waits for, and terminates the boundary. When the boundary connection drops during that sequence, each call retried reattach for up to 30s, so the supervisor outlived the runtime's stop timeout and was killed. Once shutdown starts, report a lost connection instead of recovering it. Un-ignore the conformance test that covers slow-TERM deletes. Signed-off-by: divesh <[email protected]> * fix(supervisor): bound SIGTERM shutdown and give Docker supervisors 10s Stopping a dedicated sandbox now finishes within 8 seconds, including the exit report and trace flush, while the workload keeps its 5 second TERM grace. A boundary connection lost while stopping is reported as a terminated workload instead of failing the shutdown. Only a dedicated supervisor's own shutdown skips boundary recovery; a shared supervisor removing one sandbox keeps recovering as before. The Docker driver gave the supervisor 5 seconds, the same as its wait for the workload, so stops ended in SIGKILL. It now allows 10 seconds. Signed-off-by: divesh <[email protected]> * fix(supervisor): keep shared stops unbounded and report the workload's exit Only a dedicated supervisor's own shutdown is bounded by the 8 second deadline and treats a lost boundary as the end of the workload. A shared supervisor stopping one hosted instance keeps the host's removal grace and reports a stop it could not confirm as a failure, as before. When the boundary reports the workload's exit status before it is lost, that status is reported rather than the terminated fallback. Signed-off-by: divesh <[email protected]> --------- Signed-off-by: divesh <[email protected]>
Summary
After SIGTERM, a dedicated supervisor could outlive the runtime's stop timeout and be SIGKILLed. It retried a lost boundary connection for up to 30s on each shutdown call, and nothing bounded the total shutdown time. On Docker, the supervisor's 5s wait for the workload equaled Docker's 5s supervisor stop timeout, so stopping a workload that ignores TERM always ended in SIGKILL, on main as well. A dedicated supervisor's shutdown is now bounded at 8s, and Docker gives the supervisor 10s to stop.
Related Issue
Closes #5025
Changes
openshell-supervisor: on a dedicated supervisor's own SIGTERM, stopping the instance is bounded at 8s. That covers the workload's 5s TERM grace,TerminateBoundary, the exit report, and the trace flush. Losing the boundary while stopping, or hitting the deadline, finishes shutdown instead of failing it. The workload's exit status is reported if the boundary returned one; otherwise the exit code is 143.openshell-sandbox-backend: addRunningBoundary::begin_stop(). After it is called, a lost connection is reported instead of recovered. The supervisor calls it only on a dedicated supervisor's own SIGTERM.openshell-driver-docker: raise the supervisor container stop timeout from 5s to 10s, so the 8s bound finishes before Docker kills the supervisor. Kubernetes (30s grace) and Podman (45s stop timeout) already allow more than 8s.TerminateBoundary, and reports a stop it could not confirm asFailed.lifecycle::delete_is_prompt_when_workload_is_slow_to_handle_term.Behavior change: in dedicated mode, a boundary connection that drops during the 5s TERM grace is not recovered. That includes the workload's own network traffic, for example from cleanup handlers. The supervisor exits within 8s either way.
Not covered: the OCSF exporter and OTLP tracer shut down after the 8s bound, and each can take up to 10s. With a slow or unreachable telemetry collector, the supervisor can still exceed Docker's 10s, as on main.
Testing
cargo test -p openshell-supervisor -p openshell-sandbox-backend -p openshell-driver-docker, clippy-D warnings, and fmt passpersistent_workspace_survives_stop_start20/20ssh_host_identitypasses, and every supervisor stop ends on its own in about 5s instead of a Docker SIGKILL (24/24 stops SIGKILLed on main)test:e2e)The Podman failures of
persistent_workspace_survives_stop_starton this branch are #5258 (fix in #5265).Checklist