Skip to content

Commit

Permalink
fix(jailer): honor solitary --parent-cgroup option
Browse files Browse the repository at this point in the history
If we specify `--parent-cgroup` without any `--cgroup` option, the
jailer do anything.

We could just error in that situation, and expect something else to
launch the jailer process in a cgroup, but we risk breaking anybody that
is already using it that way.

Instead, we move the process to the `--parent-cgroup` since it's the
most intuitive, although the specified `--parent-cgroup` is not a parent
in that case.

Link: firecracker-microvm#4287

Signed-off-by: Pablo Barbáchano <pablob@amazon.com>
  • Loading branch information
pb8o committed Dec 11, 2023
1 parent f8d1a9b commit 334cf63
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 2 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed

- [#4309](https://github.com/firecracker-microvm/firecracker/pull/4309): The
jailer's option `--parent-cgroup` will move the process to that cgroup if no
`cgroup` options are provided.
- Simplified and clarified the removal policy of deprecated API elements
to follow semantic versioning 2.0.0. For more information, please refer to
[this GitHub discussion](https://github.com/firecracker-microvm/firecracker/discussions/4135).
Expand Down
10 changes: 10 additions & 0 deletions src/jailer/src/cgroup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,16 @@ impl CgroupBuilder {
}
}
}

// Returns the path to the root of the hierarchy
pub fn get_v2_hierarchy_path(&mut self) -> Result<&PathBuf, JailerError> {
match self.hierarchies.entry("unified".to_string()) {
Occupied(entry) => Ok(entry.into_mut()),
Vacant(_entry) => Err(JailerError::CgroupHierarchyMissing(
"cgroupsv2 hierarchy missing".to_string(),
)),
}
}
}

#[derive(Debug)]
Expand Down
16 changes: 14 additions & 2 deletions src/jailer/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,11 +233,23 @@ impl Env {
.parse::<u8>()
.map_err(|_| JailerError::CgroupInvalidVersion(cgroup_ver.to_string()))?;

let mut cgroup_builder = None;
let cgroups_args: &[String] = arguments.multiple_values("cgroup").unwrap_or_default();

// If the --parent-cgroup exists, and we have no other cgroups,
// then the intent is to move the process to that cgroup.
// Only applies to cgroupsv2 since it's a unified hierarchy
if cgroups_args.len() == 0 && cgroup_ver == 2 {
let mut builder = CgroupBuilder::new(cgroup_ver)?;
let cg_parent = builder.get_v2_hierarchy_path()?.join(parent_cgroup);
let cg_parent_procs = cg_parent.join("cgroup.procs");
if cg_parent.exists() {
fs::write(cg_parent_procs, std::process::id().to_string());
}
}

// cgroup format: <cgroup_controller>.<cgroup_property>=<value>,...
if let Some(cgroups_args) = arguments.multiple_values("cgroup") {
let builder = cgroup_builder.get_or_insert(CgroupBuilder::new(cgroup_ver)?);
let mut builder = CgroupBuilder::new(cgroup_ver)?;
for cg in cgroups_args {
let aux: Vec<&str> = cg.split('=').collect();
if aux.len() != 2 || aux[1].is_empty() {
Expand Down

0 comments on commit 334cf63

Please sign in to comment.