From 52fb703e82af57460394308979add3e36c26e7cc Mon Sep 17 00:00:00 2001 From: Jorge Prendes Date: Thu, 5 Dec 2024 13:02:53 +0000 Subject: [PATCH] small cleanup Signed-off-by: Jorge Prendes --- crates/containerd-shim-wasm/CHANGELOG.md | 6 +++ .../src/sandbox/instance_utils.rs | 37 ---------------- .../src/sandbox/shim/instance_data.rs | 6 +-- .../src/sys/unix/container/instance.rs | 43 +++++++------------ 4 files changed, 24 insertions(+), 68 deletions(-) diff --git a/crates/containerd-shim-wasm/CHANGELOG.md b/crates/containerd-shim-wasm/CHANGELOG.md index 332d79a60..8552e7275 100644 --- a/crates/containerd-shim-wasm/CHANGELOG.md +++ b/crates/containerd-shim-wasm/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), ### Added - Added test for signal handling issue [#755](https://github.com/containerd/runwasi/issues/755) ([#756](https://github.com/containerd/runwasi/pull/756)) +### Changed +- Reuse and synchronise access to `Container` object instead of reloading form disk ([#763](https://github.com/containerd/runwasi/pull/763)) + +### Removed +- Removed `containerd_shim_wasm::sandbox::instance_utils::get_instance_root` and `containerd_shim_wasm::sandbox::instance_utils::instance_exists` functions ([#763](https://github.com/containerd/runwasi/pull/763)) + ## [v0.8.0] — 2024-12-04 ### Added diff --git a/crates/containerd-shim-wasm/src/sandbox/instance_utils.rs b/crates/containerd-shim-wasm/src/sandbox/instance_utils.rs index 1a5617dd2..824272c40 100644 --- a/crates/containerd-shim-wasm/src/sandbox/instance_utils.rs +++ b/crates/containerd-shim-wasm/src/sandbox/instance_utils.rs @@ -3,35 +3,10 @@ use std::fs::File; use std::io::ErrorKind; use std::path::{Path, PathBuf}; -use anyhow::{bail, Context, Result}; use serde::{Deserialize, Serialize}; use super::Error; -/// Return the root path for the instance. -/// -/// The root path is the path to the directory containing the container's state. -#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))] -pub fn get_instance_root>( - root_path: P, - instance_id: &str, -) -> Result { - let instance_root = construct_instance_root(root_path, instance_id)?; - if !instance_root.exists() { - bail!("container {} does not exist.", instance_id) - } - Ok(instance_root) -} - -/// Checks if the container exists. -/// -/// The root path is the path to the directory containing the container's state. -#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))] -pub fn instance_exists>(root_path: P, container_id: &str) -> Result { - let instance_root = construct_instance_root(root_path, container_id)?; - Ok(instance_root.exists()) -} - #[derive(Serialize, Deserialize)] struct Options { root: Option, @@ -56,18 +31,6 @@ pub fn determine_rootdir( Ok(path) } -#[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))] -fn construct_instance_root>(root_path: P, container_id: &str) -> Result { - let root_path = root_path.as_ref().canonicalize().with_context(|| { - format!( - "failed to canonicalize {} for container {}", - root_path.as_ref().display(), - container_id - ) - })?; - Ok(root_path.join(container_id)) -} - #[cfg(unix)] #[cfg(test)] mod tests { diff --git a/crates/containerd-shim-wasm/src/sandbox/shim/instance_data.rs b/crates/containerd-shim-wasm/src/sandbox/shim/instance_data.rs index b8bdcfb68..c920f47ad 100644 --- a/crates/containerd-shim-wasm/src/sandbox/shim/instance_data.rs +++ b/crates/containerd-shim-wasm/src/sandbox/shim/instance_data.rs @@ -1,4 +1,4 @@ -use std::sync::{Arc, OnceLock, RwLock}; +use std::sync::{OnceLock, RwLock}; use std::time::Duration; use chrono::{DateTime, Utc}; @@ -10,7 +10,7 @@ pub(super) struct InstanceData { pub instance: T, cfg: InstanceConfig, pid: OnceLock, - state: Arc>, + state: RwLock, } impl InstanceData { @@ -22,7 +22,7 @@ impl InstanceData { instance, cfg, pid: OnceLock::default(), - state: Arc::new(RwLock::new(TaskState::Created)), + state: RwLock::new(TaskState::Created), }) } diff --git a/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs b/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs index ca9b31d58..edff4811d 100644 --- a/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs +++ b/crates/containerd-shim-wasm/src/sys/unix/container/instance.rs @@ -1,5 +1,6 @@ use std::marker::PhantomData; -use std::path::{Path, PathBuf}; +use std::path::Path; +use std::sync::Mutex; use std::thread; use std::time::Duration; @@ -16,7 +17,7 @@ use oci_spec::image::Platform; use crate::container::Engine; use crate::sandbox::async_utils::AmbientRuntime as _; -use crate::sandbox::instance_utils::{determine_rootdir, get_instance_root, instance_exists}; +use crate::sandbox::instance_utils::determine_rootdir; use crate::sandbox::sync::WaitableCell; use crate::sandbox::{ containerd, Error as SandboxError, Instance as SandboxInstance, InstanceConfig, Stdio, @@ -27,7 +28,7 @@ static DEFAULT_CONTAINER_ROOT_DIR: &str = "/run/containerd"; pub struct Instance { exit_code: WaitableCell<(u32, DateTime)>, - rootdir: PathBuf, + container: Mutex, id: String, _phantom: PhantomData, } @@ -54,7 +55,7 @@ impl SandboxInstance for Instance { (vec![], Platform::default()) }); - ContainerBuilder::new(id.clone(), SyscallType::Linux) + let container = ContainerBuilder::new(id.clone(), SyscallType::Linux) .with_executor(Executor::new(engine, stdio, modules, platform)) .with_root_path(rootdir.clone())? .as_init(&bundle) @@ -64,7 +65,7 @@ impl SandboxInstance for Instance { Ok(Self { id, exit_code: WaitableCell::new(), - rootdir, + container: Mutex::new(container), _phantom: Default::default(), }) } @@ -78,8 +79,7 @@ impl SandboxInstance for Instance { // make sure we have an exit code by the time we finish (even if there's a panic) let guard = self.exit_code.set_guard_with(|| (137, Utc::now())); - let container_root = get_instance_root(&self.rootdir, &self.id)?; - let mut container = Container::load(container_root)?; + let mut container = self.container.lock().expect("Poisoned mutex"); let pid = container.pid().context("failed to get pid")?.as_raw(); container.start()?; @@ -115,11 +115,11 @@ impl SandboxInstance for Instance { let signal = Signal::try_from(signal as i32).map_err(|err| { SandboxError::InvalidArgument(format!("invalid signal number: {}", err)) })?; - let container_root = get_instance_root(&self.rootdir, &self.id)?; - let mut container = Container::load(container_root) - .with_context(|| format!("could not load state for container {}", self.id))?; - container.kill(signal, true)?; + self.container + .lock() + .expect("Poisoned mutex") + .kill(signal, true)?; Ok(()) } @@ -129,23 +129,10 @@ impl SandboxInstance for Instance { #[cfg_attr(feature = "tracing", tracing::instrument(parent = tracing::Span::current(), skip_all, level = "Info"))] fn delete(&self) -> Result<(), SandboxError> { log::info!("deleting instance: {}", self.id); - match instance_exists(&self.rootdir, &self.id) { - Ok(true) => {} - Ok(false) => return Ok(()), - Err(err) => { - log::error!("could not find the container, skipping cleanup: {}", err); - return Ok(()); - } - } - let container_root = get_instance_root(&self.rootdir, &self.id)?; - match Container::load(container_root) { - Ok(mut container) => { - container.delete(true)?; - } - Err(err) => { - log::error!("could not find the container, skipping cleanup: {}", err); - } - } + self.container + .lock() + .expect("Poisoned mutex") + .delete(true)?; Ok(()) }