don't download Crucible unless asked to

.github/buildomat/jobs/phd-run.sh

@@ -58,6 +58,7 @@ set +e
 (RUST_BACKTRACE=1 ptime -m pfexec $runner \
 	--emit-bunyan \
 	run \
+	--crucible-downstairs-commit auto \
 	--propolis-server-cmd $propolis \
 	--artifact-toml-path $artifacts \
 	--tmp-directory $tmpdir \

phd-tests/framework/src/artifacts/mod.rs

@@ -5,6 +5,7 @@
 //! Support for working with files consumed by PHD test runs.
 
 use serde::{Deserialize, Serialize};
+use std::str::FromStr;
 
 mod manifest;
 mod store;
@@ -15,6 +16,10 @@ pub const DEFAULT_PROPOLIS_ARTIFACT: &str = "__DEFAULT_PROPOLIS";
 
 pub const CRUCIBLE_DOWNSTAIRS_ARTIFACT: &str = "crucible-downstairs";
 
+#[derive(Clone, Debug, Serialize, Eq, PartialEq)]
+#[serde(transparent)]
+pub struct Commit(String);
+
 #[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq)]
 #[serde(rename_all = "snake_case")]
 enum ArtifactKind {
@@ -29,7 +34,7 @@ enum ArtifactKind {
 enum ArtifactSource {
     /// Get the artifact from Buildomat. This downloads from
     /// https://buildomat.eng.oxide.computer/public/file/REPO/SERIES/COMMIT.
-    Buildomat { repo: String, series: String, commit: String, sha256: String },
+    Buildomat { repo: String, series: String, commit: Commit, sha256: String },
 
     /// Get the artifact from the manifest's list of remote artifact servers.
     RemoteServer { sha256: String },
@@ -56,3 +61,45 @@ struct Artifact {
     /// extracted.
     untar: Option<camino::Utf8PathBuf>,
 }
+
+impl FromStr for Commit {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let s = s.trim();
+
+        // Ensure this looks like a valid Git commit.
+        anyhow::ensure!(
+            s.len() == 40,
+            "Buildomat requires full (40-character) Git commit hashes"
+        );
+
+        for c in s.chars() {
+            if !c.is_ascii_hexdigit() {
+                anyhow::bail!(
+                    "'{c}' is not a valid hexadecimal digit; Git \
+                    commit hashes should consist of the characters \
+                    [0-9, a-f, A-F]"
+                );
+            }
+        }
+
+        Ok(Self(s.to_string()))
+    }
+}
+
+impl std::fmt::Display for Commit {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(&self.0)
+    }
+}
+
+impl<'de> Deserialize<'de> for Commit {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let s = String::deserialize(deserializer)?;
+        FromStr::from_str(&s).map_err(serde::de::Error::custom)
+    }
+}

phd-tests/framework/src/artifacts/store.rs

@@ -263,15 +263,15 @@ impl Store {
                     ArtifactKind::CrucibleDownstairs,
                 )
             }
