Skip to content

fix(shim): rewrite cgroups v2 parsing logic #254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mxpv merged 5 commits into from
Mar 18, 2024
Merged

fix(shim): rewrite cgroups v2 parsing logic #254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fff90b8
fix(shim): rewrite cgroups v2 parsing logic
Mossaka Mar 14, 2024
7fe0325
fix: more gracefully handle multiple lines in the cgroups file
Mossaka Mar 15, 2024
7972c9c
fix: make parse_cgroups_v2_path clearer
Mossaka Mar 15, 2024
6e77318
cargo clippy
Mossaka Mar 15, 2024
61e33b0
Update crates/shim/src/cgroup.rs
Mossaka Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/shim/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ signal-hook-tokio = { version = "0.3.1", optional = true, features = [
tokio = { workspace = true, features = ["full"], optional = true }

[target.'cfg(target_os = "linux")'.dependencies]
cgroups-rs = "0.3.3"
cgroups-rs = "0.3.4"

[target.'cfg(unix)'.dependencies]
command-fds = "0.3.0"
Expand Down
78 changes: 65 additions & 13 deletions crates/shim/src/cgroup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@

#![cfg(target_os = "linux")]

use std::{fs, io::Read, path::Path};
use std::{
fs,
io::Read,
path::{Path, PathBuf},
};

use cgroups_rs::{
cgroup::get_cgroups_relative_paths_by_pid, hierarchies, Cgroup, CgroupPid, MaxValue, Subsystem,
Expand Down Expand Up @@ -178,18 +182,8 @@ pub fn collect_metrics(pid: u32) -> Result<Metrics> {
fn get_cgroup(pid: u32) -> Result<Cgroup> {
let hierarchies = hierarchies::auto();
let cgroup = if hierarchies.v2() {
let path = format!("/proc/{}/cgroup", pid);
let content = fs::read_to_string(path).map_err(io_error!(e, "read cgroup"))?;
let content = content.strip_suffix('\n').unwrap_or_default();

let parts: Vec<&str> = content.split("::").collect();
let path_parts: Vec<&str> = parts[1].split('/').collect();
let namespace = path_parts[1];
let cgroup_name = path_parts[2];
Cgroup::load(
hierarchies,
format!("/sys/fs/cgroup/{namespace}/{cgroup_name}").as_str(),
)
let path = get_cgroups_v2_path_by_pid(pid)?;
Cgroup::load(hierarchies, path)
} else {
// get container main process cgroup
let path = get_cgroups_relative_paths_by_pid(pid)
Expand All @@ -199,6 +193,34 @@ fn get_cgroup(pid: u32) -> Result<Cgroup> {
Ok(cgroup)
}

/// Get the cgroups v2 path given a PID
pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result<PathBuf> {
// todo: should upstream to cgroups-rs
let path = format!("/proc/{}/cgroup", pid);
let content = fs::read_to_string(path).map_err(io_error!(e, "read cgroup"))?;
let content = content.lines().next().unwrap_or("");

let Ok(path) = parse_cgroups_v2_path(content)?.canonicalize() else {
Copy link
Contributor

@jsturtevant jsturtevant Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any concern about resolving symlinks or relative paths via canonicalize here?

Copy link
Contributor

@jsturtevant jsturtevant Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the go implementation the path is cleaned here https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L48

Copy link
Contributor

@jprendes jprendes Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing is harder if the actual path has to exist. Maybe we can just normalize it instead of canonicalizing it, although rust's stdlib doesnt have a normalize method, that would require a 3rd party crate.

Copy link
Contributor

@jprendes jprendes Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do something like this:
https://github.com/containers/youki/blob/0b89e1d52089c8c328496dab02dcc2bcf5e1b53c/crates/libcontainer/src/utils.rs#L92

Copy link
Member Author

@Mossaka Mossaka Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the Go's CleanPath correctly, it's like normalizing the path but without verifying it exists in the filesystem, unlick fs::canonicalize() right?

Copy link
Contributor

@jsturtevant jsturtevant Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the key is:

//  "This ensures that a
// path resulting from prepending another path will always resolve to lexically
// be a subdirectory of the prefixed path."

Copy link
Contributor

@jsturtevant jsturtevant Mar 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the go implementation the path is cleaned here https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L48

@Burning1020 @mxpv does this path need be cleaned? It is there in the go code but I am not sure if I completely understand why it is needed. If not we should be ok to merge this.

return Err(Error::Other("cgroup path not found".to_string()));
};
Ok(path)
}

// https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83
fn parse_cgroups_v2_path(content: &str) -> Result<PathBuf> {
// the entry for cgroup v2 is always in the format like `0::$PATH`
// where 0 is the hierarchy ID, the controller name is omitted in cgroup v2
// and $PATH is the cgroup path
// see https://docs.kernel.org/admin-guide/cgroup-v2.html
let Some(path) = content.strip_prefix("0::") else {
return Err(Error::Other(format!("invalid cgroup path: {}", content)));
};

let path = path.trim_start_matches('/');

Ok(PathBuf::from(format!("/sys/fs/cgroup/{}", path)))
}

/// Update process cgroup limits
pub fn update_resources(pid: u32, resources: &LinuxResources) -> Result<()> {
// get container main process cgroup
Expand Down Expand Up @@ -305,8 +327,11 @@ pub fn update_resources(pid: u32, resources: &LinuxResources) -> Result<()> {

#[cfg(test)]
mod tests {
use std::path::PathBuf;

use cgroups_rs::{hierarchies, Cgroup, CgroupPid};

use super::parse_cgroups_v2_path;
use crate::cgroup::{
add_task_to_cgroup, adjust_oom_score, read_process_oom_score, OOM_SCORE_ADJ_MAX,
};
Expand Down Expand Up @@ -342,4 +367,31 @@ mod tests {
assert_eq!(new, OOM_SCORE_ADJ_MAX)
}
}

#[test]
fn test_parse_cgroups_v2_path() {
let path = "0::/user.slice/user-1000.slice/session-2.scope";
assert_eq!(
parse_cgroups_v2_path(path).unwrap(),
PathBuf::from("/sys/fs/cgroup/user.slice/user-1000.slice/session-2.scope")
);
}

#[test]
fn test_parse_cgroups_v2_path_empty() {
let path = "0::";
assert_eq!(
parse_cgroups_v2_path(path).unwrap(),
PathBuf::from("/sys/fs/cgroup/")
);
}

#[test]
fn test_parse_cgroups_v2_path_kube() {
let path = "0::/kubepods-besteffort-pod8.slice:cri-containerd:8";
assert_eq!(
parse_cgroups_v2_path(path).unwrap(),
PathBuf::from("/sys/fs/cgroup/kubepods-besteffort-pod8.slice:cri-containerd:8")
);
}
}