From 99342d1581a059170dffa26ce488f3fc3957ae1b Mon Sep 17 00:00:00 2001 From: Furisto <24721048+Furisto@users.noreply.github.com> Date: Mon, 28 Jun 2021 20:06:53 +0200 Subject: [PATCH] Require only cgroups that are needed to fullfill the resource restrictions --- src/cgroups/v1/blkio.rs | 12 +++++- src/cgroups/v1/controller.rs | 7 ++++ src/cgroups/v1/cpu.rs | 19 +++++++++- src/cgroups/v1/cpuacct.rs | 7 ++++ src/cgroups/v1/cpuset.rs | 16 +++++++- src/cgroups/v1/devices.rs | 7 ++++ src/cgroups/v1/freezer.rs | 20 +++++++--- src/cgroups/v1/hugetlb.rs | 16 +++++++- src/cgroups/v1/manager.rs | 55 ++++++++++++++++++++++++---- src/cgroups/v1/memory.rs | 12 +++++- src/cgroups/v1/network_classifier.rs | 12 +++++- src/cgroups/v1/network_priority.rs | 12 +++++- src/cgroups/v1/pids.rs | 16 ++++++-- src/cgroups/v1/util.rs | 11 +++--- 14 files changed, 192 insertions(+), 30 deletions(-) diff --git a/src/cgroups/v1/blkio.rs b/src/cgroups/v1/blkio.rs index d80dc36af..3480084c1 100644 --- a/src/cgroups/v1/blkio.rs +++ b/src/cgroups/v1/blkio.rs @@ -12,15 +12,25 @@ const CGROUP_BLKIO_THROTTLE_WRITE_IOPS: &str = "blkio.throttle.write_iops_device pub struct Blkio {} impl Controller for Blkio { + type Resource = LinuxBlockIo; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply blkio cgroup config"); - if let Some(blkio) = &linux_resources.block_io { + if let Some(blkio) = Self::needs_to_handle(linux_resources) { Self::apply(cgroup_root, blkio)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(blkio) = &linux_resources.block_io { + return Some(blkio); + } + + None + } } impl Blkio { diff --git a/src/cgroups/v1/controller.rs b/src/cgroups/v1/controller.rs index 408d357da..9aaa8fcae 100644 --- a/src/cgroups/v1/controller.rs +++ b/src/cgroups/v1/controller.rs @@ -8,11 +8,18 @@ use oci_spec::LinuxResources; use crate::cgroups::common::{self, CGROUP_PROCS}; pub trait Controller { + type Resource; + + /// Adds a new task specified by its pid to the cgroup fn add_task(pid: Pid, cgroup_path: &Path) -> Result<()> { fs::create_dir_all(cgroup_path)?; common::write_cgroup_file(cgroup_path.join(CGROUP_PROCS), pid)?; Ok(()) } + /// Applies resource restrictions to the cgroup fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()>; + + /// Checks if the controller needs to handle this request + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource>; } diff --git a/src/cgroups/v1/cpu.rs b/src/cgroups/v1/cpu.rs index 856d90a0b..006bc09cf 100644 --- a/src/cgroups/v1/cpu.rs +++ b/src/cgroups/v1/cpu.rs @@ -16,15 +16,32 @@ const CGROUP_CPU_RT_PERIOD: &str = "cpu.rt_period_us"; pub struct Cpu {} impl Controller for Cpu { + type Resource = LinuxCpu; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply Cpu cgroup config"); - if let Some(cpu) = &linux_resources.cpu { + if let Some(cpu) = Self::needs_to_handle(linux_resources) { Self::apply(cgroup_root, cpu)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(cpu) = &linux_resources.cpu { + if cpu.shares.is_some() + || cpu.period.is_some() + || cpu.quota.is_some() + || cpu.realtime_period.is_some() + || cpu.realtime_runtime.is_some() + { + return Some(cpu); + } + } + + None + } } impl Cpu { diff --git a/src/cgroups/v1/cpuacct.rs b/src/cgroups/v1/cpuacct.rs index 889f905d0..2632847e2 100644 --- a/src/cgroups/v1/cpuacct.rs +++ b/src/cgroups/v1/cpuacct.rs @@ -8,9 +8,16 @@ use super::Controller; pub struct CpuAcct {} impl Controller for CpuAcct { + type Resource = (); + fn apply(_linux_resources: &LinuxResources, _cgroup_path: &Path) -> Result<()> { Ok(()) } + + // apply never needs to be called, for accounting only + fn needs_to_handle(_linux_resources: &LinuxResources) -> Option<&Self::Resource> { + None + } } #[cfg(test)] diff --git a/src/cgroups/v1/cpuset.rs b/src/cgroups/v1/cpuset.rs index e232120c3..b7d669311 100644 --- a/src/cgroups/v1/cpuset.rs +++ b/src/cgroups/v1/cpuset.rs @@ -15,6 +15,8 @@ const CGROUP_CPUSET_MEMS: &str = "cpuset.mems"; pub struct CpuSet {} impl Controller for CpuSet { + type Resource = LinuxCpu; + fn add_task(pid: Pid, cgroup_path: &Path) -> Result<()> { fs::create_dir_all(cgroup_path)?; @@ -28,12 +30,22 @@ impl Controller for CpuSet { fn apply(linux_resources: &LinuxResources, cgroup_path: &Path) -> Result<()> { log::debug!("Apply CpuSet cgroup config"); - if let Some(cpuset) = &linux_resources.cpu { + if let Some(cpuset) = Self::needs_to_handle(linux_resources) { Self::apply(cgroup_path, cpuset)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(cpuset) = &linux_resources.cpu { + if cpuset.cpus.is_some() || cpuset.mems.is_some() { + return Some(cpuset); + } + } + + None + } } impl CpuSet { @@ -52,7 +64,7 @@ impl CpuSet { // if a task is moved into the cgroup and a value has not been set for cpus and mems // Errno 28 (no space left on device) will be returned. Therefore we set the value from the parent if required. fn ensure_not_empty(cgroup_path: &Path, interface_file: &str) -> Result<()> { - let mut current = util::get_subsystem_mount_points(&ControllerType::CpuSet.to_string())?; + let mut current = util::get_subsystem_mount_point(&ControllerType::CpuSet)?; let relative_cgroup_path = cgroup_path.strip_prefix(¤t)?; for component in relative_cgroup_path.components() { diff --git a/src/cgroups/v1/devices.rs b/src/cgroups/v1/devices.rs index 1f71bfcda..3e5f12705 100644 --- a/src/cgroups/v1/devices.rs +++ b/src/cgroups/v1/devices.rs @@ -9,6 +9,8 @@ use oci_spec::{LinuxDeviceCgroup, LinuxDeviceType, LinuxResources}; pub struct Devices {} impl Controller for Devices { + type Resource = (); + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply Devices cgroup config"); @@ -27,6 +29,11 @@ impl Controller for Devices { Ok(()) } + + // always needs to be called due to default devices + fn needs_to_handle(_linux_resources: &LinuxResources) -> Option<&Self::Resource> { + Some(&()) + } } impl Devices { diff --git a/src/cgroups/v1/freezer.rs b/src/cgroups/v1/freezer.rs index 27806ca0c..4a4dd090c 100644 --- a/src/cgroups/v1/freezer.rs +++ b/src/cgroups/v1/freezer.rs @@ -19,20 +19,30 @@ const FREEZER_STATE_FREEZING: &str = "FREEZING"; pub struct Freezer {} impl Controller for Freezer { + type Resource = FreezerState; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply Freezer cgroup config"); create_dir_all(&cgroup_root)?; - if let Some(freezer_state) = linux_resources.freezer { + if let Some(freezer_state) = Self::needs_to_handle(linux_resources) { Self::apply(freezer_state, cgroup_root)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(freezer_state) = &linux_resources.freezer { + return Some(freezer_state); + } + + None + } } impl Freezer { - fn apply(freezer_state: FreezerState, cgroup_root: &Path) -> Result<()> { + fn apply(freezer_state: &FreezerState, cgroup_root: &Path) -> Result<()> { match freezer_state { FreezerState::Undefined => {} FreezerState::Thawed => { @@ -129,7 +139,7 @@ mod tests { // set Frozen state. { let freezer_state = FreezerState::Frozen; - Freezer::apply(freezer_state, &tmp).expect("Set freezer state"); + Freezer::apply(&freezer_state, &tmp).expect("Set freezer state"); let state_content = std::fs::read_to_string(tmp.join(CGROUP_FREEZER_STATE)).expect("Read to string"); @@ -139,7 +149,7 @@ mod tests { // set Thawed state. { let freezer_state = FreezerState::Thawed; - Freezer::apply(freezer_state, &tmp).expect("Set freezer state"); + Freezer::apply(&freezer_state, &tmp).expect("Set freezer state"); let state_content = std::fs::read_to_string(tmp.join(CGROUP_FREEZER_STATE)).expect("Read to string"); @@ -151,7 +161,7 @@ mod tests { let old_state_content = std::fs::read_to_string(tmp.join(CGROUP_FREEZER_STATE)).expect("Read to string"); let freezer_state = FreezerState::Undefined; - Freezer::apply(freezer_state, &tmp).expect("Set freezer state"); + Freezer::apply(&freezer_state, &tmp).expect("Set freezer state"); let state_content = std::fs::read_to_string(tmp.join(CGROUP_FREEZER_STATE)).expect("Read to string"); diff --git a/src/cgroups/v1/hugetlb.rs b/src/cgroups/v1/hugetlb.rs index 1ee473f79..7b4b2b66a 100644 --- a/src/cgroups/v1/hugetlb.rs +++ b/src/cgroups/v1/hugetlb.rs @@ -9,15 +9,27 @@ use oci_spec::{LinuxHugepageLimit, LinuxResources}; pub struct Hugetlb {} impl Controller for Hugetlb { + type Resource = Vec; + fn apply(linux_resources: &LinuxResources, cgroup_root: &std::path::Path) -> Result<()> { log::debug!("Apply Hugetlb cgroup config"); - for hugetlb in &linux_resources.hugepage_limits { - Self::apply(cgroup_root, hugetlb)? + if let Some(hugepage_limits) = Self::needs_to_handle(linux_resources) { + for hugetlb in hugepage_limits { + Self::apply(cgroup_root, hugetlb)? + } } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if !linux_resources.hugepage_limits.is_empty() { + return Some(&linux_resources.hugepage_limits); + } + + None + } } impl Hugetlb { diff --git a/src/cgroups/v1/manager.rs b/src/cgroups/v1/manager.rs index 3266f3bf1..b3aae533c 100644 --- a/src/cgroups/v1/manager.rs +++ b/src/cgroups/v1/manager.rs @@ -2,6 +2,7 @@ use std::fs; use std::path::Path; use std::{collections::HashMap, path::PathBuf}; +use anyhow::bail; use anyhow::Result; use nix::unistd::Pid; @@ -28,23 +29,24 @@ impl Manager { pub fn new(cgroup_path: PathBuf) -> Result { let mut subsystems = HashMap::::new(); for subsystem in CONTROLLERS { - subsystems.insert( - subsystem.clone(), - Self::get_subsystem_path(&cgroup_path, &subsystem.to_string())?, - ); + if let Ok(subsystem_path) = Self::get_subsystem_path(&cgroup_path, subsystem) { + subsystems.insert(subsystem.clone(), subsystem_path); + } else { + log::warn!("Cgroup {} not supported on this system", subsystem); + } } Ok(Manager { subsystems }) } - fn get_subsystem_path(cgroup_path: &Path, subsystem: &str) -> anyhow::Result { + fn get_subsystem_path(cgroup_path: &Path, subsystem: &CtrlType) -> Result { log::debug!("Get path for subsystem: {}", subsystem); - let mount_point = util::get_subsystem_mount_points(subsystem)?; + let mount_point = util::get_subsystem_mount_point(subsystem)?; let cgroup = Process::myself()? .cgroups()? .into_iter() - .find(|c| c.controllers.contains(&subsystem.to_owned())) + .find(|c| c.controllers.contains(&subsystem.to_string())) .unwrap(); let p = if cgroup_path.to_string_lossy().into_owned().is_empty() { @@ -57,6 +59,43 @@ impl Manager { Ok(p) } + + fn get_required_controllers( + &self, + linux_resources: &LinuxResources, + ) -> Result> { + let mut required_controllers = HashMap::new(); + + for controller in CONTROLLERS { + let required = match controller { + CtrlType::Cpu => Cpu::needs_to_handle(linux_resources).is_some(), + CtrlType::CpuAcct => CpuAcct::needs_to_handle(linux_resources).is_some(), + CtrlType::CpuSet => CpuSet::needs_to_handle(linux_resources).is_some(), + CtrlType::Devices => Devices::needs_to_handle(linux_resources).is_some(), + CtrlType::HugeTlb => Hugetlb::needs_to_handle(linux_resources).is_some(), + CtrlType::Memory => Memory::needs_to_handle(linux_resources).is_some(), + CtrlType::Pids => Pids::needs_to_handle(linux_resources).is_some(), + CtrlType::Blkio => Blkio::needs_to_handle(linux_resources).is_some(), + CtrlType::NetworkPriority => { + NetworkPriority::needs_to_handle(linux_resources).is_some() + } + CtrlType::NetworkClassifier => { + NetworkClassifier::needs_to_handle(linux_resources).is_some() + } + CtrlType::Freezer => Freezer::needs_to_handle(linux_resources).is_some(), + }; + + if required { + if let Some(subsystem_path) = self.subsystems.get(controller) { + required_controllers.insert(controller, subsystem_path); + } else { + bail!("Cgroup {} is required to fullfill the request, but is not supported by this system", controller); + } + } + } + + Ok(required_controllers) + } } impl CgroupManager for Manager { @@ -81,7 +120,7 @@ impl CgroupManager for Manager { } fn apply(&self, linux_resources: &LinuxResources) -> Result<()> { - for subsys in &self.subsystems { + for subsys in self.get_required_controllers(linux_resources)? { match subsys.0 { CtrlType::Cpu => Cpu::apply(linux_resources, &subsys.1)?, CtrlType::CpuAcct => CpuAcct::apply(linux_resources, &subsys.1)?, diff --git a/src/cgroups/v1/memory.rs b/src/cgroups/v1/memory.rs index aa09e58e7..760f1d565 100644 --- a/src/cgroups/v1/memory.rs +++ b/src/cgroups/v1/memory.rs @@ -22,10 +22,12 @@ const CGROUP_KERNEL_TCP_MEMORY_LIMIT: &str = "memory.kmem.tcp.limit_in_bytes"; pub struct Memory {} impl Controller for Memory { + type Resource = LinuxMemory; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply Memory cgroup config"); - if let Some(memory) = &linux_resources.memory { + if let Some(memory) = Self::needs_to_handle(linux_resources) { let reservation = memory.reservation.unwrap_or(0); Self::apply(&memory, cgroup_root)?; @@ -74,6 +76,14 @@ impl Controller for Memory { Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(memory) = &linux_resources.memory { + return Some(memory); + } + + None + } } impl Memory { diff --git a/src/cgroups/v1/network_classifier.rs b/src/cgroups/v1/network_classifier.rs index fed3ae2e7..551fc6726 100644 --- a/src/cgroups/v1/network_classifier.rs +++ b/src/cgroups/v1/network_classifier.rs @@ -9,15 +9,25 @@ use oci_spec::{LinuxNetwork, LinuxResources}; pub struct NetworkClassifier {} impl Controller for NetworkClassifier { + type Resource = LinuxNetwork; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply NetworkClassifier cgroup config"); - if let Some(network) = linux_resources.network.as_ref() { + if let Some(network) = Self::needs_to_handle(linux_resources) { Self::apply(cgroup_root, network)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(network) = &linux_resources.network { + return Some(network); + } + + None + } } impl NetworkClassifier { diff --git a/src/cgroups/v1/network_priority.rs b/src/cgroups/v1/network_priority.rs index 6a25a017c..63683bc3c 100644 --- a/src/cgroups/v1/network_priority.rs +++ b/src/cgroups/v1/network_priority.rs @@ -9,15 +9,25 @@ use oci_spec::{LinuxNetwork, LinuxResources}; pub struct NetworkPriority {} impl Controller for NetworkPriority { + type Resource = LinuxNetwork; + fn apply(linux_resources: &LinuxResources, cgroup_root: &Path) -> Result<()> { log::debug!("Apply NetworkPriority cgroup config"); - if let Some(network) = linux_resources.network.as_ref() { + if let Some(network) = Self::needs_to_handle(linux_resources) { Self::apply(cgroup_root, network)?; } Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(network) = &linux_resources.network { + return Some(network); + } + + None + } } impl NetworkPriority { diff --git a/src/cgroups/v1/pids.rs b/src/cgroups/v1/pids.rs index 09f905cce..f09db1b45 100644 --- a/src/cgroups/v1/pids.rs +++ b/src/cgroups/v1/pids.rs @@ -1,4 +1,4 @@ -use std::path::Path; +use std::path::{Path}; use anyhow::Result; @@ -8,10 +8,12 @@ use oci_spec::{LinuxPids, LinuxResources}; pub struct Pids {} impl Controller for Pids { + type Resource = LinuxPids; + fn apply( linux_resources: &LinuxResources, - cgroup_root: &std::path::Path, - ) -> anyhow::Result<()> { + cgroup_root: &Path, + ) -> Result<()> { log::debug!("Apply pids cgroup config"); if let Some(pids) = &linux_resources.pids { @@ -20,6 +22,14 @@ impl Controller for Pids { Ok(()) } + + fn needs_to_handle(linux_resources: &LinuxResources) -> Option<&Self::Resource> { + if let Some(pids) = &linux_resources.pids { + return Some(pids); + } + + None + } } impl Pids { diff --git a/src/cgroups/v1/util.rs b/src/cgroups/v1/util.rs index 7a31e28bf..dd070d6b2 100644 --- a/src/cgroups/v1/util.rs +++ b/src/cgroups/v1/util.rs @@ -3,21 +3,22 @@ use std::{collections::HashMap, path::PathBuf}; use anyhow::{anyhow, Result}; use procfs::process::Process; -use super::controller_type::CONTROLLERS; +use super::{ControllerType, controller_type::CONTROLLERS}; -pub fn list_subsystem_mount_points() -> Result> { +pub fn list_subsystem_mount_points() -> Result> { let mut mount_paths = HashMap::with_capacity(CONTROLLERS.len()); for controller in CONTROLLERS { - if let Ok(mount_point) = get_subsystem_mount_points(&controller.to_string()) { - mount_paths.insert(controller.to_string(), mount_point); + if let Ok(mount_point) = get_subsystem_mount_point(controller) { + mount_paths.insert(controller.to_owned(), mount_point); } } Ok(mount_paths) } -pub fn get_subsystem_mount_points(subsystem: &str) -> Result { +pub fn get_subsystem_mount_point(subsystem: &ControllerType) -> Result { + let subsystem = subsystem.to_string(); Process::myself()? .mountinfo()? .into_iter()