monitor children and write exit paths

[haircommander]

This set of patches encompasses child handling in conmon-rs. specifically it:

• adds state to keep track of the grandchildren container processes
• sets self as subreaper to catch grandchildren exiting
• catches the sigchld from the children/grandchildren and writes their exit codes down
• kills the children its tracking upon exiting

you know, like a normal parent does 🙂

[codecov-commenter]

Codecov Report

Merging #80 (2d5e937) into main (80b5595) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main     #80   +/-   ##
=====================================
  Coverage   6.89%   6.89%           
=====================================
  Files          2       2           
  Lines         29      29           
  Branches      17      17           
=====================================
  Hits           2       2           
  Misses        27      27           

[cgwalters]

Ah...wow yeah, I think using pidfd would be cleaner.

[haircommander]

one day..

[mrunalp]

tokio-rs/tokio#4016

[cgwalters]

BTW do we need this versus PRCTL_PR_SET_PDEATHSIG? See e.g. containers/containers-image-proxy-rs#16

[haircommander]

pdeathsignal just kills conmon-rs and I'm not sure why. maybe something interacting with tokio?

[haircommander]

test failures were similar to kata-containers/kata-containers#1826 -- thanks kata folks for an inspiration for the fix

[saschagrunert]

This additional clone can be removed.

Suggested change

let grandchildren = Arc::clone(&grandchildren);

[haircommander]

if I don't have it I get

error[E0382]: use of moved value: `grandchildren`
  --> conmon-rs/server/src/child_reaper.rs:23:43
   |
23 | ...                   thread::spawn(move || {
   |                                     ^^^^^^^ value moved into closure here, in previous iteration of loop
24 | ...                       if let Err(e) = Self::forget_grandchild(grandchildren, grandchild_pid, exit_status) {
   |                                                                   ------------- use occurs due to use in closure
   |
   = note: move occurs because `grandchildren` has type `Arc<std::sync::Mutex<HashMap<i32, ReapableChild>>>`, which does not implement the `Copy` trait

[saschagrunert]

I think we can remove this clone:

Suggested change

let wait_lock = Arc::clone(&wait_lock);

[saschagrunert]

I think this can be changed to avoid the unwrap. Also, I'm not sure if we really need the let wait_lock = Arc::clone(&self.wait_lock);:

diff --git a/conmon-rs/server/src/child_reaper.rs b/conmon-rs/server/src/child_reaper.rs
index d44756c9..4264ea7f 100644
--- a/conmon-rs/server/src/child_reaper.rs
+++ b/conmon-rs/server/src/child_reaper.rs
@@ -1,6 +1,6 @@
 //! Child process reaping and management.
 
-use anyhow::{bail, Result};
+use anyhow::{bail, format_err, Context, Result};
 use getset::Getters;
 use log::{debug, error};
 use nix::sys::signal::{kill, Signal};
@@ -60,11 +60,7 @@ impl ChildReaper {
         Ok(())
     }
 
-    pub fn create_child<P, I, S>(
-        &self,
-        cmd: P,
-        args: I,
-    ) -> Result<std::process::ExitStatus, std::io::Error>
+    pub fn create_child<P, I, S>(&self, cmd: P, args: I) -> Result<std::process::ExitStatus>
     where
         P: AsRef<std::ffi::OsStr>,
         I: IntoIterator<Item = S>,
@@ -77,10 +73,12 @@ impl ChildReaper {
         // we prevent our reaper thread from calling waitpid until this child has completed.
         // Inspired by
         // https://github.com/kata-containers/kata-containers/blob/828a304883a4/src/agent/src/signal.rs#L27
-        let wait_lock = Arc::clone(&self.wait_lock);
-        let _lock = wait_lock.lock().unwrap();
+        let _ = &self
+            .wait_lock
+            .lock()
+            .map_err(|e| format_err!("take wait lock: {}", e))?;
 
-        cmd.spawn()?.wait()
+        Ok(cmd.spawn()?.wait().context("wait for child")?)
     }
 
     pub fn watch_grandchild(&self, id: String, pid: i32, exit_paths: Vec<PathBuf>) -> Result<()> {

[saschagrunert]

Let's do not unwrap:

Suggested change

	let mut map = locked_map.lock().unwrap();
	let mut map = locked_grandchildren
	.lock()
	.map_err(|e| format_err!("lock grandchildren: {}", e))?;