PHD: add test coverage for instance_ensure API endpoint

phd-tests/framework/src/test_vm/mod.rs

@@ -22,11 +22,11 @@ use core::result::Result as StdResult;
 use propolis_client::{
     support::{InstanceSerialConsoleHelper, WSClientOffset},
     types::{
-        InstanceGetResponse, InstanceMigrateInitiateRequest,
-        InstanceMigrateStatusResponse, InstanceProperties,
-        InstanceSerialConsoleHistoryResponse, InstanceSpecEnsureRequest,
-        InstanceSpecGetResponse, InstanceState, InstanceStateRequested,
-        MigrationState, VersionedInstanceSpec,
+        InstanceEnsureRequest, InstanceGetResponse,
+        InstanceMigrateInitiateRequest, InstanceMigrateStatusResponse,
+        InstanceProperties, InstanceSerialConsoleHistoryResponse,
+        InstanceSpecEnsureRequest, InstanceSpecGetResponse, InstanceState,
+        InstanceStateRequested, MigrationState, VersionedInstanceSpec,
     },
 };
 use propolis_client::{Client, ResponseValue};
@@ -126,6 +126,15 @@ enum InstanceConsoleSource<'a> {
     InheritFrom(&'a TestVm),
 }
 
+/// Specifies the propolis-server interface to use when starting a VM.
+enum InstanceEnsureApi {
+    /// Use the `instance_spec_ensure` interface.
+    SpecEnsure,
+
+    /// Use the `instance_ensure` interface.
+    Ensure,
+}
+
 enum VmState {
     New,
     Ensured { serial: SerialConsole },
@@ -281,6 +290,7 @@ impl TestVm {
     #[instrument(skip_all, fields(vm = self.spec.vm_name, vm_id = %self.id))]
     async fn instance_ensure_internal<'a>(
         &self,
+        api: InstanceEnsureApi,
         migrate: Option<InstanceMigrateInitiateRequest>,
         console_source: InstanceConsoleSource<'a>,
     ) -> Result<SerialConsole> {
@@ -305,27 +315,55 @@ impl TestVm {
             vcpus,
         };
 
-        let versioned_spec =
-            VersionedInstanceSpec::V0(self.spec.instance_spec.clone());
-        let ensure_req = InstanceSpecEnsureRequest {
-            properties,
-            instance_spec: versioned_spec,
-            migrate,
+        // The non-spec ensure interface requires a set of `DiskRequest`
+        // structures to specify disks. Create those once and clone them if the
+        // ensure call needs to be retried.
+        let disk_reqs = if let InstanceEnsureApi::Ensure = api {
+            Some(self.spec.make_disk_requests()?)
+        } else {
+            None
         };
 
         // There is a brief period where the Propolis server process has begun
         // to run but hasn't started its Dropshot server yet. Ensure requests
-        // that land in that window will fail, so retry them. This shouldn't
-        // ever take more than a couple of seconds (if it does, that should be
-        // considered a bug impacting VM startup times).
+        // that land in that window will fail, so retry them.
+        //
+        // The `instance_ensure` and `instance_spec_ensure` endpoints return the
+        // same response type, so (with some gnarly writing out of the types)
+        // it's possible to create a boxed future that abstracts over the
+        // caller's chosen endpoint.
         let ensure_fn = || async {
-            if let Err(e) = self
-                .client
-                .instance_spec_ensure()
-                .body(&ensure_req)
-                .send()
-                .await
-            {
+            let result = match api {
+                InstanceEnsureApi::SpecEnsure => {
+                    let versioned_spec = VersionedInstanceSpec::V0(
+                        self.spec.instance_spec.clone(),
+                    );
+
+                    let ensure_req = InstanceSpecEnsureRequest {
+                        properties: properties.clone(),
+                        instance_spec: versioned_spec,
+                        migrate: migrate.clone(),
+                    };
+
+                    self.client
+                        .instance_spec_ensure()
+                        .body(&ensure_req)
+                        .send()
+                        .await
+                }
+                InstanceEnsureApi::Ensure => {
+                    let ensure_req = InstanceEnsureRequest {
+                        cloud_init_bytes: None,
+                        disks: disk_reqs.clone().unwrap(),
+                        migrate: migrate.clone(),
+                        nics: vec![],
+                        properties: properties.clone(),
+                    };
+
+                    self.client.instance_ensure().body(&ensure_req).send().await
+                }
+            };
+            if let Err(e) = result {
                 match e {
                     propolis_client::Error::CommunicationError(_) => {
                         info!(%e, "retriable error from instance_spec_ensure");
@@ -341,6 +379,9 @@ impl TestVm {
             }
         };
 
+        // It shouldn't ever take more than a couple of seconds for the Propolis
+        // server to come to life. (If it does, that should be considered a bug
+        // impacting VM startup times.)
         backoff::future::retry(
             backoff::ExponentialBackoff {
                 max_elapsed_time: Some(std::time::Duration::from_secs(2)),
@@ -412,7 +453,29 @@ impl TestVm {
         match self.state {
             VmState::New => {
                 let console = self
-                    .instance_ensure_internal(None, InstanceConsoleSource::New)
+                    .instance_ensure_internal(
+                        InstanceEnsureApi::SpecEnsure,
+                        None,
+                        InstanceConsoleSource::New,
+                    )
+                    .await?;
+                self.state = VmState::Ensured { serial: console };
+            }
+            VmState::Ensured { .. } => {}
+        }
+
+        Ok(())
+    }
+
+    pub async fn instance_ensure_using_api_request(&mut self) -> Result<()> {
+        match self.state {
+            VmState::New => {
+                let console = self
+                    .instance_ensure_internal(
+                        InstanceEnsureApi::Ensure,
+                        None,
+                        InstanceConsoleSource::New,
+                    )
                     .await?;
                 self.state = VmState::Ensured { serial: console };
             }
@@ -515,6 +578,7 @@ impl TestVm {
 
                 let serial = self
                     .instance_ensure_internal(
+                        InstanceEnsureApi::SpecEnsure,
                         Some(InstanceMigrateInitiateRequest {
                             migration_id,
                             src_addr: server_addr.to_string(),

phd-tests/framework/src/test_vm/spec.rs

@@ -9,7 +9,8 @@ use crate::{
     guest_os::GuestOsKind,
 };
 use propolis_client::types::{
-    InstanceMetadata, InstanceSpecV0, StorageBackendV0,
+    DiskRequest, InstanceMetadata, InstanceSpecV0, PciPath, Slot,
+    StorageBackendV0, StorageDeviceV0,
 };
 use uuid::Uuid;
 
@@ -79,4 +80,74 @@ impl VmSpec {
         self.metadata.sled_id = id;
         self.metadata.sled_serial = id.to_string();
     }
+
+    /// Generates a set of [`propolis_client::types::DiskRequest`] structures
+    /// corresponding to the disks in this VM spec.
+    ///
+    /// All of the disks in the spec must be Crucible disks. If one is not, this
+    /// routine returns an error.
+    pub(crate) fn make_disk_requests(
+        &self,
+    ) -> anyhow::Result<Vec<DiskRequest>> {
+        struct DeviceInfo<'a> {
+            backend_name: &'a str,
+            interface: &'static str,
+            slot: Slot,
+        }
+
+        fn convert_to_slot(pci_path: PciPath) -> anyhow::Result<Slot> {
+            match pci_path.device {
+                dev @ 0x10..=0x17 => Ok(Slot(dev - 0x10)),
+                _ => Err(anyhow::anyhow!(
+                    "PCI device number {} out of range",
+                    pci_path.device
+                )),
+            }
+        }
+
+        fn get_device_info(
+            device: &StorageDeviceV0,
+        ) -> anyhow::Result<DeviceInfo> {
+            match device {
+                StorageDeviceV0::VirtioDisk(d) => Ok(DeviceInfo {
+                    backend_name: &d.backend_name,
+                    interface: "virtio",
+                    slot: convert_to_slot(d.pci_path)?,
+                }),
+                StorageDeviceV0::NvmeDisk(d) => Ok(DeviceInfo {
+                    backend_name: &d.backend_name,
+                    interface: "nvme",
+                    slot: convert_to_slot(d.pci_path)?,
+                }),
+            }
+        }
+
+        let mut reqs = vec![];
+        for (name, device) in self.instance_spec.devices.storage_devices.iter()
+        {
+            let info = get_device_info(device)?;
+            let backend = self
+                .instance_spec
+                .backends
+                .storage_backends
+                .get(info.backend_name)
+                .expect("storage device should have a matching backend");
+
+            let StorageBackendV0::Crucible(backend) = backend else {
+                anyhow::bail!("disk {name} does not have a Crucible backend");
+            };
+
+            reqs.push(DiskRequest {
+                device: info.interface.to_owned(),
+                name: name.clone(),
+                read_only: backend.readonly,
+                slot: info.slot,
+                volume_construction_request: serde_json::from_str(
+                    &backend.request_json,
+                )?,
+            })
+        }
+
+        Ok(reqs)
+    }
 }

phd-tests/tests/src/ensure_api.rs

@@ -0,0 +1,33 @@
+// 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/.
+
+//! Tests that explicitly exercise the `instance_ensure` form of the VM start
+//! API.
+
+use phd_framework::{
+    disk::BlockSize,
+    test_vm::{DiskBackend, DiskInterface},
+};
+use phd_testcase::*;
+
+#[phd_testcase]
+async fn instance_ensure_api_test(ctx: &Framework) {
+    if !ctx.crucible_enabled() {
+        phd_skip!("test requires Crucible to be enabled");
+    }
+
+    let mut config = ctx.vm_config_builder("instance_ensure_api_test");
+    config.boot_disk(
+        ctx.default_guest_os_artifact(),
+        DiskInterface::Nvme,
+        DiskBackend::Crucible {
+            min_disk_size_gib: 10,
+            block_size: BlockSize::Bytes512,
+        },
+        0x10,
+    );
+
+    let mut vm = ctx.spawn_vm(&config, None).await?;
+    vm.instance_ensure_using_api_request().await?;
+}

phd-tests/tests/src/lib.rs

@@ -6,6 +6,7 @@ pub use phd_testcase;
 
 mod crucible;
 mod disk;
+mod ensure_api;
 mod framework;
 mod hw;
 mod migrate;