review feedback

Author: davepacheco

dev-tools/omdb/src/bin/omdb/nexus/quiesce.rs

@@ -11,6 +11,7 @@ use chrono::TimeDelta;
 use chrono::Utc;
 use clap::Args;
 use clap::Subcommand;
+use nexus_client::types::PendingRecovery;
 use nexus_client::types::QuiesceState;
 use nexus_client::types::QuiesceStatus;
 use nexus_client::types::SagaQuiesceStatus;
@@ -160,6 +161,7 @@ async fn quiesce_show(
         reassignment_pending,
         recovered_blueprint_id,
         recovered_reassignment_generation,
+        recovery_pending,
     } = sagas;
 
     println!("saga quiesce:");
@@ -172,7 +174,7 @@ async fn quiesce_show(
             .unwrap_or("none")
     );
     println!(
-        "    blueprint for last recovery pass: {}",
+        "    blueprint for last completed recovery pass: {}",
         recovered_blueprint_id
             .map(|s| s.to_string())
             .as_deref()
@@ -195,6 +197,17 @@ async fn quiesce_show(
         "    recovered at least once successfully: {}",
         if first_recovery_complete { "yes" } else { "no" },
     );
+    print!("    recovery pending: ");
+    if let Some(PendingRecovery { generation, blueprint_id }) = recovery_pending
+    {
+        println!(
+            "yes (generation {}, blueprint id {})",
+            generation,
+            blueprint_id.map(|s| s.to_string()).as_deref().unwrap_or("none")
+        );
+    } else {
+        println!("no");
+    }
 
     println!("    sagas running: {}", sagas_pending.len());
     for saga in &sagas_pending {

nexus/types/src/quiesce.rs

@@ -183,11 +183,18 @@ pub struct SagaQuiesceStatus {
     /// there's a new blueprint that expunges a different Nexus zone.
     drained_blueprint_id: Option<BlueprintUuid>,
 
-    /// whether a saga recovery operation is ongoing, and if one is:
-    /// - what `reassignment_generation` was when it started
-    /// - which blueprint id we'll be fully caught up to upon completion
-    #[serde(skip)] // XXX-dap
-    recovery_pending: Option<(Generation, Option<BlueprintUuid>)>,
+    /// If a recovery pass is ongoing, a snapshot of reassignment state when it
+    /// started (which reflects what we'll be caught up to when it finishes)
+    recovery_pending: Option<PendingRecovery>,
+}
+
+/// Snapshot of reassignment state when a recovery pass started
+#[derive(Debug, Clone, Serialize, JsonSchema)]
+struct PendingRecovery {
+    /// what `reassignment_generation` was when this recovery started
+    generation: Generation,
+    /// which blueprint id we'd be fully caught up to upon completion
+    blueprint_id: Option<BlueprintUuid>,
 }
 
 impl SagaQuiesceHandle {
@@ -251,7 +258,7 @@ impl SagaQuiesceHandle {
                 }
             };
 
-            q.latch_drained_blueprint_id();
+            q.latch_blueprint_if_drained();
             changed
         });
     }
@@ -423,7 +430,7 @@ impl SagaQuiesceHandle {
                 }
             }
 
-            q.latch_drained_blueprint_id();
+            q.latch_blueprint_if_drained();
         });
     }
 
@@ -468,8 +475,10 @@ impl SagaQuiesceHandle {
                 "recovery_start() called twice without intervening \
                  recovery_done() (concurrent calls to recover()?)",
             );
-            q.recovery_pending =
-                Some((q.reassignment_generation, q.reassignment_blueprint_id));
+            q.recovery_pending = Some(PendingRecovery {
+                generation: q.reassignment_generation,
+                blueprint_id: q.reassignment_blueprint_id,
+            });
         });
 
         info!(&self.log, "saga recovery pass starting");
@@ -480,7 +489,8 @@ impl SagaQuiesceHandle {
     fn recovery_done(&self, success: bool) {
         let log = self.log.clone();
         self.inner.send_modify(|q| {
-            let Some((generation, blueprint_id)) = q.recovery_pending.take()
+            let Some(PendingRecovery { generation, blueprint_id }) =
+                q.recovery_pending.take()
             else {
                 panic!("cannot finish saga recovery when it was not running");
             };
@@ -495,7 +505,7 @@ impl SagaQuiesceHandle {
                 q.recovered_blueprint_id = blueprint_id;
                 q.recovered_reassignment_generation = generation;
                 q.first_recovery_complete = true;
-                q.latch_drained_blueprint_id();
+                q.latch_blueprint_if_drained();
             } else {
                 info!(&log, "saga recovery pass failed");
             }
@@ -636,7 +646,7 @@ impl SagaQuiesceStatus {
     /// result of that blueprint.  The rest of our bookkeeping would reflect
     /// that we're not fully drained, which is true, but we still want to be
     /// able to report that we were fully drained _as of this blueprint_.
-    fn latch_drained_blueprint_id(&mut self) {
+    fn latch_blueprint_if_drained(&mut self) {
         if self.is_fully_drained() {
             // If we've recovered up through a given blueprint id and are now
             // fully drained, then we have definitely fully drained up through
@@ -705,7 +715,7 @@ impl NewlyPendingSagaRef {
                 q.sagas_pending
                     .remove(&saga_id)
                     .expect("saga should have been running");
-                q.latch_drained_blueprint_id();
+                q.latch_blueprint_if_drained();
             });
             rv
         });

openapi/nexus-internal.json

@@ -6527,6 +6527,32 @@
           "by_baseboard"
         ]
       },
+      "PendingRecovery": {
+        "description": "Snapshot of reassignment state when a recovery pass started",
+        "type": "object",
+        "properties": {
+          "blueprint_id": {
+            "nullable": true,
+            "description": "which blueprint id we'd be fully caught up to upon completion",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/TypedUuidForBlueprintKind"
+              }
+            ]
+          },
+          "generation": {
+            "description": "what `reassignment_generation` was when this recovery started",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/Generation"
+              }
+            ]
+          }
+        },
+        "required": [
+          "generation"
+        ]
+      },
       "PendingSagaInfo": {
         "description": "Describes a pending saga (for debugging why quiesce is stuck)",
         "type": "object",
@@ -8301,6 +8327,15 @@
               }
             ]
           },
+          "recovery_pending": {
+            "nullable": true,
+            "description": "If a recovery pass is ongoing, a snapshot of reassignment state when it started (which reflects what we'll be caught up to when it finishes)",
+            "allOf": [
+              {
+                "$ref": "#/components/schemas/PendingRecovery"
+              }
+            ]
+          },
           "sagas_pending": {
             "title": "IdOrdMap",
             "description": "list of sagas we need to wait to complete before quiescing\n\nThese are basically running sagas.  They may have been created in this Nexus process lifetime or created in another process and then recovered in this one.",