diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index ed798c0ab2..b8a3e06c04 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -38,6 +38,7 @@ miette = { 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 952938d47a..6893cfb90d 100644 --- a/crates/openshell-driver-podman/src/client.rs +++ b/crates/openshell-driver-podman/src/client.rs @@ -479,15 +479,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. @@ -962,4 +983,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 aa3df9cbd7..79d89850de 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -682,7 +682,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)); } @@ -748,13 +751,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)), @@ -1798,9 +1802,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, ""), @@ -1820,10 +1822,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 94794bc220..ec5c8f7f11 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)