From 70e98b18a891f1c615a23e84d2a98a0703b9fde2 Mon Sep 17 00:00:00 2001 From: iximeow Date: Wed, 25 Mar 2026 17:35:50 +0000 Subject: [PATCH 1/2] phd: getting guest OS kinds should not involve locks Artifact types are known ahead of time when the PHD artifact manifest is parsed. These won't change throughout a test, really only the path that those artifacts are locally cached at is the only element of an artifact that *may* change - and even then, only if the artifact is not already local in manifest or cache. So, move this mutable component of StoredArtifact behind a mutex, and let the rest of the artifact's description be accessible any time. The corresponding shuffling means that accessing a guest OS kind no longer implies downloading the referenced aritfact, an edge case which no one has noticed and no one will miss probably! --- phd-tests/framework/src/artifacts/store.rs | 76 ++++++++++++---------- phd-tests/framework/src/lib.rs | 16 ++--- phd-tests/tests/src/disk.rs | 2 +- phd-tests/tests/src/framework.rs | 2 +- phd-tests/tests/src/hyperv.rs | 2 +- 5 files changed, 51 insertions(+), 47 deletions(-) diff --git a/phd-tests/framework/src/artifacts/store.rs b/phd-tests/framework/src/artifacts/store.rs index a8c17e745..9d3537355 100644 --- a/phd-tests/framework/src/artifacts/store.rs +++ b/phd-tests/framework/src/artifacts/store.rs @@ -20,28 +20,30 @@ use std::collections::BTreeMap; use std::fs::File; use std::io::{BufReader, Cursor, Read, Seek, SeekFrom, Write}; use std::time::Duration; -use tokio::sync::Mutex; +use tokio::sync::{Mutex, MutexGuard}; use tracing::{debug, info, warn}; #[derive(Debug)] struct StoredArtifact { description: super::Artifact, - cached_path: Option, + cached_path: Mutex>, } impl StoredArtifact { fn new(description: super::Artifact) -> Self { - Self { description, cached_path: None } + Self { description, cached_path: Mutex::new(None) } } async fn ensure( - &mut self, + &self, local_dir: &Utf8Path, downloader: &DownloadConfig, ) -> anyhow::Result { + let mut path_guard = self.cached_path.lock().await; + // If the artifact already exists and has been verified, return the path // to it straightaway. - if let Some(path) = &self.cached_path { + if let Some(path) = path_guard.as_ref() { debug!(%path, "Verified artifact already exists"); return Ok(path.clone()); } @@ -67,7 +69,7 @@ impl StoredArtifact { // The file is in the right place and has the right hash (if that // was checked), so mark it as cached and return the cached path. debug!(%path, "Locally-sourced artifact is valid, caching its path"); - self.cached_path = Some(path.clone()); + *path_guard = Some(path.clone()); return Ok(path.clone()); } @@ -91,7 +93,7 @@ impl StoredArtifact { if file_hash_equals(&maybe_path, expected_digest).is_ok() { debug!(%maybe_path, "Valid artifact already exists, caching its path"); - return self.cache_path(maybe_path); + return self.cache_path(path_guard, maybe_path); } else { warn!(%maybe_path, "Existing artifact is invalid, deleting it"); std::fs::remove_file(&maybe_path)?; @@ -140,11 +142,12 @@ impl StoredArtifact { permissions.set_readonly(true); new_file.set_permissions(permissions)?; - self.cache_path(maybe_path) + self.cache_path(path_guard, maybe_path) } fn cache_path( - &mut self, + &self, + mut path_guard: MutexGuard<'_, Option>, mut path: Utf8PathBuf, ) -> anyhow::Result { if let Some(ref untar_path) = self.description.untar { @@ -166,7 +169,7 @@ impl StoredArtifact { } }; - self.cached_path = Some(path.clone()); + *path_guard = Some(path.clone()); Ok(path) } } @@ -174,7 +177,7 @@ impl StoredArtifact { #[derive(Debug)] pub struct Store { local_dir: Utf8PathBuf, - artifacts: BTreeMap>, + artifacts: BTreeMap, downloader: DownloadConfig, } @@ -199,7 +202,7 @@ impl Store { let Manifest { artifacts, remote_server_uris } = manifest; let artifacts = artifacts .into_iter() - .map(|(k, v)| (k, Mutex::new(StoredArtifact::new(v)))) + .map(|(k, v)| (k, StoredArtifact::new(v))) .collect(); let buildomat_backoff = backoff::ExponentialBackoffBuilder::new() @@ -281,7 +284,7 @@ impl Store { let _old = self.artifacts.insert( BASE_PROPOLIS_ARTIFACT.to_string(), - Mutex::new(StoredArtifact::new(artifact)), + StoredArtifact::new(artifact), ); assert!(_old.is_none()); Ok(()) @@ -335,7 +338,7 @@ impl Store { let _old = self.artifacts.insert( CRUCIBLE_DOWNSTAIRS_ARTIFACT.to_string(), - Mutex::new(StoredArtifact::new(artifact)), + StoredArtifact::new(artifact), ); assert!(_old.is_none()); Ok(()) @@ -343,16 +346,28 @@ impl Store { } } + pub fn get_guest_os_image_kind( + &self, + artifact_name: &str, + ) -> anyhow::Result { + let entry = self.get_artifact(artifact_name)?; + match entry.description.kind { + super::ArtifactKind::GuestOs(kind) => Ok(kind), + _ => Err(anyhow::anyhow!( + "artifact {artifact_name} is not a guest OS image" + )), + } + } + pub async fn get_guest_os_image( &self, artifact_name: &str, ) -> anyhow::Result<(Utf8PathBuf, GuestOsKind)> { let entry = self.get_artifact(artifact_name)?; - let mut guard = entry.lock().await; - match guard.description.kind { + match entry.description.kind { super::ArtifactKind::GuestOs(kind) => { let path = - guard.ensure(&self.local_dir, &self.downloader).await?; + entry.ensure(&self.local_dir, &self.downloader).await?; Ok((path, kind)) } _ => Err(anyhow::anyhow!( @@ -366,10 +381,9 @@ impl Store { artifact_name: &str, ) -> anyhow::Result { let entry = self.get_artifact(artifact_name)?; - let mut guard = entry.lock().await; - match guard.description.kind { + match entry.description.kind { super::ArtifactKind::Bootrom => { - guard.ensure(&self.local_dir, &self.downloader).await + entry.ensure(&self.local_dir, &self.downloader).await } _ => Err(anyhow::anyhow!( "artifact {artifact_name} is not a bootrom" @@ -382,10 +396,9 @@ impl Store { artifact_name: &str, ) -> anyhow::Result { let entry = self.get_artifact(artifact_name)?; - let mut guard = entry.lock().await; - match guard.description.kind { + match entry.description.kind { super::ArtifactKind::PropolisServer => { - guard.ensure(&self.local_dir, &self.downloader).await + entry.ensure(&self.local_dir, &self.downloader).await } _ => Err(anyhow::anyhow!( "artifact {artifact_name} is not a Propolis server" @@ -395,10 +408,9 @@ impl Store { pub async fn get_crucible_downstairs(&self) -> anyhow::Result { let entry = self.get_artifact(CRUCIBLE_DOWNSTAIRS_ARTIFACT)?; - let mut guard = entry.lock().await; - match guard.description.kind { + match entry.description.kind { super::ArtifactKind::CrucibleDownstairs => { - guard.ensure(&self.local_dir, &self.downloader).await + entry.ensure(&self.local_dir, &self.downloader).await } _ => Err(anyhow::anyhow!( "artifact {CRUCIBLE_DOWNSTAIRS_ARTIFACT} is not a Crucible downstairs binary", @@ -406,10 +418,7 @@ impl Store { } } - fn get_artifact( - &self, - name: &str, - ) -> anyhow::Result<&Mutex> { + fn get_artifact(&self, name: &str) -> anyhow::Result<&StoredArtifact> { self.artifacts.get(name).ok_or_else(|| { anyhow::anyhow!("artifact {name} not found in store") }) @@ -449,10 +458,9 @@ impl Store { untar: None, }; - let _old: Option> = self.artifacts.insert( - artifact_name.to_string(), - Mutex::new(StoredArtifact::new(artifact)), - ); + let _old: Option = self + .artifacts + .insert(artifact_name.to_string(), StoredArtifact::new(artifact)); assert!(_old.is_none()); Ok(()) diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index 96cb2177b..ac5dacad5 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -150,10 +150,9 @@ impl TestCtx { self.framework.default_guest_os_artifact() } - /// Yields the guest OS adapter corresponding to the default guest OS - /// artifact. - pub async fn default_guest_os_kind(&self) -> anyhow::Result { - self.framework.default_guest_os_kind().await + /// Returns the guest OS kind corresponding to the default guest OS artifact. + pub fn default_guest_os_kind(&self) -> anyhow::Result { + self.framework.default_guest_os_kind() } /// Indicates whether the disk factory in this framework supports the @@ -366,12 +365,9 @@ impl Framework { /// Yields the guest OS adapter corresponding to the default guest OS /// artifact. - pub async fn default_guest_os_kind(&self) -> anyhow::Result { - Ok(self - .artifact_store - .get_guest_os_image(&self.default_guest_os_artifact) - .await? - .1) + pub fn default_guest_os_kind(&self) -> anyhow::Result { + self.artifact_store + .get_guest_os_image_kind(&self.default_guest_os_artifact) } /// Indicates whether the disk factory in this framework supports the diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index 9ef94ee13..8d2490804 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -114,7 +114,7 @@ async fn mount_in_memory_disk( #[phd_testcase] async fn in_memory_backend_smoke_test(ctx: &TestCtx) { - if ctx.default_guest_os_kind().await?.is_windows() { + if ctx.default_guest_os_kind()?.is_windows() { phd_skip!( "in-memory disk tests use mount options not supported by Cygwin" ); diff --git a/phd-tests/tests/src/framework.rs b/phd-tests/tests/src/framework.rs index d162b6a53..6e54cc0de 100644 --- a/phd-tests/tests/src/framework.rs +++ b/phd-tests/tests/src/framework.rs @@ -19,7 +19,7 @@ async fn multiline_serial_test(ctx: &TestCtx) { #[phd_testcase] async fn long_line_serial_test(ctx: &TestCtx) { - let os = ctx.default_guest_os_kind().await?; + let os = ctx.default_guest_os_kind()?; if matches!( os, GuestOsKind::WindowsServer2016 | GuestOsKind::WindowsServer2019 diff --git a/phd-tests/tests/src/hyperv.rs b/phd-tests/tests/src/hyperv.rs index d51b29962..5c392d8eb 100644 --- a/phd-tests/tests/src/hyperv.rs +++ b/phd-tests/tests/src/hyperv.rs @@ -284,7 +284,7 @@ async fn hyperv_reference_tsc_elapsed_time_test(ctx: &TestCtx) { Ok(()) } - if ctx.default_guest_os_kind().await?.is_windows() { + if ctx.default_guest_os_kind()?.is_windows() { phd_skip!("test requires a guest with /proc/timer_list in procfs"); } From 6b0a6a589069f236536c79762099bfb21035b991 Mon Sep 17 00:00:00 2001 From: iximeow Date: Wed, 25 Mar 2026 17:50:42 +0000 Subject: [PATCH 2/2] phd: be pedantic about what test skips are OK This gives us some additional confidence that all tests that run either: * pass * skip, but were expected to given the test hardware/guest configuration Importantly, not all PHD tests pass on all hardware/guest OS combinations, and not all PHD tests even *run* on all hardware/guest combinations. As a bit of a fluke, *Intel* happens to run one additional test that is skipped on AMD processors, even. The strategy is to run "skipped" tests and verify they skip because if guest artifacts are changed unexpectedly we may need to better understand how the artifact differs from PHD expectations. Or if Propolis/PHD implementation changes such that the skip is no longer taken, failing the test instead of silently passing will force us to notice if that change is wanted, if the test *should* pass, etc. As an example, the test Alpine image is currently expected to be an `alpine-virt` on an ISO 9660 (read-only) filesystem. Since this means the EFI system partition is also read-only, `guest_can_adjust_boot_order` is not a meaningful test (for now). We run this test anyway and *expect* a skip, because if it passed then either: * the guest Alpine is on a writable root FS; are we missing test coverage involving read-only guest images? * we've moved EFI NvVars to out-of-guest storage, and efivarfs writes succeed. In either case, the corresponding changes should have some consideration for how they relate Propolis and guest operations. --- phd-tests/framework/src/guest_os/mod.rs | 6 +- phd-tests/framework/src/lib.rs | 7 +- phd-tests/testcase/src/lib.rs | 50 +++++++++++++- phd-tests/testcase_macro/src/lib.rs | 74 ++++++++++++++++++++- phd-tests/tests/src/boot_order.rs | 23 +++++-- phd-tests/tests/src/cpuid.rs | 13 +++- phd-tests/tests/src/crucible/migrate.rs | 4 +- phd-tests/tests/src/crucible/mod.rs | 6 ++ phd-tests/tests/src/crucible/smoke.rs | 20 ++++-- phd-tests/tests/src/disk.rs | 12 ++-- phd-tests/tests/src/migrate.rs | 10 ++- phd-tests/tests/src/server_state_machine.rs | 2 +- 12 files changed, 193 insertions(+), 34 deletions(-) diff --git a/phd-tests/framework/src/guest_os/mod.rs b/phd-tests/framework/src/guest_os/mod.rs index 7e45335de..6ae71e777 100644 --- a/phd-tests/framework/src/guest_os/mod.rs +++ b/phd-tests/framework/src/guest_os/mod.rs @@ -59,7 +59,7 @@ impl<'a> CommandSequenceEntry<'a> { } } -pub(super) struct CommandSequence<'a>(pub Vec>); +pub struct CommandSequence<'a>(pub(crate) Vec>); impl<'a> CommandSequence<'a> { fn extend(mut self, other: CommandSequence<'a>) -> CommandSequence<'a> { @@ -68,7 +68,7 @@ impl<'a> CommandSequence<'a> { } } -pub(super) trait GuestOs: Send + Sync { +pub trait GuestOs: Send + Sync { /// Retrieves the command sequence used to wait for the OS to boot and log /// into it. fn get_login_sequence(&self) -> CommandSequence<'_>; @@ -76,7 +76,7 @@ pub(super) trait GuestOs: Send + Sync { /// Retrieves the default shell prompt for this OS. fn get_shell_prompt(&self) -> &'static str; - /// Indicates whether the guest has a read-only filesystem. + /// Indicates whether the guest is expected to have a read-only filesystem. fn read_only_fs(&self) -> bool; /// Returns the sequence of serial console operations a test VM should issue diff --git a/phd-tests/framework/src/lib.rs b/phd-tests/framework/src/lib.rs index ac5dacad5..fd548ecdd 100644 --- a/phd-tests/framework/src/lib.rs +++ b/phd-tests/framework/src/lib.rs @@ -35,7 +35,7 @@ use camino::Utf8PathBuf; use disk::DiskFactory; use futures::{stream::FuturesUnordered, StreamExt}; -use guest_os::GuestOsKind; +use guest_os::{GuestOs, GuestOsKind}; use log_config::LogConfig; use port_allocator::PortAllocator; pub use test_vm::TestVm; @@ -155,6 +155,11 @@ impl TestCtx { self.framework.default_guest_os_kind() } + /// Returns the guest OS adapter corresponding to the default guest OS artifact. + pub fn default_guest_os_adapter(&self) -> anyhow::Result> { + self.default_guest_os_kind().map(guest_os::get_guest_os_adapter) + } + /// Indicates whether the disk factory in this framework supports the /// creation of Crucible disks. This can be used to skip tests that require /// Crucible support. diff --git a/phd-tests/testcase/src/lib.rs b/phd-tests/testcase/src/lib.rs index 318f5d2d3..cd81651c9 100644 --- a/phd-tests/testcase/src/lib.rs +++ b/phd-tests/testcase/src/lib.rs @@ -50,6 +50,10 @@ pub struct TestCase { /// The test function to execute to run this test. pub(crate) function: TestFunction, + + /// A function used to check if a test's skip or pass was actually expected + /// for the given `TestCtx`. + pub(crate) check_skip_fn: Option bool>, } #[allow(dead_code)] @@ -59,8 +63,9 @@ impl TestCase { module_path: &'static str, name: &'static str, function: TestFunction, + check_skip_fn: Option bool>, ) -> Self { - Self { module_path, name, function } + Self { module_path, name, function, check_skip_fn } } /// Returns the test case's fully qualified name, i.e. `module_path::name`. @@ -76,7 +81,48 @@ impl TestCase { /// Runs the test case's body with the supplied test context and returns its /// outcome. pub async fn run(&self, ctx: &TestCtx) -> TestOutcome { - (self.function.f)(ctx).await + let outcome = (self.function.f)(ctx).await; + match (outcome, self.check_skip_fn) { + (TestOutcome::Passed, check_skip_fn) => { + let wanted_skip = if let Some(check_skip_fn) = check_skip_fn { + check_skip_fn(ctx) + } else { + false + }; + + if wanted_skip { + return TestOutcome::Failed(Some( + "test passed but expected skip?".to_string(), + )); + } + + TestOutcome::Passed + } + (TestOutcome::Failed(skip_msg), _) => TestOutcome::Failed(skip_msg), + (TestOutcome::Skipped(skip_msg), None) => { + let fail_msg = match skip_msg { + Some(msg) => { + format!("skipped without check_skip attribute: {msg}") + } + None => "skipped without check_skip attribute".to_string(), + }; + TestOutcome::Failed(Some(fail_msg)) + } + (TestOutcome::Skipped(skip_msg), Some(check_skip_fn)) => { + if check_skip_fn(ctx) { + return TestOutcome::Skipped(skip_msg); + } + + let fail_msg = match skip_msg { + Some(msg) => { + format!("skipped but did not expect skip: {msg}") + } + None => "skipped but did not expect skip".to_string(), + }; + + TestOutcome::Failed(Some(fail_msg)) + } + } } } diff --git a/phd-tests/testcase_macro/src/lib.rs b/phd-tests/testcase_macro/src/lib.rs index 8f0a35764..ef9cb8a30 100644 --- a/phd-tests/testcase_macro/src/lib.rs +++ b/phd-tests/testcase_macro/src/lib.rs @@ -6,7 +6,61 @@ use heck::ToShoutySnakeCase; use proc_macro::TokenStream; use proc_macro_error::{abort, proc_macro_error}; use quote::{format_ident, quote}; -use syn::{parse_macro_input, spanned::Spanned, ItemFn}; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::spanned::Spanned; +use syn::{parse_macro_input, ItemFn, Token}; + +// More or less directly inspired by Omicron's +// `nexus/test-utils-macros/src/lib.rs` +struct PhdTestAttrs { + /// The top-level function determining if a `phd_skip!()` test conclusion is + /// acceptable or not. See the documentation of `fn phd_testcase` below for + /// more. + check_skip_fn: Option, +} + +#[derive(Debug, PartialEq, Eq, Hash)] +pub(crate) enum PhdTestArg { + CheckSkipFn(syn::Path), +} + +impl Parse for PhdTestArg { + fn parse(input: ParseStream) -> syn::Result { + let name: syn::Path = input.parse()?; + let _eq_token: syn::token::Eq = input.parse()?; + + if name.is_ident("check_skip") { + let fn_ident: syn::Path = input.parse()?; + return Ok(Self::CheckSkipFn(fn_ident)); + } + + Err(syn::Error::new(name.span(), "unrecognized argument to phd_test")) + } +} + +impl Parse for PhdTestAttrs { + fn parse(input: ParseStream) -> syn::Result { + let vars = + Punctuated::::parse_terminated(input)?; + + let mut check_skip_fn = None; + + for var in vars { + match var { + PhdTestArg::CheckSkipFn(path) => { + assert!( + check_skip_fn.is_none(), + "check_skip function can be provided at most once" + ); + check_skip_fn = Some(path); + } + } + } + + Ok(PhdTestAttrs { check_skip_fn }) + } +} /// The macro for labeling PHD testcases. /// @@ -15,23 +69,37 @@ use syn::{parse_macro_input, spanned::Spanned, ItemFn}; /// wrapper function that returns a `phd_testcase::TestOutcome` and creates an /// entry in the test case inventory that allows the PHD runner to enumerate the /// test. +/// +/// PHD checks if a test is allowed to skip for a given test configuration. A +/// test that may skip must have a `check_skip(some::check::function)`; if the +/// test skips then the provided function is called to determine if skipping is +/// allowed. If such a test passes instead of skipping, the will also be failed +/// if it was expected to skip in the test environment. #[proc_macro_error] #[proc_macro_attribute] -pub fn phd_testcase(_attrib: TokenStream, input: TokenStream) -> TokenStream { +pub fn phd_testcase(attrs: TokenStream, input: TokenStream) -> TokenStream { let item_fn = parse_macro_input!(input as ItemFn); + let test_attrs = parse_macro_input!(attrs as PhdTestAttrs); + // Build the inventory record for this test. The `module_path!()` in the // generated code allows the test case to report the fully-qualified path to // itself regardless of where it's located. let fn_ident = item_fn.sig.ident.clone(); let fn_name = fn_ident.to_string(); let static_ident = format_ident!("{}", fn_name.to_shouty_snake_case()); + let check_skip_fn = if let Some(check_skip_fn) = test_attrs.check_skip_fn { + quote! { Some(#check_skip_fn) } + } else { + quote! { None } + }; let submit: proc_macro2::TokenStream = quote! { #[linkme::distributed_slice(phd_testcase::TEST_CASES)] static #static_ident: phd_testcase::TestCase = phd_testcase::TestCase::new( module_path!(), #fn_name, - phd_testcase::TestFunction { f: |ctx| Box::pin(#fn_ident(ctx)) } + phd_testcase::TestFunction { f: |ctx| Box::pin(#fn_ident(ctx)) }, + #check_skip_fn, ); }; diff --git a/phd-tests/tests/src/boot_order.rs b/phd-tests/tests/src/boot_order.rs index 6ae79ff0a..0e2e9463c 100644 --- a/phd-tests/tests/src/boot_order.rs +++ b/phd-tests/tests/src/boot_order.rs @@ -20,6 +20,14 @@ use efi_utils::{ EDK2_FIRMWARE_VOL_GUID, EDK2_UI_APP_GUID, }; +fn no_efivarfs(ctx: &TestCtx) -> bool { + // A predicate function most boot order tests are gated on. There is some + // minimum Linux version where efivarfs first shipped, but we're assuming + // that no one is testing such an old Linux under PHD; kernel version's + // aren't directly represented in the guest OS kind anyway. + !ctx.default_guest_os_kind().expect("has default guest os kind").is_linux() +} + // This test checks that with a specified boot order, the guest boots whichever // disk we wanted to come first. This is simple enough, until you want to know // "what you booted from".. @@ -48,7 +56,7 @@ use efi_utils::{ // // Unlike later tests, this test does not manipulate boot configuration from // inside the guest OS. -#[phd_testcase] +#[phd_testcase(check_skip = no_efivarfs)] async fn configurable_boot_order(ctx: &TestCtx) { let mut cfg = ctx.vm_config_builder("configurable_boot_order"); @@ -114,7 +122,7 @@ async fn configurable_boot_order(ctx: &TestCtx) { // specifically asserts that the unbootable disk is first in the boot order; the // system booting means that boot order is respected and a non-bootable disk // does not wedge startup. -#[phd_testcase] +#[phd_testcase(check_skip = no_efivarfs)] async fn unbootable_disk_skipped(ctx: &TestCtx) { let mut cfg = ctx.vm_config_builder("unbootable_disk_skipped"); @@ -233,7 +241,12 @@ async fn unbootable_disk_skipped(ctx: &TestCtx) { // Start with the boot order being `["boot-disk", "unbootable"]`, then change it // so that next boot we'll boot from `unbootable` first. Then reboot and verify // that the boot order is still "boot-disk" first. -#[phd_testcase] +// +// TODO: this test will `skip` if the guest is a read-only Alpine, for example. +// if the guest doesn't have an efivarfs, this test warns loudly and then +// passes. The predicate function checks for a much simpler "is linux". Both of +// these are hard to discover from a predicate function (probably?) +#[phd_testcase(check_skip = no_efivarfs)] async fn guest_can_adjust_boot_order(ctx: &TestCtx) { let mut cfg = ctx.vm_config_builder("guest_can_adjust_boot_order"); @@ -400,7 +413,7 @@ async fn guest_can_adjust_boot_order(ctx: &TestCtx) { // If `bootorder` is removed for subsequent reboots, the EFI System Partition's // store of NvVar variables is the source of boot order, and guests can control // their boot fates. -#[phd_testcase] +#[phd_testcase(check_skip = no_efivarfs)] async fn boot_order_source_priority(ctx: &TestCtx) { let mut cfg = ctx.vm_config_builder("boot_order_source_priority"); @@ -507,7 +520,7 @@ async fn boot_order_source_priority(ctx: &TestCtx) { ); } -#[phd_testcase] +#[phd_testcase(check_skip = no_efivarfs)] async fn nvme_boot_option_description(ctx: &TestCtx) { let mut cfg = ctx.vm_config_builder("nvme_boot_option_description"); diff --git a/phd-tests/tests/src/cpuid.rs b/phd-tests/tests/src/cpuid.rs index cbf764efa..33b3da663 100644 --- a/phd-tests/tests/src/cpuid.rs +++ b/phd-tests/tests/src/cpuid.rs @@ -2,7 +2,7 @@ // 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 cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues}; +use cpuid_utils::{CpuidIdent, CpuidSet, CpuidValues, CpuidVendor}; use itertools::Itertools; use phd_framework::{test_vm::MigrationTimeout, TestVm}; use phd_testcase::*; @@ -276,7 +276,16 @@ impl<'a> LinuxGuestTopo<'a> { } } -#[phd_testcase] +fn host_not_intel(_ctx: &TestCtx) -> bool { + let base_leaf: CpuidIdent = CpuidIdent::leaf(0); + + let values = cpuid_utils::host::query(base_leaf); + let vendor = CpuidVendor::try_from(values).expect("recognized CPU vendor"); + + vendor != CpuidVendor::Intel +} + +#[phd_testcase(check_skip = host_not_intel)] async fn guest_cpu_topo_test(ctx: &TestCtx) { let vm = launch_cpuid_smoke_test_vm(ctx, "guest_cpu_topo_test").await?; diff --git a/phd-tests/tests/src/crucible/migrate.rs b/phd-tests/tests/src/crucible/migrate.rs index c6f64cd88..6e645b040 100644 --- a/phd-tests/tests/src/crucible/migrate.rs +++ b/phd-tests/tests/src/crucible/migrate.rs @@ -7,7 +7,7 @@ use phd_testcase::*; use tracing::info; use uuid::Uuid; -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn smoke_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_migrate_smoke_source"); super::add_default_boot_disk(ctx, &mut config)?; @@ -37,7 +37,7 @@ async fn smoke_test(ctx: &TestCtx) { assert_eq!(lsout, "foo.bar"); } -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn load_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_load_test_source"); super::add_default_boot_disk(ctx, &mut config)?; diff --git a/phd-tests/tests/src/crucible/mod.rs b/phd-tests/tests/src/crucible/mod.rs index ce6ba856e..e0db030d8 100644 --- a/phd-tests/tests/src/crucible/mod.rs +++ b/phd-tests/tests/src/crucible/mod.rs @@ -13,6 +13,12 @@ use phd_testcase::{ mod migrate; mod smoke; +// This predicate is useful for tests depending on these functions +// which need to report if they expected to be skipped or not. +pub(crate) fn crucible_disabled(ctx: &TestCtx) -> bool { + !ctx.crucible_enabled() +} + fn add_crucible_boot_disk_or_skip<'a>( ctx: &TestCtx, config: &mut VmConfig<'a>, diff --git a/phd-tests/tests/src/crucible/smoke.rs b/phd-tests/tests/src/crucible/smoke.rs index 815e2782c..531e67d40 100644 --- a/phd-tests/tests/src/crucible/smoke.rs +++ b/phd-tests/tests/src/crucible/smoke.rs @@ -11,7 +11,7 @@ use phd_framework::{ use phd_testcase::*; use propolis_client::types::InstanceState; -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn boot_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_boot_test"); super::add_default_boot_disk(ctx, &mut config)?; @@ -20,7 +20,7 @@ async fn boot_test(ctx: &TestCtx) { vm.wait_to_boot().await?; } -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn api_reboot_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_guest_reboot_test"); super::add_default_boot_disk(ctx, &mut config)?; @@ -32,7 +32,7 @@ async fn api_reboot_test(ctx: &TestCtx) { vm.wait_to_boot().await?; } -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn guest_reboot_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_guest_reboot_test"); super::add_default_boot_disk(ctx, &mut config)?; @@ -44,7 +44,17 @@ async fn guest_reboot_test(ctx: &TestCtx) { vm.graceful_reboot().await?; } -#[phd_testcase] +fn no_persistence_test(ctx: &TestCtx) -> bool { + if super::crucible_disabled(ctx) { + return true; + } + + ctx.default_guest_os_adapter() + .expect("can get default guest adapter") + .read_only_fs() +} + +#[phd_testcase(check_skip = no_persistence_test)] async fn shutdown_persistence_test(ctx: &TestCtx) { let mut config = ctx.vm_config_builder("crucible_shutdown_persistence_test"); @@ -88,7 +98,7 @@ async fn shutdown_persistence_test(ctx: &TestCtx) { assert_eq!(lsout, "foo.bar"); } -#[phd_testcase] +#[phd_testcase(check_skip = super::crucible_disabled)] async fn vcr_replace_during_start_test(ctx: &TestCtx) { if !ctx.crucible_enabled() { phd_skip!("Crucible backends not enabled (no downstairs path)"); diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index 8d2490804..abc6789fd 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -112,14 +112,12 @@ async fn mount_in_memory_disk( Ok(()) } -#[phd_testcase] -async fn in_memory_backend_smoke_test(ctx: &TestCtx) { - if ctx.default_guest_os_kind()?.is_windows() { - phd_skip!( - "in-memory disk tests use mount options not supported by Cygwin" - ); - } +fn guest_is_windows(ctx: &TestCtx) -> bool { + ctx.default_guest_os_kind().unwrap().is_windows() +} +#[phd_testcase(check_skip = guest_is_windows)] +async fn in_memory_backend_smoke_test(ctx: &TestCtx) { const HELLO_MSG: &str = "hello oxide!"; let readonly = true; diff --git a/phd-tests/tests/src/migrate.rs b/phd-tests/tests/src/migrate.rs index c55436c7f..5fcb8cd4b 100644 --- a/phd-tests/tests/src/migrate.rs +++ b/phd-tests/tests/src/migrate.rs @@ -31,13 +31,17 @@ async fn serial_history(ctx: &TestCtx) { mod from_base { use super::*; - #[phd_testcase] + fn migration_disabled(ctx: &TestCtx) -> bool { + !ctx.migration_base_enabled() + } + + #[phd_testcase(check_skip = migration_disabled)] async fn can_migrate_from_base(ctx: &TestCtx) { run_smoke_test(ctx, spawn_base_vm(ctx, "migration_from_base").await?) .await?; } - #[phd_testcase] + #[phd_testcase(check_skip = migration_disabled)] async fn serial_history(ctx: &TestCtx) { run_serial_history_test( ctx, @@ -49,7 +53,7 @@ mod from_base { // Tests migrating from the "migration base" propolis artifact to the Propolis // version under test, back to "base", and back to the version under // test. - #[phd_testcase] + #[phd_testcase(check_skip = migration_disabled)] async fn migration_from_base_and_back(ctx: &TestCtx) { let mut source = spawn_base_vm(ctx, "migration_from_base_and_back").await?; diff --git a/phd-tests/tests/src/server_state_machine.rs b/phd-tests/tests/src/server_state_machine.rs index 9b9421610..8b936410a 100644 --- a/phd-tests/tests/src/server_state_machine.rs +++ b/phd-tests/tests/src/server_state_machine.rs @@ -110,7 +110,7 @@ async fn instance_reset_requires_running_test(ctx: &TestCtx) { vm.wait_for_state(InstanceState::Running, Duration::from_secs(60)).await?; } -#[phd_testcase] +#[phd_testcase(check_skip = crate::crucible::crucible_disabled)] async fn stop_while_blocked_on_start_test(ctx: &TestCtx) { // This test uses a Crucible disk backend to cause VM startup to block. if !ctx.crucible_enabled() {