Allow completely disabling cgroup manipulation

crates/libcgroups/src/common.rs

@@ -11,6 +11,7 @@ use oci_spec::runtime::LinuxResources;
 use oci_spec::runtime::{
     LinuxDevice, LinuxDeviceBuilder, LinuxDeviceCgroup, LinuxDeviceCgroupBuilder, LinuxDeviceType,
 };
+use serde::{Deserialize, Serialize};
 
 use super::stats::Stats;
 use super::{systemd, v1, v2};
@@ -326,7 +327,7 @@ pub enum CreateCgroupSetupError {
     Systemd(#[from] systemd::manager::SystemdManagerError),
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)]
 pub struct CgroupConfig {
     pub cgroup_path: PathBuf,
     pub systemd_cgroup: bool,

crates/libcontainer/src/config.rs

@@ -5,8 +5,6 @@ use std::path::{Path, PathBuf};
 use oci_spec::runtime::{Hooks, Spec};
 use serde::{Deserialize, Serialize};
 
-use crate::utils;
-
 #[derive(Debug, thiserror::Error)]
 pub enum ConfigError {
     #[error("failed to save config")]
@@ -43,20 +41,17 @@ const YOUKI_CONFIG_NAME: &str = "youki_config.json";
 #[non_exhaustive]
 pub struct YoukiConfig {
     pub hooks: Option<Hooks>,
-    pub cgroup_path: PathBuf,
+    pub cgroup_config: Option<libcgroups::common::CgroupConfig>,
 }
 
 impl<'a> YoukiConfig {
-    pub fn from_spec(spec: &'a Spec, container_id: &str) -> Result<Self> {
+    pub fn from_spec(
+        spec: &'a Spec,
+        cgroup_config: Option<libcgroups::common::CgroupConfig>,
+    ) -> Result<Self> {
         Ok(YoukiConfig {
             hooks: spec.hooks().clone(),
-            cgroup_path: utils::get_cgroup_path(
-                spec.linux()
-                    .as_ref()
-                    .ok_or(ConfigError::MissingLinux)?
-                    .cgroups_path(),
-                container_id,
-            ),
+            cgroup_config,
         })
     }
 
@@ -106,13 +101,15 @@ mod tests {
     fn test_config_from_spec() -> Result<()> {
         let container_id = "sample";
         let spec = Spec::default();
-        let config = YoukiConfig::from_spec(&spec, container_id)?;
+        let cgroup_config = libcgroups::common::CgroupConfig {
+            cgroup_path: PathBuf::from(format!(":youki:{container_id}")),
+            systemd_cgroup: true,
+            container_name: container_id.to_owned(),
+        };
+        let config = YoukiConfig::from_spec(&spec, Some(cgroup_config.clone()))?;
         assert_eq!(&config.hooks, spec.hooks());
-        dbg!(&config.cgroup_path);
-        assert_eq!(
-            config.cgroup_path,
-            PathBuf::from(format!(":youki:{container_id}"))
-        );
+        dbg!(&config.cgroup_config);
+        assert_eq!(config.cgroup_config, Some(cgroup_config));
         Ok(())
     }
 
@@ -121,7 +118,12 @@ mod tests {
         let container_id = "sample";
         let tmp = tempfile::tempdir().expect("create temp dir");
         let spec = Spec::default();
-        let config = YoukiConfig::from_spec(&spec, container_id)?;
+        let cgroup_config = libcgroups::common::CgroupConfig {
+            cgroup_path: PathBuf::from(format!(":youki:{container_id}")),
+            systemd_cgroup: true,
+            container_name: container_id.to_owned(),
+        };
+        let config = YoukiConfig::from_spec(&spec, Some(cgroup_config))?;
         config.save(&tmp)?;
         let act = YoukiConfig::load(&tmp)?;
         assert_eq!(act, config);

crates/libcontainer/src/container/builder_impl.rs

@@ -10,24 +10,22 @@ use oci_spec::runtime::Spec;
 
 use super::{Container, ContainerStatus};
 use crate::error::{CreateContainerError, LibcontainerError, MissingSpecError};
+use crate::hooks;
 use crate::notify_socket::NotifyListener;
 use crate::process::args::{ContainerArgs, ContainerType};
 use crate::process::intel_rdt::delete_resctrl_subdirectory;
 use crate::process::{self};
 use crate::syscall::syscall::SyscallType;
 use crate::user_ns::UserNamespaceConfig;
 use crate::workload::Executor;
-use crate::{hooks, utils};
 
 pub(super) struct ContainerBuilderImpl {
     /// Flag indicating if an init or a tenant container should be created
     pub container_type: ContainerType,
     /// Interface to operating system primitives
     pub syscall: SyscallType,
-    /// Flag indicating if systemd should be used for cgroup management
-    pub use_systemd: bool,
-    /// Id of the container
-    pub container_id: String,
+    /// Interface to operating system primitives
+    pub cgroup_config: Option<libcgroups::common::CgroupConfig>,
     /// OCI compliant runtime spec
     pub spec: Rc<Spec>,
     /// Root filesystem of the container
@@ -41,8 +39,6 @@ pub(super) struct ContainerBuilderImpl {
     pub user_ns_config: Option<UserNamespaceConfig>,
     /// Path to the Unix Domain Socket to communicate container start
     pub notify_path: PathBuf,
-    /// Container state
-    pub container: Option<Container>,
     /// File descriptos preserved/passed to the container init process.
     pub preserve_fds: i32,
     /// If the container is to be run in detached mode
@@ -66,42 +62,28 @@ impl ContainerBuilderImpl {
             Err(outer) => {
                 // Only the init container should be cleaned up in the case of
                 // an error.
-                let cleanup_err = if self.is_init_container() {
-                    self.cleanup_container().err()
-                } else {
-                    None
-                };
+                let cleanup_err =
+                    if let ContainerType::InitContainer { container } = &self.container_type {
+                        cleanup_container(self.cgroup_config.clone(), container).err()
+                    } else {
+                        None
+                    };
 
                 Err(CreateContainerError::new(outer, cleanup_err).into())
             }
         }
     }
 
-    fn is_init_container(&self) -> bool {
-        matches!(self.container_type, ContainerType::InitContainer)
-    }
-
     fn run_container(&mut self) -> Result<Pid, LibcontainerError> {
-        let linux = self.spec.linux().as_ref().ok_or(MissingSpecError::Linux)?;
-        let cgroups_path = utils::get_cgroup_path(linux.cgroups_path(), &self.container_id);
-        let cgroup_config = libcgroups::common::CgroupConfig {
-            cgroup_path: cgroups_path,
-            systemd_cgroup: self.use_systemd || self.user_ns_config.is_some(),
-            container_name: self.container_id.to_owned(),
-        };
         let process = self
             .spec
             .process()
             .as_ref()
             .ok_or(MissingSpecError::Process)?;
 
-        if matches!(self.container_type, ContainerType::InitContainer) {
+        if let ContainerType::InitContainer { container } = &self.container_type {
             if let Some(hooks) = self.spec.hooks() {
-                hooks::run_hooks(
-                    hooks.create_runtime().as_ref(),
-                    self.container.as_ref(),
-                    None,
-                )?
+                hooks::run_hooks(hooks.create_runtime().as_ref(), container, None)?
             }
         }
 
@@ -143,6 +125,7 @@ impl ContainerBuilderImpl {
         // going to be switching to a different security context. Thus setting
         // ourselves to be non-dumpable only breaks things (like rootless
         // containers), which is the recommendation from the kernel folks.
+        let linux = self.spec.linux().as_ref().ok_or(MissingSpecError::Linux)?;
         if linux.namespaces().is_some() {
             prctl::set_dumpable(false).map_err(|e| {
                 LibcontainerError::Other(format!(
@@ -156,16 +139,15 @@ impl ContainerBuilderImpl {
         // therefore we will have to move all the variable by value. Since self
         // is a shared reference, we have to clone these variables here.
         let container_args = ContainerArgs {
-            container_type: self.container_type,
+            container_type: self.container_type.clone(),
             syscall: self.syscall,
             spec: Rc::clone(&self.spec),
             rootfs: self.rootfs.to_owned(),
             console_socket: self.console_socket.as_ref().map(|c| c.as_raw_fd()),
             notify_listener,
             preserve_fds: self.preserve_fds,
-            container: self.container.to_owned(),
             user_ns_config: self.user_ns_config.to_owned(),
-            cgroup_config,
+            cgroup_config: self.cgroup_config.clone(),
             detached: self.detached,
             executor: self.executor.clone(),
             no_pivot: self.no_pivot,
@@ -190,7 +172,7 @@ impl ContainerBuilderImpl {
             })?;
         }
 
-        if let Some(container) = &mut self.container {
+        if let ContainerType::InitContainer { container } = &mut self.container_type {
             // update status and pid of the container process
             container
                 .set_status(ContainerStatus::Created)
@@ -202,47 +184,42 @@ impl ContainerBuilderImpl {
 
         Ok(init_pid)
     }
+}
 
-    fn cleanup_container(&self) -> Result<(), LibcontainerError> {
-        let linux = self.spec.linux().as_ref().ok_or(MissingSpecError::Linux)?;
-        let cgroups_path = utils::get_cgroup_path(linux.cgroups_path(), &self.container_id);
-        let cmanager =
-            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
-                cgroup_path: cgroups_path,
-                systemd_cgroup: self.use_systemd || self.user_ns_config.is_some(),
-                container_name: self.container_id.to_string(),
-            })?;
-
-        let mut errors = Vec::new();
+fn cleanup_container(
+    cgroup_config: Option<libcgroups::common::CgroupConfig>,
+    container: &Container,
+) -> Result<(), LibcontainerError> {
+    let mut errors = Vec::new();
 
+    if let Some(cc) = cgroup_config {
+        let cmanager = libcgroups::common::create_cgroup_manager(cc)?;
         if let Err(e) = cmanager.remove() {
             tracing::error!(error = ?e, "failed to remove cgroup manager");
             errors.push(e.to_string());
         }
+    }
 
-        if let Some(container) = &self.container {
-            if let Some(true) = container.clean_up_intel_rdt_subdirectory() {
-                if let Err(e) = delete_resctrl_subdirectory(container.id()) {
-                    tracing::error!(id = ?container.id(), error = ?e, "failed to delete resctrl subdirectory");
-                    errors.push(e.to_string());
-                }
-            }
-
-            if container.root.exists() {
-                if let Err(e) = fs::remove_dir_all(&container.root) {
-                    tracing::error!(container_root = ?container.root, error = ?e, "failed to delete container root");
-                    errors.push(e.to_string());
-                }
-            }
+    if let Some(true) = container.clean_up_intel_rdt_subdirectory() {
+        if let Err(e) = delete_resctrl_subdirectory(container.id()) {
+            tracing::error!(id = ?container.id(), error = ?e, "failed to delete resctrl subdirectory");
+            errors.push(e.to_string());
         }
+    }
 
-        if !errors.is_empty() {
-            return Err(LibcontainerError::Other(format!(
-                "failed to cleanup container: {}",
-                errors.join(";")
-            )));
+    if container.root.exists() {
+        if let Err(e) = fs::remove_dir_all(&container.root) {
+            tracing::error!(container_root = ?container.root, error = ?e, "failed to delete container root");
+            errors.push(e.to_string());
         }
+    }
 
-        Ok(())
+    if !errors.is_empty() {
+        return Err(LibcontainerError::Other(format!(
+            "failed to cleanup container: {}",
+            errors.join(";")
+        )));
     }
+
+    Ok(())
 }

crates/libcontainer/src/container/container.rs

@@ -121,15 +121,6 @@ impl Container {
         self
     }
 
-    pub fn systemd(&self) -> bool {
-        self.state.use_systemd
-    }
-
-    pub fn set_systemd(&mut self, should_use: bool) -> &mut Self {
-        self.state.use_systemd = should_use;
-        self
-    }
-
     pub fn set_clean_up_intel_rdt_directory(&mut self, clean_up: bool) -> &mut Self {
         self.state.clean_up_intel_rdt_subdirectory = Some(clean_up);
         self
@@ -282,16 +273,6 @@ mod tests {
         assert_eq!(container.state.annotations, Some(annotations));
     }
 
-    #[test]
-    fn test_get_set_systemd() {
-        let mut container = Container::default();
-        assert!(!container.systemd());
-        container.set_systemd(true);
-        assert!(container.systemd());
-        container.set_systemd(false);
-        assert!(!container.systemd());
-    }
-
     #[test]
     fn test_get_set_creator() {
         let mut container = Container::default();
@@ -331,7 +312,7 @@ mod tests {
         let tmp_dir = tempfile::tempdir().unwrap();
         use oci_spec::runtime::Spec;
         let spec = Spec::default();
-        let config = YoukiConfig::from_spec(&spec, "123").context("convert spec to config")?;
+        let config = YoukiConfig::from_spec(&spec, None).context("convert spec to config")?;
         config.save(tmp_dir.path()).context("save config")?;
 
         let container = Container {

crates/libcontainer/src/container/container_delete.rs

@@ -5,7 +5,6 @@ use libcgroups::{self};
 use nix::sys::signal;
 
 use super::{Container, ContainerStatus};
-use crate::config::YoukiConfig;
 use crate::error::LibcontainerError;
 use crate::hooks;
 use crate::process::intel_rdt::delete_resctrl_subdirectory;
@@ -78,32 +77,26 @@ impl Container {
         }
 
         if self.root.exists() {
-            match YoukiConfig::load(&self.root) {
+            match self.spec() {
                 Ok(config) => {
                     tracing::debug!("config: {:?}", config);
 
                     // remove the cgroup created for the container
                     // check https://man7.org/linux/man-pages/man7/cgroups.7.html
                     // creating and removing cgroups section for more information on cgroups
-                    let cmanager = libcgroups::common::create_cgroup_manager(
-                        libcgroups::common::CgroupConfig {
-                            cgroup_path: config.cgroup_path.to_owned(),
-                            systemd_cgroup: self.systemd(),
-                            container_name: self.id().to_string(),
-                        },
-                    )?;
-                    cmanager.remove().map_err(|err| {
-                        tracing::error!(cgroup_path = ?config.cgroup_path, "failed to remove cgroup due to: {err:?}");
-                        err
-                    })?;
+                    if let Some(cc) = config.cgroup_config {
+                        let cmanager = libcgroups::common::create_cgroup_manager(cc.clone())?;
+                        cmanager.remove().map_err(|err| {
+                            tracing::error!(cgroup_config = ?cc, "failed to remove cgroup due to: {err:?}");
+                            err
+                        })?;
+                    }
 
                     if let Some(hooks) = config.hooks.as_ref() {
-                        hooks::run_hooks(hooks.poststop().as_ref(), Some(self), None).map_err(
-                            |err| {
-                                tracing::error!(err = ?err, "failed to run post stop hooks");
-                                err
-                            },
-                        )?;
+                        hooks::run_hooks(hooks.poststop().as_ref(), self, None).map_err(|err| {
+                            tracing::error!(err = ?err, "failed to run post stop hooks");
+                            err
+                        })?;
                     }
                 }
                 Err(err) => {