Make migration status available after migration out (#330)

Use a similar technique to what we use for instance states: when a server drops its VM controller, clone the receiver side of the migration state channel and use the clone to answer subsequent migration state queries.

Tested with ad hoc live migrations & new PHD tests.

Author: gjcolombo

Cargo.lock

@@ -9,6 +9,7 @@ use std::convert::TryFrom;
 use std::sync::Arc;
 use std::{collections::BTreeMap, net::SocketAddr};
 
+use crate::migrate::MigrateError;
 use crate::serial::history_buffer::SerialHistoryOffset;
 use dropshot::{
     channel, endpoint, ApiDescription, HttpError, HttpResponseCreated,
@@ -28,6 +29,7 @@ use thiserror::Error;
 use tokio::sync::{mpsc, oneshot, MappedMutexGuard, Mutex, MutexGuard};
 use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig};
 use tokio_tungstenite::WebSocketStream;
+use uuid::Uuid;
 
 use crate::spec::{ServerSpecBuilder, ServerSpecBuilderError};
 use crate::vm::VmController;
@@ -102,8 +104,15 @@ pub enum VmControllerState {
         /// outgoing controller can publish new state changes even after the
         /// server has dropped its reference to it (its state worker may
         /// continue running for a time).
-        watcher:
+        state_watcher:
             tokio::sync::watch::Receiver<api::InstanceStateMonitorResponse>,
+
+        /// A clone of the receiver side of the former VM controller's migration
+        /// status channel, used to serve queries for migration status after an
+        /// instance is destroyed. (These are common because an instance stops
+        /// itself after successfully migrating out.)
+        migrate_state_watcher:
+            tokio::sync::watch::Receiver<Option<(Uuid, api::MigrationState)>>,
     },
 }
 
@@ -137,13 +146,15 @@ impl VmControllerState {
             // the final transition to the "destroyed" state happens only when
             // all references to the VM have been dropped, including the one
             // this routine just exchanged and will return.
-            let watcher = vm.state_watcher().clone();
+            let state_watcher = vm.state_watcher().clone();
+            let migrate_state_watcher = vm.migrate_state_watcher().clone();
             if let VmControllerState::Created(vm) = std::mem::replace(
                 self,
                 VmControllerState::Destroyed {
                     last_instance,
                     last_instance_spec: Box::new(last_instance_spec),
-                    watcher,
+                    state_watcher,
+                    migrate_state_watcher,
                 },
             ) {
                 Some(vm)
@@ -571,9 +582,10 @@ async fn instance_get_common(
         VmControllerState::Destroyed {
             last_instance,
             last_instance_spec,
-            watcher,
+            state_watcher,
+            ..
         } => {
-            let watcher = watcher.borrow();
+            let watcher = state_watcher.borrow();
             let mut last_instance = last_instance.clone();
             last_instance.state = watcher.state;
             Ok((last_instance, *last_instance_spec.clone()))
@@ -627,7 +639,9 @@ async fn instance_state_monitor(
                 ));
             }
             VmControllerState::Created(vm) => vm.state_watcher().clone(),
-            VmControllerState::Destroyed { watcher, .. } => watcher.clone(),
+            VmControllerState::Destroyed { state_watcher, .. } => {
+                state_watcher.clone()
+            }
         }
     };
 
@@ -793,10 +807,29 @@ async fn instance_migrate_status(
     path_params: Path<api::InstanceMigrateStatusRequest>,
 ) -> Result<HttpResponseOk<api::InstanceMigrateStatusResponse>, HttpError> {
     let migration_id = path_params.into_inner().migration_id;
-    let vm = rqctx.context().vm().await?;
-    vm.migrate_status(migration_id).map_err(Into::into).map(|state| {
-        HttpResponseOk(api::InstanceMigrateStatusResponse { state })
-    })
+    let ctx = rqctx.context();
+    match &*ctx.services.vm.lock().await {
+        VmControllerState::NotCreated => Err(HttpError::for_internal_error(
+            "Server not initialized (no instance)".to_string(),
+        )),
+        VmControllerState::Created(vm) => {
+            vm.migrate_status(migration_id).map_err(Into::into).map(|state| {
+                HttpResponseOk(api::InstanceMigrateStatusResponse { state })
+            })
+        }
+        VmControllerState::Destroyed { migrate_state_watcher, .. } => {
+            let watcher = migrate_state_watcher.borrow();
+            match *watcher {
+                None => Err((MigrateError::NoMigrationInProgress).into()),
+                Some((id, state)) if id == migration_id => {
+                    Ok(HttpResponseOk(api::InstanceMigrateStatusResponse {
+                        state,
+                    }))
+                }
+                Some(_) => Err((MigrateError::UuidMismatch).into()),
+            }
+        }
+    }
 }
 
 /// Issues a snapshot request to a crucible backend.

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

@@ -650,6 +650,12 @@ impl VmController {
         &self.vm_objects.monitor_rx
     }
 
+    pub fn migrate_state_watcher(
+        &self,
+    ) -> &tokio::sync::watch::Receiver<Option<(Uuid, ApiMigrationState)>> {
+        &self.vm_objects.migrate_state_rx
+    }
+
     /// Asks to queue a request to start a source migration task for this VM.
     /// The migration will have the supplied `migration_id` and will obtain its
     /// connection to the target by calling `upgrade_fn` to obtain a future that

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

