From 454bcab434463b05d382cbb3538d250d7370a509 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 7 Sep 2023 18:49:23 -0700 Subject: [PATCH 1/2] Remove nexus-test-utils build.rs --- Cargo.lock | 3 +- nexus/test-utils/Cargo.toml | 7 +--- nexus/test-utils/build.rs | 37 ------------------ nexus/test-utils/src/db.rs | 75 ++++++++++++++++++++++++++++++------- test-utils/src/dev/mod.rs | 3 +- 5 files changed, 67 insertions(+), 58 deletions(-) delete mode 100644 nexus/test-utils/build.rs diff --git a/Cargo.lock b/Cargo.lock index c43338de166..5850c68040a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4371,6 +4371,7 @@ dependencies = [ "dns-service-client 0.1.0", "dropshot", "headers", + "hex", "http", "hyper", "internal-dns 0.1.0", @@ -4386,12 +4387,12 @@ dependencies = [ "oximeter-collector", "oximeter-producer 0.1.0", "parse-display", + "ring", "serde", "serde_json", "serde_urlencoded", "slog", "tempfile", - "tokio", "trust-dns-proto", "trust-dns-resolver", "uuid", diff --git a/nexus/test-utils/Cargo.toml b/nexus/test-utils/Cargo.toml index db98a979c17..9936e7c2d7c 100644 --- a/nexus/test-utils/Cargo.toml +++ b/nexus/test-utils/Cargo.toml @@ -15,6 +15,7 @@ dns-server.workspace = true dns-service-client.workspace = true dropshot.workspace = true headers.workspace = true +hex.workspace = true http.workspace = true hyper.workspace = true internal-dns.workspace = true @@ -30,6 +31,7 @@ oximeter-client.workspace = true oximeter-collector.workspace = true oximeter-producer.workspace = true parse-display.workspace = true +ring.workspace = true serde.workspace = true serde_json.workspace = true serde_urlencoded.workspace = true @@ -38,8 +40,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 aada3fe0391..00000000000 --- 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 bf0b7ed52d2..2248f7195ec 100644 --- a/nexus/test-utils/src/db.rs +++ b/nexus/test-utils/src/db.rs @@ -4,29 +4,76 @@ //! Database testing facilities. +use camino::Utf8PathBuf; use omicron_test_utils::dev; use slog::Logger; -use std::path::PathBuf; -/// Path to the "seed" CockroachDB directory. -/// -/// Populating CockroachDB unfortunately isn't free - creation of -/// tables, indices, and users takes several seconds to complete. -/// -/// 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")) +// 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()) +} + +// Seed directories will be created within: +// +// - /tmp/crdb-base/ +// +// However, the process for creating these seed directories is not atomic. +// We create a temporary directory within: +// +// - /tmp/crdb-base/... +// +// And rename it to the final "digest" location once it has been fully created. +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 } /// Wrapper around [`dev::test_setup_database`] which uses a a -/// seed directory provided at build-time. +/// seed directory that we construct if it does not already exist. pub async fn test_setup_database(log: &Logger) -> dev::db::CockroachInstance { - let dir = seed_dir(); + let dir = ensure_seed_directory_exists(log).await; dev::test_setup_database( log, - dev::StorageSource::CopyFromSeed { input_dir: dir }, + dev::StorageSource::CopyFromSeed { input_dir: dir.into() }, ) .await } diff --git a/test-utils/src/dev/mod.rs b/test-utils/src/dev/mod.rs index b604a0f5935..00e5d32ad73 100644 --- a/test-utils/src/dev/mod.rs +++ b/test-utils/src/dev/mod.rs @@ -87,7 +87,8 @@ pub enum StorageSource { /// /// 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) { +pub async fn test_setup_database_seed>(log: &Logger, dir: P) { + let dir = dir.as_ref(); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); let mut db = setup_database( From 45a0be4c6fbd8454b4aca09e8384829e45591a54 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 7 Sep 2023 19:40:50 -0700 Subject: [PATCH 2/2] Nudge buildomat to ignore crdb-base --- .github/buildomat/build-and-test.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index 4245c7f4586..ac52439344e 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -71,6 +71,11 @@ ptime -m timeout 1h cargo test --doc --locked --verbose --no-fail-fast # to check is to try to remove it with `rmdir`. # unset TMPDIR + +# We expect the seed CRDB to be placed here, so we explicitly remove it. There +# is no "final test", so cargo test wouldn't really know when to remove it. +rm -rf "$TEST_TMPDIR/crdb-base" + echo "files in $TEST_TMPDIR (none expected on success):" >&2 find "$TEST_TMPDIR" -ls rmdir "$TEST_TMPDIR"