From 84134832bc96d41936a6af7e1d1962e7b0dfef97 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 23 Dec 2021 15:46:46 -0500 Subject: [PATCH 1/3] Gracefully terminate CRDB --- Cargo.lock | 14 ++++++++++++++ test-utils/Cargo.toml | 1 + test-utils/src/dev/db.rs | 9 +++++---- test-utils/src/dev/mod.rs | 14 ++++++++++++-- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7d0c748d377..8a9526c8241 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1816,6 +1816,19 @@ dependencies = [ "syn", ] +[[package]] +name = "nix" +version = "0.23.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f866317acbd3a240710c63f065ffb1e4fd466259045ccb504130b7f668f35c6" +dependencies = [ + "bitflags", + "cc", + "cfg-if", + "libc", + "memoffset", +] + [[package]] name = "normalize-line-endings" version = "0.3.0" @@ -2064,6 +2077,7 @@ dependencies = [ "expectorate", "futures", "libc", + "nix", "omicron-common", "postgres-protocol", "signal-hook", diff --git a/test-utils/Cargo.toml b/test-utils/Cargo.toml index 31b9c7466c8..e43ff9bc5d4 100644 --- a/test-utils/Cargo.toml +++ b/test-utils/Cargo.toml @@ -8,6 +8,7 @@ license = "MPL-2.0" anyhow = "1.0" futures = "0.3.18" libc = "0.2.111" +nix = "0.23" omicron-common = { path = "../common" } postgres-protocol = "0.6.3" signal-hook = "0.3" diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index 88361210bfa..e9520d6870e 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -8,6 +8,7 @@ use crate::dev::poll; use anyhow::anyhow; use anyhow::bail; use anyhow::Context; +use nix::{sys::signal, unistd::Pid}; use omicron_common::config::PostgresConfigWithUrl; use std::ffi::{OsStr, OsString}; use std::fmt; @@ -527,14 +528,14 @@ impl CockroachInstance { */ pub async fn cleanup(&mut self) -> Result<(), anyhow::Error> { /* - * Kill the process and wait for it to exit so that we can remove the + * SIGTERM the process and wait for it to exit so that we can remove the * temporary directory that we may have used to store its data. We * don't care what the result of the process was. */ if let Some(child_process) = self.child_process.as_mut() { - child_process - .start_kill() - .context("sending SIGKILL to child process")?; + let pid = Pid::from_raw(child_process.id().expect("Missing child PID") as i32); + + signal::kill(pid, signal::SIGTERM).context("Failed to send sigterm to process")?; child_process.wait().await.context("waiting for child process")?; self.child_process = None; } diff --git a/test-utils/src/dev/mod.rs b/test-utils/src/dev/mod.rs index 7749ed1ce70..2ddabe6051a 100644 --- a/test-utils/src/dev/mod.rs +++ b/test-utils/src/dev/mod.rs @@ -102,6 +102,15 @@ pub async fn test_setup_database_seed(log: &Logger) { std::fs::create_dir_all(&dir).unwrap(); let mut db = setup_database(log, Some(&dir), StorageSource::Populate).await; db.cleanup().await.unwrap(); + + // See https://github.com/cockroachdb/cockroach/issues/74231 for context on + // this. We use this assertion to check that our seed directory won't point + // back to itself, even if it is copied elsewhere. + assert_eq!( + 0, + dir.join("temp-dirs-record.txt").metadata().expect("Cannot access metadata").len(), + "Temporary directory record should be empty after graceful shutdown", + ); } /// Set up a [`db::CockroachInstance`] for running tests. @@ -131,11 +140,12 @@ async fn setup_database( // If we're going to copy the storage directory from the seed, // it is critical we do so before starting the DB. if matches!(storage_source, StorageSource::CopyFromSeed) { + let seed = seed_dir(); info!(&log, "cockroach: copying from seed directory ({}) to storage directory ({})", - seed_dir().to_string_lossy(), starter.store_dir().to_string_lossy(), + seed.to_string_lossy(), starter.store_dir().to_string_lossy(), ); - copy_dir(seed_dir(), starter.store_dir()) + copy_dir(seed, starter.store_dir()) .expect("Cannot copy storage from seed directory"); } From 3d266bae8a655293fb5402c5ad5175bf188e2064 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 23 Dec 2021 15:48:07 -0500 Subject: [PATCH 2/3] fmt --- test-utils/src/dev/db.rs | 7 +++++-- test-utils/src/dev/mod.rs | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index e9520d6870e..5069ee1c3a2 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -533,9 +533,12 @@ impl CockroachInstance { * don't care what the result of the process was. */ if let Some(child_process) = self.child_process.as_mut() { - let pid = Pid::from_raw(child_process.id().expect("Missing child PID") as i32); + let pid = Pid::from_raw( + child_process.id().expect("Missing child PID") as i32, + ); - signal::kill(pid, signal::SIGTERM).context("Failed to send sigterm to process")?; + signal::kill(pid, signal::SIGTERM) + .context("Failed to send sigterm to process")?; child_process.wait().await.context("waiting for child process")?; self.child_process = None; } diff --git a/test-utils/src/dev/mod.rs b/test-utils/src/dev/mod.rs index 2ddabe6051a..efb9b7df437 100644 --- a/test-utils/src/dev/mod.rs +++ b/test-utils/src/dev/mod.rs @@ -108,7 +108,10 @@ pub async fn test_setup_database_seed(log: &Logger) { // back to itself, even if it is copied elsewhere. assert_eq!( 0, - dir.join("temp-dirs-record.txt").metadata().expect("Cannot access metadata").len(), + dir.join("temp-dirs-record.txt") + .metadata() + .expect("Cannot access metadata") + .len(), "Temporary directory record should be empty after graceful shutdown", ); } From 42030b67618931b89aab7bde286a68803ae0d8c4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 27 Dec 2021 10:54:09 -0500 Subject: [PATCH 3/3] Remove nix, use unsafe libc instead --- Cargo.lock | 14 -------------- test-utils/Cargo.toml | 1 - test-utils/src/dev/db.rs | 13 ++++++------- 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8a9526c8241..7d0c748d377 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1816,19 +1816,6 @@ dependencies = [ "syn", ] -[[package]] -name = "nix" -version = "0.23.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9f866317acbd3a240710c63f065ffb1e4fd466259045ccb504130b7f668f35c6" -dependencies = [ - "bitflags", - "cc", - "cfg-if", - "libc", - "memoffset", -] - [[package]] name = "normalize-line-endings" version = "0.3.0" @@ -2077,7 +2064,6 @@ dependencies = [ "expectorate", "futures", "libc", - "nix", "omicron-common", "postgres-protocol", "signal-hook", diff --git a/test-utils/Cargo.toml b/test-utils/Cargo.toml index e43ff9bc5d4..31b9c7466c8 100644 --- a/test-utils/Cargo.toml +++ b/test-utils/Cargo.toml @@ -8,7 +8,6 @@ license = "MPL-2.0" anyhow = "1.0" futures = "0.3.18" libc = "0.2.111" -nix = "0.23" omicron-common = { path = "../common" } postgres-protocol = "0.6.3" signal-hook = "0.3" diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index 5069ee1c3a2..47291062a31 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -8,7 +8,6 @@ use crate::dev::poll; use anyhow::anyhow; use anyhow::bail; use anyhow::Context; -use nix::{sys::signal, unistd::Pid}; use omicron_common::config::PostgresConfigWithUrl; use std::ffi::{OsStr, OsString}; use std::fmt; @@ -533,12 +532,12 @@ impl CockroachInstance { * don't care what the result of the process was. */ if let Some(child_process) = self.child_process.as_mut() { - let pid = Pid::from_raw( - child_process.id().expect("Missing child PID") as i32, - ); - - signal::kill(pid, signal::SIGTERM) - .context("Failed to send sigterm to process")?; + let pid = child_process.id().expect("Missing child PID") as i32; + let success = + 0 == unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; + if !success { + anyhow!("Failed to send SIGTERM to DB"); + } child_process.wait().await.context("waiting for child process")?; self.child_process = None; }