diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index c48288fc08..1e4b655cb9 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -83,23 +83,6 @@ ptime -m cargo build -Z unstable-options --timings=json --workspace --tests --lo banner test ptime -m timeout 2h cargo nextest run --profile ci --locked --verbose -# -# If upgraded CockroachDB clusters are running on a different cluster version -# than newly-initialized ones, run database-related tests from the older cluster -# version. -# -if cmp tools/cockroachdb_version tools/prev_cockroachdb_version; then - # files are the same - true -else - # if return code is > 1, cmp failed - [[ $? -gt 1 ]] && exit 2 - # otherwise, files differ - cargo xtask download cockroach-prev - CRDB_SEED_USE_PREV=yes ptime -m timeout 1h cargo nextest run --profile ci --locked --verbose \ - --filter-expr 'rdeps(nexus-test-utils) - package(omicron-dev)' -fi - # # https://github.com/nextest-rs/nextest/issues/16 # diff --git a/dev-tools/downloader/src/lib.rs b/dev-tools/downloader/src/lib.rs index 572a732bba..c3d6e165ff 100644 --- a/dev-tools/downloader/src/lib.rs +++ b/dev-tools/downloader/src/lib.rs @@ -26,7 +26,6 @@ use strum::IntoEnumIterator; use tar::Archive; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::process::Command; -use tokio::sync::Mutex; const BUILDOMAT_URL: &'static str = "https://buildomat.eng.oxide.computer/public/file"; @@ -61,9 +60,6 @@ enum Target { /// CockroachDB binary Cockroach, - /// CockroachDB binary (previous major version) - CockroachPrev, - /// Web console assets Console, @@ -140,8 +136,7 @@ pub async fn run_cmd(args: DownloadArgs) -> Result<()> { } Target::CargoHack => downloader.download_cargo_hack().await, Target::Clickhouse => downloader.download_clickhouse().await, - Target::Cockroach => downloader.download_cockroach("").await, - Target::CockroachPrev => downloader.download_cockroach("prev_").await, + Target::Cockroach => downloader.download_cockroach().await, Target::Console => downloader.download_console().await, Target::DendriteOpenapi => { downloader.download_dendrite_openapi().await @@ -571,23 +566,20 @@ impl<'a> Downloader<'a> { Ok(()) } - async fn download_cockroach(&self, prefix: &str) -> Result<()> { + async fn download_cockroach(&self) -> Result<()> { let os = os_name()?; let download_dir = self.output_dir.join("downloads"); - let destination_dir = - self.output_dir.join(format!("{prefix}cockroachdb")); + let destination_dir = self.output_dir.join("cockroachdb"); - let checksums_path = - self.versions_dir.join(format!("{prefix}cockroachdb_checksums")); + let checksums_path = self.versions_dir.join("cockroachdb_checksums"); let [checksum] = get_values_from_file( [&format!("CIDL_SHA256_{}", os.env_name())], &checksums_path, ) .await?; - let versions_path = - self.versions_dir.join(format!("{prefix}cockroachdb_version")); + let versions_path = self.versions_dir.join("cockroachdb_version"); let version = tokio::fs::read_to_string(&versions_path) .await .context("Failed to read version from {versions_path}")?; @@ -613,11 +605,6 @@ impl<'a> Downloader<'a> { let tarball_path = download_dir.join(tarball_filename); - // Ensure that the download and unpack steps, which might write to the - // same paths, only run one at a time. - static MUTEX: Mutex<()> = Mutex::const_new(()); - let mutex_lock = MUTEX.lock().await; - tokio::fs::create_dir_all(&download_dir).await?; tokio::fs::create_dir_all(&destination_dir).await?; @@ -635,9 +622,6 @@ impl<'a> Downloader<'a> { info!(self.log, "tarball path: {tarball_path}"); unpack_tarball(&self.log, &tarball_path, &download_dir).await?; - // We are done writing to potentially shared files - drop(mutex_lock); - // This is where the binary will end up eventually let cockroach_binary = destination_dir.join("bin/cockroach"); diff --git a/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs b/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs index 2816abd62d..4c4be65e4a 100644 --- a/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs +++ b/nexus/db-queries/src/db/datastore/cockroachdb_settings.rs @@ -159,8 +159,8 @@ mod test { // This is the expected value while running tests normally. assert_eq!(version, CockroachDbClusterVersion::NEWLY_INITIALIZED); } else if settings.preserve_downgrade == version.to_string() { - // This is the expected value if we are running under - // `CRDB_SEED_USE_PREV=yes`. + // This is the expected value if the cluster was created on a + // previous version and `cluster.preserve_downgrade_option` was set. assert_eq!(version, CockroachDbClusterVersion::POLICY); } else { panic!( diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 86e5a66020..dabb47066e 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -911,15 +911,11 @@ mod tests { cockroachdb_version ); - let prev_cockroachdb_version = - include_str!("../../../../tools/prev_cockroachdb_version") - .trim_start_matches('v') - .rsplit_once('.') - .unwrap() - .0; + // In the next "tick" release, this version will be stored in a + // different file. assert_eq!( CockroachDbClusterVersion::POLICY.to_string(), - prev_cockroachdb_version + cockroachdb_version ); } } diff --git a/test-utils/src/dev/db.rs b/test-utils/src/dev/db.rs index fa5f4c647b..b6cf5f37a3 100644 --- a/test-utils/src/dev/db.rs +++ b/test-utils/src/dev/db.rs @@ -47,18 +47,10 @@ const COCKROACHDB_USER: &'static str = "root"; /// Path to the CockroachDB binary const COCKROACHDB_BIN: &str = "cockroach"; -/// Path to the CockroachDB binary for the previous major version -const PREV_COCKROACHDB_BIN: &str = concat!( - env!("CARGO_MANIFEST_DIR"), - "/../out/prev_cockroachdb/bin/cockroach" -); /// The expected CockroachDB version -pub(super) const COCKROACHDB_VERSION: &str = +const COCKROACHDB_VERSION: &str = include_str!("../../../tools/cockroachdb_version"); -/// The expected previous CockroachDB version -pub(super) const PREV_COCKROACHDB_VERSION: &str = - include_str!("../../../tools/prev_cockroachdb_version"); /// Builder for [`CockroachStarter`] that supports setting some command-line /// arguments for the `cockroach start-single-node` command @@ -84,8 +76,6 @@ pub struct CockroachStarterBuilder { args: Vec, /// describes the command line that we're going to execute cmd_builder: tokio::process::Command, - /// expected version of CockroachDB - expected_version: &'static str, /// how long to wait for CockroachDB to report itself listening start_timeout: Duration, /// redirect stdout and stderr to files @@ -93,41 +83,8 @@ pub struct CockroachStarterBuilder { } impl CockroachStarterBuilder { - /// Create a starter for `cockroach` on `$PATH`. pub fn new() -> CockroachStarterBuilder { - CockroachStarterBuilder::new_raw(COCKROACHDB_BIN) - .new_setup(COCKROACHDB_VERSION.trim()) - } - - /// Create a starter for the previous major version of `cockroach`, found in - /// `out/prev_cockroachdb/bin`. - pub fn new_prev() -> CockroachStarterBuilder { - CockroachStarterBuilder::new_raw(PREV_COCKROACHDB_BIN) - .new_setup(PREV_COCKROACHDB_VERSION.trim()) - } - - /// Helper for constructing a `CockroachStarterBuilder` that runs a specific - /// command instead of the usual `cockroach` binary - /// - /// This is used by `new()` as a starting point. It's also used by the - /// tests to trigger failure modes that would be hard to reproduce with - /// `cockroach` itself. - fn new_raw(cmd: &str) -> CockroachStarterBuilder { - CockroachStarterBuilder { - store_dir: None, - listen_port: COCKROACHDB_DEFAULT_LISTEN_PORT, - env: BTreeMap::new(), - args: vec![String::from(cmd)], - cmd_builder: tokio::process::Command::new(cmd), - expected_version: "", - start_timeout: COCKROACHDB_START_TIMEOUT_DEFAULT, - redirect_stdio: false, - } - } - - /// Common setup to run after `new_raw`. - fn new_setup(mut self, expected_version: &'static str) -> Self { - self.expected_version = expected_version; + let mut builder = CockroachStarterBuilder::new_raw(COCKROACHDB_BIN); // Copy the current set of environment variables. We could instead // allow the default behavior of inheriting the current process @@ -135,14 +92,14 @@ impl CockroachStarterBuilder { // behavior, it's possible that what we print out wouldn't match what // was used if some other thread modified the process environment in // between. - self.cmd_builder.env_clear(); + builder.cmd_builder.env_clear(); // Configure Go to generate a core file on fatal error. This behavior // may be overridden by the user if they've set this variable in their // environment. - self.env("GOTRACEBACK", "crash"); + builder.env("GOTRACEBACK", "crash"); for (key, value) in std::env::vars_os() { - self.env(key, value); + builder.env(key, value); } // We use single-node insecure mode listening only on localhost. We @@ -155,9 +112,29 @@ impl CockroachStarterBuilder { // If we decide to let callers customize various listening addresses, we // should be careful about making it too easy to generate a more // insecure configuration. - self.arg("start-single-node").arg("--insecure").arg("--http-addr=:0"); + builder + .arg("start-single-node") + .arg("--insecure") + .arg("--http-addr=:0"); + builder + } - self + /// Helper for constructing a `CockroachStarterBuilder` that runs a specific + /// command instead of the usual `cockroach` binary + /// + /// This is used by `new()` as a starting point. It's also used by the + /// tests to trigger failure modes that would be hard to reproduce with + /// `cockroach` itself. + fn new_raw(cmd: &str) -> CockroachStarterBuilder { + CockroachStarterBuilder { + store_dir: None, + listen_port: COCKROACHDB_DEFAULT_LISTEN_PORT, + env: BTreeMap::new(), + args: vec![String::from(cmd)], + cmd_builder: tokio::process::Command::new(cmd), + start_timeout: COCKROACHDB_START_TIMEOUT_DEFAULT, + redirect_stdio: false, + } } /// Redirect stdout and stderr for the "cockroach" process to files within @@ -269,7 +246,6 @@ impl CockroachStarterBuilder { args: self.args, env: self.env, cmd_builder: self.cmd_builder, - expected_version: self.expected_version, start_timeout: self.start_timeout, }) } @@ -325,8 +301,6 @@ pub struct CockroachStarter { args: Vec, /// the command line that we're going to execute cmd_builder: tokio::process::Command, - /// expected version of CockroachDB - expected_version: &'static str, /// how long to wait for the listen URL to be written start_timeout: Duration, } @@ -367,11 +341,7 @@ impl CockroachStarter { pub async fn start( mut self, ) -> Result { - check_db_version( - self.cmd_builder.as_std().get_program(), - self.expected_version, - ) - .await?; + check_db_version().await?; let mut child_process = self.cmd_builder.spawn().map_err(|source| { CockroachStartError::BadCmd { cmd: self.args[0].clone(), source } @@ -733,30 +703,18 @@ pub async fn format_sql(input: &str) -> Result { } /// Verify that CockroachDB has the correct version -async fn check_db_version( - bin: &OsStr, - expected_version: &str, -) -> Result<(), CockroachStartError> { - if expected_version.is_empty() { - // This can happen when `new_raw` is called without running `new_setup`, - // which we do in some tests. In that situation, `bin` might not be - // CockroachDB! - return Ok(()); - } - - let mut cmd = tokio::process::Command::new(bin); +pub async fn check_db_version() -> Result<(), CockroachStartError> { + let mut cmd = tokio::process::Command::new(COCKROACHDB_BIN); cmd.args(&["version", "--build-tag"]); cmd.env("GOTRACEBACK", "crash"); - let output = - cmd.output().await.map_err(|source| CockroachStartError::BadCmd { - cmd: bin.to_string_lossy().into_owned(), - source, - })?; + let output = cmd.output().await.map_err(|source| { + CockroachStartError::BadCmd { cmd: COCKROACHDB_BIN.to_string(), source } + })?; let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); if !output.status.success() { return Err(CockroachStartError::BadVersion { - expected: expected_version.to_string(), + expected: COCKROACHDB_VERSION.trim().to_string(), found: Err(anyhow!( "error {:?} when checking CockroachDB version", output.status.code() @@ -783,10 +741,10 @@ async fn check_db_version( version_str }; - if version_str != expected_version { + if version_str != COCKROACHDB_VERSION.trim() { return Err(CockroachStartError::BadVersion { found: Ok(version_str.to_string()), - expected: expected_version.to_string(), + expected: COCKROACHDB_VERSION.trim().to_string(), stdout, stderr, }); diff --git a/test-utils/src/dev/mod.rs b/test-utils/src/dev/mod.rs index eaa8f29651..31a448c988 100644 --- a/test-utils/src/dev/mod.rs +++ b/test-utils/src/dev/mod.rs @@ -60,9 +60,7 @@ pub async fn test_setup_database( source: StorageSource, ) -> db::CockroachInstance { usdt::register_probes().expect("Failed to register USDT DTrace probes"); - setup_database(log, source, db::CockroachStarterBuilder::new()) - .await - .unwrap() + setup_database(log, source).await.unwrap() } // TODO: switch to anyhow entirely -- this function is currently a mishmash of @@ -70,8 +68,8 @@ pub async fn test_setup_database( async fn setup_database( log: &Logger, storage_source: StorageSource, - builder: db::CockroachStarterBuilder, ) -> Result { + let builder = db::CockroachStarterBuilder::new(); let mut builder = match &storage_source { StorageSource::DoNotPopulate | StorageSource::CopyFromSeed { .. } => { builder diff --git a/test-utils/src/dev/seed.rs b/test-utils/src/dev/seed.rs index 28908aa753..7a75d0bbd3 100644 --- a/test-utils/src/dev/seed.rs +++ b/test-utils/src/dev/seed.rs @@ -9,31 +9,18 @@ use camino::{Utf8Path, Utf8PathBuf}; use filetime::FileTime; use slog::Logger; -use super::db::CockroachStarterBuilder; -use super::db::COCKROACHDB_VERSION; -use super::db::PREV_COCKROACHDB_VERSION; use super::CRDB_SEED_TAR_ENV; -fn use_prev() -> bool { - std::env::var("CRDB_SEED_USE_PREV").as_deref() == Ok("yes") -} - /// Creates a string identifier for the current DB schema and version. // /// The goal here is to allow to create different "seed" tarballs /// for each revision of the DB. pub 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(include_bytes!("../../../schema/crdb/dbinit.sql")); - if use_prev() { - ctx.update(PREV_COCKROACHDB_VERSION.as_bytes()); - // This seed tarball needs to be different from one generated for the - // same version without `CRDB_SEED_USE_PREV=yes`, because we set the - // preserve downgrade option during initialization. - ctx.update(b"prev"); - } else { - ctx.update(COCKROACHDB_VERSION.as_bytes()); - } + ctx.update(&schema.as_bytes()); + ctx.update(&crdb_version.as_bytes()); let digest = ctx.finish(); hex::encode(digest.as_ref()) } @@ -186,31 +173,9 @@ pub async fn test_setup_database_seed( super::StorageSource::PopulateLatest { output_dir: tmp_seed_dir.path().to_owned(), }, - if use_prev() { - CockroachStarterBuilder::new_prev() - } else { - CockroachStarterBuilder::new() - }, ) .await .context("failed to setup database")?; - if use_prev() { - // ensure the cluster doesn't get upgraded once the newer binary starts - let (major, _) = PREV_COCKROACHDB_VERSION - .trim() - .trim_start_matches('v') - .rsplit_once('.') - .context("failed to get cockroachdb major version")?; - let sql = format!( - "SET CLUSTER SETTING cluster.preserve_downgrade_option = '{major}'" - ); - db.connect() - .await - .context("failed to connect to database")? - .execute(&sql, &[]) - .await - .context("failed to set cluster.preserve_downgrade_option")?; - } db.cleanup().await.context("failed to cleanup database")?; // See https://github.com/cockroachdb/cockroach/issues/74231 for context on diff --git a/tools/prev_cockroachdb_checksums b/tools/prev_cockroachdb_checksums deleted file mode 100644 index 20b6e237f8..0000000000 --- a/tools/prev_cockroachdb_checksums +++ /dev/null @@ -1,3 +0,0 @@ -CIDL_SHA256_DARWIN="1ca69e0911af11a73305c3c6f4650b912d70754900b5bf7b80a1d361efe36561" -CIDL_SHA256_LINUX="24c321820e7ee45fa07fe91ac138befe13ad860e41c6ed595ce58823205ff4a9" -CIDL_SHA256_ILLUMOS="f151714ba3a6e02caaaa59727482c36085e60d6bd2fa963938e9a3d8c8a77088" diff --git a/tools/prev_cockroachdb_version b/tools/prev_cockroachdb_version deleted file mode 100644 index 4e11c6a7da..0000000000 --- a/tools/prev_cockroachdb_version +++ /dev/null @@ -1 +0,0 @@ -v22.1.9