diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 23185f4bcef..661e3780c98 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -2611,8 +2611,6 @@ 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, @@ -2620,12 +2618,6 @@ fn print_task_support_bundle_collector(details: &serde_json::Value) { { 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")] diff --git a/nexus/src/app/background/tasks/support_bundle_collector.rs b/nexus/src/app/background/tasks/support_bundle_collector.rs index bbad16aa165..32591c84c2c 100644 --- a/nexus/src/app/background/tasks/support_bundle_collector.rs +++ b/nexus/src/app/background/tasks/support_bundle_collector.rs @@ -712,15 +712,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 @@ -747,14 +738,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 }, + // The step spawned additional steps to execute Spawn { extra_steps: Vec }, // The step completed with nothing to report, and no follow-up steps None, @@ -1232,7 +1216,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 @@ -1268,25 +1252,25 @@ impl BundleCollection { let steps: Vec = 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(); @@ -1305,7 +1289,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| { @@ -1319,7 +1303,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, _| { @@ -1369,7 +1353,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 @@ -2509,8 +2493,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, @@ -2591,8 +2583,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 @@ -2678,8 +2678,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: @@ -2701,8 +2709,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 @@ -2827,8 +2843,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 diff --git a/nexus/tests/integration_tests/support_bundles.rs b/nexus/tests/integration_tests/support_bundles.rs index ade2cbdb2c9..80ec8af191f 100644 --- a/nexus/tests/integration_tests/support_bundles.rs +++ b/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); diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 42264d0411b..dfe008198f9 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/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,