From 55d6975f6af8b64f36283d8ec44c2987847aae7c Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 27 Sep 2023 12:17:16 -0700 Subject: [PATCH 1/3] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20in?= =?UTF-8?q?itial=20version?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 --- .config/nextest.toml | 11 ++++- .github/buildomat/build-and-test.sh | 2 +- Cargo.lock | 16 ++++++- Cargo.toml | 1 + crdb-seed/Cargo.toml | 15 ++++++ crdb-seed/src/main.rs | 72 +++++++++++++++++++++++++++++ nexus/test-utils/Cargo.toml | 5 -- nexus/test-utils/build.rs | 37 --------------- nexus/test-utils/src/db.rs | 11 +++-- test-utils/Cargo.toml | 1 + test-utils/src/dev/mod.rs | 18 ++++---- 11 files changed, 133 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..11037aae31 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 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..19942c2d88 --- /dev/null +++ b/crdb-seed/src/main.rs @@ -0,0 +1,72 @@ +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()) +} + +async fn ensure_seed_directory_exists(log: &Logger) -> Utf8PathBuf { + 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 the directory didn't exist when we started, try to create it. + // + // Note that this may be executed concurrently by many tests, so + // we should consider it possible for another caller to create this + // seed directory before we finish setting it up ourselves. + if !desired_seed_dir.exists() { + 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, we made the seed directory + // faster than other tests. + // + // 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 +} + +#[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 = ensure_seed_directory_exists(&logctx.log).await; + 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"); + write!(file, "CRDB_SEED_DIR={}", dir.as_str()) + .expect("failed to write to NEXTEST_ENV file"); + } +} 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..40280aedcf 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,13 @@ 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`?"); + // TODO: replace with temp dir written out by nextest + // Use "out/" for now. + 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"); From cb0a21f667493afc0b7f289fbd10a084ae4b4395 Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 27 Sep 2023 12:26:24 -0700 Subject: [PATCH 2/3] Fix rustfmt Created using spr 1.3.4 --- crdb-seed/src/main.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crdb-seed/src/main.rs b/crdb-seed/src/main.rs index 19942c2d88..8c2260f4b9 100644 --- a/crdb-seed/src/main.rs +++ b/crdb-seed/src/main.rs @@ -33,8 +33,7 @@ async fn ensure_seed_directory_exists(log: &Logger) -> Utf8PathBuf { if !desired_seed_dir.exists() { let tmp_seed_dir = camino_tempfile::Utf8TempDir::new_in(base_seed_dir).unwrap(); - dev::test_setup_database_seed(log, tmp_seed_dir.path()) - .await; + dev::test_setup_database_seed(log, tmp_seed_dir.path()).await; // If we can successfully perform the rename, we made the seed directory // faster than other tests. From 2fbd1017fc44395ca153bbb5c3f9c24fd71396ac Mon Sep 17 00:00:00 2001 From: Rain Date: Wed, 27 Sep 2023 13:40:48 -0700 Subject: [PATCH 3/3] Address review comments, fix test run Created using spr 1.3.4 --- .github/buildomat/build-and-test.sh | 5 +++ crdb-seed/src/main.rs | 69 +++++++++++++++++++---------- nexus/test-utils/src/db.rs | 2 - 3 files changed, 50 insertions(+), 26 deletions(-) diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index 11037aae31..8791ea2fa6 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -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/crdb-seed/src/main.rs b/crdb-seed/src/main.rs index 8c2260f4b9..b8572bd886 100644 --- a/crdb-seed/src/main.rs +++ b/crdb-seed/src/main.rs @@ -18,39 +18,47 @@ fn digest_unique_to_schema() -> String { hex::encode(digest.as_ref()) } -async fn ensure_seed_directory_exists(log: &Logger) -> Utf8PathBuf { +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 the directory didn't exist when we started, try to create it. + if desired_seed_dir.exists() { + return (desired_seed_dir, SeedDirectoryStatus::Existing); + } + + // The directory didn't exist when we started, so try to create it. // - // Note that this may be executed concurrently by many tests, so - // we should consider it possible for another caller to create this - // seed directory before we finish setting it up ourselves. - if !desired_seed_dir.exists() { - let tmp_seed_dir = - camino_tempfile::Utf8TempDir::new_in(base_seed_dir).unwrap(); - dev::test_setup_database_seed(log, tmp_seed_dir.path()).await; + // 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, we made the seed directory - // faster than other tests. - // - // 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}"); - } + // 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 + (desired_seed_dir, SeedDirectoryStatus::Created) } #[tokio::main] @@ -61,11 +69,24 @@ async fn main() { "crdb_seeding", &ConfigLogging::StderrTerminal { level: ConfigLoggingLevel::Info }, ); - let dir = ensure_seed_directory_exists(&logctx.log).await; + 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"); - write!(file, "CRDB_SEED_DIR={}", dir.as_str()) + 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/src/db.rs b/nexus/test-utils/src/db.rs index 40280aedcf..37d7128c49 100644 --- a/nexus/test-utils/src/db.rs +++ b/nexus/test-utils/src/db.rs @@ -20,8 +20,6 @@ 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`?"); - // TODO: replace with temp dir written out by nextest - // Use "out/" for now. seed_dir.into() }