Revert "Optimize CockroachDB setup for tests (96% reduction) (#493)"

This reverts commit ab8872a.

Author: smklein

.github/buildomat/jobs/build-and-test.sh

@@ -14,18 +14,6 @@ set -o xtrace
 cargo --version
 rustc --version
 
-banner clickhouse
-ptime -m ./tools/ci_download_clickhouse
-
-banner cockroach
-ptime -m bash ./tools/ci_download_cockroachdb
-
-#
-# Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test
-# suite.
-#
-export PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse"
-
 #
 # We build with:
 #
@@ -51,6 +39,18 @@ ptime -m cargo +'nightly-2021-11-24' build --locked --all-targets --verbose
 banner deploy-check
 ptime -m cargo run --bin omicron-package -- check
 
+banner clickhouse
+ptime -m ./tools/ci_download_clickhouse
+
+banner cockroach
+ptime -m bash ./tools/ci_download_cockroachdb
+
+#
+# Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test
+# suite.
+#
+export PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse"
+
 #
 # NOTE: We're using using the same RUSTFLAGS and RUSTDOCFLAGS as above to avoid
 # having to rebuild here.

.github/workflows/rust.yml

@@ -108,12 +108,6 @@ jobs:
       with:
         key: ${{ runner.os }}-clickhouse-binary-${{ hashFiles('tools/clickhouse_checksums') }}
         path: "clickhouse"
-    - name: Download ClickHouse
-      if: steps.cache-clickhouse.outputs.cache-hit != 'true'
-      run: ./tools/ci_download_clickhouse
-    - name: Download CockroachDB binary
-      if: steps.cache-cockroachdb.outputs.cache-hit != 'true'
-      run: bash ./tools/ci_download_cockroachdb
     - name: Build
       # We build with:
       # - RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings": disallow warnings
@@ -125,7 +119,13 @@ jobs:
       #   also gives us a record of which dependencies were used for each CI
       #   run.  Building with `--locked` ensures that the checked-in Cargo.lock
       #   is up to date.
-      run: PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo build --locked --all-targets --verbose
+      run: RUSTFLAGS="-D warnings" RUSTDOCFLAGS="-D warnings" cargo build --locked --all-targets --verbose
+    - name: Download ClickHouse
+      if: steps.cache-clickhouse.outputs.cache-hit != 'true'
+      run: ./tools/ci_download_clickhouse
+    - name: Download CockroachDB binary
+      if: steps.cache-cockroachdb.outputs.cache-hit != 'true'
+      run: bash ./tools/ci_download_cockroachdb
     - name: Run tests
       # Use the same RUSTFLAGS and RUSTDOCFLAGS as above to avoid having to
       # rebuild here.

Cargo.lock

