Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nexus-test-utils] replace build.rs with a new setup script #4150

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .config/nextest.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
7 changes: 6 additions & 1 deletion .github/buildomat/build-and-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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`.
Expand Down
16 changes: 15 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ members = [
"caboose-util",
"certificates",
"common",
"crdb-seed",
"ddm-admin-client",
"deploy",
"dev-tools/omdb",
Expand Down
15 changes: 15 additions & 0 deletions crdb-seed/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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
92 changes: 92 additions & 0 deletions crdb-seed/src/main.rs
Original file line number Diff line number Diff line change
@@ -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") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what sets this environment variable?

Also, if this environment variable cannot be found, should we throw an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what sets this environment variable?

Nextest does -- it's the way setup scripts can communicate with nextest.

Also, if this environment variable cannot be found, should we throw an error?

I figured folks might want to run the script by hand. Maybe we should warn in that case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. So someone could run this binary manually, and it would create the seed dir, but not pass the necessary info to nextest.

That makes sense -- I'm okay with this structure as-is, I just wanted to make sure it would be clear "where things went wrong" if the environment variable from nextest was not provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some more logging and a warning message to make this clearer.

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?)"
);
}
}
5 changes: 0 additions & 5 deletions nexus/test-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
37 changes: 0 additions & 37 deletions nexus/test-utils/build.rs

This file was deleted.

9 changes: 6 additions & 3 deletions nexus/test-utils/src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions test-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ license = "MPL-2.0"

[dependencies]
anyhow.workspace = true
camino.workspace = true
dropshot.workspace = true
futures.workspace = true
headers.workspace = true
Expand Down
18 changes: 10 additions & 8 deletions test-utils/src/dev/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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");
Expand Down