Adding tools to manage ClickHouse during development (#126)

- Updates tool for downloading ClickHouse, now using prebuilt binaries
from S3.
- Adds the `omicron_common::dev::clickhouse::ClickHouseInstance` object.
This is modeled after the existing `CockroachInstance` object, and is
used to start and manage a ClickHouse server sub-process. It currently
is quite constrained, only allowing users to specify the HTTP port the
server listens on. This is for simplicity, and further options may be
needed in the future.
- Updates the Oximeter client tests. These now spawn their own
`ClickHouseInstance` in each test, so they can be run in parallel. This
also removes the feature flag used to hide the tests on non-linux
platforms.
- Updates the GitHub workflow to use the new tool to download ClickHouse
- Adds the `ch-run` subcommand to the `omicron-dev` tool, and updates
README.adoc with details about how to run

Author: bnaecker

.github/workflows/rust.yml

@@ -95,16 +95,21 @@ jobs:
       with:
         key: ${{ runner.os }}-cockroach-binary-v2
         path: "cockroachdb"
-    - name: Install ClickHouse
-      # Workflow environment variables are not propagated to the script by default
-      run: sudo CI=$CI ./tools/ci_download_clickhouse
+    - name: Configure GitHub cache for ClickHouse binaries
+      id: cache-clickhouse
+      # actions/[email protected]
+      uses: actions/cache@26968a09c0ea4f3e233fdddbafd1166051a095f6
+      with:
+        key: ${{ runner.os }}-clickhouse-binary
+        path: "clickhouse"
     - name: Build
       run: cargo +${{ matrix.toolchain }} 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
-      # Put "./cockroachdb/bin" on the PATH for the test suite.
-      run: PATH="$PATH:$PWD/cockroachdb/bin" cargo +${{ matrix.toolchain }} test --locked --verbose
-    - name: Run ClickHouse client tests
-      run: cargo test --locked --verbose --features clickhouse-tests --package oximeter -- --test-threads 1 db::client
+      # Put "./cockroachdb/bin" and "./clickhouse" on the PATH for the test suite.
+      run: PATH="$PATH:$PWD/cockroachdb/bin:$PWD/clickhouse" cargo +${{ matrix.toolchain }} test --locked --verbose

README.adoc

@@ -56,23 +56,26 @@ See TODO.adoc for more info.
 1. CockroachDB v20.2.5.  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].
 With `cockroach` on your PATH, you can **build and run the whole test suite** with `cargo test`.  The test suite runs cleanly and should remain clean.
 
-2. ClickHouse >= v21.5.5. The test suite expects a running instance of the ClickHouse database server.
+2. ClickHouse >= v21.7.1. The test suite expects a running instance of the ClickHouse database server.
 (It currently _does not_ start the server automatically.) The script `./tools/ci_download_clickhouse`
-can be used to download and install the server on Linux platforms only. It's not currently possible
-to validate the downloaded binaries automatically on other platforms, so it must be done manually.
-Instructions can be found https://clickhouse.tech/docs/en/getting-started/install/#from-binaries-non-linux[here].
+can be used to download a pre-built binary for illumos, Linux, or macOS platforms. You may also install
+ClickHouse manually; instructions can be found https://clickhouse.tech/docs/en/getting-started/install[here].
 
 You can **format the code** using `cargo fmt`.  Make sure to run this before pushing changes.  The CI checks that the code is correctly formatted.
 
 You can **run the https://github.com/rust-lang/rust-clippy[Clippy linter]** using `cargo clippy \-- -D warnings -A clippy::style`.  Make sure to run this before pushing changes.  The CI checks that the code is clippy-clean.
 
-To **run Omicron** you need to run three programs:
+To **run Omicron** you need to run four programs:
 
 * a CockroachDB cluster.  For development, you can use the `omicron-dev` tool in this repository to start a single-node CockroachDB cluster **that will delete the database when you shut it down.**
+* a ClickHouse server. You may use the `omicron-dev` tool for this as well, see below, and as with CockroachDB,
+the database files will be deleted when you stop the program.
 * `nexus`: the guts of the control plane
 * `sled-agent-sim`: a simulator for the component that manages a single sled
 
