From 41f22f3fc13e0ee8adaf60c770460f549a70e56d Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 27 Sep 2023 16:07:13 -0700 Subject: [PATCH] [nexus-test-utils] replace build.rs with a new setup script (#4150) Nextest now has support for setup scripts, which means that we can replace the existing `build.rs` with a small binary. I also took the liberty of switching some of the related code over to camino. --- .config/nextest.toml | 11 +++- .github/buildomat/build-and-test.sh | 7 ++- Cargo.lock | 16 ++++- Cargo.toml | 1 + crdb-seed/Cargo.toml | 15 +++++ crdb-seed/src/main.rs | 92 +++++++++++++++++++++++++++++ nexus/test-utils/Cargo.toml | 5 -- nexus/test-utils/build.rs | 37 ------------ nexus/test-utils/src/db.rs | 9 ++- test-utils/Cargo.toml | 1 + test-utils/src/dev/mod.rs | 18 +++--- 11 files changed, 156 insertions(+), 56 deletions(-) create mode 100644 crdb-seed/Cargo.toml create mode 100644 crdb-seed/src/main.rs delete mode 100644 nexus/test-utils/build.rs diff --git a/.config/nextest.toml b/.config/nextest.toml index 32890f66a8..b2a8b360bb 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -3,7 +3,16 @@ # # The required version should be bumped up if we need new features, performance # improvements or bugfixes that are present in newer versions of nextest. -nextest-version = { required = "0.9.55", recommended = "0.9.57" } +nextest-version = { required = "0.9.59", recommended = "0.9.59" } + +experimental = ["setup-scripts"] + +[[profile.default.scripts]] +filter = 'rdeps(nexus-test-utils)' +setup = 'crdb-seed' [profile.ci] fail-fast = false + +[script.crdb-seed] +command = 'cargo run -p crdb-seed' diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index 4245c7f458..8791ea2fa6 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -7,7 +7,7 @@ set -o xtrace # NOTE: This version should be in sync with the recommended version in # .config/nextest.toml. (Maybe build an automated way to pull the recommended # version in the future.) -NEXTEST_VERSION='0.9.57' +NEXTEST_VERSION='0.9.59' cargo --version rustc --version @@ -66,6 +66,11 @@ ptime -m timeout 2h cargo nextest run --profile ci --locked --verbose banner doctest ptime -m timeout 1h cargo test --doc --locked --verbose --no-fail-fast +# We expect the seed CRDB to be placed here, so we explicitly remove it so the +# rmdir check below doesn't get triggered. Nextest doesn't have support for +# teardown scripts so this is the best we've got. +rm -rf "$TEST_TMPDIR/crdb-base" + # # Make sure that we have left nothing around in $TEST_TMPDIR. The easiest way # to check is to try to remove it with `rmdir`. diff --git a/Cargo.lock b/Cargo.lock index bcd07154e8..e5130b6b33 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1236,6 +1236,20 @@ dependencies = [ "cfg-if 1.0.0", ] +[[package]] +name = "crdb-seed" +version = "0.1.0" +dependencies = [ + "camino", + "camino-tempfile", + "dropshot", + "hex", + "omicron-test-utils", + "ring", + "slog", + "tokio", +] + [[package]] name = "criterion" version = "0.5.1" @@ -4438,7 +4452,6 @@ dependencies = [ "serde_urlencoded", "slog", "tempfile", - "tokio", "trust-dns-proto", "trust-dns-resolver", "uuid", @@ -5257,6 +5270,7 @@ name = "omicron-test-utils" version = "0.1.0" dependencies = [ "anyhow", + "camino", "dropshot", "expectorate", "futures", diff --git a/Cargo.toml b/Cargo.toml index 174e1052a0..9498157b28 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,7 @@ members = [ "caboose-util", "certificates", "common", + "crdb-seed", "ddm-admin-client", "deploy", "dev-tools/omdb", diff --git a/crdb-seed/Cargo.toml b/crdb-seed/Cargo.toml new file mode 100644 index 0000000000..01af7cb1d7 --- /dev/null +++ b/crdb-seed/Cargo.toml @@ -0,0 +1,15 @@ +[package] +name = "crdb-seed" +version = "0.1.0" +edition = "2021" +license = "MPL-2.0" + +[dependencies] +camino.workspace = true +camino-tempfile.workspace = true +dropshot.workspace = true +hex.workspace = true +omicron-test-utils.workspace = true +ring.workspace = true +slog.workspace = true +tokio.workspace = true diff --git a/crdb-seed/src/main.rs b/crdb-seed/src/main.rs new file mode 100644 index 0000000000..b8572bd886 --- /dev/null +++ b/crdb-seed/src/main.rs @@ -0,0 +1,92 @@ +use camino::Utf8PathBuf; +use dropshot::{test_util::LogContext, ConfigLogging, ConfigLoggingLevel}; +use omicron_test_utils::dev; +use slog::Logger; +use std::io::Write; + +// Creates a string identifier for the current DB schema and version. +// +// The goal here is to allow to create different "seed" directories +// for each revision of the DB. +fn digest_unique_to_schema() -> String { + let schema = include_str!("../../schema/crdb/dbinit.sql"); + let crdb_version = include_str!("../../tools/cockroachdb_version"); + let mut ctx = ring::digest::Context::new(&ring::digest::SHA256); + ctx.update(&schema.as_bytes()); + ctx.update(&crdb_version.as_bytes()); + let digest = ctx.finish(); + hex::encode(digest.as_ref()) +} + +enum SeedDirectoryStatus { + Created, + Existing, +} + +async fn ensure_seed_directory_exists( + log: &Logger, +) -> (Utf8PathBuf, SeedDirectoryStatus) { + let base_seed_dir = Utf8PathBuf::from_path_buf(std::env::temp_dir()) + .expect("Not a UTF-8 path") + .join("crdb-base"); + std::fs::create_dir_all(&base_seed_dir).unwrap(); + let desired_seed_dir = base_seed_dir.join(digest_unique_to_schema()); + + if desired_seed_dir.exists() { + return (desired_seed_dir, SeedDirectoryStatus::Existing); + } + + // The directory didn't exist when we started, so try to create it. + // + // Nextest will execute it just once, but it is possible for a user to start + // up multiple nextest processes to be running at the same time. So we + // should consider it possible for another caller to create this seed + // directory before we finish setting it up ourselves. + let tmp_seed_dir = + camino_tempfile::Utf8TempDir::new_in(base_seed_dir).unwrap(); + dev::test_setup_database_seed(log, tmp_seed_dir.path()).await; + + // If we can successfully perform the rename, there was either no + // contention or we won a creation race. + // + // If we couldn't perform the rename, the directory might already exist. + // Check that this is the error we encountered -- otherwise, we're + // struggling. + if let Err(err) = std::fs::rename(tmp_seed_dir.path(), &desired_seed_dir) { + if !desired_seed_dir.exists() { + panic!("Cannot rename seed directory for CockroachDB: {err}"); + } + } + + (desired_seed_dir, SeedDirectoryStatus::Created) +} + +#[tokio::main] +async fn main() { + // TODO: dropshot is v heavyweight for this, we should be able to pull in a + // smaller binary + let logctx = LogContext::new( + "crdb_seeding", + &ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Info }, + ); + let (dir, status) = ensure_seed_directory_exists(&logctx.log).await; + match status { + SeedDirectoryStatus::Created => { + slog::info!(logctx.log, "Created seed directory: `{dir}`"); + } + SeedDirectoryStatus::Existing => { + slog::info!(logctx.log, "Using existing seed directory: `{dir}`"); + } + } + if let Ok(env_path) = std::env::var("NEXTEST_ENV") { + let mut file = std::fs::File::create(&env_path) + .expect("failed to open NEXTEST_ENV file"); + writeln!(file, "CRDB_SEED_DIR={dir}") + .expect("failed to write to NEXTEST_ENV file"); + } else { + slog::warn!( + logctx.log, + "NEXTEST_ENV not set (is this script running under nextest?)" + ); + } +} diff --git a/nexus/test-utils/Cargo.toml b/nexus/test-utils/Cargo.toml index db98a979c1..bad225516a 100644 --- a/nexus/test-utils/Cargo.toml +++ b/nexus/test-utils/Cargo.toml @@ -38,8 +38,3 @@ tempfile.workspace = true trust-dns-proto.workspace = true trust-dns-resolver.workspace = true uuid.workspace = true - -[build-dependencies] -dropshot.workspace = true -omicron-test-utils.workspace = true -tokio.workspace = true diff --git a/nexus/test-utils/build.rs b/nexus/test-utils/build.rs deleted file mode 100644 index aada3fe039..0000000000 --- a/nexus/test-utils/build.rs +++ /dev/null @@ -1,37 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use dropshot::{test_util::LogContext, ConfigLogging, ConfigLoggingLevel}; -use omicron_test_utils::dev::test_setup_database_seed; -use std::env; -use std::path::Path; - -// Creates a "pre-populated" CockroachDB storage directory, which -// subsequent tests can copy instead of creating themselves. -// -// Is it critical this happens at build-time? No. However, it -// makes it more convenient for tests to assume this seeded -// directory exists, rather than all attempting to create it -// concurrently. -// -// Refer to the documentation of [`test_setup_database_seed`] for -// more context. -#[tokio::main] -async fn main() { - println!("cargo:rerun-if-changed=build.rs"); - println!("cargo:rerun-if-changed=../../schema/crdb/dbinit.sql"); - println!("cargo:rerun-if-changed=../../tools/cockroachdb_checksums"); - println!("cargo:rerun-if-changed=../../tools/cockroachdb_version"); - - let logctx = LogContext::new( - "crdb_seeding", - &ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Info }, - ); - - let seed = - Path::new(&env::var("OUT_DIR").expect("Missing output directory")) - .join("crdb-base"); - test_setup_database_seed(&logctx.log, &seed).await; - logctx.cleanup_successful(); -} diff --git a/nexus/test-utils/src/db.rs b/nexus/test-utils/src/db.rs index bf0b7ed52d..37d7128c49 100644 --- a/nexus/test-utils/src/db.rs +++ b/nexus/test-utils/src/db.rs @@ -4,9 +4,9 @@ //! Database testing facilities. +use camino::Utf8PathBuf; use omicron_test_utils::dev; use slog::Logger; -use std::path::PathBuf; /// Path to the "seed" CockroachDB directory. /// @@ -16,8 +16,11 @@ use std::path::PathBuf; /// By creating a "seed" version of the database, we can cut down /// on the time spent performing this operation. Instead, we opt /// to copy the database from this seed location. -fn seed_dir() -> PathBuf { - PathBuf::from(concat!(env!("OUT_DIR"), "/crdb-base")) +fn seed_dir() -> Utf8PathBuf { + // The setup script should set this environment variable. + let seed_dir = std::env::var("CRDB_SEED_DIR") + .expect("CRDB_SEED_DIR missing -- are you running this test with `cargo nextest run`?"); + seed_dir.into() } /// Wrapper around [`dev::test_setup_database`] which uses a a diff --git a/test-utils/Cargo.toml b/test-utils/Cargo.toml index b566f6c373..09ff12a806 100644 --- a/test-utils/Cargo.toml +++ b/test-utils/Cargo.toml @@ -6,6 +6,7 @@ license = "MPL-2.0" [dependencies] anyhow.workspace = true +camino.workspace = true dropshot.workspace = true futures.workspace = true headers.workspace = true diff --git a/test-utils/src/dev/mod.rs b/test-utils/src/dev/mod.rs index b604a0f593..ea95a1de76 100644 --- a/test-utils/src/dev/mod.rs +++ b/test-utils/src/dev/mod.rs @@ -12,12 +12,14 @@ pub mod poll; pub mod test_cmds; use anyhow::Context; +use camino::Utf8Path; +use camino::Utf8PathBuf; pub use dropshot::test_util::LogContext; use dropshot::ConfigLogging; use dropshot::ConfigLoggingIfExists; use dropshot::ConfigLoggingLevel; use slog::Logger; -use std::path::{Path, PathBuf}; +use std::path::Path; // Helper for copying all the files in one directory to another. fn copy_dir( @@ -77,22 +79,22 @@ pub enum StorageSource { /// Do not populate anything. This is primarily used for migration testing. DoNotPopulate, /// Populate the latest version of the database. - PopulateLatest { output_dir: PathBuf }, + PopulateLatest { output_dir: Utf8PathBuf }, /// Copy the database from a seed directory, which has previously /// been created with `PopulateLatest`. - CopyFromSeed { input_dir: PathBuf }, + CopyFromSeed { input_dir: Utf8PathBuf }, } /// Creates a [`db::CockroachInstance`] with a populated storage directory. /// /// This is intended to optimize subsequent calls to [`test_setup_database`] /// by reducing the latency of populating the storage directory. -pub async fn test_setup_database_seed(log: &Logger, dir: &Path) { - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); +pub async fn test_setup_database_seed(log: &Logger, dir: &Utf8Path) { + let _ = std::fs::remove_dir_all(dir); + std::fs::create_dir_all(dir).unwrap(); let mut db = setup_database( log, - StorageSource::PopulateLatest { output_dir: dir.to_path_buf() }, + StorageSource::PopulateLatest { output_dir: dir.to_owned() }, ) .await; db.cleanup().await.unwrap(); @@ -148,7 +150,7 @@ async fn setup_database( StorageSource::CopyFromSeed { input_dir } => { info!(&log, "cockroach: copying from seed directory ({}) to storage directory ({})", - input_dir.to_string_lossy(), starter.store_dir().to_string_lossy(), + input_dir, starter.store_dir().to_string_lossy(), ); copy_dir(input_dir, starter.store_dir()) .expect("Cannot copy storage from seed directory");