From fff90b8e873426951b6687ff52b3a42b6b8e24fa Mon Sep 17 00:00:00 2001 From: jiaxiao zhou Date: Thu, 14 Mar 2024 23:53:15 +0000 Subject: [PATCH 1/5] fix(shim): rewrite cgroups v2 parsing logic this commit rewrites the cgroups v2 parsing logic in get_cgroup function which is used to fetch stats of a container. The reason for the rewrite was that in some cases the original logic would panic due to index of bound for parsing paths like 0::/kubepods-besteffort-pod162385e5_7f69_4c38_ba9c_db0a8f02b35e.slice:cri-containerd:278a0aac1fff30dfbc41b4a32ba9de4519928fe7480213dba87aa1498838ef34 we ran into this issue in deleting a spin container in the spin shim. the rewrite replaces index access to properly propogate the error to the caller of the function and added a few unit tests for the parsing logic. Signed-off-by: jiaxiao zhou --- crates/shim/Cargo.toml | 2 +- crates/shim/src/cgroup.rs | 70 ++++++++++++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/crates/shim/Cargo.toml b/crates/shim/Cargo.toml index ebd5d8aa..9b3c6fad 100644 --- a/crates/shim/Cargo.toml +++ b/crates/shim/Cargo.toml @@ -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" diff --git a/crates/shim/src/cgroup.rs b/crates/shim/src/cgroup.rs index a556a70b..555b36b1 100644 --- a/crates/shim/src/cgroup.rs +++ b/crates/shim/src/cgroup.rs @@ -178,18 +178,8 @@ pub fn collect_metrics(pid: u32) -> Result { fn get_cgroup(pid: u32) -> Result { 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.as_str()) } else { // get container main process cgroup let path = get_cgroups_relative_paths_by_pid(pid) @@ -199,6 +189,37 @@ fn get_cgroup(pid: u32) -> Result { Ok(cgroup) } +/// Get the cgroups v2 path given a PID +pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { + // 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.trim_end_matches('\n'); + + parse_cgroups_v2_path(content) +} + +// https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83 +fn parse_cgroups_v2_path(content: &str) -> std::prelude::v1::Result { + // the entry for cgroup v2 is always in the format like `0::$PATH` + // where 0 is the hierarchy ID, the controller name is ommit in cgroup v2 + // and $PATH is the cgroup path + // see https://docs.kernel.org/admin-guide/cgroup-v2.html + let parts: Vec<&str> = content.splitn(3, ":").collect(); + + if parts.len() < 3 { + return Err(Error::Other(format!("invalid cgroup path: {}", content))); + } + + if parts[0] == "0" && parts[1].is_empty() { + // Check if parts[2] starts with '/', remove it if present. + let path = parts[2].strip_prefix('/').unwrap_or(parts[2]); + return Ok(format!("/sys/fs/cgroup/{}", path)); + } + + Err(Error::Other("cgroup path not found".into())) +} + /// Update process cgroup limits pub fn update_resources(pid: u32, resources: &LinuxResources) -> Result<()> { // get container main process cgroup @@ -307,6 +328,7 @@ pub fn update_resources(pid: u32, resources: &LinuxResources) -> Result<()> { mod tests { 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, }; @@ -342,4 +364,28 @@ 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(), + "/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(), "/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(), + "/sys/fs/cgroup/kubepods-besteffort-pod8.slice:cri-containerd:8" + ); + } } From 7fe0325861b299df58bb49d99f8374d489d7746c Mon Sep 17 00:00:00 2001 From: jiaxiao zhou Date: Fri, 15 Mar 2024 17:06:06 +0000 Subject: [PATCH 2/5] fix: more gracefully handle multiple lines in the cgroups file Signed-off-by: jiaxiao zhou --- crates/shim/src/cgroup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/shim/src/cgroup.rs b/crates/shim/src/cgroup.rs index 555b36b1..7fea5d59 100644 --- a/crates/shim/src/cgroup.rs +++ b/crates/shim/src/cgroup.rs @@ -194,7 +194,7 @@ pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { // 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.trim_end_matches('\n'); + let content = content.lines().next().unwrap_or(""); parse_cgroups_v2_path(content) } From 7972c9cb5b7bde2fc161a6d4eb1e7b19f1c96253 Mon Sep 17 00:00:00 2001 From: jiaxiao zhou Date: Fri, 15 Mar 2024 17:25:07 +0000 Subject: [PATCH 3/5] fix: make parse_cgroups_v2_path clearer Signed-off-by: jiaxiao zhou --- crates/shim/src/cgroup.rs | 42 ++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/crates/shim/src/cgroup.rs b/crates/shim/src/cgroup.rs index 7fea5d59..5547254e 100644 --- a/crates/shim/src/cgroup.rs +++ b/crates/shim/src/cgroup.rs @@ -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, @@ -179,7 +183,7 @@ fn get_cgroup(pid: u32) -> Result { let hierarchies = hierarchies::auto(); let cgroup = if hierarchies.v2() { let path = get_cgroups_v2_path_by_pid(pid)?; - Cgroup::load(hierarchies, path.as_str()) + Cgroup::load(hierarchies, path) } else { // get container main process cgroup let path = get_cgroups_relative_paths_by_pid(pid) @@ -190,34 +194,31 @@ fn get_cgroup(pid: u32) -> Result { } /// Get the cgroups v2 path given a PID -pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { +pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { // 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(""); - parse_cgroups_v2_path(content) + let Ok(path) = parse_cgroups_v2_path(content)?.canonicalize() else { + return Err(Error::Other(format!("cgroup path not found"))); + }; + Ok(path) } // https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83 -fn parse_cgroups_v2_path(content: &str) -> std::prelude::v1::Result { +fn parse_cgroups_v2_path(content: &str) -> Result { // the entry for cgroup v2 is always in the format like `0::$PATH` // where 0 is the hierarchy ID, the controller name is ommit in cgroup v2 // and $PATH is the cgroup path // see https://docs.kernel.org/admin-guide/cgroup-v2.html - let parts: Vec<&str> = content.splitn(3, ":").collect(); - - if parts.len() < 3 { + let Some(path) = content.strip_prefix("0::") else { return Err(Error::Other(format!("invalid cgroup path: {}", content))); - } + }; - if parts[0] == "0" && parts[1].is_empty() { - // Check if parts[2] starts with '/', remove it if present. - let path = parts[2].strip_prefix('/').unwrap_or(parts[2]); - return Ok(format!("/sys/fs/cgroup/{}", path)); - } + let path = path.trim_start_matches('/'); - Err(Error::Other("cgroup path not found".into())) + Ok(PathBuf::from(format!("/sys/fs/cgroup/{}", path))) } /// Update process cgroup limits @@ -326,6 +327,8 @@ 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; @@ -370,14 +373,17 @@ mod tests { let path = "0::/user.slice/user-1000.slice/session-2.scope"; assert_eq!( parse_cgroups_v2_path(path).unwrap(), - "/sys/fs/cgroup/user.slice/user-1000.slice/session-2.scope" + 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(), "/sys/fs/cgroup/"); + assert_eq!( + parse_cgroups_v2_path(path).unwrap(), + PathBuf::from("/sys/fs/cgroup/") + ); } #[test] @@ -385,7 +391,7 @@ mod tests { let path = "0::/kubepods-besteffort-pod8.slice:cri-containerd:8"; assert_eq!( parse_cgroups_v2_path(path).unwrap(), - "/sys/fs/cgroup/kubepods-besteffort-pod8.slice:cri-containerd:8" + PathBuf::from("/sys/fs/cgroup/kubepods-besteffort-pod8.slice:cri-containerd:8") ); } } From 6e7731890bc0c5b062c4c168b40d29ca85dac053 Mon Sep 17 00:00:00 2001 From: jiaxiao zhou Date: Fri, 15 Mar 2024 18:58:32 +0000 Subject: [PATCH 4/5] cargo clippy Signed-off-by: jiaxiao zhou --- crates/shim/src/cgroup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/shim/src/cgroup.rs b/crates/shim/src/cgroup.rs index 5547254e..93ebf0db 100644 --- a/crates/shim/src/cgroup.rs +++ b/crates/shim/src/cgroup.rs @@ -201,7 +201,7 @@ pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { let content = content.lines().next().unwrap_or(""); let Ok(path) = parse_cgroups_v2_path(content)?.canonicalize() else { - return Err(Error::Other(format!("cgroup path not found"))); + return Err(Error::Other("cgroup path not found".to_string())); }; Ok(path) } From 61e33b00025e646e5f523f73f88735c4a2d8f55a Mon Sep 17 00:00:00 2001 From: Jiaxiao Zhou Date: Fri, 15 Mar 2024 12:00:11 -0700 Subject: [PATCH 5/5] Update crates/shim/src/cgroup.rs Co-authored-by: James Sturtevant Signed-off-by: Jiaxiao Zhou --- crates/shim/src/cgroup.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/shim/src/cgroup.rs b/crates/shim/src/cgroup.rs index 93ebf0db..5e4222c0 100644 --- a/crates/shim/src/cgroup.rs +++ b/crates/shim/src/cgroup.rs @@ -209,7 +209,7 @@ pub fn get_cgroups_v2_path_by_pid(pid: u32) -> Result { // https://github.com/opencontainers/runc/blob/1950892f69597aa844cbf000fbdf77610dda3a44/libcontainer/cgroups/fs2/defaultpath.go#L83 fn parse_cgroups_v2_path(content: &str) -> Result { // the entry for cgroup v2 is always in the format like `0::$PATH` - // where 0 is the hierarchy ID, the controller name is ommit in cgroup v2 + // 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 {