-The easiest way to start a CockroachDB cluster is to use the built in `omicron-dev` tool.  This tool assumes that the `cockroach` executable on your PATH comes from CockroachDB v20.2.5.
+The easiest way to start the required databases is to use the built in `omicron-dev` tool.
+This tool assumes that the `cockroach` and `clickhouse` executables are on your PATH,
+and match the versions above.
 
 1. Start CockroachDB using `omicron-dev db-run`:
 +
@@ -129,7 +132,18 @@ omicron-dev: populated database
 +
 Note that as the output indicates, this cluster will be available to anybody that can reach 127.0.0.1.
 
-2. `nexus` requires a configuration file to run.  You can use `omicron-nexus/examples/config.toml` to start with.  Build and run it like this:
+2. Start the ClickHouse database server:
++
+[source,text]
+----
+$ cargo run --bin omicron-dev -- ch-run
+    Finished dev [unoptimized + debuginfo] target(s) in 0.47s
+     Running `target/debug/omicron-dev ch-run`
+omicron-dev: running ClickHouse (PID: 2463), full command is "clickhouse server --log-file /var/folders/67/2tlym22x1r3d2kwbh84j298w0000gn/T/.tmpJ5nhot/clickhouse-server.log --errorlog-file /var/folders/67/2tlym22x1r3d2kwbh84j298w0000gn/T/.tmpJ5nhot/clickhouse-server.errlog -- --http_port 8123 --path /var/folders/67/2tlym22x1r3d2kwbh84j298w0000gn/T/.tmpJ5nhot"
+omicron-dev: using /var/folders/67/2tlym22x1r3d2kwbh84j298w0000gn/T/.tmpJ5nhot for ClickHouse data storage
+----
+
+3. `nexus` requires a configuration file to run.  You can use `omicron-nexus/examples/config.toml` to start with.  Build and run it like this:
 +
 [source,text]
 ----
@@ -138,11 +152,11 @@ $ cargo run --bin=nexus -- omicron-nexus/examples/config.toml
 listening: http://127.0.0.1:12220
 ----
 