@@ -291,12 +291,12 @@ impl TestVm {
     pub fn migrate_from(
         &mut self,
         source: &Self,
+        migration_id: Uuid,
         timeout_duration: Duration,
     ) -> Result<()> {
         let _vm_guard = self.tracing_span.enter();
         match self.state {
             VmState::New => {
-                let migration_id = Uuid::new_v4();
                 info!(
                     ?migration_id,
                     "Migrating from source at address {}",
@@ -348,17 +348,15 @@ impl TestVm {
                     ..Default::default()
                 };
                 backoff::retry(backoff, watch_migrate)
-                    .map_err(|e| anyhow!("error during migration: {}", e))?;
+                    .map_err(|e| anyhow!("error during migration: {}", e))
             }
             VmState::Ensured { .. } => {
                 return Err(VmStateError::InstanceAlreadyEnsured.into());
             }
         }
-
-        Ok(())
     }
 
-    fn get_migration_state(
+    pub fn get_migration_state(
         &self,
         migration_id: Uuid,
     ) -> Result<MigrationState> {

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

@@ -9,3 +9,4 @@ edition = "2021"
 propolis-client = { workspace = true, features = ["generated"] }
 phd-testcase.workspace = true
 tracing.workspace = true
+uuid.workspace = true

phd-tests/tests/Cargo.toml

@@ -3,6 +3,7 @@ use std::time::Duration;
 use phd_framework::test_vm::vm_config::DiskInterface;
 use phd_testcase::*;
 use tracing::info;
+use uuid::Uuid;
 
 #[phd_testcase]
 fn smoke_test(ctx: &TestContext) {
@@ -30,7 +31,7 @@ fn smoke_test(ctx: &TestContext) {
 
     let mut target =
         ctx.vm_factory.new_vm("crucible_migrate_smoke_target", config)?;
-    target.migrate_from(&source, Duration::from_secs(60))?;
+    target.migrate_from(&source, Uuid::new_v4(), Duration::from_secs(60))?;
     let lsout = target.run_shell_command("ls foo.bar")?;
     assert_eq!(lsout, "foo.bar");
 }
@@ -72,7 +73,7 @@ fn load_test(ctx: &TestContext) {
     // Start copying the generated file into a second file, then start a
     // migration while that copy is in progress.
     source.run_shell_command("dd if=./rand.txt of=./rand_new.txt &")?;
-    target.migrate_from(&source, Duration::from_secs(60))?;
+    target.migrate_from(&source, Uuid::new_v4(), Duration::from_secs(60))?;
 
     // Wait for the background command to finish running, then compute the
     // hash of the copied file. If all went well this will match the hash of

phd-tests/tests/src/crucible/migrate.rs

@@ -1,6 +1,8 @@
 use std::time::Duration;
 
 use phd_testcase::*;
+use propolis_client::handmade::api::MigrationState;
+use uuid::Uuid;
 
 #[phd_testcase]
 fn smoke_test(ctx: &TestContext) {
@@ -19,7 +21,16 @@ fn smoke_test(ctx: &TestContext) {
         .vm_factory
         .new_vm_from_cloned_config("migration_smoke_target", &source)?;
 
-    target.migrate_from(&source, Duration::from_secs(60))?;
+    let migration_id = Uuid::new_v4();
+    target.migrate_from(&source, migration_id, Duration::from_secs(60))?;
+
+    // Explicitly check migration status on both the source and target to make
+    // sure it is available even after migration has finished.
+    let src_migration_state = source.get_migration_state(migration_id)?;
+    assert_eq!(src_migration_state, MigrationState::Finish);
+    let target_migration_state = target.get_migration_state(migration_id)?;
+    assert_eq!(target_migration_state, MigrationState::Finish);
+
     let lsout = target.run_shell_command("ls foo.bar")?;
     assert_eq!(lsout, "foo.bar");
 }
@@ -44,7 +55,18 @@ fn incompatible_vms(ctx: &TestContext) {
             cfg,
         )?;
 
-        assert!(target.migrate_from(&source, Duration::from_secs(60)).is_err());
+        let migration_id = Uuid::new_v4();
+        assert!(target
+            .migrate_from(&source, migration_id, Duration::from_secs(60))
+            .is_err());
+
+        // Explicitly check migration status on both the source and target to
+        // make sure it is available even after migration has finished.
+        let src_migration_state = source.get_migration_state(migration_id)?;
+        assert_eq!(src_migration_state, MigrationState::Error);
+        let target_migration_state =
+            target.get_migration_state(migration_id)?;
+        assert_eq!(target_migration_state, MigrationState::Error);
     }
 }
 
@@ -62,9 +84,9 @@ fn multiple_migrations(ctx: &TestContext) {
 
     vm0.launch()?;
     vm0.wait_to_boot()?;
-    vm1.migrate_from(&vm0, Duration::from_secs(60))?;
+    vm1.migrate_from(&vm0, Uuid::new_v4(), Duration::from_secs(60))?;
     assert_eq!(vm1.run_shell_command("echo Hello world")?, "Hello world");
-    vm2.migrate_from(&vm1, Duration::from_secs(60))?;
+    vm2.migrate_from(&vm1, Uuid::new_v4(), Duration::from_secs(60))?;
     assert_eq!(
         vm2.run_shell_command("echo I have migrated!")?,
         "I have migrated!"