Merge pull request #597 from Furisto/rootless-cg-path

Improve cgroup path handling for rootless containers

crates/libcgroups/src/stats.rs

@@ -481,7 +481,7 @@ mod tests {
 
     #[test]
     fn test_parse_space_separated_as_nested_keyed_data() {
-        let tmp = create_temp_dir("test_parse_newline_separated_as_nested_keyed_data").unwrap();
+        let tmp = create_temp_dir("test_parse_space_separated_as_nested_keyed_data").unwrap();
         let file_content = ["key1", "key2", "key3", "key4"].join(" ");
         let file_path = set_fixture(&tmp, "space_separated", &file_content).unwrap();
 
@@ -501,7 +501,7 @@ mod tests {
 
     #[test]
     fn test_parse_flat_keyed_as_nested_keyed_data() {
-        let tmp = create_temp_dir("test_parse_newline_separated_as_nested_keyed_data").unwrap();
+        let tmp = create_temp_dir("test_parse_flat_keyed_as_nested_keyed_data").unwrap();
         let file_content = ["key1 1", "key2 2", "key3 3"].join("\n");
         let file_path = set_fixture(&tmp, "newline_separated", &file_content).unwrap();
 

crates/libcgroups/src/systemd/dbus/client.rs

@@ -112,8 +112,7 @@ impl SystemdClient for Client {
         properties.push(("DefaultDependencies", Variant(Box::new(false))));
         properties.push(("PIDs", Variant(Box::new(vec![pid]))));
 
-        log::debug!("START UNIT: {:?}", properties);
-
+        log::debug!("Starting transient unit: {:?}", properties);
         proxy
             .start_transient_unit(unit_name, "replace", properties, vec![])
             .with_context(|| {

crates/libcgroups/src/systemd/manager.rs

@@ -7,7 +7,7 @@ use std::{
 
 use anyhow::{anyhow, bail, Context, Result};
 use dbus::arg::RefArg;
-use nix::unistd::Pid;
+use nix::{unistd::Pid, NixPath};
 use std::path::{Path, PathBuf};
 
 use super::{
@@ -65,35 +65,33 @@ impl TryFrom<&Path> for CgroupsPath {
     type Error = anyhow::Error;
 
     fn try_from(cgroups_path: &Path) -> Result<Self, Self::Error> {
-        // cgroups path may never be empty as it is defaulted to `/youki`
-        // see 'get_cgroup_path' under utils.rs.
         // if cgroups_path was provided it should be of the form [slice]:[prefix]:[name],
         // for example: "system.slice:docker:1234".
-        let mut parent = "";
-        let prefix;
-        let name;
-        if cgroups_path.starts_with("/youki") {
-            prefix = "youki";
-            name = cgroups_path
-                .strip_prefix("/youki/")?
-                .to_str()
-                .ok_or_else(|| anyhow!("failed to parse cgroups path {:?}", cgroups_path))?;
-        } else {
-            let parts = cgroups_path
-                .to_str()
-                .ok_or_else(|| anyhow!("failed to parse cgroups path {:?}", cgroups_path))?
-                .split(':')
-                .collect::<Vec<&str>>();
-            parent = parts[0];
-            prefix = parts[1];
-            name = parts[2];
+        if cgroups_path.len() == 0 {
+            bail!("no cgroups path has been provided");
         }
 
-        Ok(CgroupsPath {
-            parent: parent.to_owned(),
-            prefix: prefix.to_owned(),
-            name: name.to_owned(),
-        })
+        let parts = cgroups_path
+            .to_str()
+            .ok_or_else(|| anyhow!("failed to parse cgroups path {:?}", cgroups_path))?
+            .split(':')
+            .collect::<Vec<&str>>();
+
+        let destructured_path = match parts.len() {
+            2 => CgroupsPath {
+                parent: "".to_owned(),
+                prefix: parts[0].to_owned(),
+                name: parts[1].to_owned(),
+            },
+            3 => CgroupsPath {
+                parent: parts[0].to_owned(),
+                prefix: parts[1].to_owned(),
+                name: parts[2].to_owned(),
+            },
+            _ => bail!("cgroup path {:?} is invalid", cgroups_path),
+        };
+
+        Ok(destructured_path)
     }
 }
 
@@ -103,6 +101,16 @@ impl Display for CgroupsPath {
     }
 }
 
+/// ensures that a parent unit for the current unit is specified
+fn ensure_parent_unit(cgroups_path: &mut CgroupsPath, use_system: bool) {
+    if cgroups_path.parent.is_empty() {
+        cgroups_path.parent = match use_system {
+            true => "system.slice".to_owned(),
+            false => "user.slice".to_owned(),
+        }
+    }
+}
+
 // custom debug impl as Manager contains fields that do not implement Debug
 // and therefore Debug cannot be derived
 impl Debug for Manager {
@@ -125,10 +133,12 @@ impl Manager {
         container_name: String,
         use_system: bool,
     ) -> Result<Self> {
-        let destructured_path = cgroups_path
+        let mut destructured_path = cgroups_path
             .as_path()
             .try_into()
             .with_context(|| format!("failed to destructure cgroups path {:?}", cgroups_path))?;
+        ensure_parent_unit(&mut destructured_path, use_system);
+
         let client = match use_system {
             true => Client::new_system().context("failed to create system dbus client")?,
             false => Client::new_session().context("failed to create session dbus client")?,
@@ -170,17 +180,10 @@ impl Manager {
         cgroups_path: &CgroupsPath,
         client: &dyn SystemdClient,
     ) -> Result<(PathBuf, PathBuf)> {
-        let mut parent = match client.is_system() {
-            true => PathBuf::from("/system.slice"),
-            false => PathBuf::from("/user.slice"),
-        };
-
         // if the user provided a '.slice' (as in a branch of a tree)
         // we need to convert it to a filesystem path.
-        if !cgroups_path.parent.is_empty() {
-            parent = Self::expand_slice(&cgroups_path.parent)?;
-        }
 
+        let parent = Self::expand_slice(&cgroups_path.parent)?;
         let systemd_root = client.control_cgroup_root()?;
         let unit_name = Self::get_unit_name(cgroups_path);
 
@@ -472,10 +475,11 @@ mod tests {
     }
 
     #[test]
-    fn get_cgroups_path_works_with_scope() -> Result<()> {
-        let cgroups_path = Path::new(":docker:foo")
+    fn get_cgroups_path_works_without_parent() -> Result<()> {
+        let mut cgroups_path = Path::new(":docker:foo")
             .try_into()
             .context("construct path")?;
+        ensure_parent_unit(&mut cgroups_path, true);
 
         assert_eq!(
             Manager::construct_cgroups_path(&cgroups_path, &TestSystemdClient {})?.0,

crates/libcontainer/src/config.rs

@@ -22,7 +22,7 @@ pub struct YoukiConfig {
 }
 
 impl<'a> YoukiConfig {
-    pub fn from_spec(spec: &'a Spec, container_id: &str) -> Result<Self> {
+    pub fn from_spec(spec: &'a Spec, container_id: &str, rootless: bool) -> Result<Self> {
         Ok(YoukiConfig {
             hooks: spec.hooks().clone(),
             cgroup_path: utils::get_cgroup_path(
@@ -31,6 +31,7 @@ impl<'a> YoukiConfig {
                     .context("no linux in spec")?
                     .cgroups_path(),
                 container_id,
+                rootless,
             ),
         })
     }
@@ -61,7 +62,7 @@ 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 config = YoukiConfig::from_spec(&spec, container_id, false)?;
         assert_eq!(&config.hooks, spec.hooks());
         dbg!(&config.cgroup_path);
         assert_eq!(config.cgroup_path, PathBuf::from(container_id));
@@ -73,7 +74,7 @@ mod tests {
         let container_id = "sample";
         let tmp = create_temp_dir("test_config_save_and_load").expect("create test directory");
         let spec = Spec::default();
-        let config = YoukiConfig::from_spec(&spec, container_id)?;
+        let config = YoukiConfig::from_spec(&spec, container_id, false)?;
         config.save(&tmp)?;
         let act = YoukiConfig::load(&tmp)?;
         assert_eq!(act, config);

crates/libcontainer/src/container/builder_impl.rs

@@ -54,7 +54,11 @@ impl<'a> ContainerBuilderImpl<'a> {
 
     fn run_container(&mut self) -> Result<()> {
         let linux = self.spec.linux().as_ref().context("no linux in spec")?;
-        let cgroups_path = utils::get_cgroup_path(linux.cgroups_path(), &self.container_id);
+        let cgroups_path = utils::get_cgroup_path(
+            linux.cgroups_path(),
+            &self.container_id,
+            self.rootless.is_some(),
+        );
         let cmanager = libcgroups::common::create_cgroup_manager(
             &cgroups_path,
             self.use_systemd || self.rootless.is_some(),
@@ -139,7 +143,11 @@ impl<'a> ContainerBuilderImpl<'a> {
 
     fn cleanup_container(&self) -> Result<()> {
         let linux = self.spec.linux().as_ref().context("no linux in spec")?;
-        let cgroups_path = utils::get_cgroup_path(linux.cgroups_path(), &self.container_id);
+        let cgroups_path = utils::get_cgroup_path(
+            linux.cgroups_path(),
+            &self.container_id,
+            self.rootless.is_some(),
+        );
         let cmanager = libcgroups::common::create_cgroup_manager(
             &cgroups_path,
             self.use_systemd || self.rootless.is_some(),

crates/libcontainer/src/container/container.rs

@@ -301,7 +301,8 @@ mod tests {
         let tmp_dir = create_temp_dir("test_get_spec")?;
         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, "123", false).context("convert spec to config")?;
         config.save(tmp_dir.path()).context("save config")?;
 
         let container = Container {

crates/libcontainer/src/container/container_delete.rs

@@ -1,7 +1,6 @@
 use super::{Container, ContainerStatus};
 use crate::config::YoukiConfig;
 use crate::hooks;
-use crate::utils;
 use anyhow::{bail, Context, Result};
 use libcgroups;
 use nix::sys::signal;
@@ -48,8 +47,6 @@ impl Container {
                     format!("failed to remove container dir {}", self.root.display())
                 })?;
 
-                let cgroups_path = utils::get_cgroup_path(&Some(config.cgroup_path), self.id());
-
                 // 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
@@ -57,13 +54,13 @@ impl Container {
                     .systemd()
                     .context("container state does not contain cgroup manager")?;
                 let cmanager = libcgroups::common::create_cgroup_manager(
-                    &cgroups_path,
+                    &config.cgroup_path,
                     use_systemd,
                     self.id(),
                 )
                 .context("failed to create cgroup manager")?;
                 cmanager.remove().with_context(|| {
-                    format!("failed to remove cgroup {}", cgroups_path.display())
+                    format!("failed to remove cgroup {}", config.cgroup_path.display())
                 })?;
 
                 if let Some(hooks) = config.hooks.as_ref() {

crates/libcontainer/src/container/init_builder.rs

@@ -47,9 +47,6 @@ impl<'a> InitContainerBuilder<'a> {
             .set_systemd(self.use_systemd)
             .set_annotations(spec.annotations().clone());
 
-        let config = YoukiConfig::from_spec(&spec, container.id())?;
-        config.save(&container_dir)?;
-
         unistd::chdir(&container_dir)?;
         let notify_path = container_dir.join(NOTIFY_FILE);
         // convert path of root file system of the container to absolute path
@@ -68,6 +65,9 @@ impl<'a> InitContainerBuilder<'a> {
         };
 
         let rootless = Rootless::new(&spec)?;
+        let config = YoukiConfig::from_spec(&spec, container.id(), rootless.is_some())?;
+        config.save(&container_dir)?;
+
         let mut builder_impl = ContainerBuilderImpl {
             init: true,
             syscall: self.base.syscall,

crates/libcontainer/src/utils.rs

@@ -68,10 +68,17 @@ pub fn do_exec(path: impl AsRef<Path>, args: &[String]) -> Result<()> {
 }
 
 /// If None, it will generate a default path for cgroups.
-pub fn get_cgroup_path(cgroups_path: &Option<PathBuf>, container_id: &str) -> PathBuf {
+pub fn get_cgroup_path(
+    cgroups_path: &Option<PathBuf>,
+    container_id: &str,
+    rootless: bool,
+) -> PathBuf {
     match cgroups_path {
         Some(cpath) => cpath.clone(),
-        None => PathBuf::from(container_id),
+        None => match rootless {
+            false => PathBuf::from(container_id),
+            true => PathBuf::from(format!(":youki:{}", container_id)),
+        },
     }
 }
 
@@ -315,11 +322,11 @@ mod tests {
     fn test_get_cgroup_path() {
         let cid = "sample_container_id";
         assert_eq!(
-            get_cgroup_path(&None, cid),
+            get_cgroup_path(&None, cid, false),
             PathBuf::from("sample_container_id")
         );
         assert_eq!(
-            get_cgroup_path(&Some(PathBuf::from("/youki")), cid),
+            get_cgroup_path(&Some(PathBuf::from("/youki")), cid, false),
             PathBuf::from("/youki")
         );
     }