server: improve behavior of starting VMs that are waiting for Crucible activation (#873)

Modify the state driver's VM startup procedure to allow the driver to
process Crucible volume configuration changes while block backends are
being activated. This fixes a livelock that occurs when starting a VM
with a Crucible VCR that points to an unavailable downstairs: the
unavailable downstairs prevents Crucible activation from proceeding;
Nexus sends a corrected VCR that, if applied, would allow the upstairs
to activate; but the state driver never applies the new VCR because it's
blocked trying to activate using the broken VCR.

Since the state driver can now dequeue VM state change requests during
startup, also teach it to abort startup if a stop request is received
while block backends are starting. This is very slightly spicy, because
  it can cancel a block backend startup future in a way that was not
  possible before, but (a) the only affected backend is the Crucible
  backend, and (b) there should be no requests in flight anyway because,
  if this case is reached, the VM's vCPUs have not started.

Add PHD coverage of the new behaviors:

- Modify the PHD VCR replacement smoke test to check that start requests
  with a bad set of Crucible targets can be unblocked by replacing a bad
  target with a good target.
- Add a server state machine test that checks that a VM that is blocked
  waiting for Crucible activation can be explicitly stopped.

To assist with this, add a feature to PHD Crucible disks that allows a
test to specify that a disk's generated VCRs should contain an invalid
downstairs IP address. Starting a VM with a disk configured this way
will cause activation to block until the disk's VCR is replaced with a
corrected VCR.

Author: gjcolombo

bin/propolis-cli/src/main.rs

@@ -155,8 +155,8 @@ enum Command {
     /// Call the VolumeConstructionRequest replace endpoint
     Vcr {
         /// Uuid for the disk
-        #[clap(short = 'u', action)]
-        uuid: Uuid,
+        #[clap(short = 'd', action)]
+        disk_id: String,
 
         /// File with a JSON InstanceVcrReplace struct
         #[clap(long, action)]
@@ -510,7 +510,7 @@ async fn new_instance(
 
 async fn replace_vcr(
     client: &Client,
-    id: Uuid,
+    id: String,
     vcr_replace: InstanceVcrReplace,
 ) -> anyhow::Result<()> {
     // Try to call the endpoint
@@ -941,9 +941,9 @@ async fn main() -> anyhow::Result<()> {
         }
         Command::Monitor => monitor(addr).await?,
         Command::InjectNmi => inject_nmi(&client).await?,
-        Command::Vcr { uuid, vcr_replace } => {
+        Command::Vcr { disk_id, vcr_replace } => {
             let replace: InstanceVcrReplace = parse_json_file(&vcr_replace)?;
-            replace_vcr(&client, uuid, replace).await?
+            replace_vcr(&client, disk_id, replace).await?
         }
     }
 

bin/propolis-server/src/lib/vm/objects.rs

@@ -23,9 +23,7 @@ use tokio::sync::{RwLock, RwLockReadGuard, RwLockWriteGuard};
 
 use crate::{serial::Serial, spec::Spec, vcpu_tasks::VcpuTaskController};
 
-use super::{
-    state_driver::VmStartReason, BlockBackendMap, CrucibleBackendMap, DeviceMap,
-};
+use super::{BlockBackendMap, CrucibleBackendMap, DeviceMap};
 
 /// A collection of components that make up a Propolis VM instance.
 pub(crate) struct VmObjects {
@@ -189,6 +187,14 @@ impl VmObjectsLocked {
         &self.ps2ctrl
     }
 
+    pub(crate) fn device_map(&self) -> &DeviceMap {
+        &self.devices
+    }
+
+    pub(crate) fn block_backend_map(&self) -> &BlockBackendMap {
+        &self.block_backends
+    }
+
     /// Iterates over all of the lifecycle trait objects in this VM and calls
     /// `func` on each one.
     pub(crate) fn for_each_device(
@@ -244,33 +250,6 @@ impl VmObjectsLocked {
         self.machine.reinitialize().unwrap();
     }
 
-    /// Starts a VM's devices and allows all of its vCPU tasks to run.
-    ///
-    /// This function may be called either after initializing a new VM from
-    /// scratch or after an inbound live migration. In the latter case, this
-    /// routine assumes that the caller initialized and activated the VM's vCPUs
-    /// prior to importing state from the migration source.
-    pub(super) async fn start(
-        &mut self,
-        reason: VmStartReason,
-    ) -> anyhow::Result<()> {
-        match reason {
-            VmStartReason::ExplicitRequest => {
-                self.reset_vcpus();
-            }
-            VmStartReason::MigratedIn => {
-                self.resume_kernel_vm();
-            }
-        }
-
-        let result = self.start_devices().await;
-        if result.is_ok() {
-            self.vcpu_tasks.resume_all();
-        }
-
-        result
-    }
-
     /// Pauses this VM's devices and its kernel VMM.
     pub(crate) async fn pause(&mut self) {
         // Order matters here: the Propolis lifecycle trait's pause function
@@ -288,6 +267,16 @@ impl VmObjectsLocked {
         // that all devices resume before any vCPUs do.
         self.resume_kernel_vm();
         self.resume_devices();
+        self.resume_vcpus();
+    }
+
+    /// Resumes this VM's vCPU tasks.
+    ///
+    /// This is intended for use in VM startup sequences where the state driver
+    /// needs fine-grained control over the order in which devices and vCPUs
+    /// start. When pausing and resuming a VM that's already been started, use
+    /// [`Self::pause`] and [`Self::resume`] instead.
+    pub(crate) fn resume_vcpus(&mut self) {
         self.vcpu_tasks.resume_all();
     }
 
@@ -321,31 +310,7 @@ impl VmObjectsLocked {
         // Resume devices so they're ready to do more work, then resume
         // vCPUs.
         self.resume_devices();
-        self.vcpu_tasks.resume_all();
-    }
-
-    /// Starts all of a VM's devices and allows its block backends to process
-    /// requests from their devices.
-    async fn start_devices(&self) -> anyhow::Result<()> {
-        self.for_each_device_fallible(|name, dev| {
-            info!(self.log, "sending startup complete to {}", name);
-            let res = dev.start();
-            if let Err(e) = &res {
-                error!(self.log, "startup failed for {}: {:?}", name, e);
-            }
-            res
-        })?;
-
-        for (name, backend) in self.block_backends.iter() {
-            info!(self.log, "starting block backend {}", name);
-            let res = backend.start().await;
-            if let Err(e) = &res {
-                error!(self.log, "startup failed for {}: {:?}", name, e);
-                return res;
-            }
-        }
-
-        Ok(())
+        self.resume_vcpus();
     }
 
     /// Pauses all of a VM's devices.

bin/propolis-server/src/lib/vm/request_queue.rs

@@ -88,10 +88,6 @@ impl std::fmt::Debug for StateChangeRequest {
 pub enum ComponentChangeRequest {
     /// Attempts to update the volume construction request for the supplied
     /// Crucible volume.
-    ///
-    /// TODO: Due to https://github.com/oxidecomputer/crucible/issues/871, this
-    /// is only allowed once the VM is started and the volume has activated, but
-    /// it should be allowed even before the VM has started.
     ReconfigureCrucibleVolume {
         /// The ID of the Crucible backend in the VM's Crucible backend map.
         backend_id: SpecKey,
@@ -356,7 +352,6 @@ impl ExternalRequestQueue {
                     "request" => ?request,
                     "reason" => %reason
                 );
-
                 return Err(reason);
             }
         }
@@ -795,7 +790,6 @@ mod test {
     fn mutation_disallowed_after_stopped() {
         let mut queue =
             ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes);
-
         queue.notify_request_completed(CompletedRequest::Start {
             succeeded: true,
         });