From b821d24f19773c92af49652af3ca932bebd9794b Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 17 Jan 2024 12:17:21 -0800 Subject: [PATCH] don't download Crucible unless asked to --- .github/buildomat/jobs/phd-run.sh | 1 + phd-tests/framework/src/artifacts/mod.rs | 49 +++++++++- phd-tests/framework/src/artifacts/store.rs | 15 ++-- phd-tests/framework/src/lib.rs | 2 +- phd-tests/runner/src/config.rs | 100 +++++++++++++++------ 5 files changed, 128 insertions(+), 39 deletions(-) diff --git a/.github/buildomat/jobs/phd-run.sh b/.github/buildomat/jobs/phd-run.sh index 593a40cb0..6d25712cb 100644 --- a/.github/buildomat/jobs/phd-run.sh +++ b/.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 \ diff --git a/phd-tests/framework/src/artifacts/mod.rs b/phd-tests/framework/src/artifacts/mod.rs index b21f6bb4f..b866ce9e9 100644 --- a/phd-tests/framework/src/artifacts/mod.rs +++ b/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, } + +impl FromStr for Commit { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + 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(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + FromStr::from_str(&s).map_err(serde::de::Error::custom) + } +} diff --git a/phd-tests/framework/src/artifacts/store.rs b/phd-tests/framework/src/artifacts/store.rs index e40c759ef..3857bffc4 100644 --- a/phd-tests/framework/src/artifacts/store.rs +++ b/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, series: impl AsRef, - commit: impl AsRef, + commit: &super::Commit, file: impl AsRef, ) -> 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(), ) } diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index abe130ba2..1203d8ed6 100644 --- a/phd-tests/framework/src/lib.rs +++ b/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), } diff --git a/phd-tests/runner/src/config.rs b/phd-tests/runner/src/config.rs index d79d68c2b..5ce1c29d1 100644 --- a/phd-tests/runner/src/config.rs +++ b/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, - /// 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, /// 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, } +#[derive(Debug, Clone)] +enum CrucibleCommit { + Auto, + Explicit(artifacts::Commit), +} + +impl FromStr for CrucibleCommit { + type Err = anyhow::Error; + fn from_str(s: &str) -> Result { + 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> { - // Crucible tests are disabled. - if self.no_crucible { - return Ok(None); - } - + ) -> anyhow::Result> { // 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), + } } }