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) {