[reconfigurator] a couple of type improvements (#6395)

* Use a typestate for the physical disk dependency. Just want to make
sure the ordering is right once this moves to being expressed via the
update engine.
* Add strong typing around the output of `realize_blueprint`.

nexus/reconfigurator/execution/src/cockroachdb.rs

@@ -34,6 +34,7 @@ pub(crate) async fn ensure_settings(
 mod test {
     use super::*;
     use crate::overridables::Overridables;
+    use crate::RealizeBlueprintOutput;
     use nexus_db_queries::authn;
     use nexus_db_queries::authz;
     use nexus_test_utils_macros::nexus_test;
@@ -97,16 +98,17 @@ mod test {
         .await;
         // Execute the initial blueprint.
         let overrides = Overridables::for_test(cptestctx);
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute initial blueprint");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute initial blueprint");
         // The CockroachDB settings should not have changed.
         assert_eq!(
             settings,

nexus/reconfigurator/execution/src/dns.rs

@@ -458,6 +458,7 @@ pub fn blueprint_nexus_external_ips(blueprint: &Blueprint) -> Vec<IpAddr> {
 mod test {
     use super::*;
     use crate::overridables::Overridables;
+    use crate::RealizeBlueprintOutput;
     use crate::Sled;
     use dns_service_client::DnsDiff;
     use internal_dns::config::Host;
@@ -1245,16 +1246,17 @@ mod test {
 
         // Now, execute the initial blueprint.
         let overrides = Overridables::for_test(cptestctx);
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute initial blueprint");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute initial blueprint");
 
         // DNS ought not to have changed.
         verify_dns_unchanged(
@@ -1385,16 +1387,17 @@ mod test {
             .await
             .expect("failed to set blueprint as target");
 
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint2,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute second blueprint");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint2,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute second blueprint");
 
         // Now fetch DNS again.  Both should have changed this time.
         let dns_latest_internal = datastore
@@ -1459,16 +1462,17 @@ mod test {
         }
 
         // If we execute it again, we should see no more changes.
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint2,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute second blueprint again");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint2,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute second blueprint again");
         verify_dns_unchanged(
             &opctx,
             datastore,
@@ -1495,16 +1499,17 @@ mod test {
 
         // One more time, make sure that executing the blueprint does not do
         // anything.
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint2,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute second blueprint again");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint2,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute second blueprint again");
         verify_dns_unchanged(
             &opctx,
             datastore,
@@ -1589,16 +1594,17 @@ mod test {
         );
 
         // If we execute the blueprint, DNS should not be changed.
-        crate::realize_blueprint_with_overrides(
-            &opctx,
-            datastore,
-            resolver,
-            &blueprint,
-            Uuid::new_v4(),
-            &overrides,
-        )
-        .await
-        .expect("failed to execute blueprint");
+        let _: RealizeBlueprintOutput =
+            crate::realize_blueprint_with_overrides(
+                &opctx,
+                datastore,
+                resolver,
+                &blueprint,
+                Uuid::new_v4(),
+                &overrides,
+            )
+            .await
+            .expect("failed to execute blueprint");
         let dns_latest_internal = datastore
             .dns_config_read(&opctx, DnsGroup::Internal)
             .await

nexus/reconfigurator/execution/src/lib.rs

@@ -70,6 +70,15 @@ impl From<nexus_db_model::Sled> for Sled {
     }
 }
 
+/// The result of calling [`realize_blueprint`] or
+/// [`realize_blueprint_with_overrides`].
+#[derive(Debug)]
+#[must_use = "the output of realize_blueprint should probably be used"]
+pub struct RealizeBlueprintOutput {
+    /// Whether any sagas need to be reassigned to a new Nexus.
+    pub needs_saga_recovery: bool,
+}
+
 /// Make one attempt to realize the given blueprint, meaning to take actions to
 /// alter the real system to match the blueprint
 ///
@@ -81,7 +90,7 @@ pub async fn realize_blueprint(
     resolver: &Resolver,
     blueprint: &Blueprint,
     nexus_id: Uuid,
-) -> Result<bool, Vec<anyhow::Error>> {
+) -> Result<RealizeBlueprintOutput, Vec<anyhow::Error>> {
     realize_blueprint_with_overrides(
         opctx,
         datastore,
@@ -100,7 +109,7 @@ pub async fn realize_blueprint_with_overrides(
     blueprint: &Blueprint,
     nexus_id: Uuid,
     overrides: &Overridables,
-) -> Result<bool, Vec<anyhow::Error>> {
+) -> Result<RealizeBlueprintOutput, Vec<anyhow::Error>> {
     let opctx = opctx.child(BTreeMap::from([(
         "comment".to_string(),
         blueprint.comment.clone(),
@@ -132,7 +141,7 @@ pub async fn realize_blueprint_with_overrides(
         })
         .collect();
 
-    omicron_physical_disks::deploy_disks(
+    let deploy_disks_done = omicron_physical_disks::deploy_disks(
         &opctx,
         &sleds_by_id,
         &blueprint.blueprint_disks,
@@ -205,11 +214,12 @@ pub async fn realize_blueprint_with_overrides(
     )
     .await?;
 
-    // This depends on the "deploy_disks" call earlier -- disk expungement is a
-    // statement of policy, but we need to be assured that the Sled Agent has
-    // stopped using that disk before we can mark its state as decommissioned.
-    omicron_physical_disks::decommission_expunged_disks(&opctx, datastore)
-        .await?;
+    omicron_physical_disks::decommission_expunged_disks(
+        &opctx,
+        datastore,
+        deploy_disks_done,
+    )
+    .await?;
 
     // From this point on, we'll assume that any errors that we encounter do
     // *not* require stopping execution.  We'll just accumulate them and return
@@ -244,7 +254,7 @@ pub async fn realize_blueprint_with_overrides(
     }
 
     if errors.is_empty() {
-        Ok(needs_saga_recovery)
+        Ok(RealizeBlueprintOutput { needs_saga_recovery })
     } else {
         Err(errors)
     }

nexus/reconfigurator/execution/src/omicron_physical_disks.rs

@@ -25,7 +25,7 @@ pub(crate) async fn deploy_disks(
     opctx: &OpContext,
     sleds_by_id: &BTreeMap<SledUuid, Sled>,
     sled_configs: &BTreeMap<SledUuid, BlueprintPhysicalDisksConfig>,
-) -> Result<(), Vec<anyhow::Error>> {
+) -> Result<DeployDisksDone, Vec<anyhow::Error>> {
     let errors: Vec<_> = stream::iter(sled_configs)
         .filter_map(|(sled_id, config)| async move {
             let log = opctx.log.new(o!(
@@ -92,16 +92,26 @@ pub(crate) async fn deploy_disks(
         .await;
 
     if errors.is_empty() {
-        Ok(())
+        Ok(DeployDisksDone {})
     } else {
         Err(errors)
     }
 }
 
-/// Decommissions all disks which are currently expunged
+/// Typestate indicating that the deploy disks step was performed.
+#[derive(Debug)]
+#[must_use = "this should be passed into decommission_expunged_disks"]
+pub(crate) struct DeployDisksDone {}
+
+/// Decommissions all disks which are currently expunged.
 pub(crate) async fn decommission_expunged_disks(
     opctx: &OpContext,
     datastore: &DataStore,
+    // This is taken as a parameter to ensure that this depends on a
+    // "deploy_disks" call made earlier. Disk expungement is a statement of
+    // policy, but we need to be assured that the Sled Agent has stopped using
+    // that disk before we can mark its state as decommissioned.
+    _deploy_disks_done: DeployDisksDone,
 ) -> Result<(), Vec<anyhow::Error>> {
     datastore
         .physical_disk_decommission_all_expunged(&opctx)
@@ -113,6 +123,7 @@ pub(crate) async fn decommission_expunged_disks(
 #[cfg(test)]
 mod test {
     use super::deploy_disks;
+    use super::DeployDisksDone;
 
     use crate::DataStore;
     use crate::Sled;
@@ -217,9 +228,13 @@ mod test {
         // Get a success result back when the blueprint has an empty set of
         // disks.
         let (_, blueprint) = create_blueprint(BTreeMap::new());
-        deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
-            .await
-            .expect("failed to deploy no disks");
+        // Use an explicit type here because not doing so can cause errors to
+        // be ignored (this behavior is genuinely terrible). Instead, ensure
+        // that the type has the right result.
+        let _: DeployDisksDone =
+            deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
+                .await
+                .expect("failed to deploy no disks");
 
         // Disks are updated in a particular order, but each request contains
         // the full set of disks that must be running.
@@ -272,9 +287,10 @@ mod test {
         }
 
         // Execute it.
-        deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
-            .await
-            .expect("failed to deploy initial disks");
+        let _: DeployDisksDone =
+            deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
+                .await
+                .expect("failed to deploy initial disks");
 
         s1.verify_and_clear();
         s2.verify_and_clear();
@@ -293,9 +309,10 @@ mod test {
                 )),
             );
         }
-        deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
-            .await
-            .expect("failed to deploy same disks");
+        let _: DeployDisksDone =
+            deploy_disks(&opctx, &sleds_by_id, &blueprint.blueprint_disks)
+                .await
+                .expect("failed to deploy same disks");
         s1.verify_and_clear();
         s2.verify_and_clear();
 
@@ -567,7 +584,15 @@ mod test {
         assert_eq!(d.disk_state, PhysicalDiskState::Active);
         assert_eq!(d.disk_policy, PhysicalDiskPolicy::InService);
 
-        super::decommission_expunged_disks(&opctx, &datastore).await.unwrap();
+        super::decommission_expunged_disks(
+            &opctx,
+            &datastore,
+            // This is an internal test, and we're testing decommissioning in
+            // isolation, so it's okay to create the typestate here.
+            DeployDisksDone {},
+        )
+        .await
+        .unwrap();
 
         // After decommissioning, we see the expunged disk become
         // decommissioned. The other disk remains in-service.