From 06c2db75dcaeff0ebd7d151ce347c1ac60595d70 Mon Sep 17 00:00:00 2001 From: Piotr Mlocek Date: Fri, 31 Jul 2026 14:22:44 -0700 Subject: [PATCH] fix(podman): combine sandbox stop and removal (#2570) * fix(podman): combine sandbox stop and removal Signed-off-by: Piotr Mlocek * fix(podman): use libpod volume removal parameter Signed-off-by: Piotr Mlocek * fix(podman): preserve removal timeout margin Signed-off-by: Piotr Mlocek * test(podman): use virtual time for removal delay Signed-off-by: Piotr Mlocek --------- Signed-off-by: Piotr Mlocek --- crates/openshell-driver-podman/Cargo.toml | 1 + crates/openshell-driver-podman/src/client.rs | 89 +++++++++++++++++-- crates/openshell-driver-podman/src/driver.rs | 35 +++++--- .../openshell-driver-podman/src/test_utils.rs | 10 ++- 4 files changed, 113 insertions(+), 22 deletions(-) diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index 5f108fd9c..e46d2eed8 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -39,6 +39,7 @@ url = { workspace = true } [dev-dependencies] prost-types = { workspace = true } temp-env = "0.3" +tokio = { workspace = true, features = ["test-util"] } [lints] workspace = true diff --git a/crates/openshell-driver-podman/src/client.rs b/crates/openshell-driver-podman/src/client.rs index 5291f09ea..9fe39cf7e 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -481,15 +481,36 @@ impl PodmanClient { } } - /// Force-remove a container and its anonymous volumes. - pub async fn remove_container(&self, name: &str) -> Result<(), PodmanApiError> { + /// Remove a container in one timed, forced Libpod delete operation. + /// + /// The Libpod endpoint uses `volumes` for anonymous-volume removal. Its + /// Docker-compatible counterpart uses the shorter `v` parameter. + pub async fn remove_container( + &self, + name: &str, + timeout_secs: u32, + ) -> Result<(), PodmanApiError> { validate_name(name)?; - self.request_ok( - hyper::Method::DELETE, - &format!("/libpod/containers/{name}?force=true&v=true"), - None, - ) - .await + // The delete request covers both the graceful stop and the subsequent + // storage, network, and anonymous-volume cleanup. Preserve the normal + // API timeout as cleanup headroom after the stop grace period. + let http_timeout = Duration::from_secs(u64::from(timeout_secs)) + API_TIMEOUT; + let (status, bytes) = self + .request( + hyper::Method::DELETE, + &format!( + "/libpod/containers/{name}?force=true&volumes=true&timeout={timeout_secs}" + ), + None, + http_timeout, + ) + .await?; + let code = status.as_u16(); + if status.is_success() || code == 304 { + Ok(()) + } else { + Err(error_from_response(code, &bytes)) + } } /// Inspect a container by name or ID. @@ -982,4 +1003,56 @@ mod tests { ); let _ = std::fs::remove_file(socket_path); } + + #[tokio::test] + async fn remove_container_uses_single_timed_libpod_removal() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "remove-container", + vec![StubResponse::new(StatusCode::NO_CONTENT, "")], + ); + let client = PodmanClient::new(socket_path.clone()); + + client + .remove_container("sandbox-123", 10) + .await + .expect("container removal should succeed"); + + handle.await.expect("stub task should finish"); + assert_eq!( + request_log + .lock() + .expect("request log lock should not be poisoned") + .as_slice(), + ["DELETE /v5.0.0/libpod/containers/sandbox-123?force=true&volumes=true&timeout=10"] + ); + let _ = std::fs::remove_file(socket_path); + } + + #[tokio::test(start_paused = true)] + async fn remove_container_allows_cleanup_after_stop_timeout() { + let (socket_path, request_log, handle) = spawn_podman_stub( + "remove-container-delayed", + vec![StubResponse::new(StatusCode::NO_CONTENT, "").with_delay(Duration::from_secs(6))], + ); + let client = PodmanClient::new(socket_path.clone()); + + let removal = tokio::spawn(async move { client.remove_container("sandbox-123", 0).await }); + while request_log + .lock() + .expect("request log lock should not be poisoned") + .is_empty() + { + tokio::task::yield_now().await; + } + tokio::task::yield_now().await; + tokio::time::advance(Duration::from_secs(6)).await; + + removal + .await + .expect("removal task should finish") + .expect("container removal should retain the API timeout for cleanup"); + + handle.await.expect("stub task should finish"); + let _ = std::fs::remove_file(socket_path); + } } diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index f871caf7b..dd196f799 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -793,7 +793,10 @@ impl PodmanComputeDriver { error = %e, "Failed to start container; cleaning up" ); - let _ = self.client.remove_container(&name).await; + let _ = self + .client + .remove_container(&name, self.config.stop_timeout_secs) + .await; cleanup_created().await; return Err(ComputeDriverError::from(e)); } @@ -859,13 +862,14 @@ impl PodmanComputeDriver { }; info!(sandbox_id = %sandbox_id, container = %container_id, "Deleting sandbox container"); - // Stop (best-effort). - let _ = self + // Keep stop, timeout, and removal in one Podman operation. Splitting + // stop and remove can race with another container starting an image + // mount when the stop reaches its timeout. + let container_existed = match self .client - .stop_container(&container_id, self.config.stop_timeout_secs) - .await; - - let container_existed = match self.client.remove_container(&container_id).await { + .remove_container(&container_id, self.config.stop_timeout_secs) + .await + { Ok(()) => true, Err(PodmanApiError::NotFound(_)) => false, Err(e) => return Err(ComputeDriverError::from(e)), @@ -2202,9 +2206,7 @@ mod tests { vec![ // list_containers by label StubResponse::new(StatusCode::OK, list_body), - // stop_container - StubResponse::new(StatusCode::NO_CONTENT, ""), - // remove_container + // single timed remove_container operation StubResponse::new(StatusCode::NO_CONTENT, ""), // remove_volume StubResponse::new(StatusCode::NO_CONTENT, ""), @@ -2224,10 +2226,17 @@ mod tests { .expect("request log lock should not be poisoned") .clone(); assert!(requests[0].contains("/libpod/containers/json")); - assert!(requests[1].contains(&format!("/libpod/containers/{container_id}/stop"))); - assert!(requests[2].contains(&format!("/libpod/containers/{container_id}"))); assert_eq!( - requests[3], + requests[1], + format!( + "DELETE {}", + api_path(&format!( + "/libpod/containers/{container_id}?force=true&volumes=true&timeout=10" + )) + ) + ); + assert_eq!( + requests[2], format!( "DELETE {}", api_path(&format!("/libpod/volumes/{volume_name}")) diff --git a/crates/openshell-driver-podman/src/test_utils.rs b/crates/openshell-driver-podman/src/test_utils.rs index 94794bc22..ec5c8f7f1 100644 --- a/crates/openshell-driver-podman/src/test_utils.rs +++ b/crates/openshell-driver-podman/src/test_utils.rs @@ -13,7 +13,7 @@ use std::collections::VecDeque; use std::convert::Infallible; use std::path::PathBuf; use std::sync::{Arc, Mutex}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tokio::net::UnixListener; /// A canned HTTP response for the Podman stub server. @@ -21,6 +21,7 @@ use tokio::net::UnixListener; pub struct StubResponse { pub status: StatusCode, pub body: String, + pub delay: Duration, } impl StubResponse { @@ -28,8 +29,14 @@ impl StubResponse { Self { status, body: body.into(), + delay: Duration::ZERO, } } + + pub fn with_delay(mut self, delay: Duration) -> Self { + self.delay = delay; + self + } } /// Generate a unique Unix socket path for a test. @@ -97,6 +104,7 @@ pub fn spawn_podman_stub( .expect("response queue lock should not be poisoned") .pop_front() .expect("stub response should exist"); + tokio::time::sleep(response.delay).await; Ok::<_, Infallible>( hyper::Response::builder() .status(response.status)