Merge pull request #114 from Furisto/cg-only-required

Require only requested cgroups to be present
youki-dev · Jun 29, 2021 · 843c75a · 843c75a
2 parents 0f91dca + 99342d1
commit 843c75a
Show file tree

Hide file tree

Showing 14 changed files with 192 additions and 30 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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(&current)?;
 
         for component in relative_cgroup_path.components() {

diff --git 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
@@ -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
@@ -9,15 +9,27 @@ use oci_spec::{LinuxHugepageLimit, LinuxResources};
 pub struct Hugetlb {}
 
 impl Controller for Hugetlb {
+    type Resource = Vec<LinuxHugepageLimit>;
+
     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
@@ -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<Self> {
         let mut subsystems = HashMap::<CtrlType, PathBuf>::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<PathBuf> {
+    fn get_subsystem_path(cgroup_path: &Path, subsystem: &CtrlType) -> Result<PathBuf> {
         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<HashMap<&CtrlType, &PathBuf>> {
+        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
@@ -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
@@ -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 {