From ed21e45006791815920cc93285d01a276f5dc6ed Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 17 Jan 2024 14:59:28 -0800 Subject: [PATCH] phd: automatically fetch `crucible-downstairs` from Buildomat (#604) Currently, in order to run the `phd` tests that involve Crucible disks, the path to a local `crucible-downstairs` binary must be provided via a command-line argument. When no `crucible-downstairs` binary path is provided, the tests that use Crucible are skipped. This means that the tests which use Crucible disks are not currently able to run on CI, as no `crucible-downstairs` binary is present. This branch extends the `phd-framework` crate's artifact store to support automatically fetch a `crucible-downstairs` binary from Buildomat when configured via the command line. This PR adds a new `--crucible-downstairs-commit` CLI argument to `phd-runner`. If present, this argument configures PHD to fetch a `crucible-downstairs` binary from Buildomat. The `--crucible-downstairs-commit` argument can either be the string `auto`, or a 40-character Git hash. When `auto` is passed, Git revision to fetch from Buildomat is determined automatically based on the `propolis` workspace's Git dependency on the `crucible` crate, to ensure that the downstairs binary matches the version of the upstairs library that Propolis was built against. Otherwise, if a Git hash is provided, the artifact for that commit hash is fetched from Buildomat, instead. The behavior of the existing `--crucible-downstairs-cmd` is unchanged, so a local `crucible-downstairs` binary can still be used rather than downloading it from Buildomat. This way, users can still run `phd` tests against a local Crucible build, if necessary. The new `--crucible-downstairs-commit` argument conflicts with `--crucible-downstairs-cmd`, so if both are provided, `phd-runner` will print an error. If _neither_ argument is provided, tests that require Crucible are skipped, maintaining the default behavior of not downloading anything unless explicitly configured to. The Buildomat job for Crucible publishes the entire Illumos Crucible zone as a tarball, rather than separate files for each artifact. Therefore, it was necessary to extend the `phd-framework` artifact store to support a configuration for extracting an individual file from within a tarball. The SHA256 used for the artifact is the SHA of the entire tarball, since Crucible's Buildomat job publishes the tarball's SHA, but once the tarball is downloaded, the `crucible-downstairs` binary is extracted from the tarball into the object store. --- .github/buildomat/jobs/phd-run.sh | 1 + Cargo.lock | 30 +- Cargo.toml | 3 + phd-tests/framework/Cargo.toml | 2 + phd-tests/framework/src/artifacts/mod.rs | 58 +++- phd-tests/framework/src/artifacts/store.rs | 348 ++++++++++++++++----- phd-tests/framework/src/disk/mod.rs | 22 +- phd-tests/framework/src/guest_os/mod.rs | 2 +- phd-tests/framework/src/lib.rs | 33 +- phd-tests/runner/Cargo.toml | 4 + phd-tests/runner/build.rs | 65 ++++ phd-tests/runner/src/config.rs | 90 +++++- phd-tests/runner/src/main.rs | 12 +- 13 files changed, 566 insertions(+), 104 deletions(-) create mode 100644 phd-tests/runner/build.rs 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/Cargo.lock b/Cargo.lock index 4912eddf0..546f34d47 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -452,6 +452,29 @@ dependencies = [ "serde", ] +[[package]] +name = "cargo-platform" +version = "0.1.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ceed8ef69d8518a5dda55c07425450b58a4e1946f4951eab6d7191ee86c2443d" +dependencies = [ + "serde", +] + +[[package]] +name = "cargo_metadata" +version = "0.18.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2d886547e41f740c616ae73108f6eb70afe6d940c7bc697cb30f13daec073037" +dependencies = [ + "camino", + "cargo-platform", + "semver", + "serde", + "serde_json", + "thiserror", +] + [[package]] name = "cc" version = "1.0.83" @@ -1301,9 +1324,9 @@ dependencies = [ [[package]] name = "flate2" -version = "1.0.27" +version = "1.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c6c98ee8095e9d1dcbf2fcc6d95acccb90d1c81db1e44725c6a984b1dbdfb010" +checksum = "46303f565772937ffe1d394a4fac6f411c6013172fadde9dcdb1e147a086940e" dependencies = [ "crc32fast", "miniz_oxide", @@ -2957,6 +2980,7 @@ dependencies = [ "camino", "cfg-if", "errno 0.2.8", + "flate2", "futures", "hex", "libc", @@ -2971,6 +2995,7 @@ dependencies = [ "slog", "slog-async", "slog-term", + "tar", "thiserror", "tokio", "tokio-tungstenite", @@ -2987,6 +3012,7 @@ dependencies = [ "anyhow", "backtrace", "camino", + "cargo_metadata", "clap 4.4.0", "phd-framework", "phd-tests", diff --git a/Cargo.toml b/Cargo.toml index fab5ecd3c..ac1718c8b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -95,6 +95,7 @@ bitvec = "1.0" byteorder = "1" bytes = "1.1" camino = "1.1.6" +cargo_metadata = "0.18.1" cc = "1.0.73" cfg-if = "1.0.0" chrono = "0.4.19" @@ -108,6 +109,7 @@ errno = "0.2.8" expectorate = "1.0.5" fatfs = "0.3.6" futures = "0.3" +flate2 = "1.0.28" hex = "0.4.3" http = "0.2.9" hyper = "0.14" @@ -139,6 +141,7 @@ slog-dtrace = "0.2.3" slog-term = "2.8" strum = "0.25" syn = "1.0" +tar = "0.4" tempfile = "3.2" thiserror = "1.0" tokio = "1" diff --git a/phd-tests/framework/Cargo.toml b/phd-tests/framework/Cargo.toml index 64fbe1d89..24ccc06d4 100644 --- a/phd-tests/framework/Cargo.toml +++ b/phd-tests/framework/Cargo.toml @@ -15,6 +15,7 @@ camino = { workspace = true, features = ["serde1"] } cfg-if.workspace = true errno.workspace = true futures.workspace = true +flate2.workspace = true hex.workspace = true libc.workspace = true propolis-client.workspace = true @@ -27,6 +28,7 @@ serde_json.workspace = true slog.workspace = true slog-async.workspace = true slog-term.workspace = true +tar.workspace = true thiserror.workspace = true tokio = { workspace = true, features = ["full"] } tokio-tungstenite.workspace = true diff --git a/phd-tests/framework/src/artifacts/mod.rs b/phd-tests/framework/src/artifacts/mod.rs index 0659607ec..bbebfa5b1 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; @@ -13,12 +14,19 @@ pub use store::Store as ArtifactStore; pub const DEFAULT_PROPOLIS_ARTIFACT: &str = "__DEFAULT_PROPOLIS"; -#[derive(Clone, Debug, Serialize, Deserialize)] +pub const CRUCIBLE_DOWNSTAIRS_ARTIFACT: &str = "__DEFAULT_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 { GuestOs(crate::guest_os::GuestOsKind), Bootrom, PropolisServer, + CrucibleDownstairs, } #[derive(Clone, Debug, Serialize, Deserialize)] @@ -26,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 }, @@ -48,4 +56,50 @@ struct Artifact { /// The source to use to obtain this artifact if it's not present on the /// host system. source: ArtifactSource, + + /// If present, this artifact is a tarball, and the provided file should be + /// 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 f10e58223..fd80bb24b 100644 --- a/phd-tests/framework/src/artifacts/store.rs +++ b/phd-tests/framework/src/artifacts/store.rs @@ -4,12 +4,13 @@ use crate::{ artifacts::{ - manifest::Manifest, ArtifactSource, DEFAULT_PROPOLIS_ARTIFACT, + manifest::Manifest, ArtifactKind, ArtifactSource, + CRUCIBLE_DOWNSTAIRS_ARTIFACT, DEFAULT_PROPOLIS_ARTIFACT, }, guest_os::GuestOsKind, }; -use anyhow::bail; +use anyhow::{bail, Context}; use camino::{Utf8Path, Utf8PathBuf}; use ring::digest::{Digest, SHA256}; use std::collections::BTreeMap; @@ -87,9 +88,7 @@ impl StoredArtifact { if hash_equals(&maybe_path, expected_digest).is_ok() { info!(%maybe_path, "Valid artifact already exists, caching its path"); - - self.cached_path = Some(maybe_path.clone()); - return Ok(maybe_path); + return self.cache_path(maybe_path); } else { info!(%maybe_path, "Existing artifact is invalid, deleting it"); std::fs::remove_file(&maybe_path)?; @@ -106,10 +105,11 @@ impl StoredArtifact { // artifact can be reacquired. let source_uris = match &self.description.source { ArtifactSource::Buildomat { repo, series, commit, .. } => { - let buildomat_uri = format!( - "https://buildomat.eng.oxide.computer/public/file\ - /{}/{}/{}/{}", - repo, series, commit, self.description.filename + let buildomat_uri = buildomat_url( + repo, + series, + commit, + &self.description.filename, ); vec![buildomat_uri] @@ -157,21 +157,19 @@ impl StoredArtifact { let mut new_file = std::fs::File::create(&maybe_path)?; new_file.write_all(&response.bytes()?)?; - match hash_equals(&maybe_path, expected_digest) { - Ok(_) => { - // Make the newly-downloaded artifact read-only to try to - // ensure tests won't change it. Disks created from an - // artifact can be edited to be writable. - let mut permissions = new_file.metadata()?.permissions(); - permissions.set_readonly(true); - new_file.set_permissions(permissions)?; - self.cached_path = Some(maybe_path.clone()); - return Ok(maybe_path); - } - Err(e) => { - info!(?e, uri, "Failed to verify digest for artifact"); - } + if let Err(e) = hash_equals(&maybe_path, expected_digest) { + info!(?e, uri, "Failed to verify digest for artifact"); + continue; } + + // Make the newly-downloaded artifact read-only to try to + // ensure tests won't change it. Disks created from an + // artifact can be edited to be writable. + let mut permissions = new_file.metadata()?.permissions(); + permissions.set_readonly(true); + new_file.set_permissions(permissions)?; + + return self.cache_path(maybe_path); } Err(anyhow::anyhow!( @@ -179,6 +177,34 @@ impl StoredArtifact { maybe_path )) } + + fn cache_path( + &mut self, + mut path: Utf8PathBuf, + ) -> anyhow::Result { + if let Some(ref untar_path) = self.description.untar { + // This artifact is a tarball, and a file must be extracted from it. + let filename = untar_path.file_name().ok_or_else(|| { + anyhow::anyhow!( + "untar path '{}' has no file name component", + untar_path + ) + })?; + let extracted_path = path.with_file_name(filename); + + path = if !extracted_path.exists() { + info!(%extracted_path, %untar_path, "Extracting artifact from tarball"); + + extract_tar_gz(&path, untar_path)? + } else { + info!(%extracted_path, "Artifact already extracted from tarball"); + extracted_path + } + }; + + self.cached_path = Some(path.clone()); + Ok(path) + } } #[derive(Debug)] @@ -212,52 +238,94 @@ impl Store { &mut self, propolis_server_cmd: &Utf8Path, ) -> anyhow::Result<()> { - if self.artifacts.contains_key(DEFAULT_PROPOLIS_ARTIFACT) { - anyhow::bail!( - "artifact store already contains key {}", - DEFAULT_PROPOLIS_ARTIFACT - ); - } - - let full_path = propolis_server_cmd.canonicalize_utf8()?; - let filename = full_path.file_name().ok_or_else(|| { - anyhow::anyhow!( - "Propolis server command '{}' contains no file component", - propolis_server_cmd - ) - })?; - let dir = full_path.parent().ok_or_else(|| { - anyhow::anyhow!( - "canonicalized Propolis path '{}' has no directory component", - full_path - ) - })?; - - let artifact = super::Artifact { - filename: filename.to_string(), - kind: super::ArtifactKind::PropolisServer, - source: super::ArtifactSource::LocalPath { - path: dir.to_path_buf(), - sha256: None, - }, - }; + tracing::info!(%propolis_server_cmd, "Adding Propolis server from local command"); + self.add_local_artifact( + propolis_server_cmd, + DEFAULT_PROPOLIS_ARTIFACT, + ArtifactKind::PropolisServer, + ) + } - let _old = self.artifacts.insert( - DEFAULT_PROPOLIS_ARTIFACT.to_string(), - Mutex::new(StoredArtifact::new(artifact)), - ); - assert!(_old.is_none()); - Ok(()) + pub fn add_crucible_downstairs( + &mut self, + source: &crate::CrucibleDownstairsSource, + ) -> anyhow::Result<()> { + anyhow::ensure!(!self.artifacts.contains_key(CRUCIBLE_DOWNSTAIRS_ARTIFACT), "artifact store already contains key {CRUCIBLE_DOWNSTAIRS_ARTIFACT}"); + + match source { + crate::CrucibleDownstairsSource::Local( + ref crucible_downstairs_cmd, + ) => { + tracing::info!(%crucible_downstairs_cmd, "Adding crucible-downstairs from local command"); + self.add_local_artifact( + crucible_downstairs_cmd, + CRUCIBLE_DOWNSTAIRS_ARTIFACT, + ArtifactKind::CrucibleDownstairs, + ) + } + 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, + commit, + "crucible-nightly.sha256.txt", + ); + let sha256 = (|| { + let client = reqwest::blocking::ClientBuilder::new() + .timeout(Duration::from_secs(5)) + .build()?; + let req = client.get(&sha256_url).build()?; + let rsp = client.execute(req)?; + let status = rsp.status(); + anyhow::ensure!( + status == reqwest::StatusCode::OK, + "HTTP status: {status}" + ); + let sha256 = String::from_utf8(rsp.bytes()?.to_vec())? + // the text file downloaded from Buildomat has a trailing newline, + // so get rid of that... + .trim().to_string(); + Ok::<_, anyhow::Error>(sha256) + })() + .with_context(|| { + format!("Failed to get Buildomat SHA256 for {REPO}/{SERIES}/{commit}\nurl={sha256_url}") + })?; + + let artifact = super::Artifact { + filename: "crucible-nightly.tar.gz".to_string(), + kind: ArtifactKind::CrucibleDownstairs, + source: ArtifactSource::Buildomat { + repo: "oxidecomputer/crucible".to_string(), + series: "nightly-image".to_string(), + commit: commit.clone(), + sha256, + }, + untar: Some( + ["target", "release", "crucible-downstairs"] + .iter() + .collect::(), + ), + }; + + let _old = self.artifacts.insert( + CRUCIBLE_DOWNSTAIRS_ARTIFACT.to_string(), + Mutex::new(StoredArtifact::new(artifact)), + ); + assert!(_old.is_none()); + Ok(()) + } + } } pub fn get_guest_os_image( &self, artifact_name: &str, ) -> anyhow::Result<(Utf8PathBuf, GuestOsKind)> { - let entry = self.artifacts.get(artifact_name).ok_or_else(|| { - anyhow::anyhow!("artifact {} not found in store", artifact_name) - })?; - + let entry = self.get_artifact(artifact_name)?; let mut guard = entry.lock().unwrap(); match guard.description.kind { super::ArtifactKind::GuestOs(kind) => { @@ -276,10 +344,7 @@ impl Store { &self, artifact_name: &str, ) -> anyhow::Result { - let entry = self.artifacts.get(artifact_name).ok_or_else(|| { - anyhow::anyhow!("artifact {} not found in store", artifact_name) - })?; - + let entry = self.get_artifact(artifact_name)?; let mut guard = entry.lock().unwrap(); match guard.description.kind { super::ArtifactKind::Bootrom => { @@ -296,10 +361,7 @@ impl Store { &self, artifact_name: &str, ) -> anyhow::Result { - let entry = self.artifacts.get(artifact_name).ok_or_else(|| { - anyhow::anyhow!("artifact {} not found in store", artifact_name) - })?; - + let entry = self.get_artifact(artifact_name)?; let mut guard = entry.lock().unwrap(); match guard.description.kind { super::ArtifactKind::PropolisServer => { @@ -311,6 +373,87 @@ impl Store { )), } } + + pub fn get_crucible_downstairs(&self) -> anyhow::Result { + let entry = self.get_artifact(CRUCIBLE_DOWNSTAIRS_ARTIFACT)?; + let mut guard = entry.lock().unwrap(); + match guard.description.kind { + super::ArtifactKind::CrucibleDownstairs => { + guard.ensure(&self.local_dir, &self.remote_server_uris) + } + _ => Err(anyhow::anyhow!( + "artifact {CRUCIBLE_DOWNSTAIRS_ARTIFACT} is not a Crucible downstairs binary", + )), + } + } + + fn get_artifact( + &self, + name: &str, + ) -> anyhow::Result<&Mutex> { + self.artifacts.get(name).ok_or_else(|| { + anyhow::anyhow!("artifact {} not found in store", name) + }) + } + + fn add_local_artifact( + &mut self, + cmd: &Utf8Path, + artifact_name: &str, + kind: super::ArtifactKind, + ) -> anyhow::Result<()> { + if self.artifacts.contains_key(artifact_name) { + anyhow::bail!( + "artifact store already contains key {artifact_name:?}" + ); + } + + let full_path = cmd.canonicalize_utf8()?; + let filename = full_path.file_name().ok_or_else(|| { + anyhow::anyhow!( + "local artifact command '{}' contains no file component", + cmd + ) + })?; + let dir = full_path.parent().ok_or_else(|| { + anyhow::anyhow!( + "canonicalized local artifact path '{}' has no directory component", + full_path + ) + })?; + + let artifact = super::Artifact { + filename: filename.to_string(), + kind, + source: super::ArtifactSource::LocalPath { + path: dir.to_path_buf(), + sha256: None, + }, + untar: None, + }; + + let _old: Option> = self.artifacts.insert( + artifact_name.to_string(), + Mutex::new(StoredArtifact::new(artifact)), + ); + assert!(_old.is_none()); + + Ok(()) + } +} + +fn buildomat_url( + repo: impl AsRef, + series: impl AsRef, + commit: &super::Commit, + file: impl AsRef, +) -> String { + format!( + "https://buildomat.eng.oxide.computer/public/file/{}/{}/{commit}/{}", + repo.as_ref(), + series.as_ref(), + file.as_ref(), + ) } fn sha256_digest(file: &mut File) -> anyhow::Result { @@ -344,3 +487,68 @@ fn hash_equals(path: &Utf8Path, expected_digest: &str) -> anyhow::Result<()> { Ok(()) } + +fn extract_tar_gz( + tarball_path: &Utf8Path, + bin_path: &Utf8Path, +) -> anyhow::Result { + (|| { + let dir_path = + tarball_path.parent().context("Tarball path missing parent")?; + + if tarball_path.extension() == Some("gz") { + tracing::debug!("Extracting gzipped tarball..."); + let file = File::open(tarball_path)?; + let gz = flate2::read::GzDecoder::new(file); + return extract_tarball(bin_path, dir_path, gz); + } + + if tarball_path.extension() == Some("tar") { + tracing::debug!("Extracting tarball..."); + let file = File::open(tarball_path)?; + return extract_tarball(bin_path, dir_path, file); + } + + bail!("File '{tarball_path}' is (probably) not a tarball?") + })() + .with_context(|| { + format!( + "Failed to extract file '{bin_path}' from tarball '{tarball_path}'" + ) + }) +} + +fn extract_tarball( + bin_path: &Utf8Path, + dir_path: &Utf8Path, + file: impl std::io::Read, +) -> anyhow::Result { + let mut archive = tar::Archive::new(file); + + let entries = + archive.entries().context("Failed to iterate over tarball entries")?; + for entry in entries { + let mut entry = match entry { + Ok(e) => e, + Err(error) => { + tracing::warn!(%error, "skipping bad tarball entry"); + continue; + } + }; + let path = entry.path().context("Tarball entry path was not UTF-8")?; + if path == bin_path { + let filename = bin_path + .file_name() + .expect("binary path in tarball must include a filename"); + let out_path = dir_path.join(filename); + entry.unpack(&out_path).with_context(|| { + format!( + "Failed to unpack '{bin_path}' from tarball to {out_path}" + ) + })?; + return Ok(out_path); + } + } + + Err(anyhow::anyhow!("No file named '{bin_path}' found in tarball")) +} diff --git a/phd-tests/framework/src/disk/mod.rs b/phd-tests/framework/src/disk/mod.rs index 1ebaaed85..6ac961836 100644 --- a/phd-tests/framework/src/disk/mod.rs +++ b/phd-tests/framework/src/disk/mod.rs @@ -33,8 +33,8 @@ mod file; /// Errors that can arise while working with disks. #[derive(Debug, Error)] pub enum DiskError { - #[error("Disk factory has no Crucible downstairs path")] - NoCrucibleDownstairsPath, + #[error("Disk factory has no Crucible downstairs artifact")] + NoCrucibleDownstairs, #[error(transparent)] PortAllocatorError(#[from] PortAllocatorError), @@ -115,10 +115,6 @@ pub(crate) struct DiskFactory { /// when those are used as a disk source. artifact_store: Rc, - /// The path to the Crucible downstairs binary to launch to serve Crucible - /// disks. - crucible_downstairs_binary: Option, - /// The port allocator to use to allocate ports to Crucible server /// processes. port_allocator: Rc, @@ -134,23 +130,16 @@ impl DiskFactory { pub fn new( storage_dir: &impl AsRef, artifact_store: Rc, - crucible_downstairs_binary: Option<&impl AsRef>, port_allocator: Rc, log_mode: ServerLogMode, ) -> Self { Self { storage_dir: storage_dir.as_ref().to_path_buf(), artifact_store, - crucible_downstairs_binary: crucible_downstairs_binary - .map(|p| p.as_ref().to_path_buf()), port_allocator, log_mode, } } - - pub fn crucible_enabled(&self) -> bool { - self.crucible_downstairs_binary.is_some() - } } impl DiskFactory { @@ -206,10 +195,7 @@ impl DiskFactory { disk_size_gib: u64, block_size: BlockSize, ) -> Result, DiskError> { - let binary_path = self - .crucible_downstairs_binary - .as_ref() - .ok_or(DiskError::NoCrucibleDownstairsPath)?; + let binary_path = self.artifact_store.get_crucible_downstairs()?; let DiskSource::Artifact(artifact_name) = source; let (artifact_path, guest_os) = @@ -224,7 +210,7 @@ impl DiskFactory { name, disk_size_gib, block_size, - binary_path, + &binary_path.as_std_path(), &ports, &self.storage_dir, Some(&artifact_path), diff --git a/phd-tests/framework/src/guest_os/mod.rs b/phd-tests/framework/src/guest_os/mod.rs index 5cd109f7e..83f917dea 100644 --- a/phd-tests/framework/src/guest_os/mod.rs +++ b/phd-tests/framework/src/guest_os/mod.rs @@ -38,7 +38,7 @@ pub(super) trait GuestOs { } #[allow(dead_code)] -#[derive(Clone, Copy, Debug, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, Serialize, Deserialize, Eq, PartialEq)] #[serde(rename_all = "lowercase")] pub enum GuestOsKind { Alpine, diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index 017f1bbba..1203d8ed6 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -70,11 +70,13 @@ pub struct Framework { pub(crate) artifact_store: Rc, pub(crate) disk_factory: DiskFactory, pub(crate) port_allocator: Rc, + + pub(crate) crucible_enabled: bool, } pub struct FrameworkParameters { pub propolis_server_path: Utf8PathBuf, - pub crucible_downstairs_cmd: Option, + pub crucible_downstairs: Option, pub tmp_directory: Utf8PathBuf, pub artifact_toml: Utf8PathBuf, @@ -88,6 +90,12 @@ pub struct FrameworkParameters { pub port_range: Range, } +#[derive(Debug)] +pub enum CrucibleDownstairsSource { + BuildomatGitRev(artifacts::Commit), + Local(Utf8PathBuf), +} + // The framework implementation includes some "runner-only" functions // (constructing and resetting a framework) that are marked `pub`. This could be // improved by splitting the "test case" functions into a trait and giving test @@ -111,12 +119,30 @@ impl Framework { ) })?; + let crucible_enabled = match params.crucible_downstairs { + Some(crucible_downstairs) => { + artifact_store + .add_crucible_downstairs(&crucible_downstairs) + .with_context(|| { + format!( + "adding Crucible downstairs '{crucible_downstairs:?}' from options", + ) + })?; + true + } + None => { + tracing::warn!( + "Crucible disabled. Crucible tests will be skipped" + ); + false + } + }; + let artifact_store = Rc::new(artifact_store); let port_allocator = Rc::new(PortAllocator::new(params.port_range)); let disk_factory = DiskFactory::new( ¶ms.tmp_directory, artifact_store.clone(), - params.crucible_downstairs_cmd.clone().as_ref(), port_allocator.clone(), params.server_log_mode, ); @@ -131,6 +157,7 @@ impl Framework { artifact_store, disk_factory, port_allocator, + crucible_enabled, }) } @@ -219,6 +246,6 @@ impl Framework { /// creation of Crucible disks. This can be used to skip tests that require /// Crucible support. pub fn crucible_enabled(&self) -> bool { - self.disk_factory.crucible_enabled() + self.crucible_enabled } } diff --git a/phd-tests/runner/Cargo.toml b/phd-tests/runner/Cargo.toml index c54db0c55..91b6f762e 100644 --- a/phd-tests/runner/Cargo.toml +++ b/phd-tests/runner/Cargo.toml @@ -21,3 +21,7 @@ tracing-appender.workspace = true tracing-bunyan-formatter.workspace = true tracing-subscriber = { workspace = true, features = ["env-filter"] } uuid.workspace = true + +[build-dependencies] +anyhow.workspace = true +cargo_metadata.workspace = true \ No newline at end of file diff --git a/phd-tests/runner/build.rs b/phd-tests/runner/build.rs new file mode 100644 index 000000000..46b0bab40 --- /dev/null +++ b/phd-tests/runner/build.rs @@ -0,0 +1,65 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// 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; + +fn main() -> anyhow::Result<()> { + set_crucible_git_rev() + .context("Failed to determine Crucible Git revision")?; + + Ok(()) +} + +fn set_crucible_git_rev() -> anyhow::Result<()> { + fn extract_crucible_dep_sha( + src: &cargo_metadata::Source, + ) -> anyhow::Result<&str> { + const CRUCIBLE_REPO: &str = "https://github.com/oxidecomputer/crucible"; + + let src = src.repr.strip_prefix("git+").ok_or_else(|| { + anyhow::anyhow!("Crucible package's source should be from git") + })?; + + if !src.starts_with(CRUCIBLE_REPO) { + println!("cargo:warning=expected Crucible package's source to be {CRUCIBLE_REPO:?}, but is {src:?}"); + } + + let rev = src.split("?rev=").nth(1).ok_or_else(|| { + anyhow::anyhow!("Crucible package's source should have a revision") + })?; + let mut parts = rev.split('#'); + let sha = parts.next().ok_or_else(|| { + anyhow::anyhow!("Crucible package's source should have a revision") + })?; + assert_eq!(Some(sha), parts.next()); + Ok(sha) + } + + let metadata = cargo_metadata::MetadataCommand::new() + .exec() + .context("Failed to get cargo metadata")?; + + let crucible_pkg = metadata + .packages + .iter() + .find(|pkg| pkg.name == "crucible") + .ok_or_else(|| { + anyhow::anyhow!("Failed to find Crucible package in cargo metadata") + })?; + + let crucible_src = crucible_pkg.source.as_ref().ok_or_else(|| { + anyhow::anyhow!("Crucible package should not be a workspace member, and therefore should have source metadata") + })?; + + let crucible_sha = + extract_crucible_dep_sha(crucible_src).with_context(|| { + format!( + "Failed to extract Crucible source SHA from {crucible_src:?}" + ) + })?; + + println!("cargo:rustc-env=PHD_CRUCIBLE_GIT_REV={crucible_sha}"); + + Ok(()) +} diff --git a/phd-tests/runner/src/config.rs b/phd-tests/runner/src/config.rs index 6446f9000..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,9 +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. + /// + /// 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)] - pub crucible_downstairs_cmd: Option, + crucible_downstairs_cmd: Option, + + /// Git revision to use to download Crucible downstairs artifacts from + /// Buildomat. + /// + /// 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"))] + crucible_downstairs_commit: Option, /// The directory into which to write temporary files (config TOMLs, log /// files, etc.) generated during test execution. @@ -111,3 +142,56 @@ pub struct ListOptions { #[clap(long, value_parser)] 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> { + // 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(CrucibleDownstairsSource::Local(cmd))); + } + + 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), + } + } +} diff --git a/phd-tests/runner/src/main.rs b/phd-tests/runner/src/main.rs index b49ebd21d..ba5b1b49f 100644 --- a/phd-tests/runner/src/main.rs +++ b/phd-tests/runner/src/main.rs @@ -17,7 +17,7 @@ use tracing_subscriber::{EnvFilter, Registry}; use crate::execute::ExecutionStats; use crate::fixtures::TestFixtures; -fn main() { +fn main() -> anyhow::Result<()> { let runner_args = ProcessArgs::parse(); set_tracing_subscriber(&runner_args); @@ -33,18 +33,20 @@ fn main() { match &runner_args.command { config::Command::Run(opts) => { - let exit_code = run_tests(opts).tests_failed; + let exit_code = run_tests(opts)?.tests_failed; debug!(exit_code); std::process::exit(exit_code.try_into().unwrap()); } config::Command::List(opts) => list_tests(opts), } + + Ok(()) } -fn run_tests(run_opts: &RunOptions) -> ExecutionStats { +fn run_tests(run_opts: &RunOptions) -> anyhow::Result { let ctx_params = FrameworkParameters { propolis_server_path: run_opts.propolis_server_cmd.clone(), - crucible_downstairs_cmd: run_opts.crucible_downstairs_cmd.clone(), + crucible_downstairs: run_opts.crucible_downstairs()?, tmp_directory: run_opts.tmp_directory.clone(), artifact_toml: run_opts.artifact_toml_path.clone(), server_log_mode: run_opts.server_logging_mode, @@ -81,7 +83,7 @@ fn run_tests(run_opts: &RunOptions) -> ExecutionStats { execution_stats.duration.as_secs_f64() ); - execution_stats + Ok(execution_stats) } fn list_tests(list_opts: &ListOptions) {