Repository navigation
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 <dgude@nvidia.com>
|
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.
| )); | ||
| } | ||
| () = &mut shutdown_requested => { | ||
| running.begin_stop(); |
There was a problem hiding this comment.
Two questions about the scope of this:
-
shutdown_requestedisn't only SIGTERM. For hosted instances in a shared supervisor it also fires onDeparture::StopfromSandboxInstanceHost(sandbox_instance_host.rs:440,:685). In that case the supervisor process isn't exiting and the remote boundary is usually still up. Before this change, a transient connection drop during that stop was recovered, and the boundary got a cleanTerminateBoundary. Now the instance reportsFailed,TerminateBoundaryis never sent, and boundary cleanup is left to the driver. Is that intended? If not, shouldbegin_stop()be limited to the dedicated-process signal path, or use a short recovery window rather than none? -
The flag is client-wide and never cleared, so once it's set nothing on this client reconnects. That includes the workload's network mediation during the TERM grace window, which is exactly the "slow to handle TERM, running cleanup handlers" scenario the un-ignored conformance test describes. If the connection really does drop in that window, the workload's cleanup traffic fails instead of recovering. Probably rare, but it's a behavior change the PR description doesn't mention.
There was a problem hiding this comment.
- Not intended.
begin_stop()now only runs when a dedicated supervisor gets its own SIGTERM. Shared-modeDeparture::Stopis back to recovering and sendingTerminateBoundaryas before. - Fair point. In dedicated mode the flag still applies during the TERM grace window, so a connection drop there won't reconnect. I think that's acceptable since the process exits within 8s anyway, and I'll call it out in the PR description.
| if self.stopping.load(Ordering::Acquire) { | ||
| return Err(BackendError::Unavailable( | ||
| "boundary connection lost while stopping".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
When this fires during the supervisor's shutdown, BoundaryProcess::wait() maps it to Terminated("boundary lost during wait: ..."). Then the .map_err(...)? after the 5s wait in run_sandbox_instance returns Err. The supervisor exits with an error and skips publish_main_exit, the OTLP relay flush, and the gateway exit report.
That's no worse than main, where it got SIGKILLed. But now that we fail fast, should the supervisor treat "boundary lost while stopping" as the workload having ended? It could record an exit status, skip TerminateBoundary, and continue through normal teardown. Otherwise every Podman/K8s delete where the boundary goes first ends with an error-level failure from the supervisor on a routine operation.
There was a problem hiding this comment.
Done. A lost boundary (Unavailable or Terminated) while stopping is now treated as the workload having ended, with exit code 143. The supervisor then skips TerminateBoundary and continues through publish_main_exit and the OTLP flush. The 8s deadline expiring is handled the same way, so a routine delete no longer ends in an 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 <dgude@nvidia.com>
|
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 |
Summary
After SIGTERM, the supervisor kept retrying a lost boundary connection for up to 30s on every shutdown call, so it outlived the runtime's stop timeout and was SIGKILLed. Once shutdown starts, a lost connection is now reported instead of recovered.
Related Issue
Closes #5025
Changes
openshell-sandbox-backend: addRunningBoundary::begin_stop(); after it,recover_after_unavailablereturns an error instead of retrying reattach.openshell-supervisor: callbegin_stop()when SIGTERM wins the lifecycle select.lifecycle::delete_is_prompt_when_workload_is_slow_to_handle_term.Testing
cargo test -p openshell-sandbox-backend -p openshell-supervisor, clippy-D warnings, and fmt passrust-podman-cirunner): the un-ignored test passed 24/24 with the fix vs 9/24 on main (15 runs had the supervisor SIGKILLed after 15s); full conformance suite 16/16test:e2e)Checklist