Refactor the libcgroups interface (#2168)

* fix notify_listener

- fix the name to notify listener
- fix the structure to be clone-able

Signed-off-by: yihuaf <yihuaf@unkies.org>

* changed the libcgroup creation interface

Changed the libcgroup creation interface to use config struct rather
than variables. The creation will also own/consume the config struct.
In this way, we don't have to create the cgroup manager upfront.
Instead, we can delay the creation of cgroup manager in the process when
it is needed.

Signed-off-by: yihuaf <yihuaf@unkies.org>

* Add a notify listener test

Signed-off-by: yihuaf <yihuaf@unkies.org>

* fix clippy

Signed-off-by: yihuaf <yihuaf@unkies.org>

* fix spellcheck

Signed-off-by: yihuaf <yihuaf@unkies.org>

---------

Signed-off-by: yihuaf <yihuaf@unkies.org>

crates/libcgroups/src/common.rs

@@ -316,65 +316,70 @@ pub enum CreateCgroupSetupError {
     Systemd(#[from] systemd::manager::SystemdManagerError),
 }
 
-pub fn create_cgroup_manager<P: Into<PathBuf>>(
-    cgroup_path: P,
-    systemd_cgroup: bool,
-    container_name: &str,
+#[derive(Clone)]
+pub struct CgroupConfig {
+    pub cgroup_path: PathBuf,
+    pub systemd_cgroup: bool,
+    pub container_name: String,
+}
+
+pub fn create_cgroup_manager(
+    config: CgroupConfig,
 ) -> Result<AnyCgroupManager, CreateCgroupSetupError> {
     let cgroup_setup = get_cgroup_setup().map_err(|err| match err {
         GetCgroupSetupError::WrappedIo(err) => CreateCgroupSetupError::WrappedIo(err),
         GetCgroupSetupError::NonDefault => CreateCgroupSetupError::NonDefault,
         GetCgroupSetupError::FailedToDetect => CreateCgroupSetupError::FailedToDetect,
     })?;
-    let cgroup_path = cgroup_path.into();
+    let cgroup_path = config.cgroup_path.as_path();
 
     match cgroup_setup {
         CgroupSetup::Legacy | CgroupSetup::Hybrid => {
             Ok(create_v1_cgroup_manager(cgroup_path)?.any())
         }
         CgroupSetup::Unified => {
             // ref https://github.com/opencontainers/runtime-spec/blob/main/config-linux.md#cgroups-path
-            if cgroup_path.is_absolute() || !systemd_cgroup {
+            if cgroup_path.is_absolute() || !config.systemd_cgroup {
                 return Ok(create_v2_cgroup_manager(cgroup_path)?.any());
             }
-            Ok(create_systemd_cgroup_manager(cgroup_path, container_name)?.any())
+            Ok(create_systemd_cgroup_manager(cgroup_path, config.container_name.as_str())?.any())
         }
     }
 }
 
 #[cfg(feature = "v1")]
 fn create_v1_cgroup_manager(
-    cgroup_path: PathBuf,
+    cgroup_path: &Path,
 ) -> Result<v1::manager::Manager, v1::manager::V1ManagerError> {
     tracing::info!("cgroup manager V1 will be used");
     v1::manager::Manager::new(cgroup_path)
 }
 
 #[cfg(not(feature = "v1"))]
 fn create_v1_cgroup_manager(
-    _cgroup_path: PathBuf,
+    _cgroup_path: &Path,
 ) -> Result<v1::manager::Manager, v1::manager::V1ManagerError> {
     Err(v1::manager::V1ManagerError::NotEnabled)
 }
 
 #[cfg(feature = "v2")]
 fn create_v2_cgroup_manager(
-    cgroup_path: PathBuf,
+    cgroup_path: &Path,
 ) -> Result<v2::manager::Manager, v2::manager::V2ManagerError> {
     tracing::info!("cgroup manager V2 will be used");
-    v2::manager::Manager::new(DEFAULT_CGROUP_ROOT.into(), cgroup_path)
+    v2::manager::Manager::new(DEFAULT_CGROUP_ROOT.into(), cgroup_path.to_owned())
 }
 
 #[cfg(not(feature = "v2"))]
 fn create_v2_cgroup_manager(
-    _cgroup_path: PathBuf,
+    _cgroup_path: &Path,
 ) -> Result<v2::manager::Manager, v2::manager::V2ManagerError> {
     Err(v2::manager::V2ManagerError::NotEnabled)
 }
 
 #[cfg(feature = "systemd")]
 fn create_systemd_cgroup_manager(
-    cgroup_path: PathBuf,
+    cgroup_path: &Path,
     container_name: &str,
 ) -> Result<systemd::manager::Manager, systemd::manager::SystemdManagerError> {
     if !systemd::booted() {
@@ -391,15 +396,15 @@ fn create_systemd_cgroup_manager(
     );
     systemd::manager::Manager::new(
         DEFAULT_CGROUP_ROOT.into(),
-        cgroup_path,
+        cgroup_path.to_owned(),
         container_name.into(),
         use_system,
     )
 }
 
 #[cfg(not(feature = "systemd"))]
 fn create_systemd_cgroup_manager(
-    _cgroup_path: PathBuf,
+    _cgroup_path: &Path,
     _container_name: &str,
 ) -> Result<systemd::manager::Manager, systemd::manager::SystemdManagerError> {
     Err(systemd::manager::SystemdManagerError::NotEnabled)

crates/libcgroups/src/systemd/manager.rs

@@ -180,7 +180,7 @@ impl Manager {
         container_name: String,
         use_system: bool,
     ) -> Result<Self, SystemdManagerError> {
-        let mut destructured_path = cgroups_path.as_path().try_into()?;
+        let mut destructured_path: CgroupsPath = cgroups_path.as_path().try_into()?;
         ensure_parent_unit(&mut destructured_path, use_system);
 
         let client = match use_system {

crates/libcgroups/src/v1/manager.rs

@@ -82,10 +82,10 @@ pub enum V1ManagerError {
 
 impl Manager {
     /// Constructs a new cgroup manager with cgroups_path being relative to the root of the subsystem
-    pub fn new(cgroup_path: PathBuf) -> Result<Self, V1ManagerError> {
+    pub fn new(cgroup_path: &Path) -> Result<Self, V1ManagerError> {
         let mut subsystems = HashMap::<CtrlType, PathBuf>::new();
         for subsystem in CONTROLLERS {
-            if let Ok(subsystem_path) = Self::get_subsystem_path(&cgroup_path, subsystem) {
+            if let Ok(subsystem_path) = Self::get_subsystem_path(cgroup_path, subsystem) {
                 subsystems.insert(*subsystem, subsystem_path);
             } else {
                 tracing::warn!("cgroup {} not supported on this system", subsystem);

crates/libcontainer/src/container/builder_impl.rs

@@ -73,11 +73,11 @@ impl<'a> ContainerBuilderImpl<'a> {
             &self.container_id,
             self.rootless.is_some(),
         );
-        let cmanager = libcgroups::common::create_cgroup_manager(
-            cgroups_path,
-            self.use_systemd || self.rootless.is_some(),
-            &self.container_id,
-        )?;
+        let cgroup_config = libcgroups::common::CgroupConfig {
+            cgroup_path: cgroups_path,
+            systemd_cgroup: self.use_systemd || self.rootless.is_some(),
+            container_name: self.container_id.to_owned(),
+        };
         let process = self
             .spec
             .process()
@@ -93,8 +93,10 @@ impl<'a> ContainerBuilderImpl<'a> {
         // Need to create the notify socket before we pivot root, since the unix
         // domain socket used here is outside of the rootfs of container. During
         // exec, need to create the socket before we enter into existing mount
-        // namespace.
-        let notify_socket: NotifyListener = NotifyListener::new(&self.notify_path)?;
+        // namespace. We also need to create to socket before entering into the
+        // user namespace in the case that the path is located in paths only
+        // root can access.
+        let notify_listener = NotifyListener::new(&self.notify_path)?;
 
         // If Out-of-memory score adjustment is set in specification.  set the score
         // value for the current process check
@@ -139,11 +141,11 @@ impl<'a> ContainerBuilderImpl<'a> {
             spec: self.spec,
             rootfs: &self.rootfs,
             console_socket: self.console_socket,
-            notify_socket,
+            notify_listener,
             preserve_fds: self.preserve_fds,
             container: &self.container,
             rootless: &self.rootless,
-            cgroup_manager: cmanager,
+            cgroup_config,
             detached: self.detached,
             executor_manager: &self.executor_manager,
         };
@@ -184,11 +186,12 @@ impl<'a> ContainerBuilderImpl<'a> {
             &self.container_id,
             self.rootless.is_some(),
         );
-        let cmanager = libcgroups::common::create_cgroup_manager(
-            cgroups_path,
-            self.use_systemd || self.rootless.is_some(),
-            &self.container_id,
-        )?;
+        let cmanager =
+            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
+                cgroup_path: cgroups_path,
+                systemd_cgroup: self.use_systemd || self.rootless.is_some(),
+                container_name: self.container_id.to_string(),
+            })?;
 
         let mut errors = Vec::new();
 

crates/libcontainer/src/container/container_delete.rs

@@ -81,11 +81,12 @@ impl Container {
                     // 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 use_systemd = self.systemd();
                     let cmanager = libcgroups::common::create_cgroup_manager(
-                        &config.cgroup_path,
-                        use_systemd,
-                        self.id(),
+                        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:?}");

crates/libcontainer/src/container/container_events.rs

@@ -33,11 +33,12 @@ impl Container {
             return Err(LibcontainerError::IncorrectStatus);
         }
 
-        let cgroups_path = self.spec()?.cgroup_path;
-        let use_systemd = self.systemd();
-
         let cgroup_manager =
-            libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?;
+            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
+                cgroup_path: self.spec()?.cgroup_path,
+                systemd_cgroup: self.systemd(),
+                container_name: self.id().to_string(),
+            })?;
         match stats {
             true => {
                 let stats = cgroup_manager.stats()?;

crates/libcontainer/src/container/container_kill.rs

@@ -1,6 +1,6 @@
 use super::{Container, ContainerStatus};
 use crate::{error::LibcontainerError, signal::Signal};
-use libcgroups::common::{create_cgroup_manager, get_cgroup_setup, CgroupManager};
+use libcgroups::common::{get_cgroup_setup, CgroupManager};
 use nix::sys::signal::{self};
 
 impl Container {
@@ -78,10 +78,14 @@ impl Container {
             match get_cgroup_setup()? {
                 libcgroups::common::CgroupSetup::Legacy
                 | libcgroups::common::CgroupSetup::Hybrid => {
-                    let cgroups_path = self.spec()?.cgroup_path;
-                    let use_systemd = self.systemd();
-                    let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id())?;
-                    cmanger.freeze(libcgroups::common::FreezerState::Thawed)?;
+                    let cmanager = libcgroups::common::create_cgroup_manager(
+                        libcgroups::common::CgroupConfig {
+                            cgroup_path: self.spec()?.cgroup_path,
+                            systemd_cgroup: self.systemd(),
+                            container_name: self.id().to_string(),
+                        },
+                    )?;
+                    cmanager.freeze(libcgroups::common::FreezerState::Thawed)?;
                 }
                 libcgroups::common::CgroupSetup::Unified => {}
             }
@@ -91,19 +95,22 @@ impl Container {
 
     fn kill_all_processes<S: Into<Signal>>(&self, signal: S) -> Result<(), LibcontainerError> {
         let signal = signal.into().into_raw();
-        let cgroups_path = self.spec()?.cgroup_path;
-        let use_systemd = self.systemd();
-        let cmanger = create_cgroup_manager(cgroups_path, use_systemd, self.id())?;
+        let cmanager =
+            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
+                cgroup_path: self.spec()?.cgroup_path,
+                systemd_cgroup: self.systemd(),
+                container_name: self.id().to_string(),
+            })?;
 
-        if let Err(e) = cmanger.freeze(libcgroups::common::FreezerState::Frozen) {
+        if let Err(e) = cmanager.freeze(libcgroups::common::FreezerState::Frozen) {
             tracing::warn!(
                 err = ?e,
                 id = ?self.id(),
                 "failed to freeze container",
             );
         }
 
-        let pids = cmanger.get_all_pids()?;
+        let pids = cmanager.get_all_pids()?;
         pids.iter()
             .try_for_each(|&pid| {
                 tracing::debug!("kill signal {} to {}", signal, pid);
@@ -117,7 +124,7 @@ impl Container {
                 }
             })
             .map_err(LibcontainerError::OtherSyscall)?;
-        if let Err(err) = cmanger.freeze(libcgroups::common::FreezerState::Thawed) {
+        if let Err(err) = cmanager.freeze(libcgroups::common::FreezerState::Thawed) {
             tracing::warn!(
                 err = ?err,
                 id = ?self.id(),

crates/libcontainer/src/container/container_pause.rs

@@ -32,10 +32,12 @@ impl Container {
             return Err(LibcontainerError::IncorrectStatus);
         }
 
-        let cgroups_path = self.spec()?.cgroup_path;
-        let use_systemd = self.systemd();
         let cmanager =
-            libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?;
+            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
+                cgroup_path: self.spec()?.cgroup_path,
+                systemd_cgroup: self.systemd(),
+                container_name: self.id().to_string(),
+            })?;
         cmanager.freeze(FreezerState::Frozen)?;
 
         tracing::debug!("saving paused status");

crates/libcontainer/src/container/container_resume.rs

@@ -34,10 +34,12 @@ impl Container {
             return Err(LibcontainerError::IncorrectStatus);
         }
 
-        let cgroups_path = self.spec()?.cgroup_path;
-        let use_systemd = self.systemd();
         let cmanager =
-            libcgroups::common::create_cgroup_manager(cgroups_path, use_systemd, self.id())?;
+            libcgroups::common::create_cgroup_manager(libcgroups::common::CgroupConfig {
+                cgroup_path: self.spec()?.cgroup_path,
+                systemd_cgroup: self.systemd(),
+                container_name: self.id().to_string(),
+            })?;
         // resume the frozen container
         cmanager.freeze(FreezerState::Thawed)?;