@@ -24,6 +24,7 @@ default-members = [
     "common",
     "nexus",
     "nexus/src/db/db-macros",
+    "nexus/test-utils",
     "package",
     "rpaths",
     "sled-agent",

Cargo.toml

@@ -83,7 +83,7 @@ example, on Helios, you'd want `/usr/bin` on your PATH.
 --
 . CockroachDB v21.1.10.
 +
-The build and test suite expects to be able to start a single-node CockroachDB cluster using the `cockroach` executable on your PATH.
+The test suite expects to be able to start a single-node CockroachDB cluster using the `cockroach` executable on your PATH.
 On illumos, MacOS, and Linux, you should be able to use the `tools/ci_download_cockroachdb` script to fetch the official CockroachDB binary.  It will be put into `./cockroachdb/bin/cockroach`.
 Alternatively, you can follow the https://www.cockroachlabs.com/docs/stable/install-cockroachdb.html[official CockroachDB installation instructions for your platform].
 

README.adoc

@@ -24,8 +24,3 @@ serde = { version = "1.0",  features = [ "derive" ] }
 serde_json = "1.0"
 slog = { version = "2.7",  features = [ "max_level_trace", "release_max_level_debug" ] }
 uuid = { version = "0.8", features = [ "serde", "v4" ] }
-
-[build-dependencies]
-dropshot = { git = "https://github.com/oxidecomputer/dropshot", branch = "main", features = [ "usdt-probes" ] }
-omicron-test-utils = { path = "../../test-utils" }
-tokio = { version = "1.14" }

nexus/test-utils/Cargo.toml

@@ -204,7 +204,7 @@ impl CockroachStarterBuilder {
             CockroachStarterBuilder::temp_path(&temp_dir, "listen-url");
         let listen_arg = format!("127.0.0.1:{}", self.listen_port);
         self.arg("--store")
-            .arg(&store_dir)
+            .arg(store_dir)
             .arg("--listen-addr")
             .arg(&listen_arg)
             .arg("--listening-url-file")
@@ -222,7 +222,6 @@ impl CockroachStarterBuilder {
 
         Ok(CockroachStarter {
             temp_dir,
-            store_dir: store_dir.into(),
             listen_url_file,
             args: self.args,
             cmd_builder: self.cmd_builder,
@@ -261,8 +260,6 @@ impl CockroachStarterBuilder {
 pub struct CockroachStarter {
     /// temporary directory used for URL file and potentially data storage
     temp_dir: TempDir,
-    /// path to storage directory
-    store_dir: PathBuf,
     /// path to listen URL file (inside temp_dir)
     listen_url_file: PathBuf,
     /// command-line arguments, mirrored here for reporting to the user
@@ -286,11 +283,6 @@ impl CockroachStarter {
         self.temp_dir.path()
     }
 
-    /// Returns the path to the storage directory created for this execution.
-    pub fn store_dir(&self) -> &Path {
-        self.store_dir.as_path()
-    }
-
     /**
      * Spawns a new process to run the configured command
      *

nexus/test-utils/build.rs

@@ -12,63 +12,11 @@ pub mod db;
 pub mod poll;
 pub mod test_cmds;
 
-use anyhow::Context;
 use dropshot::test_util::LogContext;
 use dropshot::ConfigLogging;
 use dropshot::ConfigLoggingIfExists;
 use dropshot::ConfigLoggingLevel;
 use slog::Logger;
-use std::path::{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 {
-    std::env::temp_dir().join("crdb-base")
-}
-
-// Helper for copying all the files in one directory to another.
-fn copy_dir(
-    src: impl AsRef<Path>,
-    dst: impl AsRef<Path>,
-) -> Result<(), anyhow::Error> {
-    let src = src.as_ref();
-    let dst = dst.as_ref();
-    std::fs::create_dir_all(&dst)
-        .with_context(|| format!("Failed to create dst {}", dst.display()))?;
-    for entry in std::fs::read_dir(src)
-        .with_context(|| format!("Failed to read_dir {}", src.display()))?
-    {
-        let entry = entry.with_context(|| {
-            format!("Failed to read entry in {}", src.display())
-        })?;
-        let ty = entry.file_type().context("Failed to access file type")?;
-        let target = dst.join(entry.file_name());
-        if ty.is_dir() {
-            copy_dir(entry.path(), &target).with_context(|| {
-                format!(
-                    "Failed to copy subdirectory {} to {}",
-                    entry.path().display(),
-                    target.display()
-                )
-            })?;
-        } else {
-            std::fs::copy(entry.path(), &target).with_context(|| {
-                format!(
-                    "Failed to copy file at {} to {}",
-                    entry.path().display(),
-                    target.display()
-                )
-            })?;
-        }
-    }
-    Ok(())
-}
 
 /**
  * Set up a [`dropshot::test_util::LogContext`] appropriate for a test named
@@ -87,71 +35,26 @@ pub fn test_setup_log(test_name: &str) -> LogContext {
     LogContext::new(test_name, &log_config)
 }
 
-enum StorageSource {
-    Populate,
-    CopyFromSeed,
-}
-
-/// 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) {
-    let dir = seed_dir();
-    let _ = std::fs::remove_dir_all(&dir);
-    std::fs::create_dir_all(&dir).unwrap();
-    let mut db = setup_database(log, Some(&dir), StorageSource::Populate).await;
-    db.cleanup().await.unwrap();
-}
-
-/// Set up a [`db::CockroachInstance`] for running tests.
+/**
+ * Set up a [`db::CockroachInstance`] for running tests against.
+ */
 pub async fn test_setup_database(log: &Logger) -> db::CockroachInstance {
-    setup_database(log, None, StorageSource::CopyFromSeed).await
-}
-
-async fn setup_database(
-    log: &Logger,
-    store_dir: Option<&Path>,
-    storage_source: StorageSource,
-) -> db::CockroachInstance {
-    let builder = db::CockroachStarterBuilder::new();
-    let mut builder = if let Some(store_dir) = store_dir {
-        builder.store_dir(store_dir)
-    } else {
-        builder
-    };
+    let mut builder = db::CockroachStarterBuilder::new();
     builder.redirect_stdio_to_files();
     let starter = builder.build().unwrap();
     info!(
         &log,
         "cockroach temporary directory: {}",
         starter.temp_dir().display()
     );
-
-    // If we're going to copy the storage directory from the seed,
-    // it is critical we do so before starting the DB.
-    if matches!(storage_source, StorageSource::CopyFromSeed) {
-        info!(&log,
-            "cockroach: copying from seed directory ({}) to storage directory ({})",
-            seed_dir().to_string_lossy(), starter.store_dir().to_string_lossy(),
-        );
-        copy_dir(seed_dir(), starter.store_dir())
-            .expect("Cannot copy storage from seed directory");
-    }
-
     info!(&log, "cockroach command line: {}", starter.cmdline());
     let database = starter.start().await.unwrap();
     info!(&log, "cockroach pid: {}", database.pid());
     let db_url = database.pg_config();
     info!(&log, "cockroach listen URL: {}", db_url);
-
-    // If we populate the storage directory by importing the '.sql'
-    // file, we must do so after the DB has started.
-    if matches!(storage_source, StorageSource::Populate) {
-        info!(&log, "cockroach: populating");
-        database.populate().await.expect("failed to populate database");
-        info!(&log, "cockroach: populated");
-    }
+    info!(&log, "cockroach: populating");
+    database.populate().await.expect("failed to populate database");
+    info!(&log, "cockroach: populated");
     database
 }