diff --git a/crates/libcgroups/src/stats.rs b/crates/libcgroups/src/stats.rs index 89f8a392c..bdc833cf0 100644 --- a/crates/libcgroups/src/stats.rs +++ b/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(); diff --git a/crates/libcgroups/src/systemd/dbus/client.rs b/crates/libcgroups/src/systemd/dbus/client.rs index d2ab1e13a..e274481f1 100644 --- a/crates/libcgroups/src/systemd/dbus/client.rs +++ b/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(|| { diff --git a/crates/libcgroups/src/systemd/manager.rs b/crates/libcgroups/src/systemd/manager.rs index 6de28a4a9..7b411e3eb 100644 --- a/crates/libcgroups/src/systemd/manager.rs +++ b/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 { - // 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::>(); - 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::>(); + + 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 { - 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, diff --git a/crates/libcontainer/src/config.rs b/crates/libcontainer/src/config.rs index d7e214274..2591128a0 100644 --- a/crates/libcontainer/src/config.rs +++ b/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 { + pub fn from_spec(spec: &'a Spec, container_id: &str, rootless: bool) -> Result { 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); diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 47bdb312c..59afcd818 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/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(), diff --git a/crates/libcontainer/src/container/container.rs b/crates/libcontainer/src/container/container.rs index ee07c1a38..37af5b523 100644 --- a/crates/libcontainer/src/container/container.rs +++ b/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 { diff --git a/crates/libcontainer/src/container/container_delete.rs b/crates/libcontainer/src/container/container_delete.rs index 129c2aef7..fa59af2e4 100644 --- a/crates/libcontainer/src/container/container_delete.rs +++ b/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() { diff --git a/crates/libcontainer/src/container/init_builder.rs b/crates/libcontainer/src/container/init_builder.rs index 846541f54..19d39cb6a 100644 --- a/crates/libcontainer/src/container/init_builder.rs +++ b/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, diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs index d56306171..5f3482f0b 100644 --- a/crates/libcontainer/src/utils.rs +++ b/crates/libcontainer/src/utils.rs @@ -68,10 +68,17 @@ pub fn do_exec(path: impl AsRef, args: &[String]) -> Result<()> { } /// If None, it will generate a default path for cgroups. -pub fn get_cgroup_path(cgroups_path: &Option, container_id: &str) -> PathBuf { +pub fn get_cgroup_path( + cgroups_path: &Option, + 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") ); }