-            crate::CrucibleDownstairsSource::BuildomatGitRev(ref rev) => {
-                tracing::info!(%rev, "Adding crucible-downstairs from Buildomat Git revision");
+            crate::CrucibleDownstairsSource::BuildomatGitRev(ref commit) => {
+                tracing::info!(%commit, "Adding crucible-downstairs from Buildomat Git revision");
 
                 const REPO: &str = "oxidecomputer/crucible";
                 const SERIES: &str = "nightly-image";
                 let sha256_url = buildomat_url(
                     REPO,
                     SERIES,
-                    rev,
+                    commit,
                     "crucible-nightly.sha256.txt",
                 );
                 let sha256 = (|| {
@@ -292,7 +292,7 @@ impl Store {
                     Ok::<_, anyhow::Error>(sha256)
                 })()
                 .with_context(|| {
-                    format!("Failed to get Buildomat SHA256 for {REPO}/{SERIES}/{rev}\nurl={sha256_url}")
+                    format!("Failed to get Buildomat SHA256 for {REPO}/{SERIES}/{commit}\nurl={sha256_url}")
                 })?;
 
                 let artifact = super::Artifact {
@@ -301,7 +301,7 @@ impl Store {
                     source: ArtifactSource::Buildomat {
                         repo: "oxidecomputer/crucible".to_string(),
                         series: "nightly-image".to_string(),
-                        commit: rev.to_string(),
+                        commit: commit.clone(),
                         sha256,
                     },
                     untar: Some(
@@ -445,14 +445,13 @@ impl Store {
 fn buildomat_url(
     repo: impl AsRef<str>,
     series: impl AsRef<str>,
-    commit: impl AsRef<str>,
+    commit: &super::Commit,
     file: impl AsRef<str>,
 ) -> String {
     format!(
-        "https://buildomat.eng.oxide.computer/public/file/{}/{}/{}/{}",
+        "https://buildomat.eng.oxide.computer/public/file/{}/{}/{commit}/{}",
         repo.as_ref(),
         series.as_ref(),
-        commit.as_ref(),
         file.as_ref(),
     )
 }

phd-tests/framework/src/lib.rs

@@ -92,7 +92,7 @@ pub struct FrameworkParameters {
 
 #[derive(Debug)]
 pub enum CrucibleDownstairsSource {
-    BuildomatGitRev(String),
+    BuildomatGitRev(artifacts::Commit),
     Local(Utf8PathBuf),
 }
 

phd-tests/runner/src/config.rs

@@ -2,11 +2,16 @@
 // 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 anyhow::Context;
 use camino::Utf8PathBuf;
 use clap::{Args, Parser, Subcommand};
-use phd_framework::server_log_mode::ServerLogMode;
+use phd_framework::{
+    artifacts, server_log_mode::ServerLogMode, CrucibleDownstairsSource,
+};
+use std::str::FromStr;
 
 #[derive(Debug, Subcommand)]
+#[allow(clippy::large_enum_variant)]
 pub enum Command {
     Run(RunOptions),
     List(ListOptions),
@@ -35,20 +40,35 @@ pub struct RunOptions {
     #[clap(long, value_parser)]
     pub propolis_server_cmd: Utf8PathBuf,
 
-    /// The command to use to launch Crucible downstairs servers.
+    /// The path of a local command to use to launch Crucible downstairs
+    /// servers.
     ///
-    /// If this is not present, then a Crucible revision will be determined
-    /// based on the current Git revision of `propolis`' dependency on the
-    /// `crucible` crate.
+    /// This argument conflicts with the `--crucible-downstairs-commit`
+    /// argument, which configures PHD to download a Crucible downstairs
+    /// artifact from Buildomat. If neither the `--crucible-downstairs-cmd` OR
+    /// `--crucible-downstairs-commit` arguments are provided, then PHD will not
+    /// run tests that require Crucible.
     #[clap(long, value_parser)]
     crucible_downstairs_cmd: Option<Utf8PathBuf>,
 
-    /// Disable Crucible.
+    /// Git revision to use to download Crucible downstairs artifacts from
+    /// Buildomat.
     ///
-    /// If this is set, no crucible-downstairs binary will be downloaded, and
-    /// tests which use Crucible will be skipped.
+    /// This may either be the string 'auto' or a 40-character Git commit
+    /// hash. If this is 'auto', then the Git revision of Crucible is determined
+    /// automatically based on the Propolis workspace's Cargo git dependency on
+    /// the `crucible` crate (determined when `phd-runner` is built). If an
+    /// explicit commit hash is provided, that commit is downloaded from
+    /// Buildomat, regardless of which version of the `crucible` crate Propolis
+    /// depends on.
+    ///
+    /// This argument conflicts with the `--crucible-downstairs-cmd`
+    /// argument, which configures PHD to use a local command for running
+    /// Crucible downstairs servers. If neither the `--crucible-downstairs-cmd`
+    /// OR `--crucible-downstairs-commit` arguments are provided, then PHD will
+    /// not run tests that require Crucible.
     #[clap(long, conflicts_with("crucible_downstairs_cmd"))]
-    no_crucible: bool,
+    crucible_downstairs_commit: Option<CrucibleCommit>,
 
     /// The directory into which to write temporary files (config TOMLs, log
     /// files, etc.) generated during test execution.
@@ -123,33 +143,55 @@ pub struct ListOptions {
     pub exclude_filter: Vec<String>,
 }
 
+#[derive(Debug, Clone)]
+enum CrucibleCommit {
+    Auto,
+    Explicit(artifacts::Commit),
+}
+
+impl FromStr for CrucibleCommit {
+    type Err = anyhow::Error;
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let s = s.trim();
+
+        if s.eq_ignore_ascii_case("auto") {
+            return Ok(CrucibleCommit::Auto);
+        }
+
+        s.parse().context(
+            "Crucible commit must be either 'auto' or a valid Git commit hash",
+        ).map(CrucibleCommit::Explicit)
+    }
+}
+
 impl RunOptions {
     pub fn crucible_downstairs(
         &self,
-    ) -> anyhow::Result<Option<phd_framework::CrucibleDownstairsSource>> {
-        // Crucible tests are disabled.
-        if self.no_crucible {
-            return Ok(None);
-        }
-
+    ) -> anyhow::Result<Option<CrucibleDownstairsSource>> {
         // If a local crucible-downstairs command was provided on the command
         // line, use that.
         if let Some(cmd) = self.crucible_downstairs_cmd.clone() {
-            return Ok(Some(phd_framework::CrucibleDownstairsSource::Local(
-                cmd,
-            )));
+            return Ok(Some(CrucibleDownstairsSource::Local(cmd)));
         }
 
-        // Otherwise, use the Git revision of the workspace's Cargo git dep on
-        // crucible-upstairs, and use the same revision for the downstairs
-        // binary artifact.
-        //
-        // The Git revision of Crucible we depend on is determined when building
-        // `phd-runner` by the build script, so that the `phd-runner` binary can
-        // be run even after moving it out of the Propolis cargo workspace.
-        let crucible_git_rev = env!("PHD_CRUCIBLE_GIT_REV");
-        Ok(Some(phd_framework::CrucibleDownstairsSource::BuildomatGitRev(
-            crucible_git_rev.to_string(),
-        )))
+        match self.crucible_downstairs_commit {
+            Some(CrucibleCommit::Explicit(ref commit)) => Ok(Some(
+                CrucibleDownstairsSource::BuildomatGitRev(commit.clone()),
+            )),
+            Some(CrucibleCommit::Auto) => {
+                // Otherwise, use the Git revision of the workspace's Cargo git dep on
+                // crucible-upstairs, and use the same revision for the downstairs
+                // binary artifact.
+                //
+                // The Git revision of Crucible we depend on is determined when building
+                // `phd-runner` by the build script, so that the `phd-runner` binary can
+                // be run even after moving it out of the Propolis cargo workspace.
+                let commit = env!("PHD_CRUCIBLE_GIT_REV").parse().context(
+                "PHD_CRUCIBLE_GIT_REV must be set to a valid Git revision by the build script",
+            )?;
+                Ok(Some(CrucibleDownstairsSource::BuildomatGitRev(commit)))
+            }
+            None => Ok(None),
+        }
     }
 }