-3. `sled-agent-sim` only accepts configuration on the command line.  Run it with a uuid identifying itself (this would be a uuid for the sled it's managing), an IP:port for itself, and the IP:port of `nexus`'s _internal_ interface.  Using default config, this might look like this:
+4. `sled-agent` only accepts configuration on the command line.  Run it with a uuid identifying itself (this would be a uuid for the sled it's managing), an IP:port for itself, and the IP:port of `nexus`'s _internal_ interface.  Using default config, this might look like this:
 +
 [source,text]
 ----
-$ cargo run --bin=sled-agent-sim -- $(uuidgen) 127.0.0.1:12345 127.0.0.1:12221
+$ cargo run --bin=sled-agent -- $(uuidgen) 127.0.0.1:12345 127.0.0.1:12221
 ...
 Jun 02 12:21:50.989 INFO listening, local_addr: 127.0.0.1:12345, component: dropshot
 ----

omicron-common/src/bin/omicron-dev.rs

@@ -22,6 +22,7 @@ async fn main() -> Result<(), anyhow::Error> {
         OmicronDb::DbRun { ref args } => cmd_db_run(args).await,
         OmicronDb::DbPopulate { ref args } => cmd_db_populate(args).await,
         OmicronDb::DbWipe { ref args } => cmd_db_wipe(args).await,
+        OmicronDb::ChRun { ref args } => cmd_clickhouse_run(args).await,
     };
     if let Err(error) = result {
         fatal(CmdError::Failure(format!("{:#}", error)));
@@ -50,6 +51,12 @@ enum OmicronDb {
         #[structopt(flatten)]
         args: DbWipeArgs,
     },
+
+    /// Run a ClickHouse database server for development
+    ChRun {
+        #[structopt(flatten)]
+        args: ChRunArgs,
+    },
 }
 
 #[derive(Debug, StructOpt)]
@@ -227,3 +234,54 @@ async fn cmd_db_wipe(args: &DbWipeArgs) -> Result<(), anyhow::Error> {
     client.cleanup().await.expect("connection failed");
     Ok(())
 }
+
+#[derive(Debug, StructOpt)]
+struct ChRunArgs {
+    /// The HTTP port on which the server will listen
+    #[structopt(short, long, default_value = "8123")]
+    port: u16,
+}
+
+async fn cmd_clickhouse_run(args: &ChRunArgs) -> Result<(), anyhow::Error> {
+    // Start a stream listening for SIGINT
+    let signals = Signals::new(&[SIGINT]).expect("failed to wait for SIGINT");
+    let mut signal_stream = signals.fuse();
+
+    // Start the database server process, possibly on a specific port
+    let mut db_instance =
+        dev::clickhouse::ClickHouseInstance::new(args.port).await?;
+    println!(
+        "omicron-dev: running ClickHouse (PID: {}), full command is \"clickhouse {}\"",
+        db_instance.pid().expect("Failed to get process PID, it may not have started"),
+        db_instance.cmdline().join(" ")
+    );
+    println!(
+        "omicron-dev: using {} for ClickHouse data storage",
+        db_instance.data_path().display()
+    );
+
+    // Wait for the DB to exit itself (an error), or for SIGINT
+    tokio::select! {
+        _ = db_instance.wait_for_shutdown() => {
+            db_instance.cleanup().await.context("clean up after shutdown")?;
+            bail!("omicron-dev: ClickHouse shutdown unexpectedly");
+        }
+        caught_signal = signal_stream.next() => {
+            assert_eq!(caught_signal.unwrap(), SIGINT);
+
+            // As above, we don't need to explicitly kill the DB process, since
+            // the shell will have delivered the signal to the whole process group.
+            eprintln!(
+                "omicron-dev: caught signal, shutting down and removing \
+                temporary directory"
+            );
+
+            // Remove the data directory.
+            db_instance
+                .wait_for_shutdown()
+                .await
+                .context("clean up after SIGINT shutdown")?;
+        }
+    }
+    Ok(())
+}

omicron-common/src/dev/clickhouse.rs

@@ -0,0 +1,154 @@
+//! Tools for managing ClickHouse during development
+
+use std::path::{Path, PathBuf};
+use std::process::Stdio;
+use std::time::Duration;
+
+use anyhow::Context;
+use tempfile::TempDir;
+
+use crate::dev::poll;
+
+/// A `ClickHouseInstance` is used to start and manage a ClickHouse server process.
+#[derive(Debug)]
+pub struct ClickHouseInstance {
+    // Directory in which all data, logs, etc are stored.
+    data_dir: Option<TempDir>,
+    data_path: PathBuf,
+    // The HTTP port the server is listening on
+    port: u16,
+    // Full list of command-line arguments
+    args: Vec<String>,
+    // Subprocess handle
+    child: Option<tokio::process::Child>,
+}
+
+impl ClickHouseInstance {
+    /// Start a new ClickHouse server
+    pub async fn new(port: u16) -> Result<Self, anyhow::Error> {
+        let data_dir = TempDir::new()?;
+        let log_path = data_dir.path().join("clickhouse-server.log");
+        let err_log_path = data_dir.path().join("clickhouse-server.errlog");
+        let args = vec![
+            "server".to_string(),
+            "--log-file".to_string(),
+            log_path.display().to_string(),
+            "--errorlog-file".to_string(),
+            err_log_path.display().to_string(),
+            "--".to_string(),
+            "--http_port".to_string(),
+            format!("{}", port),
+            "--path".to_string(),
+            data_dir.path().display().to_string(),
+        ];
+
+        let child = tokio::process::Command::new("clickhouse")
+            .args(&args)
+            // ClickHouse internall tees its logs to a file, so we throw away std{in,out,err}
+            .stdin(Stdio::null())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            // By default ClickHouse forks a child if it's been explicitly requested via the
+            // following environment variable, _or_ if it's not attached to a TTY. Avoid this
+            // behavior, so that we can correctly deliver SIGINT. The "watchdog" masks SIGINT,
+            // meaning we'd have to deliver that to the _child_, which is more complicated.
+            .env("CLICKHOUSE_WATCHDOG_ENABLE", "0")
+            .spawn()?;
+
+        // Wait for the "status" file to become available, at least with a newline.
+        let data_path = data_dir.path().to_path_buf();
+        let status_file = data_path.join("status");
+        poll::wait_for_condition(
+            || async {
+                let contents =
+                    match tokio::fs::read_to_string(&status_file).await {
+                        Ok(contents) => contents,
+                        Err(e) => match e.kind() {
+                            std::io::ErrorKind::NotFound => {
+                                return Err(poll::CondCheckError::NotYet)
+                            }
+                            _ => return Err(poll::CondCheckError::from(e)),
+                        },
+                    };
+                if !contents.is_empty() && contents.contains('\n') {
+                    Ok(())
+                } else {
+                    Err(poll::CondCheckError::NotYet)
+                }
+            },
+            &Duration::from_millis(500),
+            &Duration::from_secs(30),
+        )
+        .await?;
+
+        Ok(Self {
+            data_dir: Some(data_dir),
+            data_path,
+            port,
+            args,
+            child: Some(child),
+        })
+    }
+
+    /// Wait for the ClickHouse server process to shutdown, after it's been killed.
+    pub async fn wait_for_shutdown(&mut self) -> Result<(), anyhow::Error> {
+        if let Some(mut child) = self.child.take() {
+            child.wait().await?;
+        }
+        self.cleanup().await
+    }
+
+    /// Kill the ClickHouse server process and cleanup the data directory.
+    pub async fn cleanup(&mut self) -> Result<(), anyhow::Error> {
+        if let Some(mut child) = self.child.take() {
+            child.start_kill().context("Sending SIGKILL to child")?;
+            child.wait().await.context("waiting for child")?;
+        }
+        if let Some(dir) = self.data_dir.take() {
+            dir.close().context("Cleaning up temporary directory")?;
+
+            // ClickHouse doesn't fully respect the `--path` flag, and still seems
+            // to put the `preprocessed_configs` directory in $CWD.
+            let _ = std::fs::remove_dir_all("./preprocessed_configs");
+        }
+        Ok(())
+    }
+
+    /// Return the full path to the directory used for the server's data.
+    pub fn data_path(&self) -> &Path {
+        &self.data_path
+    }
+
+    /// Return the command-line used to start the ClickHouse server process
+    pub fn cmdline(&self) -> &Vec<String> {
+        &self.args
+    }
+
+    /// Return the child PID, if any
+    pub fn pid(&self) -> Option<u32> {
+        self.child.as_ref().and_then(|child| child.id())
+    }
+
+    /// Return the HTTP port the server is listening on.
+    pub fn port(&self) -> u16 {
+        self.port
+    }
+}
+
+impl Drop for ClickHouseInstance {
+    fn drop(&mut self) {
+        if self.child.is_some() || self.data_dir.is_some() {
+            eprintln!(
+                "WARN: dropped CockroachInstance without cleaning it up first \
+                (there may still be a child process running and a \
+                temporary directory leaked)"
+            );
+            if let Some(child) = self.child.as_mut() {
+                let _ = child.start_kill();
+            }
+            if let Some(dir) = self.data_dir.take() {
+                let _ = dir.close();
+            }
+        }
+    }
+}

omicron-common/src/dev/mod.rs

@@ -8,6 +8,7 @@
  */
 #![cfg_attr(not(test), allow(dead_code))]
 
+pub mod clickhouse;
 pub mod db;
 pub mod poll;
 pub mod test_cmds;

omicron-common/tests/output/cmd-omicron-dev-noargs-stderr

@@ -9,6 +9,7 @@ FLAGS:
     -V, --version    Prints version information
 
 SUBCOMMANDS:
+    ch-run         Run a ClickHouse database server for development
     db-populate    Populate an existing CockroachDB cluster with the Omicron schema
     db-run         Start a CockroachDB cluster for development
     db-wipe        Wipe the Omicron schema (and all data) from an existing CockroachDB cluster

oximeter/oximeter/Cargo.toml

@@ -25,6 +25,3 @@ uuid = { version = "0.8.2", features = [ "v4", "serde" ] }
 
 [dev-dependencies]
 trybuild = "1.0.42"
-
-[features]
-clickhouse-tests = []