[support bundle] Simplify report, relying on new 'steps' infrastructure

Author: smklein

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

@@ -2610,21 +2610,13 @@ fn print_task_support_bundle_collector(details: &serde_json::Value) {
 
             if let Some(SupportBundleCollectionReport {
                 bundle,
-                listed_in_service_sleds,
-                listed_sps,
                 activated_in_db_ok,
                 mut steps,
                 ereports,
             }) = collection_report
             {
                 println!("    Support Bundle Collection Report:");
                 println!("      Bundle ID: {bundle}");
-                println!(
-                    "      Bundle was able to list in-service sleds: {listed_in_service_sleds}"
-                );
-                println!(
-                    "      Bundle was able to list service processors: {listed_sps}"
-                );
 
                 #[derive(Tabled)]
                 #[tabled(rename_all = "SCREAMING_SNAKE_CASE")]

nexus/src/app/background/tasks/support_bundle_collector.rs

@@ -629,15 +629,6 @@ impl CompletedCollectionStep {
                 report.ereports = Some(status);
                 Status::Ok
             }
-            CollectionStepOutput::SavingSpDumps { listed_sps } => {
-                report.listed_sps = listed_sps;
-                Status::Ok
-            }
-            CollectionStepOutput::SpawnSleds { extra_steps } => {
-                report.listed_in_service_sleds = true;
-                steps.extend(extra_steps);
-                Status::Ok
-            }
             CollectionStepOutput::Spawn { extra_steps } => {
                 steps.extend(extra_steps);
                 Status::Ok
@@ -664,14 +655,7 @@ enum CollectionStepOutput {
     // It may have still saved a partial set of data to the bundle.
     Failed(anyhow::Error),
     Ereports(SupportBundleEreportStatus),
-    SavingSpDumps { listed_sps: bool },
-    // NOTE: The distinction between this and "Spawn" is pretty artificial -
-    // it's just to preserve a part of the report which says "we tried to
-    // list in-service sleds".
-    //
-    // If we changed the collection report, this could easily be combined
-    // with the "Spawn" variant.
-    SpawnSleds { extra_steps: Vec<CollectionStep> },
+    // The step spawned additional steps to execute
     Spawn { extra_steps: Vec<CollectionStep> },
     // The step completed with nothing to report, and no follow-up steps
     None,
@@ -1149,7 +1133,7 @@ impl BundleCollection {
             format!("failed to save SP dump from: {} {}", sp.type_, sp.slot)
         })?;
 
-        Ok(CollectionStepOutput::SavingSpDumps { listed_sps: true })
+        Ok(CollectionStepOutput::None)
     }
 
     // Perform the work of collecting the support bundle into a temporary directory
@@ -1185,25 +1169,25 @@ impl BundleCollection {
 
         let steps: Vec<CollectionStep> = vec![
             CollectionStep::new(
-                "bundle id",
+                SupportBundleCollectionStep::STEP_BUNDLE_ID,
                 Box::new(|collection, dir| {
                     collection.collect_bundle_id(dir).boxed()
                 }),
             ),
             CollectionStep::new(
-                "reconfigurator state",
+                SupportBundleCollectionStep::STEP_RECONFIGURATOR_STATE,
                 Box::new(|collection, dir| {
                     collection.collect_reconfigurator_state(dir).boxed()
                 }),
             ),
             CollectionStep::new(
-                "ereports",
+                SupportBundleCollectionStep::STEP_EREPORTS,
                 Box::new(|collection, dir| {
                     collection.collect_ereports(dir).boxed()
                 }),
             ),
             CollectionStep::new(
-                "sled cubby info",
+                SupportBundleCollectionStep::STEP_SLED_CUBBY_INFO,
                 Box::new({
                     let all_sleds = all_sleds.clone();
                     let mgs_client = mgs_client.clone();
@@ -1222,7 +1206,7 @@ impl BundleCollection {
                 }),
             ),
             CollectionStep::new(
-                "spawn steps to query all SP dumps",
+                SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS,
                 Box::new({
                     let mgs_client = mgs_client.clone();
                     move |collection, dir| {
@@ -1236,7 +1220,7 @@ impl BundleCollection {
                 }),
             ),
             CollectionStep::new(
-                "spawn steps to query all sleds",
+                SupportBundleCollectionStep::STEP_SPAWN_SLEDS,
                 Box::new({
                     let all_sleds = all_sleds.clone();
                     move |collection, _| {
@@ -1286,7 +1270,7 @@ impl BundleCollection {
             ));
         }
 
-        return Ok(CollectionStepOutput::SpawnSleds { extra_steps });
+        return Ok(CollectionStepOutput::Spawn { extra_steps });
     }
 
     // Collect data from a sled, storing it into a directory that will
@@ -2425,8 +2409,16 @@ mod test {
             .expect("Collection should have succeeded under test")
             .expect("Collecting the bundle should have generated a report");
         assert_eq!(report.bundle, bundle.id.into());
-        assert!(report.listed_in_service_sleds);
-        assert!(report.listed_sps);
+        // Verify that we spawned steps to query sleds and SPs
+        let step_names: Vec<_> =
+            report.steps.iter().map(|s| s.name.as_str()).collect();
+        assert!(
+            step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS)
+        );
+        assert!(
+            step_names
+                .contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS)
+        );
         assert!(report.activated_in_db_ok);
         assert_eq!(
             report.ereports,
@@ -2502,8 +2494,16 @@ mod test {
             .expect("Collection should have succeeded under test")
             .expect("Collecting the bundle should have generated a report");
         assert_eq!(report.bundle, bundle.id.into());
-        assert!(report.listed_in_service_sleds);
-        assert!(report.listed_sps);
+        // Verify that we spawned steps to query sleds and SPs
+        let step_names: Vec<_> =
+            report.steps.iter().map(|s| s.name.as_str()).collect();
+        assert!(
+            step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS)
+        );
+        assert!(
+            step_names
+                .contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS)
+        );
         assert!(report.activated_in_db_ok);
 
         let observed_bundle = datastore
@@ -2591,8 +2591,16 @@ mod test {
             .expect("Collection should have succeeded under test")
             .expect("Collecting the bundle should have generated a report");
         assert_eq!(report.bundle, bundle1.id.into());
-        assert!(report.listed_in_service_sleds);
-        assert!(report.listed_sps);
+        // Verify that we spawned steps to query sleds and SPs
+        let step_names: Vec<_> =
+            report.steps.iter().map(|s| s.name.as_str()).collect();
+        assert!(
+            step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS)
+        );
+        assert!(
+            step_names
+                .contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS)
+        );
         assert!(report.activated_in_db_ok);
 
         // This is observable by checking the state of bundle1 and bundle2:
@@ -2614,8 +2622,16 @@ mod test {
             .expect("Collection should have succeeded under test")
             .expect("Collecting the bundle should have generated a report");
         assert_eq!(report.bundle, bundle2.id.into());
-        assert!(report.listed_in_service_sleds);
-        assert!(report.listed_sps);
+        // Verify that we spawned steps to query sleds and SPs
+        let step_names: Vec<_> =
+            report.steps.iter().map(|s| s.name.as_str()).collect();
+        assert!(
+            step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS)
+        );
+        assert!(
+            step_names
+                .contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS)
+        );
         assert!(report.activated_in_db_ok);
 
         // After another collection request, we'll see that both bundles have
@@ -2742,8 +2758,16 @@ mod test {
             .expect("Collection should have succeeded under test")
             .expect("Collecting the bundle should have generated a report");
         assert_eq!(report.bundle, bundle.id.into());
-        assert!(report.listed_in_service_sleds);
-        assert!(report.listed_sps);
+        // Verify that we spawned steps to query sleds and SPs
+        let step_names: Vec<_> =
+            report.steps.iter().map(|s| s.name.as_str()).collect();
+        assert!(
+            step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS)
+        );
+        assert!(
+            step_names
+                .contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS)
+        );
         assert!(report.activated_in_db_ok);
 
         // Cancel the bundle after collection has completed

nexus/tests/integration_tests/support_bundles.rs

@@ -19,6 +19,7 @@ use nexus_types::external_api::shared::SupportBundleInfo;
 use nexus_types::external_api::shared::SupportBundleState;
 use nexus_types::internal_api::background::SupportBundleCleanupReport;
 use nexus_types::internal_api::background::SupportBundleCollectionReport;
+use nexus_types::internal_api::background::SupportBundleCollectionStep;
 use nexus_types::internal_api::background::SupportBundleEreportStatus;
 use omicron_uuid_kinds::SupportBundleUuid;
 use serde::Deserialize;
@@ -489,8 +490,6 @@ async fn test_support_bundle_lifecycle(cptestctx: &ControlPlaneTestContext) {
 
     let report = output.collection_report.as_ref().expect("Missing report");
     assert_eq!(report.bundle, bundle.id);
-    assert!(report.listed_in_service_sleds);
-    assert!(report.listed_sps);
     assert!(report.activated_in_db_ok);
     assert_eq!(
         report.ereports,
@@ -511,6 +510,18 @@ async fn test_support_bundle_lifecycle(cptestctx: &ControlPlaneTestContext) {
         );
     }
 
+    // Verify that we successfully spawned steps to query sleds and SPs
+    let step_names: Vec<_> =
+        report.steps.iter().map(|s| s.name.as_str()).collect();
+    assert!(
+        step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS),
+        "Should have attempted to list in-service sleds"
+    );
+    assert!(
+        step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS),
+        "Should have attempted to list service processors"
+    );
+
     let bundle = bundle_get(&client, bundle.id).await.unwrap();
     assert_eq!(bundle.state, SupportBundleState::Active);
 
@@ -601,8 +612,6 @@ async fn test_support_bundle_range_requests(
     assert_eq!(output.collection_err, None);
     let report = output.collection_report.as_ref().expect("Missing report");
     assert_eq!(report.bundle, bundle.id);
-    assert!(report.listed_in_service_sleds);
-    assert!(report.listed_sps);
     assert!(report.activated_in_db_ok);
     assert_eq!(
         report.ereports,
@@ -623,6 +632,18 @@ async fn test_support_bundle_range_requests(
         );
     }
 
+    // Verify that we successfully spawned steps to query sleds and SPs
+    let step_names: Vec<_> =
+        report.steps.iter().map(|s| s.name.as_str()).collect();
+    assert!(
+        step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SLEDS),
+        "Should have attempted to list in-service sleds"
+    );
+    assert!(
+        step_names.contains(&SupportBundleCollectionStep::STEP_SPAWN_SP_DUMPS),
+        "Should have attempted to list service processors"
+    );
+
     let bundle = bundle_get(&client, bundle.id).await.unwrap();
     assert_eq!(bundle.state, SupportBundleState::Active);
 

nexus/types/src/internal_api/background.rs

@@ -272,12 +272,6 @@ pub struct SupportBundleCleanupReport {
 pub struct SupportBundleCollectionReport {
     pub bundle: SupportBundleUuid,
 
-    /// True iff we could list in-service sleds
-    pub listed_in_service_sleds: bool,
-
-    /// True iff we could list the service processors.
-    pub listed_sps: bool,
-
     /// True iff the bundle was successfully made 'active' in the database.
     pub activated_in_db_ok: bool,
 
@@ -298,6 +292,19 @@ pub struct SupportBundleCollectionStep {
     pub status: SupportBundleCollectionStepStatus,
 }
 
+impl SupportBundleCollectionStep {
+    /// Step name constants for the main collection steps.
+    ///
+    /// These are used both when creating steps and when validating in tests.
+    pub const STEP_BUNDLE_ID: &'static str = "bundle id";
+    pub const STEP_RECONFIGURATOR_STATE: &'static str = "reconfigurator state";
+    pub const STEP_EREPORTS: &'static str = "ereports";
+    pub const STEP_SLED_CUBBY_INFO: &'static str = "sled cubby info";
+    pub const STEP_SPAWN_SP_DUMPS: &'static str =
+        "spawn steps to query all SP dumps";
+    pub const STEP_SPAWN_SLEDS: &'static str = "spawn steps to query all sleds";
+}
+
 #[derive(Debug, Deserialize, Serialize, PartialEq, Eq)]
 pub enum SupportBundleCollectionStepStatus {
     Ok,
@@ -336,8 +343,6 @@ impl SupportBundleCollectionReport {
     pub fn new(bundle: SupportBundleUuid) -> Self {
         Self {
             bundle,
-            listed_in_service_sleds: false,
-            listed_sps: false,
             activated_in_db_ok: false,
             steps: vec![],
             ereports: None,