PHD: improve ctrl-c handling (#634)

Install a signal handler in the PHD runner that catches SIGINT and relays it via
`tokio::sync::watch` to any tasks that are currently executing tests. This
allows the test tasks to exit at the next available `await` point. Since these
tasks are themselves awaited before the runner exits, this ensures that all VMs
and disks created by a test can be dropped and cleaned up before the runner goes
away.

Ctrl-C characters sent to a shell will typically cause SIGINT to be sent to all
processes in the foreground process group. This is bad for PHD because any
Propolis servers the runner spawned will be killed before `TestVm::drop` gets a
chance to gracefully stop the VMs inside. Since dropping a `PropolisServer`
kills the process in any case, get around this by configuring PHD-spawned
Propolis servers to ignore SIGINT.

Tested with local Debian 11 runs, observing in particular that interrupting a
run with Ctrl-C does not leak any VMMs or temporary disk objects.

Author: gjcolombo

phd-tests/framework/src/test_vm/server.rs

@@ -4,7 +4,7 @@
 
 //! Routines and data structures for working with Propolis server processes.
 
-use std::{fmt::Debug, net::SocketAddrV4};
+use std::{fmt::Debug, net::SocketAddrV4, os::unix::process::CommandExt};
 
 use anyhow::Result;
 use camino::{Utf8Path, Utf8PathBuf};
@@ -62,18 +62,45 @@ impl PropolisServer {
         let (server_stdout, server_stderr) =
             log_mode.get_handles(&data_dir, vm_name)?;
 
+        let mut server_cmd = std::process::Command::new("pfexec");
+        server_cmd
+            .args([
+                server_path.as_str(),
+                "run",
+                config_toml_path.as_str(),
+                server_addr.to_string().as_str(),
+                vnc_addr.to_string().as_str(),
+            ])
+            .stdout(server_stdout)
+            .stderr(server_stderr);
+
+        // Gracefully shutting down a Propolis server requires PHD to send an
+        // instance stop request to the server before it is actually terminated.
+        // This ensures that the server has a chance to clean up kernel VMM
+        // resources. It's desirable for the server to do this and not PHD
+        // because a failure to clean up VMMs on stop is a Propolis bug.
+        //
+        // The PHD runner sets up a SIGINT handler that tries to give the
+        // framework an opportunity to issue these requests before the runner
+        // exits. However, pressing Ctrl-C in a shell will typically broadcast
+        // SIGINT to all of the processes in the foreground process's group, not
+        // just to the foreground process itself. This means that a Ctrl-C press
+        // will usually kill all of PHD's Propolis servers before the cleanup
+        // logic can run.
+        //
+        // To avoid this problem, add a pre-`exec` hook that directs Propolis
+        // servers to ignore SIGINT. On Ctrl-C, the runner will drop all active
+        // `TestVm`s, and this drop path (if allowed to complete) will kill all
+        // these processes.
+        unsafe {
+            server_cmd.pre_exec(move || {
+                libc::signal(libc::SIGINT, libc::SIG_IGN);
+                Ok(())
+            });
+        }
+
         let server = PropolisServer {
-            server: std::process::Command::new("pfexec")
-                .args([
-                    server_path.as_str(),
-                    "run",
-                    config_toml_path.as_str(),
-                    server_addr.to_string().as_str(),
-                    vnc_addr.to_string().as_str(),
-                ])
-                .stdout(server_stdout)
-                .stderr(server_stderr)
-                .spawn()?,
+            server: server_cmd.spawn()?,
             address: server_addr,
         };
 

phd-tests/runner/src/execute.rs

@@ -6,7 +6,9 @@ use std::sync::Arc;
 use std::time::{Duration, Instant};
 
 use phd_tests::phd_testcase::{Framework, TestCase, TestOutcome};
-use tracing::{error, info};
+use tokio::signal::unix::{signal, SignalKind};
+use tokio::sync::watch;
+use tracing::{error, info, warn};
 
 use crate::config::RunOptions;
 use crate::fixtures::TestFixtures;
@@ -76,32 +78,53 @@ pub async fn run_tests_with_ctx(
     }
 
     fixtures.execution_setup().unwrap();
-
+    let sigint_rx = set_sigint_handler();
     info!("Running {} test(s)", executions.len());
     let start_time = Instant::now();
-    'exec_loop: for execution in &mut executions {
+    for execution in &mut executions {
+        if *sigint_rx.borrow() {
+            info!("Test run interrupted by SIGINT");
+            break;
+        }
+
         info!("Starting test {}", execution.tc.fully_qualified_name());
 
         // Failure to run a setup fixture is fatal to the rest of the run, but
         // it's still possible to report results, so return gracefully instead
         // of panicking.
         if let Err(e) = fixtures.test_setup() {
             error!("Error running test setup fixture: {}", e);
-            break 'exec_loop;
+            break;
         }
 
         stats.tests_not_run -= 1;
         let test_ctx = ctx.clone();
         let tc = execution.tc;
-        let test_outcome =
-            match tokio::spawn(async move { tc.run(test_ctx.as_ref()).await })
-                .await
-            {
-                Ok(outcome) => outcome,
-                Err(_) => TestOutcome::Failed(Some(
-                    "test task panicked, see test logs".to_string(),
-                )),
-            };
+        let mut sigint_rx_task = sigint_rx.clone();
+        let test_outcome = tokio::spawn(async move {
+            tokio::select! {
+                // Ensure interrupt signals are always handled instead of
+                // continuing to run the test.
+                biased;
+                result = sigint_rx_task.changed() => {
+                    assert!(
+                        result.is_ok(),
+                        "SIGINT channel shouldn't drop while tests are running"
+                    );
+
+                    TestOutcome::Failed(
+                        Some("test interrupted by SIGINT".to_string())
+                    )
+                }
+                outcome = tc.run(test_ctx.as_ref()) => outcome
+            }
+        })
+        .await
+        .unwrap_or_else(|_| {
+            TestOutcome::Failed(Some(
+                "test task panicked, see test logs".to_string(),
+            ))
+        });
 
         info!(
             "test {} ... {}{}",
@@ -132,7 +155,7 @@ pub async fn run_tests_with_ctx(
         execution.status = Status::Ran(test_outcome);
         if let Err(e) = fixtures.test_cleanup().await {
             error!("Error running cleanup fixture: {}", e);
-            break 'exec_loop;
+            break;
         }
     }
     stats.duration = start_time.elapsed();
@@ -141,3 +164,35 @@ pub async fn run_tests_with_ctx(
 
     stats
 }
+
+/// Sets a global handler for SIGINT and hands the resulting signal channel over
+/// to a task that handles this signal. Returns a receiver to which the signal
+/// handler task publishes `true` to the channel when SIGINT is received.
+fn set_sigint_handler() -> watch::Receiver<bool> {
+    let mut sigint =
+        signal(SignalKind::interrupt()).expect("failed to set SIGINT handler");
+
+    let (sigint_tx, sigint_rx) = watch::channel(false);
+    tokio::spawn(async move {
+        loop {
+            sigint.recv().await;
+
+            // If a signal was previously dispatched to the channel, exit
+            // immediately with the customary SIGINT exit code (130 is 128 +
+            // SIGINT). This allows users to interrupt tests even if they aren't
+            // at an await point (at the cost of not having destructors run).
+            if *sigint_tx.borrow() {
+                error!(
+                    "SIGINT received while shutting down, rudely terminating"
+                );
+                error!("some processes and resources may have been leaked!");
+                std::process::exit(130);
+            }
+
+            warn!("SIGINT received, sending shutdown signal to tests");
+            let _ = sigint_tx.send(true);
+        }
+    });
+
+    sigint_rx
+}