Skip to content

Commit

Permalink
agent: Drop the Option for LinuxContainer.cgroup_manager
Browse files Browse the repository at this point in the history 
Cgroup manager for a container will always be created.
Thus, dropping the option for LinuxContainer.cgroup_manager
is feasible and could simplify the code.

Fixes: kata-containers#5778

Signed-off-by: Yuan-Zhuo <[email protected]>
  • Loading branch information
@Yuan-Zhuo
Yuan-Zhuo committed Dec 7, 2022
1 parent 326d589 commit 7fdbbcd
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 93 deletions.
109 changes: 34 additions & 75 deletions src/agent/rustjail/src/container.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ impl Default for ContainerStatus {
}

// We might want to change this to thiserror in the future
const MissingCGroupManager: &str = "failed to get container's cgroup Manager";
const MissingLinux: &str = "no linux config";
const InvalidNamespace: &str = "invalid namespace type";

Expand Down Expand Up @@ -243,7 +242,7 @@ pub struct LinuxContainer {
pub id: String,
pub root: String,
pub config: Config,
pub cgroup_manager: Option<Box<dyn Manager + Send + Sync>>,
pub cgroup_manager: Box<dyn Manager + Send + Sync>,
pub init_process_pid: pid_t,
pub init_process_start_time: u64,
pub uid_map_path: String,
Expand Down Expand Up @@ -292,16 +291,11 @@ impl Container for LinuxContainer {
));
}

if self.cgroup_manager.is_some() {
self.cgroup_manager
.as_ref()
.unwrap()
.freeze(FreezerState::Frozen)?;
self.cgroup_manager.as_ref().freeze(FreezerState::Frozen)?;

self.status.transition(ContainerState::Paused);
return Ok(());
}
Err(anyhow!(MissingCGroupManager))
self.status.transition(ContainerState::Paused);

Ok(())
}

fn resume(&mut self) -> Result<()> {
Expand All @@ -310,16 +304,11 @@ impl Container for LinuxContainer {
return Err(anyhow!("container status is: {:?}, not paused", status));
}

if self.cgroup_manager.is_some() {
self.cgroup_manager
.as_ref()
.unwrap()
.freeze(FreezerState::Thawed)?;
self.cgroup_manager.as_ref().freeze(FreezerState::Thawed)?;

self.status.transition(ContainerState::Running);
return Ok(());
}
Err(anyhow!(MissingCGroupManager))
self.status.transition(ContainerState::Running);

Ok(())
}
}

Expand Down Expand Up @@ -847,22 +836,17 @@ impl BaseContainer for LinuxContainer {
}

fn stats(&self) -> Result<StatsContainerResponse> {
let mut r = StatsContainerResponse::default();

if self.cgroup_manager.is_some() {
r.cgroup_stats =
SingularPtrField::some(self.cgroup_manager.as_ref().unwrap().get_stats()?);
}

// what about network interface stats?

Ok(r)
Ok(StatsContainerResponse {
cgroup_stats: SingularPtrField::some(self.cgroup_manager.as_ref().get_stats()?),
..Default::default()
})
}

fn set(&mut self, r: LinuxResources) -> Result<()> {
if self.cgroup_manager.is_some() {
self.cgroup_manager.as_ref().unwrap().set(&r, true)?;
}
self.cgroup_manager.as_ref().set(&r, true)?;

self.config
.spec
.as_mut()
Expand Down Expand Up @@ -1035,7 +1019,7 @@ impl BaseContainer for LinuxContainer {
&logger,
spec,
&p,
self.cgroup_manager.as_ref().unwrap().as_ref(),
self.cgroup_manager.as_ref(),
self.config.use_systemd_cgroup,
&st,
&mut pipe_w,
Expand Down Expand Up @@ -1114,19 +1098,19 @@ impl BaseContainer for LinuxContainer {
)?;
fs::remove_dir_all(&self.root)?;

if let Some(cgm) = self.cgroup_manager.as_mut() {
// Kill all of the processes created in this container to prevent
// the leak of some daemon process when this container shared pidns
// with the sandbox.
let pids = cgm.get_pids().context("get cgroup pids")?;
for i in pids {
if let Err(e) = signal::kill(Pid::from_raw(i), Signal::SIGKILL) {
warn!(self.logger, "kill the process {} error: {:?}", i, e);
}
let cgm = self.cgroup_manager.as_mut();
// Kill all of the processes created in this container to prevent
// the leak of some daemon process when this container shared pidns
// with the sandbox.
let pids = cgm.get_pids().context("get cgroup pids")?;
for i in pids {
if let Err(e) = signal::kill(Pid::from_raw(i), Signal::SIGKILL) {
warn!(self.logger, "kill the process {} error: {:?}", i, e);
}

cgm.destroy().context("destroy cgroups")?;
}

cgm.destroy().context("destroy cgroups")?;

Ok(())
}

Expand Down Expand Up @@ -1514,7 +1498,7 @@ impl LinuxContainer {
Ok(LinuxContainer {
id: id.clone(),
root,
cgroup_manager: Some(cgroup_manager),
cgroup_manager,
status: ContainerStatus::new(),
uid_map_path: String::from(""),
gid_map_path: "".to_string(),
Expand Down Expand Up @@ -1980,24 +1964,12 @@ mod tests {
assert!(format!("{:?}", ret).contains("failed to pause container"))
}

#[test]
fn test_linuxcontainer_pause_cgroupmgr_is_none() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
c.cgroup_manager = None;
c.pause().map_err(|e| anyhow!(e))
});

assert!(ret.is_err(), "Expecting error, Got {:?}", ret);
}

#[test]
fn test_linuxcontainer_pause() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
let cgroup_manager: Box<dyn Manager + Send + Sync> =
Box::new(FsManager::new("").map_err(|e| {
anyhow!(format!("fail to create cgroup manager with path: {:}", e))
})?);
c.cgroup_manager = Some(cgroup_manager);
c.cgroup_manager = Box::new(FsManager::new("").map_err(|e| {
anyhow!(format!("fail to create cgroup manager with path: {:}", e))
})?);
c.pause().map_err(|e| anyhow!(e))
});

Expand All @@ -2016,25 +1988,12 @@ mod tests {
assert!(format!("{:?}", ret).contains("not paused"))
}

#[test]
fn test_linuxcontainer_resume_cgroupmgr_is_none() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
c.status.transition(ContainerState::Paused);
c.cgroup_manager = None;
c.resume().map_err(|e| anyhow!(e))
});

