Repository navigation
fix(server): keep sandboxes created by another compute driver across a driver change - #5071
vineethsai7 wants to merge 1 commit into
Conversation
…a driver change After the gateway's compute driver changes (for example vm to docker), the reconcile sweep treated every store record the new driver did not list as missing: a Stopped record moved to Error and the next sweep deleted it and sent DeleteSandbox to the new driver. Switching back found no record. Record the configured driver that created each sandbox in a new internal.openshell.ai/created-by-compute-driver annotation. It is separate from the compute-driver annotation, which stays the sandbox-authentication binding and is still written only for drivers that authenticate sandboxes. When a record names another driver (or, for older records, carries another driver's authentication binding): - the reconcile sweep leaves it unchanged, with no driver call; - gateway startup does not restart it; - sandbox start fails with FailedPrecondition and names both drivers; - sandbox delete removes the record without calling the active driver. Records that name no driver keep the previous behaviour. Closes NVIDIA#4769 Signed-off-by: Vineeth Sai Narajala <vnarajal@cisco.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thank you for your interest in contributing to OpenShell, @vineethsai7. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
|
All contributors have signed the DCO ✍️ ✅ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21e6f9c6b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const ISOLATION_BACKEND_ANNOTATION: &str = "internal.openshell.ai/isolation-backend"; | ||
| pub const COMPUTE_RUNTIME_IDENTITY_ANNOTATION: &str = | ||
| "internal.openshell.ai/compute-runtime-identity"; | ||
| /// Configured name of the compute driver that created the sandbox. Unlike |
There was a problem hiding this comment.
The reviewed commit body has no Signed-off-by trailer and ends with Generated with Claude Code; the repository's AGENTS.md requires DCO sign-off and forbids AI attribution or “Generated with” footers, so this commit cannot land as-is.
AGENTS.md reference: AGENTS.md:L191-L192
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| let sandbox = decode_sandbox_record(¤t_record)?; | ||
| if let Some(owner) = self.other_driver_owner(&sandbox) { |
There was a problem hiding this comment.
When the driver is changed while a marked sandbox is Provisioning or Starting and its durable deadline later expires, this early return exists only in prune_missing_sandbox; provisioning_deadline::reconcile_provisioning_deadlines still claims the timeout, changes the phase to Error, and reclaim_provisioning_timeout sends StopSandbox to the newly configured driver. That automatically mutates the record this change promises to leave untouched and hands a foreign ID to the wrong backend, so apply the owner check before deadline refresh, expiry, and reclamation.
Useful? React with 👍 / 👎.
| if !sandbox_phase_should_be_running(phase) && !recoverable_error { | ||
| continue; | ||
| } | ||
| if let Some(owner) = self.other_driver_owner(&sandbox) { |
There was a problem hiding this comment.
When the driver changes while a marked sandbox has a due automatic-restart Starting transition, this startup skip does not suppress restart_loop: restart_due_sandboxes still calls restart_sandbox_runtime, which claims and updates the row and sends StopSandbox to the active driver before driver_start_request eventually performs ownership validation. The wrong backend is therefore contacted and the foreign row no longer stays unchanged; skip it before the restart claim.
Useful? React with 👍 / 👎.
|
|
||
| /// Refuses lifecycle work on a sandbox another compute driver created: this | ||
| /// gateway's driver does not own its resources. | ||
| fn validate_compute_driver_binding(&self, sandbox: &Sandbox) -> Result<(), Status> { |
There was a problem hiding this comment.
When a retained VM sandbox is Ready while a Docker or Podman gateway shuts down, stop_persisted_sandboxes_on_shutdown does not call this validation and sends StopSandbox for the foreign ID; the normal NotFound response is counted as a failure and makes cleanup_on_shutdown return an error. recover_persisted_lifecycle_transitions has the same gap for Stopping or Suspending rows, so these background quiesce paths should skip records returned by other_driver_owner.
Useful? React with 👍 / 👎.
|
|
||
| The gateway selector is `compute_driver = "<driver>"`. It accepts one scalar driver name. Omit it to retain auto-detection. | ||
|
|
||
| Changing the compute driver keeps the sandboxes the previous driver created. The gateway records the creating driver on each sandbox and leaves those sandboxes in their last phase: it does not prune them, and `openshell sandbox start` refuses them. Switch back to the creating driver to use them again. `openshell sandbox delete` removes the gateway record only; resources that the other driver holds, such as a VM disk, stay until you delete the sandbox from a gateway that runs its driver. Sandboxes created before OpenShell recorded the creating driver are treated as belonging to the current driver. |
There was a problem hiding this comment.
The final sentence says every sandbox created before the new annotation is assigned to the current driver, but other_driver_owner explicitly falls back to COMPUTE_DRIVER_ANNOTATION; legacy Kubernetes or other authenticated records naming the previous driver are retained, and the new regression test asserts that behavior. Qualify this sentence so it applies only to legacy records lacking both annotations.
AGENTS.md reference: AGENTS.md:L230-L230
Useful? React with 👍 / 👎.
|
I have read the DCO document and I hereby sign the DCO. |
|
Superseded by #5133. |
Summary
After a gateway's compute driver is changed (for example
vmtodocker), the reconcile sweep treated every sandbox the new driver could not list as missing. A stopped sandbox moved toErrorand was deleted on the next sweep, so switching back to the old driver found the VM disk but no sandbox. This PR records the creating driver on every new sandbox and leaves sandboxes of another driver alone.Related Issue
Closes #4769
Downstream report: cisco-ai-defense/defenseclaw#1220
We saw that @letv1nnn asked to take #4769. We are happy to merge efforts: fold this into their PR, or the reverse, whichever the maintainers prefer.
Changes
reconcile_store_with_backendsends every store record that the active driver'sListSandboxesdoes not return toprune_missing_sandbox, which never checks which driver created the record. The grace period runs fromcreated_at, so any sandbox older than 5 minutes is acted on at once:Stoppedgoes toError(ComputeResourceMissing), and the next sweep deletes the record and sendsDeleteSandboxto the new driver.internal.openshell.ai/compute-driveris written only bypersist_runtime_binding, which runs only for drivers withsupports_sandbox_authentication(Kubernetes), so VM, Docker and Podman records carry no driver name (as the triage comment noted).create_sandboxnow writesinternal.openshell.ai/created-by-compute-driverwith the configured driver name for every sandbox, next to the existing isolation-backend annotation. It is a separate key on purpose:internal.openshell.ai/compute-driverstays the sandbox-authentication binding thatIssueSandboxTokencompares (grpc/auth_rpc.rs), and it is still written only bypersist_runtime_binding, so sandbox authentication is unchanged. Theinternal.openshell.ai/prefix is already reserved, so callers cannot set the new key.ComputeRuntime::other_driver_ownerreturns the creating driver when it is not the configured one. It reads the new annotation and falls back to the authentication binding, so existing Kubernetes records are covered too.prune_missing_sandboxreturns early for a sandbox of another driver: no phase change, noGetSandboxorDeleteSandboxcall, no store delete. It logs at debug level.start_persisted_sandboxesdoes not restart a sandbox of another driver. It logs once at info level per gateway start.sandbox start(and every path throughdriver_start_request) fails withFailedPreconditionand names both drivers, without a driver call. This follows the existing isolation-backend binding check.sandbox deleteof a sandbox of another driver removes the gateway record and its local state and does not call the active driver with an ID it never created. Resources held by the other driver (for example the VM disk) stay; deleting from a gateway that runs that driver releases them.docs/how-it-works/gateways/configuration.mdxsays what happens to existing sandboxes whencompute_driverchanges.Limitations:
sandbox stopandexecon a sandbox of another driver still reach the active driver, which does not know the sandbox. Stopped sandboxes, the case in the issue, are not affected. I can add the same precondition there if maintainers want it in this PR.created_at. A separate "missing since" time would help with real orphans but is not needed for this fix.Release Notes
Changing a gateway's compute driver no longer deletes sandboxes that the previous driver created. They stay in their last phase until you switch back to that driver.
openshell sandbox startrefuses them, andopenshell sandbox deleteremoves only the gateway record.Testing
Ran on RHEL 9 x86_64, Rust 1.95.0, on the commit in this PR:
cargo fmt -p openshell-server -- --check: clean.cargo clippy -p openshell-server --features vendored-z3 --all-targets: no warnings.cargo test -p openshell-server --features vendored-z3 --libwith filters forprune_missing_sandbox,reconcile_store_with_backend,start_persisted_sandboxes,delete_sandbox,prune_sweep,runtime_binding,grpc::auth_rpcand the new tests: 53 passed, 0 failed.cargo test -p openshell-server --features vendored-z3 --lib: 2527 passed, 1 failed, 33 ignored. The one failure,persistence::tests::sqlite_concurrent_writes_return_their_own_resource_version("database is locked"), fails the same way on unpatchedmainon this host and is unrelated.New unit tests in
crates/openshell-server/src/compute/mod.rs:prune_missing_sandbox_keeps_records_of_another_compute_driver: Stopped, Error and Provisioning records created byvm, plus a record with akubernetesauthentication binding, survive two zero-grace sweeps on adockergateway with phase and conditions unchanged and no driver delete.prune_missing_sandbox_still_prunes_records_of_the_configured_driver: a missing record of the configured driver still goes toError, then is removed with one driver delete.prune_missing_sandbox_treats_records_without_a_driver_as_the_configured_drivers: an unannotated record keeps today's behaviour (Error,ComputeResourceMissing).create_sandbox_records_the_creating_compute_driver: create writes the new annotation and does not write the authentication binding for a driver without sandbox authentication.delete_sandbox_of_another_compute_driver_removes_only_the_record: delete completes, the record is gone, no driver delete.start_persisted_sandboxes_skips_records_of_another_compute_driver: no start call, phase staysReady.start_sandbox_of_another_compute_driver_fails_precondition:FailedPrecondition, no start call, phase staysStopped.start_persisted_sandboxes_preserves_and_updates_runtime_bindingnow builds its runtime with the driver name that its fixture binding records (test-driver). In production the binding always holds the configured driver name; the old fixture paired atest-driverbinding with anarbitrarygateway, which the new owner check correctly treats as another driver's sandbox.Before/after (live)
Tested on RHEL 9.8 x86_64 with nested KVM and Docker, with the v0.1.6
openshellCLI andopenshell-driver-vm. Only the gateway binary changed. Each run used one state dir and one gateway: a kept vm sandbox was created, a marker file was written in it, the sandbox was stopped, and it was left until it was older than 5 minutes. Then the gateway was restarted withcompute_driver = "docker"and watched for about 7 minutes, and switched back tovm.Retained sandbox resource disappeared from the compute driver; the sandbox isErrorStopped(debug:Keeping sandbox created by another compute driver)Removing sandbox from store after it disappeared from the compute driver snapshot;sandbox listis empty from then onStoppedafter 417 s, more than 6 sweeps; noError, no deletesandbox starton dockerFailedPrecondition:sandbox was created by compute driver 'vm', but this gateway uses 'docker'; ...sandbox listempty;getandstartreturn not found, but the VM overlay disk is still on diskStopped;sandbox startreaches Ready; the marker file is intactErrorat the first sweep, deleted 60 s latersandbox deleteof the vm sandbox on the docker gateway removed only the record. The VM disk stayed, as described above. Not run live: a docker to podman switch, and a sandbox created before this change (it has no annotation, so it keeps today's behaviour).Checklist
🤖 Generated with Claude Code