From 21e6f9c6b0a67c0150eac519b9690a18c8529ba1 Mon Sep 17 00:00:00 2001 From: Vineeth Sai Narajala Date: Fri, 30 Oct 2026 16:31:26 +0000 Subject: [PATCH] fix(server): keep sandboxes created by another compute driver across 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 #4769 Signed-off-by: Vineeth Sai Narajala --- crates/openshell-server/src/compute/mod.rs | 377 ++++++++++++++++++- docs/how-it-works/gateways/configuration.mdx | 2 + 2 files changed, 359 insertions(+), 20 deletions(-) diff --git a/crates/openshell-server/src/compute/mod.rs b/crates/openshell-server/src/compute/mod.rs index 215b864e4b..6cd5c279e9 100644 --- a/crates/openshell-server/src/compute/mod.rs +++ b/crates/openshell-server/src/compute/mod.rs @@ -71,6 +71,12 @@ pub const COMPUTE_DRIVER_ANNOTATION: &str = "internal.openshell.ai/compute-drive 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 +/// `COMPUTE_DRIVER_ANNOTATION`, which is the sandbox-authentication binding and +/// is written only for drivers that authenticate sandboxes, this is recorded +/// for every sandbox so that a gateway whose compute driver was changed leaves +/// the other driver's records alone. +const CREATED_BY_DRIVER_ANNOTATION: &str = "internal.openshell.ai/created-by-compute-driver"; /// Durable intent of an in-flight `Starting` transition that resumes retained /// state. Bound to the runtime generation it was written for so retries and /// gateway-restart recovery send the same driver request, and so a stale value @@ -1194,6 +1200,31 @@ impl ComputeRuntime { Ok(()) } + /// The configured compute driver that owns `sandbox`, when that is not this + /// gateway's driver. Records that name no creating driver (created before + /// it was recorded) fall back to the sandbox-authentication binding and + /// otherwise count as this driver's. + fn other_driver_owner<'a>(&self, sandbox: &'a Sandbox) -> Option<&'a str> { + let annotations = &sandbox.metadata.as_ref()?.annotations; + annotations + .get(CREATED_BY_DRIVER_ANNOTATION) + .or_else(|| annotations.get(COMPUTE_DRIVER_ANNOTATION)) + .map(String::as_str) + .filter(|owner| !owner.is_empty() && *owner != self.configured_driver_name()) + } + + /// 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> { + if let Some(owner) = self.other_driver_owner(sandbox) { + return Err(Status::failed_precondition(format!( + "sandbox was created by compute driver '{owner}', but this gateway uses '{}'; switch the gateway back to '{owner}' to use it, or delete it", + self.configured_driver_name() + ))); + } + Ok(()) + } + fn driver_sandbox_for_launch(&self, sandbox: &Sandbox) -> Result { let mut driver_sandbox = driver_sandbox_from_public(sandbox, &self.driver_info.name) .map_err(|status| *status)?; @@ -1209,6 +1240,7 @@ impl ComputeRuntime { launch_authentication: Vec, expected_runtime_identity: String, ) -> Result { + self.validate_compute_driver_binding(sandbox)?; self.validate_isolation_backend_binding(sandbox)?; if self.isolation_backend.is_some() { self.validate_launch_signer_configured(!launch_authentication.is_empty())?; @@ -1571,6 +1603,10 @@ impl ComputeRuntime { ISOLATION_BACKEND_ANNOTATION.into(), self.isolation_backend_name().into(), ); + metadata.annotations.insert( + CREATED_BY_DRIVER_ANNOTATION.into(), + self.configured_driver_name().into(), + ); // Strip the staging token from the public sandbox before anything // persists it: the object store copy is readable by every member of the @@ -2259,6 +2295,7 @@ impl ComputeRuntime { .await .map_err(|e| Status::internal(format!("fetch sandbox failed: {e}")))? .ok_or_else(|| Status::not_found("sandbox not found"))?; + self.validate_compute_driver_binding(&candidate)?; self.validate_isolation_backend_binding(&candidate)?; if self.isolation_backend.is_some() { self.validate_launch_signer_configured(authority.is_some())?; @@ -3329,25 +3366,38 @@ impl ComputeRuntime { self.sandbox_watch_bus.notify(&target.sandbox_id); drop(guard); - let result = self - .driver - .call( - openshell_otel::rpc::DELETE_SANDBOX, - Some(transition.deleting.object_id()), - |driver| { - let sandbox_id = transition.deleting.object_id().to_string(); - let sandbox_name = transition.deleting.object_name().to_string(); - async move { - driver - .delete_sandbox(Request::new(DeleteSandboxRequest { - sandbox_id, - name: sandbox_name, - })) - .await - } - }, - ) - .await; + let result = if let Some(owner) = self.other_driver_owner(&transition.deleting) { + // The active driver does not own this sandbox's resources, so do + // not hand it an ID it never created. Remove only the record. + info!( + sandbox_id = %target.sandbox_id, + owner_driver = owner, + driver = self.configured_driver_name(), + "Deleting the record of a sandbox created by another compute driver; resources held by that driver are not released" + ); + Ok(tonic::Response::new( + openshell_core::proto::compute::v1::DeleteSandboxResponse { deleted: false }, + )) + } else { + self.driver + .call( + openshell_otel::rpc::DELETE_SANDBOX, + Some(transition.deleting.object_id()), + |driver| { + let sandbox_id = transition.deleting.object_id().to_string(); + let sandbox_name = transition.deleting.object_name().to_string(); + async move { + driver + .delete_sandbox(Request::new(DeleteSandboxRequest { + sandbox_id, + name: sandbox_name, + })) + .await + } + }, + ) + .await + }; match result { Ok(response) => { @@ -4118,6 +4168,15 @@ impl ComputeRuntime { if !sandbox_phase_should_be_running(phase) && !recoverable_error { continue; } + if let Some(owner) = self.other_driver_owner(&sandbox) { + info!( + sandbox_id, + owner_driver = owner, + driver = self.configured_driver_name(), + "Not starting sandbox created by another compute driver" + ); + continue; + } if let Err(error) = self.validate_isolation_backend_binding(&sandbox) { warn!(sandbox_id, %error, "Rejected recovered isolation backend selection"); failed += 1; @@ -6446,6 +6505,19 @@ impl ComputeRuntime { } let sandbox = decode_sandbox_record(¤t_record)?; + if let Some(owner) = self.other_driver_owner(&sandbox) { + // The active driver cannot list what another driver created, so + // its absence from the snapshot says nothing about the + // sandbox. Keep the record unchanged for its own driver. + debug!( + sandbox_id = %sandbox.object_id(), + sandbox_name = %sandbox.object_name(), + owner_driver = owner, + driver = self.configured_driver_name(), + "Keeping sandbox created by another compute driver" + ); + return Ok(()); + } if provisioning_deadline::driver_operation_pending(&sandbox) { return Ok(()); } @@ -15105,6 +15177,271 @@ mod tests { ); } + fn sandbox_record_created_by( + id: &str, + name: &str, + phase: SandboxPhase, + driver_name: &str, + ) -> Sandbox { + let mut sandbox = sandbox_record(id, name, phase); + sandbox.metadata.as_mut().unwrap().annotations.insert( + CREATED_BY_DRIVER_ANNOTATION.to_string(), + driver_name.to_string(), + ); + sandbox + } + + async fn assert_no_driver_delete(driver: &ControlledDriver) { + assert!( + tokio::time::timeout(Duration::from_millis(100), driver.delete_started.notified()) + .await + .is_err(), + "no driver delete may be sent" + ); + assert_eq!(driver.delete_calls(), 0); + } + + #[tokio::test] + async fn prune_missing_sandbox_keeps_records_of_another_compute_driver() { + // Regression test for #4769: after the gateway's compute driver is + // changed (vm -> docker), the new driver cannot list what the old one + // created. Those records must survive the sweep unchanged. + let driver = ControlledDriver::new(); + driver.set_get_outcome(ControlledGetOutcome::Missing); + let runtime = test_runtime_for_driver(driver.clone(), "docker").await; + let kept = [ + ("sb-stopped", SandboxPhase::Stopped), + ("sb-error", SandboxPhase::Error), + ("sb-provisioning", SandboxPhase::Provisioning), + ]; + for (id, phase) in kept { + runtime + .store + .put_message(&sandbox_record_created_by(id, id, phase, "vm")) + .await + .unwrap(); + } + // Records created before the creating driver was recorded still name + // their driver through the sandbox-authentication binding. + let mut bound = sandbox_record("sb-bound", "sb-bound", SandboxPhase::Stopped); + bound.metadata.as_mut().unwrap().annotations.insert( + COMPUTE_DRIVER_ANNOTATION.to_string(), + "kubernetes".to_string(), + ); + runtime.store.put_message(&bound).await.unwrap(); + + // Without the fix the first sweep moves Stopped to Error and the + // second removes the record and sends DeleteSandbox. + for _ in 0..2 { + runtime + .reconcile_store_with_backend(Duration::ZERO) + .await + .unwrap(); + } + + for (id, phase) in kept + .into_iter() + .chain([("sb-bound", SandboxPhase::Stopped)]) + { + let stored = runtime + .store + .get_message::(id) + .await + .unwrap() + .unwrap_or_else(|| panic!("{id} must be kept")); + assert_eq!(SandboxPhase::try_from(stored.phase()).unwrap(), phase); + assert!( + stored + .status + .as_ref() + .is_none_or(|status| status.conditions.is_empty()), + "{id} must not gain a condition" + ); + } + assert_no_driver_delete(&driver).await; + } + + #[tokio::test] + async fn prune_missing_sandbox_still_prunes_records_of_the_configured_driver() { + let driver = ControlledDriver::new(); + driver.set_get_outcome(ControlledGetOutcome::Missing); + let runtime = test_runtime_for_driver(driver.clone(), "docker").await; + let sandbox = + sandbox_record_created_by("sb-1", "sandbox-a", SandboxPhase::Stopped, "docker"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime + .reconcile_store_with_backend(Duration::ZERO) + .await + .unwrap(); + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + + runtime + .reconcile_store_with_backend(Duration::ZERO) + .await + .unwrap(); + assert!( + runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .is_none() + ); + tokio::time::timeout(Duration::from_secs(1), driver.delete_started.notified()) + .await + .expect("background driver cleanup did not run"); + assert_eq!( + driver.delete_requests(), + vec![("sb-1".to_string(), "sandbox-a".to_string())] + ); + } + + #[tokio::test] + async fn prune_missing_sandbox_treats_records_without_a_driver_as_the_configured_drivers() { + // Records written before the creating driver was recorded keep the + // earlier behaviour: a missing retained resource moves to Error. + let driver = ControlledDriver::new(); + driver.set_get_outcome(ControlledGetOutcome::Missing); + let runtime = test_runtime_for_driver(driver.clone(), "docker").await; + let sandbox = sandbox_record("sb-1", "sandbox-a", SandboxPhase::Stopped); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime + .reconcile_store_with_backend(Duration::ZERO) + .await + .unwrap(); + + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Error + ); + let ready = stored + .status + .as_ref() + .and_then(|status| status.conditions.iter().find(|c| c.r#type == "Ready")) + .expect("Ready condition present"); + assert_eq!(ready.reason, "ComputeResourceMissing"); + } + + #[tokio::test] + async fn create_sandbox_records_the_creating_compute_driver() { + let runtime = test_runtime_for_driver(Arc::new(TestDriver::default()), "vm").await; + let sandbox = sandbox_record("sb-1", "sandbox-a", SandboxPhase::Provisioning); + + runtime.create_sandbox(sandbox, None, false).await.unwrap(); + + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + let annotations = &stored.metadata.as_ref().unwrap().annotations; + assert_eq!( + annotations + .get(CREATED_BY_DRIVER_ANNOTATION) + .map(String::as_str), + Some("vm") + ); + // The sandbox-authentication binding is unchanged for drivers that do + // not authenticate sandboxes. + assert!(!annotations.contains_key(COMPUTE_DRIVER_ANNOTATION)); + } + + #[tokio::test] + async fn delete_sandbox_of_another_compute_driver_removes_only_the_record() { + let driver = ControlledDriver::new(); + let runtime = test_runtime_for_driver(driver.clone(), "docker").await; + let sandbox = sandbox_record_created_by("sb-1", "sandbox-a", SandboxPhase::Stopped, "vm"); + runtime.store.put_message(&sandbox).await.unwrap(); + runtime.sandbox_index.update_from_sandbox(&sandbox); + + let result = runtime + .delete_sandbox("default", "sandbox-a") + .await + .unwrap(); + + assert_eq!( + result.outcome, + openshell_core::proto::DeletionOutcome::Completed + ); + assert!( + runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .is_none() + ); + assert_no_driver_delete(&driver).await; + } + + #[tokio::test] + async fn start_persisted_sandboxes_skips_records_of_another_compute_driver() { + let driver = ControlledDriver::new(); + let runtime = test_runtime_with_gateway_managed_lifecycle(driver.clone(), "docker").await; + let sandbox = sandbox_record_created_by("sb-1", "sandbox-a", SandboxPhase::Ready, "vm"); + runtime.store.put_message(&sandbox).await.unwrap(); + + runtime.start_persisted_sandboxes().await.unwrap(); + + assert_eq!(driver.start_calls(), 0); + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Ready + ); + } + + #[tokio::test] + async fn start_sandbox_of_another_compute_driver_fails_precondition() { + let driver = ControlledDriver::new(); + let runtime = test_runtime_for_driver(driver.clone(), "docker").await; + let sandbox = sandbox_record_created_by("sb-1", "sandbox-a", SandboxPhase::Stopped, "vm"); + runtime.store.put_message(&sandbox).await.unwrap(); + runtime.sandbox_index.update_from_sandbox(&sandbox); + + let error = runtime + .start_sandbox("default", "sandbox-a") + .await + .unwrap_err(); + + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("compute driver 'vm'")); + assert_eq!(driver.start_calls(), 0); + let stored = runtime + .store + .get_message::("sb-1") + .await + .unwrap() + .unwrap(); + assert_eq!( + SandboxPhase::try_from(stored.phase()).unwrap(), + SandboxPhase::Stopped + ); + } + #[tokio::test] async fn prune_sweep_does_not_block_on_a_stuck_driver_delete_call() { // Regression test: the prune sweep's driver cleanup must not be @@ -17124,7 +17461,7 @@ mod tests { let driver = ControlledDriver::new(); driver.set_runtime_identity("new-runtime-identity"); let mut runtime = - test_runtime_with_gateway_managed_lifecycle(driver.clone(), "arbitrary").await; + test_runtime_with_gateway_managed_lifecycle(driver.clone(), "test-driver").await; enable_runtime_identity_binding(&mut runtime); let mut sandbox = sandbox_record("sb-1", "sandbox", SandboxPhase::Ready); set_compute_runtime_binding(&mut sandbox, "previous-runtime-identity"); diff --git a/docs/how-it-works/gateways/configuration.mdx b/docs/how-it-works/gateways/configuration.mdx index 715135e63f..b940cbbd3e 100644 --- a/docs/how-it-works/gateways/configuration.mdx +++ b/docs/how-it-works/gateways/configuration.mdx @@ -102,6 +102,8 @@ version = 2 The gateway selector is `compute_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. + ## Additional Destination CAs Use the global supervisor network table to trust private certificate authorities