Repository navigation
fix(server): skip the reconcile poll of a sandbox whose start is in progress - #5265
Open
matthewgrossman wants to merge 4 commits into
Open
matthewgrossman wants to merge 4 commits into
matthewgrossman wants to merge 4 commits into
Conversation
The gateway's reconcile sweep applies polled snapshots without waiting for an in-progress start. During a stop/start restart, get_sandbox and list_sandboxes built the stopped workload from its list entry, so the previous run's exit was reported as ContainerExited and moved the sandbox from Starting to Error. While a start is in progress, inspect a stopped workload before reporting it. If it is running again, or still shows the exit recorded before the start, report it as ContainerStarting. A new exit, and any exit once the start has returned, is still reported. The start is tracked by a guard that clears the in-progress mark on every return path. Closes #5258 Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
Label |
4 tasks
A restart starts the workload before its exited supervisor. Watch reconciliation, start events, and polled snapshots could inspect the workload in that gap, see a running workload beside an exited supervisor, and stop it. Record the supervisor exit that the start observed. While that start is in progress, an exited supervisor with the recorded FinishedAt reports the workload as starting instead of containing it. A missing supervisor, a new supervisor exit, and any exit after the start returns are still contained. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
4 tasks
…rogress StartSandbox holds the sandbox lifecycle gate for the whole driver call, and the workload can still show its previous exit until the driver starts it. The watch path already waits on that gate for Starting sandboxes; the reconcile sweep did not, so a poll could move a restarting sandbox from Starting to Error. Skip such a sandbox and let the next tick or event report the settled state. This replaces the Podman-specific fence of polled snapshots. Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
1 task done
3 tasks
2 of 15 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #5258. The gateway's reconcile sweep no longer polls a sandbox while its
StartSandboxcall is in progress. A poll that lands mid-start could move thesandbox from
StartingtoError(ContainerExited) even though itscontainers came up.
Root cause
StartSandboxholds the sandbox's lifecycle gate for the whole driver call. OnPodman that call stops the supervisor, copies restart metadata, starts the
workload, then starts the supervisor, which takes roughly 500 ms. Until the
driver starts the workload again, it still shows its previous
exitedstate.The two paths that apply driver snapshots treat this differently:
apply_sandbox_updatesees aStartingsandbox, waits on thelifecycle gate, and re-reads the driver before applying anything.
reconcile_snapshot_sandboxpolls the driver and appliesthe result under only the local mutation lock. It never looks at the gate.
A sweep tick inside the start window therefore applied a stale
Exitedsnapshot. The phase merge lets
ErrorreplaceStarting, and the SDK'swait_readystops at the firstError.Fix
If the stored phase is
Starting, the sweep takes the lifecycle gate withtry_lock_for. If a start holds the gate, the sweep skips that sandbox. Itnever waits, so a slow start cannot stall the sweep. The next tick or watch
event reports the settled state, so a genuine exit of the restarted workload is
still reported, just after the start returns. The sweep holds the gate while it
polls, so a start cannot begin between the check and the poll. The same check is
applied to the prune path.
How the PR got here
This PR first fixed the problem in the Podman driver, which was the wrong layer:
get_sandboxandlist_sandboxesinspected stopped containers during astart and rewrote a snapshot that still showed the previous
FinishedAtasContainerStarting.inspect_workloadso a restart would notcontain its own workload while the supervisor was still exited.
That approach worked but was more complex than it needed to be:
FinishedAttimestamps in three places that had to stayconsistent:
matches_previous_exit,polled_workload_still_starting, andsupervisor_restart_in_progress.inspectcalls to the poll path.driver, and Docker has the same shape (bug(docker): delayed exit snapshot regresses restarted sandbox from Ready to Error #4129).
inspect_workloadcontainment fence isa separate race and was not part of bug(podman): polled snapshot moves a restarting sandbox from Starting to Error (ContainerExited) #5258.
The gateway already had the right mechanism for the watch path. The sweep was
the only path that skipped it, so the final commit reverts the Podman driver
changes and applies the gate in the sweep instead. The net diff against the
base is
crates/openshell-server/src/compute/mod.rsonly.Follow-up
Whether a restart can contain its own workload in
inspect_workloadisunconfirmed and is tracked separately. It is out of scope here.
Testing
reconcile_sweep_skips_a_sandbox_while_its_start_is_in_progress. A sweepruns during a blocked start while the driver reports
ContainerExited. Itasserts the sandbox stays
Starting. It fails without the fix (phase 8instead of
Starting).cargo test -p openshell-server --lib compute::passes (403 passed, 3 ignored).persistent_workspace_survives_stop_starthas notbeen run.