@smklein's review feedback

nexus/db-model/src/migration.rs

@@ -30,8 +30,6 @@ pub struct Migration {
     /// The time at which this migration record was deleted,
     pub time_deleted: Option<DateTime<Utc>>,
 
-    /// The time at which the source VMM state was last updated.
-
     /// The state of the migration source VMM.
     pub source_state: MigrationState,
 

nexus/db-queries/src/db/datastore/instance.rs

@@ -142,6 +142,25 @@ pub enum UpdaterLockError {
     Query(#[from] Error),
 }
 
+/// The result of an [`DataStore::instance_and_vmm_update_runtime`] call,
+/// indicating which records were updated.
+#[derive(Copy, Clone, Debug)]
+pub struct InstanceUpdateResult {
+    /// `true` if the instance record was updated, `false` otherwise.
+    pub instance_updated: bool,
+    /// `true` if the VMM record was updated, `false` otherwise.
+    pub vmm_updated: bool,
+    /// Indicates whether a migration record for this instance was updated, if a
+    /// [`MigrationRuntimeState`] was provided to
+    /// [`DataStore::instance_and_vmm_update_runtime`].
+    ///
+    /// - `Some(true)` if a migration record was updated
+    /// - `Some(false)` if a [`MigrationRuntimeState`] was provided, but the
+    ///   migration record was not updated
+    /// - `None` if no [`MigrationRuntimeState`] was provided
+    pub migration_updated: Option<bool>,
+}
+
 impl DataStore {
     /// Idempotently insert a database record for an Instance
     ///
@@ -373,12 +392,11 @@ impl DataStore {
     ///
     /// # Return value
     ///
-    /// - `Ok((instance_updated, vmm_updated))` if the query was issued
-    ///   successfully. `instance_updated` and `vmm_updated` are each true if
-    ///   the relevant item was updated and false otherwise. Note that an update
-    ///   can fail because it was inapplicable (i.e. the database has state with
-    ///   a newer generation already) or because the relevant record was not
-    ///   found.
+    /// - `Ok(`[`InstanceUpdateResult`]`)` if the query was issued
+    ///   successfully. The returned [`InstanceUpdateResult`] indicates which
+    ///   database record(s) were updated. Note that an update can fail because
+    ///   it was inapplicable (i.e. the database has state with a newer
+    ///   generation already) or because the relevant record was not found.
     /// - `Err` if another error occurred while accessing the database.
     pub async fn instance_and_vmm_update_runtime(
         &self,
@@ -387,7 +405,7 @@ impl DataStore {
         vmm_id: &Uuid,
         new_vmm: &VmmRuntimeState,
         migration: &Option<MigrationRuntimeState>,
-    ) -> Result<(bool, bool, Option<bool>), Error> {
+    ) -> Result<InstanceUpdateResult, Error> {
         let query = crate::db::queries::instance::InstanceAndVmmUpdate::new(
             *instance_id,
             new_instance.clone(),
@@ -427,7 +445,11 @@ impl DataStore {
             None
         };
 
-        Ok((instance_updated, vmm_updated, migration_updated))
+        Ok(InstanceUpdateResult {
+            instance_updated,
+            vmm_updated,
+            migration_updated,
+        })
     }
 
     /// Lists all instances on in-service sleds with active Propolis VMM

nexus/src/app/instance.rs

@@ -25,6 +25,7 @@ use nexus_db_queries::authn;
 use nexus_db_queries::authz;
 use nexus_db_queries::context::OpContext;
 use nexus_db_queries::db;
+use nexus_db_queries::db::datastore::instance::InstanceUpdateResult;
 use nexus_db_queries::db::datastore::InstanceAndActiveVmm;
 use nexus_db_queries::db::identity::Resource;
 use nexus_db_queries::db::lookup;
@@ -563,28 +564,27 @@ impl super::Nexus {
         // outright fails, this operation fails. If the operation nominally
         // succeeds but nothing was updated, this action is outdated and the
         // caller should not proceed with migration.
-        let (updated, _, _) = match instance_put_result {
-            Ok(state) => {
-                self.write_returned_instance_state(&instance_id, state).await?
-
-                // TODO(eliza): this is where we would also want to insert to the
-                // migration table...
-            }
-            Err(e) => {
-                if e.instance_unhealthy() {
-                    let _ = self
-                        .mark_instance_failed(
-                            &instance_id,
-                            &prev_instance_runtime,
-                            &e,
-                        )
-                        .await;
+        let InstanceUpdateResult { instance_updated, .. } =
+            match instance_put_result {
+                Ok(state) => {
+                    self.write_returned_instance_state(&instance_id, state)
+                        .await?
                 }
-                return Err(e.into());
-            }
-        };
+                Err(e) => {
+                    if e.instance_unhealthy() {
+                        let _ = self
+                            .mark_instance_failed(
+                                &instance_id,
+                                &prev_instance_runtime,
+                                &e,
+                            )
+                            .await;
+                    }
+                    return Err(e.into());
+                }
+            };
 
-        if updated {
+        if instance_updated {
             Ok(self
                 .db_datastore
                 .instance_refetch(opctx, &authz_instance)
@@ -1324,7 +1324,7 @@ impl super::Nexus {
         &self,
         instance_id: &Uuid,
         state: Option<nexus::SledInstanceState>,
-    ) -> Result<(bool, bool, Option<bool>), Error> {
+    ) -> Result<InstanceUpdateResult, Error> {
         slog::debug!(&self.log,
                      "writing instance state returned from sled agent";
                      "instance_id" => %instance_id,
@@ -1350,7 +1350,13 @@ impl super::Nexus {
 
             update_result
         } else {
-            Ok((false, false, None))
+            // There was no instance state to write back, so --- perhaps
+            // obviously --- nothing happened.
+            Ok(InstanceUpdateResult {
+                instance_updated: false,
+                vmm_updated: false,
+                migration_updated: None,
+            })
         }
     }
 
@@ -1958,16 +1964,6 @@ impl super::Nexus {
     }
 }
 
-/// Records what aspects of an instance's state were actually changed in a
-/// [`notify_instance_updated`] call.
-///
-/// This is (presently) used for debugging purposes only.
-#[derive(Copy, Clone)]
-pub(crate) struct InstanceUpdated {
-    pub instance_updated: bool,
-    pub vmm_updated: bool,
-}
-
 /// Invoked by a sled agent to publish an updated runtime state for an
 /// Instance.
 #[allow(clippy::too_many_arguments)] // :(
@@ -1980,7 +1976,7 @@ pub(crate) async fn notify_instance_updated(
     instance_id: &Uuid,
     new_runtime_state: &nexus::SledInstanceState,
     v2p_notification_tx: tokio::sync::watch::Sender<()>,
-) -> Result<Option<InstanceUpdated>, Error> {
+) -> Result<Option<InstanceUpdateResult>, Error> {
     let propolis_id = new_runtime_state.propolis_id;
 
     info!(log, "received new runtime state from sled agent";
@@ -2118,14 +2114,14 @@ pub(crate) async fn notify_instance_updated(
     }
 
     match result {
-        Ok((instance_updated, vmm_updated, migration_updated)) => {
+        Ok(result) => {
             info!(log, "instance and vmm updated by sled agent";
                     "instance_id" => %instance_id,
                     "propolis_id" => %propolis_id,
-                    "instance_updated" => instance_updated,
-                    "vmm_updated" => vmm_updated,
-                    "migration_updated" => ?migration_updated);
-            Ok(Some(InstanceUpdated { instance_updated, vmm_updated }))
+                    "instance_updated" => result.instance_updated,
+                    "vmm_updated" => result.vmm_updated,
+                    "migration_updated" => ?result.migration_updated);
+            Ok(Some(result))
         }
 
         // The update command should swallow object-not-found errors and