assert!(ret.is_err(), "Expecting error, Got {:?}", ret);
}

#[test]
fn test_linuxcontainer_resume() {
let ret = new_linux_container_and_then(|mut c: LinuxContainer| {
let cgroup_manager: Box<dyn Manager + Send + Sync> =
Box::new(FsManager::new("").map_err(|e| {
anyhow!(format!("fail to create cgroup manager with path: {:}", e))
})?);
c.cgroup_manager = Some(cgroup_manager);
c.cgroup_manager = Box::new(FsManager::new("").map_err(|e| {
anyhow!(format!("fail to create cgroup manager with path: {:}", e))
})?);
// Change status to paused, this way we can resume it
c.status.transition(ContainerState::Paused);
c.resume().map_err(|e| anyhow!(e))
Expand Down
23 changes: 7 additions & 16 deletions src/agent/src/rpc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,13 @@ impl AgentService {
}

// start oom event loop
if let Some(ref ctr) = ctr.cgroup_manager {
let cg_path = ctr.get_cgroup_path("memory");

if let Ok(cg_path) = cg_path {
let rx = notifier::notify_oom(cid.as_str(), cg_path.to_string()).await?;
let cg_path = ctr.cgroup_manager.as_ref().get_cgroup_path("memory");

s.run_oom_event_monitor(rx, cid.clone()).await;
}
if let Ok(cg_path) = cg_path {
let rx = notifier::notify_oom(cid.as_str(), cg_path.to_string()).await?;

s.run_oom_event_monitor(rx, cid.clone()).await;
}

Ok(())
Expand Down Expand Up @@ -465,11 +464,7 @@ impl AgentService {
let ctr = sandbox
.get_container(cid)
.ok_or_else(|| anyhow!("Invalid container id {}", cid))?;
let cm = ctr
.cgroup_manager
.as_ref()
.ok_or_else(|| anyhow!("cgroup manager not exist"))?;
cm.freeze(state)?;
ctr.cgroup_manager.as_ref().freeze(state)?;
Ok(())
}

Expand All @@ -479,11 +474,7 @@ impl AgentService {
let ctr = sandbox
.get_container(cid)
.ok_or_else(|| anyhow!("Invalid container id {}", cid))?;
let cm = ctr
.cgroup_manager
.as_ref()
.ok_or_else(|| anyhow!("cgroup manager not exist"))?;
let pids = cm.get_pids()?;
let pids = ctr.cgroup_manager.as_ref().get_pids()?;
Ok(pids)
}

Expand Down
1 change: 0 additions & 1 deletion src/agent/src/sandbox.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,6 @@ impl Sandbox {
info!(self.logger, "updating {}", ctr.id.as_str());
ctr.cgroup_manager
.as_ref()
.unwrap()
.update_cpuset_path(guest_cpuset.as_str(), container_cpust)?;
}

Expand Down
1 change: 0 additions & 1 deletion src/tools/runk/libcontainer/src/container.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,6 @@ impl ContainerLauncher {
self.runner
.cgroup_manager
.as_ref()
.unwrap()
.as_any()?
.downcast_ref::<CgroupManager>()
.unwrap()
Expand Down

0 comments on commit 7fdbbcd

Please sign in to comment.