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 |
| // Unignore this when the supervisor half of #5025 lands: on Podman the | ||
| // supervisor container still waits out its stop timeout. | ||
| #[testkit::test] | ||
| #[ignore = "https://github.com/NVIDIA/OpenShell/issues/5025"] |
There was a problem hiding this comment.
thanks for remembering to remove this!
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.
| 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.
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