Repository navigation
fix(server): defer reconciliation during sandbox lifecycle operations - #4983
TaylorMutch wants to merge 1 commit into
Conversation
Signed-off-by: Taylor Mutch <taylormutch@gmail.com>
|
Label |
|
Hey @TaylorMutch, I reproduced this on main at This comment just puts the two cases side by side: the race without #4983, and the fixed outcome with #4983. The loop used to reproduce it is below. The full experiment definition is in sandbox-stop-start.yaml. The 57.5 s delay aims each start at the next sweep. The gateway starts a Two runs of that loop made 248 sandbox starts, and 7 failed with The loop is an experiment on the chaos harness I was playing with, and proposed in mpereira#1. The mechanism matches the issue's Investigation, and nothing below is a problem with #4983. What this comment adds is how often the race hits when the starts are aimed at the sweep, the timing of one hit, and the (correct) runs against #4983. How to reproduceThe experiments are against local Docker only for now. A run takes about 26 minutes, or 41 when it hits the race, because a failed sandbox then waits out the experiment's 40-minute instance timeout. The first run also builds the gateway. git clone -b feat/chaos-experiment-harness/mpereira https://github.com/mpereira/OpenShell
cd OpenShell
mise run e2e:chaos e2e/chaos/experiments/sandbox-stop-start.yaml
To run it against #4983, cherry-pick the PR on top first. The branch is main git fetch https://github.com/NVIDIA/OpenShell pull/4983/head && git cherry-pick FETCH_HEADResult
Two notes on the table. The #4983 rows use the PR's head In the gateway log, each failed start leaves two lines at the moment of the hit: The gateway's Prometheus metrics, captured by the chaos tool every 2 s, show nothing wrong. There is one later trace. When the counter in a failed sandbox finishes its 1500 ticks, 20 minutes on, the gateway drops the new supervisor's exit report as stale, because the record still names the old instance. So the sandbox never reaches The 1 in 35 rate comes from aiming the starts at the sweep. Without that aim, an earlier version of the loop (the same stop and start per sandbox, but no wait for the sweep) hit the race once in about 1,300 starts, on main One hit in detail, from run
The journal in that sandbox has all 1500 lines and the script exited 0, so the workload itself restarted fine. Timeline on main: run
|
|
@TaylorMutch, I have a follow-up for #4932 at #5237, currently based on #5204. It overlaps #4983’s reconciliation guard and adds atomic start ownership and retry handling. I propose landing #4983 first, then reducing my PR to the remaining changes. Would you prefer that sequence or folding those additions into #4983? |
Summary
Prevent reconciliation from failing a sandbox restart with the previous runtime's
ContainerExitedobservation. Release Dev's Docker SSH identity test exposed this race when a sweep observed the stopped container after the gateway had already committedStarting.Related Issue
Closes #4981
Changes
Release Notes
Testing
2c31ac7e606c4e175288cee3656b23192a1a36b0, using a freshly built workload fixture. A temporary wrapper routes containers throughhost.docker.internalfor Docker Desktop; repository test code is unchanged.Checklist