server: allow VCR replacement during block backend start

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.

Modify the PHD VCR replacement smoke test so that it checks this
behavior. Add an affordance to PHD Crucible disks that allows a test to
specify that the disk's generated VCRs should contain an invalid
downstairs IP. Start a VM with a disk configured this way, then replace
the broken VCR with a corrected VCR and verify that the VM boots
normally.

Author: gjcolombo

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
@@ -291,6 +270,16 @@ impl VmObjectsLocked {
         self.vcpu_tasks.resume_all();
     }
 
+    /// 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) async fn resume_vcpus(&mut self) {
+        self.vcpu_tasks.resume_all();
+    }
+
     /// Stops the VM's vCPU tasks and devices.
     pub(super) async fn halt(&mut self) {
         self.vcpu_tasks.exit_all();
@@ -324,30 +313,6 @@ impl VmObjectsLocked {
         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(())
-    }
-
     /// Pauses all of a VM's devices.
     async fn pause_devices(&self) {
         // Take care not to wedge the runtime with any device pause

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

@@ -210,9 +210,7 @@ impl ExternalRequestQueue {
                 reboot: RequestDisposition::Deny(
                     RequestDeniedReason::InstanceNotActive,
                 ),
-                mutate: RequestDisposition::Deny(
-                    RequestDeniedReason::InstanceNotActive,
-                ),
+                mutate: RequestDisposition::Enqueue,
                 stop: RequestDisposition::Enqueue,
             },
             log,
@@ -295,7 +293,7 @@ impl ExternalRequestQueue {
                     start: Disposition::Ignore,
                     migrate_as_source: Disposition::Deny(reason),
                     reboot: Disposition::Deny(reason),
-                    mutate: Disposition::Deny(reason),
+                    mutate: Disposition::Enqueue,
                     stop: self.allowed.stop,
                 }
             }
@@ -577,18 +575,27 @@ mod test {
     }
 
     #[tokio::test]
-    async fn mutation_requires_running_and_not_migrating_out() {
+    async fn mutation_requires_not_migrating_out() {
         let mut queue =
             ExternalRequestQueue::new(test_logger(), InstanceAutoStart::No);
 
-        // Mutating a VM before it has started is not allowed.
-        assert!(queue.try_queue(make_reconfigure_crucible_request()).is_err());
+        // Mutating a VM before it has started is allowed.
+        assert!(queue.try_queue(make_reconfigure_crucible_request()).is_ok());
+        assert!(matches!(
+            queue.pop_front(),
+            Some(ExternalRequest::ReconfigureCrucibleVolume { .. })
+        ));
 
-        // Merely dequeuing the start request doesn't allow mutation; the VM
-        // actually has to be running.
+        // Mutating a VM is also allowed while it is starting.
         assert!(queue.try_queue(ExternalRequest::Start).is_ok());
         assert!(matches!(queue.pop_front(), Some(ExternalRequest::Start)));
-        assert!(queue.try_queue(make_reconfigure_crucible_request()).is_err());
+        assert!(queue.try_queue(make_reconfigure_crucible_request()).is_ok());
+        assert!(matches!(
+            queue.pop_front(),
+            Some(ExternalRequest::ReconfigureCrucibleVolume { .. })
+        ));
+
+        // And it's allowed once the VM has started running.
         queue.notify_instance_state_change(InstanceStateChange::StartedRunning);
         assert!(queue.try_queue(make_reconfigure_crucible_request()).is_ok());
         assert!(matches!(
@@ -610,10 +617,14 @@ mod test {
     }
 
     #[tokio::test]
-    async fn mutation_disallowed_after_stop() {
+    async fn mutation_disallowed_after_stop_requested() {
         let mut queue =
             ExternalRequestQueue::new(test_logger(), InstanceAutoStart::Yes);
         queue.notify_instance_state_change(InstanceStateChange::StartedRunning);
+
+        assert!(queue.try_queue(ExternalRequest::Stop).is_ok());
+        assert!(queue.try_queue(make_reconfigure_crucible_request()).is_err());
+
         queue.notify_instance_state_change(InstanceStateChange::Stopped);
         assert!(queue.try_queue(make_reconfigure_crucible_request()).is_err());
     }