refactor(compute): unify gateway restart reconciliation - #2743
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-2743.docs.buildwithfern.com/openshell |
2f74bca to
a036b59
Compare
b103f36 to
f8ce804
Compare
|
Label |
Remove the Docker-specific gateway shutdown cleanup and reconcile persisted running intent through ComputeDriver::StartSandbox for Docker, Podman, and VM drivers. Explicitly stopped sandboxes remain stopped. Closes #2417 Signed-off-by: Drew Newberry <anewberry@nvidia.com>
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
f8ce804 to
9cad488
Compare
|
/ok to test 9cad488 |
Signed-off-by: Drew Newberry <anewberry@nvidia.com>
| &namespace, | ||
| &stopped_sandbox.name, | ||
| false, | ||
| Duration::from_secs(120), |
There was a problem hiding this comment.
Is there a reason that we use 120 seconds here instead of 30?
elezar
left a comment
elezar
left a comment
There was a problem hiding this comment.
Requesting changes for two lifecycle correctness issues: shutdown currently stops sandboxes serially despite the stated bounded-concurrency design, and lifecycle sweeps can issue driver RPCs from stale persisted intent. The remaining review notes are non-blocking follow-ups.
| let mut stopped = 0usize; | ||
| let mut failed = 0usize; | ||
|
|
||
| for record in records { |
There was a problem hiding this comment.
P1 — bounded shutdown concurrency is missing. This is sequential rather than bounded-concurrent. A slow Podman or VM stop blocks every subsequent sandbox, so a gateway with multiple running sandboxes can exceed its graceful-shutdown deadline. Please collect eligible records and run the stop RPCs with bounded concurrency while retaining the per-sandbox lifecycle gates, plus add the stated concurrency test. The PR description promises a concurrency bound of 16, but the implementation currently awaits each stop before starting the next one.
| } | ||
| }; | ||
|
|
||
| let phase = SandboxPhase::try_from(sandbox.phase()).unwrap_or(SandboxPhase::Unknown); |
There was a problem hiding this comment.
P1 — re-read lifecycle intent under the per-sandbox gate in both sweeps. Can we apply candidate → lifecycle gate → re-fetch → current-phase check ordering to both shutdown and startup reconciliation? We should not call the normal public lifecycle methods because they persist explicit user transitions, but their locking pattern prevents a sweep from acting on stale intent. Shutdown currently checks phase before acquiring its gate, while startup does not acquire the gate at all. An explicit stop or delete can otherwise complete after the initial snapshot and before the driver RPC, violating the guarantee that explicitly stopped sandboxes remain excluded.
|
|
||
| let records = self | ||
| .store | ||
| .list_by_type(Sandbox::object_type(), 1000, 0) |
There was a problem hiding this comment.
P2 — page through all persisted sandboxes in both lifecycle sweeps. Both lifecycle sweeps use list_by_type(..., 1000, 0), which processes only the first page. Sandboxes beyond that limit are omitted from shutdown and startup reconciliation, so their resources can remain running during gateway shutdown and will not receive startup recovery. Please page through the complete result set in both paths, or document and enforce a supported upper bound.
| .collect()) | ||
| } | ||
|
|
||
| pub async fn wait_for_sandbox_phase( |
There was a problem hiding this comment.
Minor cleanup: wait_for_healthy, wait_for_sandbox_phase, and wait_for_sandbox_exec_contains duplicate the polling, timeout, and diagnostic-output loop. Consider extracting a private E2E polling helper while retaining the descriptive public wait functions.
2 participants
Summary
Unify graceful gateway shutdown and startup for local compute through the public
ComputeDriverlifecycle RPCs. Shutdown callsStopSandboxfor running-intent Docker, Podman, and VM sandboxes without persisting an explicit user stop; startup calls the idempotentStartSandboxRPC for that retained intent. Kubernetes remains cluster-owned.Related Issue
Part of #2417
Changes
ComputeDriver::StopSandboxduring graceful gateway shutdownopenshell sandbox stopComputeDriver::StartSandboxbefore startup watch processingTesting
mise run pre-commitcargo test -p openshell-server shutdown_stop_sweep --libcargo test -p openshell-server start_persisted_sandboxes --libOPENSHELL_E2E_DOCKER_TEST=gateway_start mise run e2e:dockermise run test: all completed suites passed except the unrelatedgateway_completer_returns_empty_when_no_config, which reproduces in isolation because this host has registered system gateway configurationChecklist