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!

Author: iximeow

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<Utf8PathBuf>,
+    cached_path: Mutex<Option<Utf8PathBuf>>,
 }
 
 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<Utf8PathBuf> {
+        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<Utf8PathBuf>>,
         mut path: Utf8PathBuf,
     ) -> anyhow::Result<Utf8PathBuf> {
         if let Some(ref untar_path) = self.description.untar {
@@ -166,15 +169,15 @@ impl StoredArtifact {
             }
         };
 
-        self.cached_path = Some(path.clone());
+        *path_guard = Some(path.clone());
         Ok(path)
     }
 }
 
 #[derive(Debug)]
 pub struct Store {
     local_dir: Utf8PathBuf,
-    artifacts: BTreeMap<String, Mutex<StoredArtifact>>,
+    artifacts: BTreeMap<String, StoredArtifact>,
     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,24 +338,36 @@ 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(())
             }
         }
     }
 
+    pub fn get_guest_os_image_kind(
+        &self,
+        artifact_name: &str,
+    ) -> anyhow::Result<GuestOsKind> {
+        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<Utf8PathBuf> {
         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<Utf8PathBuf> {
         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,21 +408,17 @@ impl Store {
 
     pub async fn get_crucible_downstairs(&self) -> anyhow::Result<Utf8PathBuf> {
         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",
             )),
         }
     }
 
-    fn get_artifact(
-        &self,
-        name: &str,
-    ) -> anyhow::Result<&Mutex<StoredArtifact>> {
+    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<Mutex<StoredArtifact>> = self.artifacts.insert(
-            artifact_name.to_string(),
-            Mutex::new(StoredArtifact::new(artifact)),
-        );
+        let _old: Option<StoredArtifact> = self
+            .artifacts
+            .insert(artifact_name.to_string(), StoredArtifact::new(artifact));
         assert!(_old.is_none());
 
         Ok(())

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<GuestOsKind> {
-        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<GuestOsKind> {
+        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<GuestOsKind> {
-        Ok(self
-            .artifact_store
-            .get_guest_os_image(&self.default_guest_os_artifact)
-            .await?
-            .1)
+    pub fn default_guest_os_kind(&self) -> anyhow::Result<GuestOsKind> {
+        self.artifact_store
+            .get_guest_os_image_kind(&self.default_guest_os_artifact)
     }
 
     /// Indicates whether the disk factory in this framework supports the

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"
         );

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

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");
     }