From 6e094f45e186745facb7d2f61ec04faf05e9aa8c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 2 Jun 2026 16:41:45 -0700 Subject: [PATCH 01/20] [fm] Add disk diagnoser with typed fact tables --- dev-tools/omdb/tests/successes.out | 50 +- dev-tools/omdb/tests/test_all_output.rs | 15 + nexus/db-model/src/fm.rs | 2 + nexus/db-model/src/fm/case.rs | 1 + nexus/db-model/src/fm/diagnosis_engine.rs | 7 + nexus/db-model/src/fm/fact_physical_disk.rs | 152 ++++ nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-queries/src/db/datastore/fm.rs | 251 ++++-- nexus/db-schema/src/enums.rs | 1 + nexus/db-schema/src/schema.rs | 18 + nexus/fm/src/analysis_input.rs | 33 +- nexus/fm/src/builder/case.rs | 59 ++ nexus/fm/src/builder/rng.rs | 18 +- .../fm/src/{diagnosis.rs => diagnosis/mod.rs} | 24 +- nexus/fm/src/diagnosis/physical_disk.rs | 810 ++++++++++++++++++ nexus/src/app/background/tasks/fm_analysis.rs | 48 +- .../src/app/background/tasks/fm_rendezvous.rs | 7 + .../app/background/tasks/fm_sitrep_load.rs | 4 +- nexus/src/app/mod.rs | 9 +- nexus/src/lib.rs | 6 + nexus/test-interface/src/lib.rs | 4 + nexus/test-utils/src/nexus_test.rs | 32 + .../output/analysis_input_report_empty.out | 2 + .../output/analysis_input_report_same_inv.out | 2 + .../analysis_input_report_with_cases.out | 2 + nexus/types/src/fm.rs | 7 + nexus/types/src/fm/analysis_reports.rs | 26 +- nexus/types/src/fm/case.rs | 134 ++- nexus/types/src/fm/fact.rs | 77 ++ nexus/types/src/in_service_disk.rs | 41 + nexus/types/src/inventory.rs | 2 +- nexus/types/src/lib.rs | 1 + schema/crdb/dbinit.sql | 62 +- schema/crdb/fm-disk-de-and-facts/up1.sql | 1 + schema/crdb/fm-disk-de-and-facts/up2.sql | 3 + schema/crdb/fm-disk-de-and-facts/up3.sql | 44 + schema/crdb/fm-disk-de-and-facts/up4.sql | 1 + .../crdb/fm-disk-de-and-facts/up4.verify.sql | 2 + test-utils/src/dev/test_cmds.rs | 13 + uuid-kinds/src/lib.rs | 1 + 40 files changed, 1837 insertions(+), 138 deletions(-) create mode 100644 nexus/db-model/src/fm/fact_physical_disk.rs rename nexus/fm/src/{diagnosis.rs => diagnosis/mod.rs} (64%) create mode 100644 nexus/fm/src/diagnosis/physical_disk.rs create mode 100644 nexus/types/src/fm/fact.rs create mode 100644 nexus/types/src/in_service_disk.rs create mode 100644 schema/crdb/fm-disk-de-and-facts/up1.sql create mode 100644 schema/crdb/fm-disk-de-and-facts/up2.sql create mode 100644 schema/crdb/fm-disk-de-and-facts/up3.sql create mode 100644 schema/crdb/fm-disk-de-and-facts/up4.sql create mode 100644 schema/crdb/fm-disk-de-and-facts/up4.verify.sql diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index d6d6f89b40b..4aec2bb7e41 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -700,20 +700,23 @@ task: "fm_analysis" configured period: every m last completed activation: , triggered by started at (s ago) and ran for ms - parent sitrep ID: None + parent sitrep ID: Some(..................... (sitrep)) current inventory collection ID: Some(..................... (collection)) ereport classes consumed: (none) FAULT MANAGEMENT ANALYSIS SUMMARY ================================= -/!\ analysis failed: FM analysis is not yet implemented + no changes from the current situation report (Some(..................... (sitrep))) fault management analysis inputs -------------------------------- - parent sitrep: + parent sitrep: ..................... inventory collection: ..................... + no new ereports since the parent sitrep no cases copied forward + no in-service control plane disks + fault management analysis report -------------------------------- sitrep ID: ..................... @@ -724,28 +727,28 @@ task: "fm_rendezvous" configured period: every m last completed activation: , triggered by started at (s ago) and ran for ms -(i) no FM situation report loaded, so rendezvous was not performed + current sitrep: ..................... creating requested alerts: -(i) note: this operation was not executed + started at (s ago) and ran for ms alerts requested: 0 requested in this sitrep: 0 created in this activation: 0 already created: 0 errors: 0 creating requested support bundles: -(i) note: this operation was not executed + started at (s ago) and ran for ms support bundles requested: 0 requested in this sitrep: 0 created in this activation: 0 already created: 0 errors: 0 marking ereports as seen: -(i) note: this operation was not executed + started at (s ago) and ran for ms total ereports in sitrep: 0 not marked when the sitrep was loaded: 0 marked seen by this activation: 0 already marked seen: 0 - batch size: 0 + batch size: 1000 batches: 0 errors: 0 @@ -762,6 +765,8 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_ereport_in_case rows deleted: 0 batches: 1 + orphaned fm_fact_physical_disk rows deleted: 0 + batches: 1 orphaned fm_support_bundle_request rows deleted: 0 batches: 1 orphaned fm_support_bundle_request_data_selection_ereports rows deleted: 0 @@ -775,7 +780,9 @@ task: "fm_sitrep_loader" configured period: every s last completed activation: , triggered by started at (s ago) and ran for ms - no FM situation report available to load + loaded latest FM situation report as of : + sitrep ..................... (sitrep) (v1) + made current at: task: "instance_reincarnation" configured period: every m @@ -1391,20 +1398,23 @@ task: "fm_analysis" configured period: every m last completed activation: , triggered by started at (s ago) and ran for ms - parent sitrep ID: None + parent sitrep ID: Some(..................... (sitrep)) current inventory collection ID: Some(..................... (collection)) ereport classes consumed: (none) FAULT MANAGEMENT ANALYSIS SUMMARY ================================= -/!\ analysis failed: FM analysis is not yet implemented + no changes from the current situation report (Some(..................... (sitrep))) fault management analysis inputs -------------------------------- - parent sitrep: + parent sitrep: ..................... inventory collection: ..................... + no new ereports since the parent sitrep no cases copied forward + no in-service control plane disks + fault management analysis report -------------------------------- sitrep ID: ..................... @@ -1415,28 +1425,28 @@ task: "fm_rendezvous" configured period: every m last completed activation: , triggered by started at (s ago) and ran for ms -(i) no FM situation report loaded, so rendezvous was not performed + current sitrep: ..................... creating requested alerts: -(i) note: this operation was not executed + started at (s ago) and ran for ms alerts requested: 0 requested in this sitrep: 0 created in this activation: 0 already created: 0 errors: 0 creating requested support bundles: -(i) note: this operation was not executed + started at (s ago) and ran for ms support bundles requested: 0 requested in this sitrep: 0 created in this activation: 0 already created: 0 errors: 0 marking ereports as seen: -(i) note: this operation was not executed + started at (s ago) and ran for ms total ereports in sitrep: 0 not marked when the sitrep was loaded: 0 marked seen by this activation: 0 already marked seen: 0 - batch size: 0 + batch size: 1000 batches: 0 errors: 0 @@ -1453,6 +1463,8 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_ereport_in_case rows deleted: 0 batches: 1 + orphaned fm_fact_physical_disk rows deleted: 0 + batches: 1 orphaned fm_support_bundle_request rows deleted: 0 batches: 1 orphaned fm_support_bundle_request_data_selection_ereports rows deleted: 0 @@ -1466,7 +1478,9 @@ task: "fm_sitrep_loader" configured period: every s last completed activation: , triggered by started at (s ago) and ran for ms - no FM situation report available to load + loaded latest FM situation report as of : + sitrep ..................... (sitrep) (v1) + made current at: task: "instance_reincarnation" configured period: every m diff --git a/dev-tools/omdb/tests/test_all_output.rs b/dev-tools/omdb/tests/test_all_output.rs index bd124caae13..42bde1b627f 100644 --- a/dev-tools/omdb/tests/test_all_output.rs +++ b/dev-tools/omdb/tests/test_all_output.rs @@ -187,6 +187,12 @@ async fn test_omdb_success_cases() { .wait_for_at_least_one_inventory_collection(Duration::from_secs(60)) .await; + // Wait until `fm_analysis` has committed at least one sitrep, so that the + // omdb snapshot for FM tasks is stable. (Otherwise sitrep IDs render as + // `None` or `Some(...)` depending on whether the task's natural cadence + // had landed by the time we sample it.) + cptestctx.wait_for_at_least_one_sitrep(Duration::from_secs(60)).await; + let mut output = String::new(); let invocations: &[&[&str]] = &[ @@ -350,6 +356,15 @@ async fn test_omdb_success_cases() { .field("triggered by", r"[\w ]+") .section(&["task: \"tuf_artifact_replication\"", "request ringbuf:"]); + // The `fm_analysis` task's input report includes a line comparing the + // current inventory collection against the parent sitrep's collection, + // which can be either "same" or "different" depending on whether a new + // inventory was collected between sitreps. Collapse both forms. + redactor.variable_regex( + "fm_input_inv_comparison", + r" --> (same collection as parent sitrep|different from parent sitrep \(collection [-a-f0-9]+\))", + ); + // The `sp_ereport_ingester` task's output depends on how many simulated // sled agents ahppen to register with Nexus before its first execution. // These redactions work around the issue described in diff --git a/nexus/db-model/src/fm.rs b/nexus/db-model/src/fm.rs index 2f2d1693d8a..0d2f4cf85f6 100644 --- a/nexus/db-model/src/fm.rs +++ b/nexus/db-model/src/fm.rs @@ -26,6 +26,8 @@ mod case; pub use case::*; mod diagnosis_engine; pub use diagnosis_engine::*; +mod fact_physical_disk; +pub use fact_physical_disk::*; mod support_bundle_request; pub use support_bundle_request::*; diff --git a/nexus/db-model/src/fm/case.rs b/nexus/db-model/src/fm/case.rs index 04776f99539..d2e34cf06ed 100644 --- a/nexus/db-model/src/fm/case.rs +++ b/nexus/db-model/src/fm/case.rs @@ -64,6 +64,7 @@ impl CaseMetadata { alerts_requested: _, support_bundles_requested: _, ereports: _, + facts: _, } = case; Self { sitrep_id: sitrep_id.into(), diff --git a/nexus/db-model/src/fm/diagnosis_engine.rs b/nexus/db-model/src/fm/diagnosis_engine.rs index 7d354142bbb..a99ceeac1ad 100644 --- a/nexus/db-model/src/fm/diagnosis_engine.rs +++ b/nexus/db-model/src/fm/diagnosis_engine.rs @@ -24,6 +24,7 @@ impl_enum_type!( pub enum DiagnosisEngine; PowerShelf => b"power_shelf" + PhysicalDisk => b"physical_disk" ); @@ -31,6 +32,9 @@ impl From for fm::DiagnosisEngineKind { fn from(de: DiagnosisEngine) -> Self { match de { DiagnosisEngine::PowerShelf => fm::DiagnosisEngineKind::PowerShelf, + DiagnosisEngine::PhysicalDisk => { + fm::DiagnosisEngineKind::PhysicalDisk + } } } } @@ -39,6 +43,9 @@ impl From for DiagnosisEngine { fn from(fm_de: fm::DiagnosisEngineKind) -> Self { match fm_de { fm::DiagnosisEngineKind::PowerShelf => DiagnosisEngine::PowerShelf, + fm::DiagnosisEngineKind::PhysicalDisk => { + DiagnosisEngine::PhysicalDisk + } } } } diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs new file mode 100644 index 00000000000..b6510590bff --- /dev/null +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -0,0 +1,152 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Database representation of the physical-disk diagnosis engine's facts. +//! +//! Each physical-disk fact is stored as typed columns in the +//! `fm_fact_physical_disk` table. The `kind` discriminant selects which payload +//! columns are populated; a CHECK constraint +//! (`zpool_unhealthy_columns_present`) enforces that the right columns are +//! non-NULL for each kind. See [`nexus_types::fm::DiskFact`] for semantics. + +use crate::DbTypedUuid; +use crate::impl_enum_type; +use crate::inventory::InvZpoolHealth; +use chrono::{DateTime, Utc}; +use nexus_db_schema::schema::fm_fact_physical_disk; +use nexus_types::fm; +use nexus_types::fm::{DiskFact, FactPayload, ZpoolUnhealthyFactPayload}; +use omicron_common::api::external::Error; +use omicron_uuid_kinds::{ + CaseKind, CollectionKind, FactKind, PhysicalDiskKind, SitrepKind, ZpoolKind, +}; + +impl_enum_type!( + FmFactPhysicalDiskKindEnum: + + #[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, PartialEq, Eq)] + pub enum FmFactPhysicalDiskKind; + + ZpoolUnhealthy => b"zpool_unhealthy" +); + +/// Diesel row for the `fm_fact_physical_disk` table. +/// +/// The payload columns are populated according to `kind`: a column is `Some` +/// iff it belongs to that `kind`'s payload. This is enforced in the database +/// by the per-kind CHECK constraints (e.g. `zpool_unhealthy_columns_present`). +#[derive(Queryable, Insertable, Clone, Debug, Selectable)] +#[diesel(table_name = fm_fact_physical_disk)] +pub struct FmFactPhysicalDisk { + pub id: DbTypedUuid, + /// The sitrep to which this fact belongs. + /// + /// This will change as the fact is carried forward from one sitrep to the + /// next. + pub sitrep_id: DbTypedUuid, + pub case_id: DbTypedUuid, + /// Sitrep in which this fact was first added. + /// + /// Preserved unchanged when the fact is carried forward; debug-only. + pub created_sitrep_id: DbTypedUuid, + pub comment: String, + pub kind: FmFactPhysicalDiskKind, + + // Columns for the `ZpoolUnhealthy` kind. + pub physical_disk_id: Option>, + pub zpool_id: Option>, + pub last_seen_health: Option, + pub observed_in_inv: Option>, + pub time_observed: Option>, +} + +impl FmFactPhysicalDisk { + /// Build a row from a fact's shared metadata (`fact`) and its + /// already-dispatched physical-disk payload (`disk_fact`). + /// + /// Callers route each fact to its engine's table by matching on + /// [`fact.payload`](fm::case::Fact::payload) and pass the matched payload + /// here, so this never has to interpret another engine's payload. + pub fn from_sitrep( + sitrep_id: impl Into>, + case_id: impl Into>, + fact: &fm::case::Fact, + disk_fact: &DiskFact, + ) -> Self { + let base = Self { + id: fact.id.into(), + sitrep_id: sitrep_id.into(), + case_id: case_id.into(), + created_sitrep_id: fact.created_sitrep_id.into(), + comment: fact.comment.clone(), + kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, + physical_disk_id: None, + zpool_id: None, + last_seen_health: None, + observed_in_inv: None, + time_observed: None, + }; + match disk_fact { + DiskFact::ZpoolUnhealthy(p) => Self { + kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, + physical_disk_id: Some(p.physical_disk_id.into()), + zpool_id: Some(p.zpool_id.into()), + last_seen_health: Some(p.last_seen_health.into()), + observed_in_inv: Some(p.observed_in_inv.into()), + time_observed: Some(p.time_observed), + ..base + }, + } + } + + /// Reconstruct an in-memory fact from a row. + /// + /// The payload columns the database's CHECK constraint guarantees are + /// non-NULL for this `kind` are unwrapped; a NULL where one is required + /// indicates a corrupt row (e.g. hand-edited) and yields an internal + /// error rather than a panic. + pub fn into_fact(self) -> Result { + let payload = match self.kind { + FmFactPhysicalDiskKind::ZpoolUnhealthy => { + FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( + ZpoolUnhealthyFactPayload { + physical_disk_id: self + .physical_disk_id + .ok_or_else(|| missing_column("physical_disk_id"))? + .into(), + zpool_id: self + .zpool_id + .ok_or_else(|| missing_column("zpool_id"))? + .into(), + last_seen_health: self + .last_seen_health + .ok_or_else(|| missing_column("last_seen_health"))? + .into(), + observed_in_inv: self + .observed_in_inv + .ok_or_else(|| missing_column("observed_in_inv"))? + .into(), + time_observed: self + .time_observed + .ok_or_else(|| missing_column("time_observed"))?, + }, + )) + } + }; + Ok(fm::case::Fact { + id: self.id.into(), + created_sitrep_id: self.created_sitrep_id.into(), + payload, + comment: self.comment, + }) + } +} + +fn missing_column(column: &str) -> Error { + Error::internal_error(&format!( + "fm_fact_physical_disk row of kind 'zpool_unhealthy' has a NULL \ + {column} (violates the zpool_unhealthy_columns_present CHECK \ + constraint)" + )) +} diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 5caf71e9e28..f14fbb76b0d 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(261, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(262, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(262, "fm-disk-de-and-facts"), KnownVersion::new(261, "remove-add-zones-with-mupdate-override"), KnownVersion::new(260, "ereport-trim-serial-trailing-nulls"), KnownVersion::new(259, "vmm-failure-reason"), diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 1552d463dfb..f16e1608c2d 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -34,6 +34,7 @@ use nexus_db_schema::schema::ereport::dsl as ereport_dsl; use nexus_db_schema::schema::fm_alert_request::dsl as alert_req_dsl; use nexus_db_schema::schema::fm_case::dsl as case_dsl; use nexus_db_schema::schema::fm_ereport_in_case::dsl as case_ereport_dsl; +use nexus_db_schema::schema::fm_fact_physical_disk::dsl as fact_pd_dsl; use nexus_db_schema::schema::fm_sitrep::dsl as sitrep_dsl; use nexus_db_schema::schema::fm_sitrep_history::dsl as history_dsl; use nexus_db_schema::schema::fm_support_bundle_request::dsl as support_bundle_req_dsl; @@ -48,6 +49,7 @@ use omicron_uuid_kinds::AlertUuid; use omicron_uuid_kinds::CaseEreportKind; use omicron_uuid_kinds::CaseKind; use omicron_uuid_kinds::CaseUuid; +use omicron_uuid_kinds::FactKind; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::SitrepUuid; use omicron_uuid_kinds::SupportBundleKind; @@ -121,6 +123,7 @@ sitrep_child_tables! { SupportBundleRequestDataSelectionEreports => { table: "fm_support_bundle_request_data_selection_ereports" }, SupportBundleRequest => { table: "fm_support_bundle_request" }, Case => { table: "fm_case" }, + FmFactPhysicalDisk => { table: "fm_fact_physical_disk" }, } /// Per-child-table statistics from a single GC pass. @@ -366,6 +369,8 @@ impl DataStore { let mut support_bundle_requests = self.support_bundle_requests_read_on_conn(id, conn).await?; + let mut case_facts = self.fm_facts_read_on_conn(id, conn).await?; + // Next, load the case metadata entries and marry them to the sets of // ereports, alert requests, and support bundle requests for those // cases that we loaded in the previous steps. @@ -408,6 +413,7 @@ impl DataStore { alert_requests.remove(&id).unwrap_or_default(); let support_bundles_requested = support_bundle_requests.remove(&id).unwrap_or_default(); + let facts = case_facts.remove(&id).unwrap_or_default(); fm::Case { id, metadata: fm::case::Metadata { @@ -419,6 +425,7 @@ impl DataStore { alerts_requested, ereports, support_bundles_requested, + facts, } })); } @@ -497,6 +504,69 @@ impl DataStore { Ok(by_case) } + /// Fetch all case facts belonging to cases in the given sitrep, grouped + /// by `case_id`. + /// + /// Each diagnosis engine stores its facts in its own typed table, read by + /// its own paginated loop here and unioned into the same `by_case` map. A + /// case belongs to exactly one engine, so keys never collide across tables. + async fn fm_facts_read_on_conn( + &self, + id: SitrepUuid, + conn: &async_bb8_diesel::Connection, + ) -> Result>, Error> { + let mut by_case = + HashMap::>::new(); + + // NOTE: Each per-DE set of facts belongs to a distinct table. + // This would be a reasonable spot to insert parallel_task_set + // and do some concurrent queries. + // + // (... gonna punt on that until we actually have a couple different + // tables to query from). + + // --- physical-disk diagnosis engine facts --- + let mut paginator: Paginator> = + Paginator::new(SQL_BATCH_SIZE, PaginationOrder::Descending); + while let Some(p) = paginator.next() { + let batch = paginated( + fact_pd_dsl::fm_fact_physical_disk, + fact_pd_dsl::id, + &p.current_pagparams(), + ) + .filter(fact_pd_dsl::sitrep_id.eq(id.into_untyped_uuid())) + .select(model::fm::FmFactPhysicalDisk::as_select()) + .load_async(conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + .internal_context("failed to load physical-disk case facts") + })?; + + paginator = p.found_batch(&batch, &|f| f.id); + for row in batch { + let case_id: CaseUuid = row.case_id.into(); + let fact = row.into_fact()?; + let id = fact.id; + by_case + .entry(case_id) + .or_default() + .insert_unique(fact) + .map_err(|_| { + let internal_message = format!( + "encountered multiple case facts for case \ + {case_id} with the same fact UUID {id}. this \ + should really not be possible, as the fact \ + UUID is a primary key!", + ); + Error::InternalError { internal_message } + })?; + } + } + + Ok(by_case) + } + /// Fetch all support bundle requests belonging to cases in the given /// sitrep, including their child data selection tables. async fn support_bundle_requests_read_on_conn( @@ -740,6 +810,7 @@ impl DataStore { let mut support_bundles_requested = Vec::new(); let mut bundle_data_selections_requested = Vec::new(); let mut case_ereports = Vec::new(); + let mut physical_disk_facts = Vec::new(); for case in sitrep.cases { let case_id = case.id; cases.push(model::fm::CaseMetadata::from_sitrep(sitrep_id, &case)); @@ -763,6 +834,18 @@ impl DataStore { ); bundle_data_selections_requested.push((req_id, data_selection)); } + for fact in case.facts.iter() { + // Dispatch each fact to its diagnosis engine's typed table. + match &fact.payload { + fm::FactPayload::PhysicalDisk(disk_fact) => { + physical_disk_facts.push( + model::fm::FmFactPhysicalDisk::from_sitrep( + sitrep_id, case_id, fact, disk_fact, + ), + ); + } + } + } } if !case_ereports.is_empty() { @@ -797,6 +880,19 @@ impl DataStore { ) .await?; + if !physical_disk_facts.is_empty() { + diesel::insert_into(fact_pd_dsl::fm_fact_physical_disk) + .values(physical_disk_facts) + .execute_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + .internal_context( + "failed to insert physical-disk case facts", + ) + })?; + } + if !cases.is_empty() { diesel::insert_into(case_dsl::fm_case) .values(cases) @@ -1132,22 +1228,8 @@ impl DataStore { .map(|id| id.into_untyped_uuid()) .collect::>(); - struct SitrepDeleteResult { - sitreps_deleted: usize, - case_ereports_deleted: usize, - alert_requests_deleted: usize, - support_bundle_requests_deleted: usize, - cases_deleted: usize, - } - let err = OptionalError::new(); - let SitrepDeleteResult { - sitreps_deleted, - case_ereports_deleted, - alert_requests_deleted, - support_bundle_requests_deleted, - cases_deleted, - } = self + let (sitreps_deleted, child_rows_deleted) = self // Sitrep deletion is transactional to prevent a sitrep from being // left in a partially-deleted state should the Nexus instance // attempting the delete operation die suddenly. @@ -1168,37 +1250,33 @@ impl DataStore { } } - // Delete case ereport assignments - let case_ereports_deleted = diesel::delete( - case_ereport_dsl::fm_ereport_in_case.filter( - case_ereport_dsl::sitrep_id.eq_any(ids.clone()), - ), - ) - .execute_async(&conn) - .await?; - - // Delete case alert requests. - let alert_requests_deleted = diesel::delete( - alert_req_dsl::fm_alert_request.filter(alert_req_dsl::sitrep_id.eq_any(ids.clone())) - ) - .execute_async(&conn) - .await?; - - // Delete support bundle request child data selection rows, - // then the requests themselves. - let support_bundle_requests_deleted = - Self::support_bundle_requests_delete_on_conn(&conn, ids.clone()) - .await?; - - // Delete case metadata records. - let cases_deleted = diesel::delete( - case_dsl::fm_case - .filter(case_dsl::sitrep_id.eq_any(ids.clone())), - ) - .execute_async(&conn) - .await?; + // Delete every child row by sitrep_id. Driving this off + // SitrepChildTable::ALL keeps it in lockstep with the + // orphan GC: a child table registered in the + // `sitrep_child_tables!` macro is deleted here + // automatically. There are no foreign keys between these + // tables, so deletion order doesn't matter. + let mut child_rows_deleted = + BTreeMap::::new(); + for &table in SitrepChildTable::ALL { + let mut builder = QueryBuilder::new(); + builder.sql("DELETE FROM omicron.public."); + builder.sql(table.table_name()); + builder.sql(" WHERE "); + builder.sql(table.sitrep_id_column()); + builder.sql(" = ANY("); + builder + .param() + .bind::, _>( + ids.clone(), + ); + builder.sql(")"); + let deleted = + builder.query::<()>().execute_async(&conn).await?; + child_rows_deleted.insert(table, deleted); + } - // Delete sitrep metadata records. + // Delete sitrep metadata records last. let sitreps_deleted = diesel::delete( sitrep_dsl::fm_sitrep .filter(sitrep_dsl::id.eq_any(ids.clone())), @@ -1206,13 +1284,7 @@ impl DataStore { .execute_async(&conn) .await?; - Ok(SitrepDeleteResult { - sitreps_deleted, - cases_deleted, - alert_requests_deleted, - support_bundle_requests_deleted, - case_ereports_deleted, - }) + Ok((sitreps_deleted, child_rows_deleted)) } }) .await @@ -1226,10 +1298,7 @@ impl DataStore { "deleted {sitreps_deleted} of {} sitreps", ids.len(); "ids" => ?ids, "sitreps_deleted" => sitreps_deleted, - "cases_deleted" => cases_deleted, - "case_ereports_deleted" => case_ereports_deleted, - "alert_requests_deleted" => alert_requests_deleted, - "support_bundle_requests_deleted" => support_bundle_requests_deleted, + "child_rows_deleted" => ?child_rows_deleted, ); Ok(sitreps_deleted) @@ -1474,43 +1543,6 @@ impl DataStore { builder.query() } - /// Delete child data selection rows for support bundle requests, then the - /// support bundle request rows themselves. - #[cfg(test)] - async fn support_bundle_requests_delete_on_conn( - conn: &async_bb8_diesel::Connection, - sitrep_ids: Vec, - ) -> Result { - use nexus_db_schema::schema::fm_support_bundle_request_data_selection_ereports::dsl as ereports_dsl; - use nexus_db_schema::schema::fm_support_bundle_request_data_selection_flags::dsl as flags_dsl; - use nexus_db_schema::schema::fm_support_bundle_request_data_selection_host_info::dsl as host_info_dsl; - - diesel::delete( - flags_dsl::fm_support_bundle_request_data_selection_flags - .filter(flags_dsl::sitrep_id.eq_any(sitrep_ids.clone())), - ) - .execute_async(conn) - .await?; - diesel::delete( - host_info_dsl::fm_support_bundle_request_data_selection_host_info - .filter(host_info_dsl::sitrep_id.eq_any(sitrep_ids.clone())), - ) - .execute_async(conn) - .await?; - diesel::delete( - ereports_dsl::fm_support_bundle_request_data_selection_ereports - .filter(ereports_dsl::sitrep_id.eq_any(sitrep_ids.clone())), - ) - .execute_async(conn) - .await?; - diesel::delete( - support_bundle_req_dsl::fm_support_bundle_request - .filter(support_bundle_req_dsl::sitrep_id.eq_any(sitrep_ids)), - ) - .execute_async(conn) - .await - } - pub async fn fm_sitrep_version_list( &self, opctx: &OpContext, @@ -1569,6 +1601,7 @@ mod tests { use nexus_types::fm::ereport::{EreportData, Reporter}; use omicron_test_utils::dev; use omicron_uuid_kinds::CollectionUuid; + use omicron_uuid_kinds::FactUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::SupportBundleUuid; use std::collections::BTreeMap; @@ -2037,6 +2070,7 @@ mod tests { ereports, alerts_requested, support_bundles_requested, + facts, } = case; let case_id = id; let Some(expected) = this.cases.get(&case_id) else { @@ -2068,6 +2102,7 @@ mod tests { &expected.metadata.de, de, "while checking case {case_id}" ); + assert_eq!(&expected.facts, facts, "while checking case {case_id}"); // Now, check that all the ereports are present in both cases. assert_eq!(ereports.len(), expected.ereports.len()); @@ -2241,17 +2276,43 @@ mod tests { .unwrap(); } + let mut facts = iddqd::IdOrdMap::new(); + facts + .insert_unique(fm::case::Fact { + id: FactUuid::new_v4(), + created_sitrep_id: sitrep_id, + payload: fm::FactPayload::PhysicalDisk( + fm::DiskFact::ZpoolUnhealthy( + fm::ZpoolUnhealthyFactPayload { + physical_disk_id: + omicron_uuid_kinds::PhysicalDiskUuid::new_v4( + ), + zpool_id: + omicron_uuid_kinds::ZpoolUuid::new_v4(), + last_seen_health: + nexus_types::inventory::ZpoolHealth::Degraded, + observed_in_inv: CollectionUuid::new_v4(), + time_observed: omicron_common::now_db_precision( + ), + }, + ), + ), + comment: "a representative fact for case 1".to_string(), + }) + .unwrap(); + fm::Case { id: omicron_uuid_kinds::CaseUuid::new_v4(), metadata: fm::case::Metadata { created_sitrep_id: sitrep_id, closed_sitrep_id: None, - de: fm::DiagnosisEngineKind::PowerShelf, + de: fm::DiagnosisEngineKind::PhysicalDisk, comment: "my cool case".to_string(), }, ereports, alerts_requested, support_bundles_requested, + facts, } }; @@ -2288,6 +2349,7 @@ mod tests { ereports, alerts_requested, support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), } }; @@ -2406,6 +2468,7 @@ mod tests { ereports: iddqd::IdOrdMap::new(), alerts_requested: iddqd::IdOrdMap::new(), support_bundles_requested, + facts: iddqd::IdOrdMap::new(), }; let mut cases = iddqd::IdOrdMap::new(); diff --git a/nexus/db-schema/src/enums.rs b/nexus/db-schema/src/enums.rs index cddaf8f4d36..88b7deb1b08 100644 --- a/nexus/db-schema/src/enums.rs +++ b/nexus/db-schema/src/enums.rs @@ -58,6 +58,7 @@ define_enums! { DownstairsClientStoppedReasonEnum => "downstairs_client_stopped_reason_type", EreporterTypeEnum => "ereporter_type", FailureDomainEnum => "failure_domain", + FmFactPhysicalDiskKindEnum => "fm_fact_physical_disk_kind", HwM2SlotEnum => "hw_m2_slot", HwPowerStateEnum => "hw_power_state", HwRotSlotEnum => "hw_rot_slot", diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 05d6c96b1db..b1275bfe138 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -3243,6 +3243,22 @@ table! { } } +table! { + fm_fact_physical_disk (sitrep_id, id) { + id -> Uuid, + sitrep_id -> Uuid, + case_id -> Uuid, + created_sitrep_id -> Uuid, + comment -> Text, + kind -> crate::enums::FmFactPhysicalDiskKindEnum, + physical_disk_id -> Nullable, + zpool_id -> Nullable, + last_seen_health -> Nullable, + observed_in_inv -> Nullable, + time_observed -> Nullable, + } +} + table! { fm_ereport_in_case (sitrep_id, id) { id -> Uuid, @@ -3258,6 +3274,8 @@ table! { allow_tables_to_appear_in_same_query!(fm_ereport_in_case, ereport); allow_tables_to_appear_in_same_query!(fm_sitrep, fm_case); +allow_tables_to_appear_in_same_query!(fm_sitrep, fm_fact_physical_disk); +allow_tables_to_appear_in_same_query!(fm_case, fm_fact_physical_disk); table! { fm_alert_request (sitrep_id, id) { diff --git a/nexus/fm/src/analysis_input.rs b/nexus/fm/src/analysis_input.rs index c9d1ae57428..2f8e135cbb2 100644 --- a/nexus/fm/src/analysis_input.rs +++ b/nexus/fm/src/analysis_input.rs @@ -8,6 +8,7 @@ use chrono::{DateTime, Utc}; use iddqd::IdOrdMap; use nexus_types::fm::analysis_reports::ClosedCaseReport; use nexus_types::fm::{self, Sitrep, SitrepVersion}; +use nexus_types::in_service_disk::InServiceDisk; use nexus_types::inventory; use omicron_uuid_kinds::CollectionUuid; use std::collections::BTreeMap; @@ -39,6 +40,8 @@ pub struct Input { new_ereports: IdOrdMap, open_cases: IdOrdMap, closed_cases_copied_forward: IdOrdMap, + /// All control plane managed disks + in_service_disks: Arc>, } impl Input { @@ -65,11 +68,19 @@ impl Input { &self.closed_cases_copied_forward } + /// All control-plane-managed disks (`physical_disk.disk_policy = + /// in_service` in the DB), indexed by `physical_disk_id`. See the + /// field-level documentation on `Input::in_service_disks` for semantics. + pub fn in_service_disks(&self) -> &IdOrdMap { + &self.in_service_disks + } + /// Returns a [`Builder`] for constructing a new `Input` from the provided - /// `parent_sitrep` and inventory collection. + /// `parent_sitrep`, inventory collection, and in-service disks. pub fn builder( parent_sitrep: Option>, inv: Arc, + in_service_disks: Arc>, ) -> Result { // Before preparing analysis inputs, check that the proposed input // inventory collection is at least as new as the parent sitrep's @@ -94,6 +105,7 @@ impl Input { Ok(Builder { parent_sitrep, inv, + in_service_disks, new_ereports: IdOrdMap::default(), unmarked_seen_ereports: BTreeSet::default(), }) @@ -117,6 +129,7 @@ pub enum InvalidInputs { pub struct Builder { parent_sitrep: Option>, inv: Arc, + in_service_disks: Arc>, /// Ereports which are new and should be input to analysis in the next /// sitrep. new_ereports: IdOrdMap, @@ -184,6 +197,11 @@ impl Builder { .collect(), open_cases: BTreeMap::new(), closed_cases_copied_forward: BTreeMap::new(), + in_service_disks: self + .in_service_disks + .iter() + .map(|d| d.physical_disk_id) + .collect(), }; // Determine which cases must be copied forwards into the next sitrep. @@ -234,6 +252,7 @@ impl Builder { new_ereports: self.new_ereports, open_cases, closed_cases_copied_forward, + in_service_disks: self.in_service_disks, }; (input, report) @@ -346,6 +365,7 @@ mod tests { .collect(), alerts_requested: Default::default(), support_bundles_requested: Default::default(), + facts: Default::default(), } }; let open_case2 = { @@ -366,6 +386,7 @@ mod tests { .collect(), alerts_requested: Default::default(), support_bundles_requested: Default::default(), + facts: Default::default(), } }; let closed_case_with_unmarked = { @@ -393,6 +414,7 @@ mod tests { .collect(), alerts_requested: Default::default(), support_bundles_requested: Default::default(), + facts: Default::default(), } }; let closed_case_without_unmarked = { @@ -414,6 +436,7 @@ mod tests { .collect(), alerts_requested: Default::default(), support_bundles_requested: Default::default(), + facts: Default::default(), } }; @@ -463,8 +486,12 @@ mod tests { // Build analysis input let (input, report) = { - let mut builder = Input::builder(Some(parent_sitrep), inv) - .expect("collection start time check should always pass"); + let mut builder = Input::builder( + Some(parent_sitrep), + inv, + Arc::new(IdOrdMap::new()), + ) + .expect("collection start time check should always pass"); // Pass in four ereports: // - two that are in the open cases of the parent sitrep // - one that is in the (to-be-copied-forward) closed case diff --git a/nexus/fm/src/builder/case.rs b/nexus/fm/src/builder/case.rs index d4d8c5e4d4e..8f9e2bd022a 100644 --- a/nexus/fm/src/builder/case.rs +++ b/nexus/fm/src/builder/case.rs @@ -10,6 +10,7 @@ use nexus_types::alert::AlertClass; use nexus_types::fm; use nexus_types::support_bundle::BundleDataSelection; use omicron_uuid_kinds::CaseUuid; +use omicron_uuid_kinds::FactUuid; use omicron_uuid_kinds::SitrepUuid; use std::sync::Arc; @@ -76,6 +77,7 @@ impl AllCases { ereports: Default::default(), alerts_requested: Default::default(), support_bundles_requested: Default::default(), + facts: Default::default(), }; let mut builder = CaseBuilder::new(&self.log, sitrep_id, case, case_rng); @@ -215,6 +217,63 @@ impl CaseBuilder { self.report_log.entry("case closed").comment(comment); } + /// Replace this case's free-form comment string. + pub fn set_comment(&mut self, comment: impl ToString) { + self.case.metadata.comment = comment.to_string(); + } + + /// Emit a new fact under this case. The fact's UUID is freshly + /// allocated from the case's deterministic RNG. + pub fn add_fact( + &mut self, + payload: impl Into, + comment: impl ToString, + ) -> FactUuid { + let id = loop { + let id = self.rng.next_fact(); + if !self.case.facts.contains_key(&id) { + break id; + } + }; + let payload = payload.into(); + let comment = comment.to_string(); + slog::info!( + &self.log, + "added a fact"; + "fact_id" => %id, + "payload" => ?payload, + "comment" => %comment, + ); + self.report_log + .entry("added fact") + .kv("fact_id", id) + .kv("payload", &payload) + .comment(comment.clone()); + let fact = fm::case::Fact { + id, + created_sitrep_id: self.sitrep_id, + payload, + comment, + }; + self.case.facts.insert_unique(fact).expect("UUID should be unused"); + id + } + + /// Remove a fact from this case. The fact will not be carried forward + /// into the next sitrep. + pub fn remove_fact(&mut self, id: FactUuid) { + if self.case.facts.remove(&id).is_some() { + slog::info!(&self.log, "removed a fact"; "fact_id" => %id); + self.report_log.entry("removed fact").kv("fact_id", id); + } + } + + /// Iterate the facts currently attached to this case (including any that + /// were carried forward from the parent sitrep). + pub fn facts(&self) -> impl Iterator { + self.case.facts.iter() + } + pub fn add_ereport( &mut self, report: &Arc, diff --git a/nexus/fm/src/builder/rng.rs b/nexus/fm/src/builder/rng.rs index 5490ef67ae9..9ba02eca5cc 100644 --- a/nexus/fm/src/builder/rng.rs +++ b/nexus/fm/src/builder/rng.rs @@ -14,6 +14,8 @@ use omicron_uuid_kinds::CaseEreportKind; use omicron_uuid_kinds::CaseEreportUuid; use omicron_uuid_kinds::CaseKind; use omicron_uuid_kinds::CaseUuid; +use omicron_uuid_kinds::FactKind; +use omicron_uuid_kinds::FactUuid; use omicron_uuid_kinds::SitrepUuid; use omicron_uuid_kinds::SupportBundleKind; use omicron_uuid_kinds::SupportBundleUuid; @@ -59,11 +61,17 @@ impl SitrepBuilderRng { } } +/// Per-case child RNGs. Each `next_*` returns the next deterministic UUID +/// in its stream; collisions across calls are statistically impossible, but +/// callers in `builder/case.rs` still guard inserts with a `contains_key` +/// loop so that a future change here (e.g., reseeding mid-build) can't +/// silently corrupt a case. #[derive(Clone, Debug)] pub(super) struct CaseBuilderRng { ereport_assignment_rng: TypedUuidRng, alert_rng: TypedUuidRng, support_bundle_rng: TypedUuidRng, + fact_rng: TypedUuidRng, } impl CaseBuilderRng { @@ -85,7 +93,11 @@ impl CaseBuilderRng { &mut sitrep.parent, (case_id, "support-bundle"), ); - Self { alert_rng, ereport_assignment_rng, support_bundle_rng } + let fact_rng = TypedUuidRng::from_parent_rng( + &mut sitrep.parent, + (case_id, "case-fact"), + ); + Self { alert_rng, ereport_assignment_rng, support_bundle_rng, fact_rng } } pub(super) fn next_alert(&mut self) -> AlertUuid { @@ -99,4 +111,8 @@ impl CaseBuilderRng { pub(super) fn next_support_bundle(&mut self) -> SupportBundleUuid { self.support_bundle_rng.next() } + + pub(super) fn next_fact(&mut self) -> FactUuid { + self.fact_rng.next() + } } diff --git a/nexus/fm/src/diagnosis.rs b/nexus/fm/src/diagnosis/mod.rs similarity index 64% rename from nexus/fm/src/diagnosis.rs rename to nexus/fm/src/diagnosis/mod.rs index 96ea491f1b1..b62e676fb36 100644 --- a/nexus/fm/src/diagnosis.rs +++ b/nexus/fm/src/diagnosis/mod.rs @@ -2,22 +2,30 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +//! Fault management diagnosis engines. +//! +//! Each submodule defines one diagnosis engine (DE). `analyze` dispatches to +//! each engine in turn; engines are deterministic and idempotent per RFD 603, +//! so the dispatch order does not matter. + use crate::SitrepBuilder; use crate::analysis_input::Input; +mod physical_disk; + pub fn analyze( - _input: &Input, - _builder: &mut SitrepBuilder<'_>, + input: &Input, + builder: &mut SitrepBuilder<'_>, ) -> anyhow::Result<()> { - anyhow::bail!("FM analysis is not yet implemented") + physical_disk::analyze(input, builder)?; + Ok(()) } -/// Ereport classes that the diagnosis engine currently understands. -/// Preparation only surfaces ereports whose class is in this set — there is -/// no value in loading ereports FM analysis cannot consume. +/// Ereport classes that any diagnosis engine in this build of Nexus knows +/// how to consume. The background task uses this to filter loaded ereports. /// -/// Empty until [`analyze`] gains real handling. Grow this alongside FM -/// analysis as new classes gain support. +/// Empty today: Grow this list alongside FM analysis as new classes gain +/// ereport support. /// /// **NULL-class ereports are intentionally excluded by the loader's SQL /// filter** (`class = ANY(...)` never matches NULL). If FM analysis ever diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs new file mode 100644 index 00000000000..3992c9cb46c --- /dev/null +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -0,0 +1,810 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Disk diagnosis engine. + +use crate::SitrepBuilder; +use crate::analysis_input::Input; +use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; +use nexus_types::fm::DiagnosisEngineKind; +use nexus_types::fm::{DiskFact, ZpoolUnhealthyFactPayload}; +use nexus_types::inventory::ZpoolHealth; +use omicron_uuid_kinds::{CaseUuid, FactUuid, PhysicalDiskUuid, ZpoolUuid}; +use std::collections::BTreeMap; + +/// A [`DiskFact::ZpoolUnhealthy`] payload paired with the `FactUuid` it +/// lives under. Used to build in-memory indices over facts during +/// analysis; not serialized. +#[derive(Clone, Copy, Debug)] +struct ZpoolUnhealthyFact { + fact_id: FactUuid, + payload: ZpoolUnhealthyFactPayload, +} + +impl IdOrdItem for ZpoolUnhealthyFact { + type Key<'a> = FactUuid; + fn key(&self) -> Self::Key<'_> { + self.fact_id + } + id_upcast!(); +} + +/// One in-service disk paired with the current observed health of its zpool. +/// `zpool_health` is `None` when the disk's zpool was not seen in the current +/// inventory (e.g., sled down, lossy collection). +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +struct DiskHealthSnapshot { + physical_disk_id: PhysicalDiskUuid, + zpool_id: ZpoolUuid, + zpool_health: Option, +} + +impl IdOrdItem for DiskHealthSnapshot { + type Key<'a> = PhysicalDiskUuid; + fn key(&self) -> Self::Key<'_> { + self.physical_disk_id + } + id_upcast!(); +} + +/// Per-case summary built from a case's facts. Each Disk case is about a +/// single physical disk; every fact on the case must reference that disk. +struct ParentCaseSummary { + /// The physical disk this case is about. + physical_disk_id: PhysicalDiskUuid, + /// All `ZpoolUnhealthy` facts on this case. Normally one; pathological + /// cases may have multiple. Regardless, the diagnosis engine keeps all of + /// them. + unhealthy_facts: IdOrdMap, +} + +pub(super) fn analyze( + input: &Input, + builder: &mut SitrepBuilder<'_>, +) -> anyhow::Result<()> { + let inv_collection_id = input.inventory().id; + let inv_time_done = input.inventory().time_done; + + // Index every zpool we observed in this inventory, so we can distinguish + // "saw it, it's Online" from "didn't see it at all" when looking up by + // an in-service disk's zpool below. + let observed_health: BTreeMap = input + .inventory() + .sled_agents + .iter() + .flat_map(|sa| sa.zpools.iter()) + .map(|z| (z.id, z.health)) + .collect(); + + // The current health snapshot for every in-service disk, keyed by + // physical_disk_id. Absence from this index is a positive signal that + // the control plane has moved on from the disk (expungement / + // decommissioning); see prepare_inputs in + // nexus/src/app/background/tasks/fm_analysis.rs. + let in_service_health: IdOrdMap = input + .in_service_disks() + .iter() + .map(|d| DiskHealthSnapshot { + physical_disk_id: d.physical_disk_id, + zpool_id: d.zpool_id, + zpool_health: observed_health.get(&d.zpool_id).copied(), + }) + .collect(); + + // Index parent-forwarded Disk cases from the input — the state copied + // from the parent sitrep. + // Every case is about one physical disk; we derive the disk + // from its facts. Skip (with a warning) any case we can't safely + // interpret. + let parent_cases: BTreeMap = input + .open_cases() + .iter() + .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .filter_map(|c| { + let case_id = c.id; + let mut unhealthy_facts: IdOrdMap = + IdOrdMap::new(); + let mut case_disk_id: Option = None; + for fact in c.facts.iter() { + // Every fact on a physical-disk case must carry a + // physical-disk payload. A foreign payload is a data-model + // violation; skip the whole case rather than acting on + // inconsistent state. + let Some(disk_fact) = fact.payload.as_physical_disk() else { + slog::warn!( + &builder.log, + "skipping Disk case: fact payload does not belong to \ + the physical-disk diagnosis engine"; + "case_id" => %case_id, + "fact_id" => %fact.id, + ); + return None; + }; + match disk_fact { + DiskFact::ZpoolUnhealthy(payload) => { + let payload = *payload; + let disk_id = *case_disk_id + .get_or_insert(payload.physical_disk_id); + if disk_id != payload.physical_disk_id { + slog::warn!( + &builder.log, + "skipping Disk case: facts reference \ + different physical disks (1 expected)"; + "case_id" => %case_id, + "expected_physical_disk_id" => %disk_id, + "fact_physical_disk_id" => + %payload.physical_disk_id, + ); + return None; + } + unhealthy_facts + .insert_unique(ZpoolUnhealthyFact { + fact_id: fact.id, + payload, + }) + .expect("fact ids are unique within a case"); + } + } + } + let Some(physical_disk_id) = case_disk_id else { + slog::warn!( + &builder.log, + "skipping Disk case with no facts; cannot derive disk id"; + "case_id" => %case_id, + ); + return None; + }; + Some(( + case_id, + ParentCaseSummary { physical_disk_id, unhealthy_facts }, + )) + }) + .collect(); + + // For each parent case, decide what to do based on its disk's current + // state: + // - disk no longer in service → close the case (expungement) + // - disk's zpool back to Online → close the case (recovery) + // - disk still unhealthy → drop any facts whose recorded health no + // longer matches; the matching loop below will re-add a fresh fact + // - disk in service but absent from inventory → leave alone (absence + // is NOT a recovery signal: sled could be powered off, or + // inventory could be lossy) + for (case_id, summary) in &parent_cases { + let mut case_mut = builder + .cases + .case_mut(case_id) + .expect("case_id came from iterating builder.cases"); + match in_service_health.get(&summary.physical_disk_id) { + None => { + case_mut.close(format!( + "disk {} no longer in service", + summary.physical_disk_id, + )); + } + Some(snap) if snap.zpool_health == Some(ZpoolHealth::Online) => { + case_mut + .close(format!("zpool {} back to Online", snap.zpool_id,)); + } + Some(snap) => { + let Some(current_health) = snap.zpool_health else { + continue; + }; + for fact_ref in summary.unhealthy_facts.iter() { + if fact_ref.payload.last_seen_health != current_health { + case_mut.remove_fact(fact_ref.fact_id); + } + } + } + } + } + + // For each currently-faulty in-service disk: ensure a case exists + // (reusing the parent-forwarded one for this disk if any) and add a + // fresh fact if one with this exact health isn't already present. + for disk in in_service_health.iter() { + let Some(current_health) = disk.zpool_health else { + continue; + }; + if current_health == ZpoolHealth::Online { + continue; + } + + let parent_for_disk = + parent_cases.iter().find_map(|(case_id, summary)| { + if summary.physical_disk_id == disk.physical_disk_id { + Some((*case_id, summary)) + } else { + None + } + }); + + let case_id_for_fact = match parent_for_disk { + // Parent case already has an accurate fact — fully covered. + Some((_, summary)) + if summary + .unhealthy_facts + .iter() + .any(|f| f.payload.last_seen_health == current_health) => + { + continue; + } + // Parent case exists; its stale facts were removed above. + // Refresh under the same case. + Some((case_id, _)) => case_id, + // No parent case for this disk — open one. + None => { + let mut new_case = + builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk); + new_case.set_comment(format!( + "physical disk {} unhealthy", + disk.physical_disk_id, + )); + new_case.id + } + }; + + builder + .cases + .case_mut(&case_id_for_fact) + .expect("case_id came from this fn") + .add_fact( + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id: disk.physical_disk_id, + zpool_id: disk.zpool_id, + last_seen_health: current_health, + observed_in_inv: inv_collection_id, + time_observed: inv_time_done, + }), + format!("zpool {} health={current_health}", disk.zpool_id,), + ); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::analysis_input::Input; + use crate::builder::{SitrepBuilder, SitrepBuilderRng}; + use crate::test_util::FmTest; + use chrono::Utc; + use iddqd::IdOrdMap; + use nexus_types::external_api::physical_disk::PhysicalDiskKind; + use nexus_types::fm::{self, Sitrep, SitrepVersion}; + use nexus_types::in_service_disk::InServiceDisk; + use nexus_types::inventory; + use omicron_test_utils::dev; + use omicron_uuid_kinds::{ + OmicronZoneUuid, PhysicalDiskUuid, SitrepUuid, SledUuid, + }; + use std::sync::Arc; + + /// Synthesize a synthetic in-service disk set from a list of zpool IDs. + /// Each zpool gets its own fresh `PhysicalDiskUuid` and dummy identity + /// facts — tests in this module only care about the zpool dimension. + fn mk_in_service( + zpool_ids: impl IntoIterator, + ) -> IdOrdMap { + zpool_ids + .into_iter() + .map(|zpool_id| InServiceDisk { + physical_disk_id: PhysicalDiskUuid::new_v4(), + zpool_id, + sled_id: SledUuid::new_v4(), + vendor: "test-vendor".to_string(), + serial: format!("test-serial-{zpool_id}"), + model: "test-model".to_string(), + variant: PhysicalDiskKind::U2, + }) + .collect() + } + + /// Find the `physical_disk_id` for the given `zpool_id` in the + /// in-service set, or fabricate a fresh one if not present (e.g., when + /// simulating an expunged disk whose case should still reference some + /// stable PhysicalDiskUuid). + fn disk_id_for( + in_service: &IdOrdMap, + zpool_id: ZpoolUuid, + ) -> PhysicalDiskUuid { + in_service + .iter() + .find(|d| d.zpool_id == zpool_id) + .map(|d| d.physical_disk_id) + .unwrap_or_else(PhysicalDiskUuid::new_v4) + } + + /// Make a synthetic test scenario from the example system: returns a + /// `LogContext` (the caller must `cleanup_successful()` it), the + /// example collection, and every zpool ID in that collection. + fn setup( + test_name: &'static str, + ) -> (dev::LogContext, inventory::Collection, Vec) { + let (fm_test, logctx) = FmTest::new_with_logctx(test_name); + // Build the example system once to get a Collection with zpools. + let (example, _bp) = fm_test.system_builder.build(); + let zpool_ids: Vec = example + .collection + .sled_agents + .iter() + .flat_map(|sa| sa.zpools.iter().map(|z| z.id)) + .collect(); + assert!( + !zpool_ids.is_empty(), + "example system should have at least one zpool" + ); + (logctx, example.collection, zpool_ids) + } + + /// Set the zpool with `zpool_id` to `health`, panicking if not found. + fn set_health( + collection: &mut inventory::Collection, + zpool_id: ZpoolUuid, + health: ZpoolHealth, + ) { + for mut sa in collection.sled_agents.iter_mut() { + for z in sa.zpools.iter_mut() { + if z.id == zpool_id { + z.health = health; + return; + } + } + } + panic!("zpool {zpool_id} not found in collection"); + } + + /// Build an `Input` from a collection, an optional parent sitrep, and a + /// pre-built set of in-service disks. + fn build_input( + collection: inventory::Collection, + parent_sitrep: Option, + in_service: IdOrdMap, + ) -> Input { + let parent = parent_sitrep.map(|s| { + Arc::new(( + SitrepVersion { + id: s.id(), + version: 0, + time_made_current: Utc::now(), + }, + s, + )) + }); + let builder = + Input::builder(parent, Arc::new(collection), Arc::new(in_service)) + .expect("input builder should accept fresh inventory"); + let (input, _report) = builder.build(); + input + } + + /// Run `disk::analyze` over an input and return the resulting Sitrep + /// along with the analysis report (whose log entries the close-comment + /// assertions in `closes_*` tests inspect). + fn run_analyze( + log: &slog::Logger, + input: &Input, + ) -> (Sitrep, fm::analysis_reports::AnalysisReport) { + let mut builder = SitrepBuilder::new_with_rng( + log, + input, + SitrepBuilderRng::from_seed("disk-analyze"), + ); + analyze(input, &mut builder).expect("analyze ok"); + builder.build(OmicronZoneUuid::new_v4(), Utc::now()) + } + + fn make_parent_with_disk_case( + parent_sitrep_id: SitrepUuid, + inv_collection_id: omicron_uuid_kinds::CollectionUuid, + physical_disk_id: PhysicalDiskUuid, + zpool_id: ZpoolUuid, + ) -> Sitrep { + let mut cases = iddqd::IdOrdMap::new(); + let case_id = omicron_uuid_kinds::CaseUuid::new_v4(); + let mut facts = iddqd::IdOrdMap::new(); + facts + .insert_unique(fm::case::Fact { + id: omicron_uuid_kinds::FactUuid::new_v4(), + created_sitrep_id: parent_sitrep_id, + payload: DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id, + zpool_id, + last_seen_health: ZpoolHealth::Degraded, + observed_in_inv: inv_collection_id, + time_observed: Utc::now(), + }) + .into(), + comment: format!("zpool {zpool_id} degraded"), + }) + .unwrap(); + cases + .insert_unique(fm::Case { + id: case_id, + metadata: fm::case::Metadata { + created_sitrep_id: parent_sitrep_id, + closed_sitrep_id: None, + de: DiagnosisEngineKind::PhysicalDisk, + comment: format!("zpool {zpool_id} degraded"), + }, + ereports: Default::default(), + alerts_requested: Default::default(), + support_bundles_requested: Default::default(), + facts, + }) + .unwrap(); + Sitrep { + metadata: fm::SitrepMetadata { + id: parent_sitrep_id, + inv_collection_id, + creator_id: OmicronZoneUuid::new_v4(), + parent_sitrep_id: None, + time_created: Utc::now(), + next_inv_min_time_started: Utc::now(), + comment: String::new(), + }, + cases, + ereports_by_id: Default::default(), + } + } + + /// Helper: collect (case, fact, DiskFact) triples for every fact on a + /// physical-disk case in a sitrep. Optionally filtered to open cases + /// only. + fn disk_facts( + sitrep: &Sitrep, + open_only: bool, + ) -> Vec<(&fm::Case, &fm::case::Fact, DiskFact)> { + sitrep + .cases + .iter() + .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .filter(|c| !open_only || c.is_open()) + .flat_map(|c| { + c.facts.iter().filter_map(move |f| { + f.payload.as_physical_disk().map(|d| (c, f, d.clone())) + }) + }) + .collect() + } + + #[test] + fn opens_on_degraded_in_service() { + let (logctx, mut collection, zpools) = + setup("disk_open_degraded_in_service"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let expected_disk_id = disk_id_for(&in_service, target); + let input = build_input(collection, None, in_service); + + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let facts = disk_facts(&sitrep, true); + assert_eq!(facts.len(), 1); + match &facts[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id, + zpool_id, + last_seen_health, + .. + }) => { + assert_eq!(*physical_disk_id, expected_disk_id); + assert_eq!(*zpool_id, target); + assert_eq!(*last_seen_health, ZpoolHealth::Degraded); + } + } + logctx.cleanup_successful(); + } + + #[test] + fn skips_degraded_when_expunged() { + let (logctx, mut collection, zpools) = setup("disk_skip_expunged"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Faulted); + // target is *not* in the in-service set. + let in_service = mk_in_service(zpools.iter().copied().skip(1)); + let input = build_input(collection, None, in_service); + + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let cases = disk_facts(&sitrep, false); + assert!( + cases.is_empty(), + "no Disk cases should be opened for expunged zpool, got: {:?}", + cases + ); + logctx.cleanup_successful(); + } + + #[test] + fn idempotent_when_case_already_open() { + let (logctx, mut collection, zpools) = setup("disk_idempotent"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + target, + ); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let open_cases = disk_facts(&sitrep, true); + assert_eq!(open_cases.len(), 1); + match &open_cases[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + zpool_id, + .. + }) => { + assert_eq!(*zpool_id, target); + } + } + logctx.cleanup_successful(); + } + + #[test] + fn closes_on_recovery() { + let (logctx, collection, zpools) = setup("disk_close_on_recovery"); + let target = zpools[0]; + // The example system reports zpools as Online by default. + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + target, + ); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, report) = run_analyze(&logctx.log, &input); + let all = disk_facts(&sitrep, false); + assert_eq!(all.len(), 1); + assert!( + !all[0].0.is_open(), + "case should be closed when zpool returns to Online", + ); + let report_str = format!("{}", report.display_multiline(0)); + assert!( + report_str.contains("back to Online"), + "close comment should call out the recovery cause, got: \ + {report_str}", + ); + logctx.cleanup_successful(); + } + + #[test] + fn closes_on_expungement() { + let (logctx, mut collection, zpools) = + setup("disk_close_on_expungement"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + // Target is NOT in-service in this sitrep (just expunged). + let in_service = mk_in_service(zpools.iter().copied().skip(1)); + // Target isn't in the in-service set; fabricate a stable PhysicalDiskUuid. + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + target, + ); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, report) = run_analyze(&logctx.log, &input); + let all = disk_facts(&sitrep, false); + assert_eq!(all.len(), 1); + assert!( + !all[0].0.is_open(), + "case should be closed when zpool's disk is expunged", + ); + let report_str = format!("{}", report.display_multiline(0)); + assert!( + report_str.contains("no longer in service"), + "close comment should call out the expungement cause, got: \ + {report_str}", + ); + logctx.cleanup_successful(); + } + + #[test] + fn keeps_open_on_absence_from_inventory() { + // A zpool the case is about does NOT appear in the inventory at all + // (sled powered off, lossy collection, etc.). The case should stay + // open: absence is not a recovery signal. + let (logctx, collection, zpools) = setup("disk_keep_open_on_absence"); + let phantom = ZpoolUuid::new_v4(); + assert!(!zpools.contains(&phantom)); + let in_service = mk_in_service(zpools.iter().copied().chain([phantom])); + let phantom_disk_id = disk_id_for(&in_service, phantom); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + phantom_disk_id, + phantom, + ); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let all = disk_facts(&sitrep, false); + assert_eq!(all.len(), 1); + assert!( + all[0].0.is_open(), + "case should remain open when its zpool is absent from the \ + current inventory collection (sled could be down or inventory \ + is lossy)", + ); + logctx.cleanup_successful(); + } + + /// A parent Disk case with zero facts has no derivable disk ID, so + /// the diagnosis engine leaves it alone (carried forward unchanged). + #[test] + fn empty_case_is_left_open() { + let (logctx, collection, _zpools) = setup("disk_empty_case_left_open"); + let in_service = mk_in_service(std::iter::empty()); + + let parent_sitrep_id = SitrepUuid::new_v4(); + let empty_case_id = omicron_uuid_kinds::CaseUuid::new_v4(); + let mut parent_cases = iddqd::IdOrdMap::new(); + parent_cases + .insert_unique(fm::Case { + id: empty_case_id, + metadata: fm::case::Metadata { + created_sitrep_id: parent_sitrep_id, + closed_sitrep_id: None, + de: DiagnosisEngineKind::PhysicalDisk, + comment: "an open case with no facts".to_string(), + }, + ereports: Default::default(), + alerts_requested: Default::default(), + support_bundles_requested: Default::default(), + facts: Default::default(), + }) + .unwrap(); + let parent = Sitrep { + metadata: fm::SitrepMetadata { + id: parent_sitrep_id, + inv_collection_id: collection.id, + creator_id: OmicronZoneUuid::new_v4(), + parent_sitrep_id: None, + time_created: Utc::now(), + next_inv_min_time_started: Utc::now(), + comment: String::new(), + }, + cases: parent_cases, + ereports_by_id: Default::default(), + }; + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + + let case = sitrep + .cases + .iter() + .find(|c| c.id == empty_case_id) + .expect("empty case should still be in the output sitrep"); + assert!( + case.is_open(), + "empty case should be left open (no disk to verify)", + ); + logctx.cleanup_successful(); + } + + /// When the parent sitrep's fact content matches the diagnosis engine's current + /// observation, the fact carries forward with the same UUID — no + /// remove-and-readd churn. + #[test] + fn fact_uuid_stable_when_observation_unchanged() { + let (logctx, mut collection, zpools) = setup("disk_fact_uuid_stable"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + target, + ); + // Capture the parent's fact UUID for the target zpool. + let parent_fact_id = parent + .cases + .iter() + .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .expect("parent should have one Disk case") + .facts + .iter() + .next() + .expect("parent case should have one fact") + .id; + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let open = disk_facts(&sitrep, true); + assert_eq!(open.len(), 1, "expected exactly one open Disk fact"); + assert_eq!( + open[0].1.id, parent_fact_id, + "fact UUID should be stable across sitreps when the \ + observation hasn't changed", + ); + match &open[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + zpool_id, + last_seen_health, + .. + }) => { + assert_eq!(*zpool_id, target); + assert_eq!(*last_seen_health, ZpoolHealth::Degraded); + } + } + logctx.cleanup_successful(); + } + + /// When the parent's fact recorded a different `last_seen_health` than + /// what we observe now, the diagnosis engine removes the stale fact and emits + /// a fresh one (new UUID). The case stays open because the zpool is + /// still unhealthy — just with a different value. + #[test] + fn fact_uuid_rotates_when_observation_changes() { + let (logctx, mut collection, zpools) = setup("disk_fact_uuid_rotates"); + let target = zpools[0]; + // Parent recorded Degraded; current inventory shows Faulted. + set_health(&mut collection, target, ZpoolHealth::Faulted); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + target, + ); + let parent_fact_id = parent + .cases + .iter() + .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .expect("parent should have one Disk case") + .facts + .iter() + .next() + .expect("parent case should have one fact") + .id; + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let open = disk_facts(&sitrep, true); + assert_eq!( + open.len(), + 1, + "expected exactly one open Disk fact (the refreshed one)", + ); + assert_ne!( + open[0].1.id, parent_fact_id, + "fact UUID should rotate because last_seen_health changed", + ); + match &open[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + zpool_id, + last_seen_health, + .. + }) => { + assert_eq!(*zpool_id, target); + assert_eq!(*last_seen_health, ZpoolHealth::Faulted); + } + } + // The case itself should still be the same one that was carried + // forward — only the fact rotated. + assert!(open[0].0.is_open()); + logctx.cleanup_successful(); + } +} diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 10173be4021..17cb3fddd48 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -9,11 +9,15 @@ use anyhow::Context; use chrono::Utc; use fm::analysis_input::InvalidInputs; use futures::future::BoxFuture; +use iddqd::IdOrdMap; +use nexus_db_model::PhysicalDiskPolicy; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore; +use nexus_db_queries::db::identity::Asset; use nexus_db_queries::db::pagination::Paginator; use nexus_fm as fm; +use nexus_types::in_service_disk::InServiceDisk; use nexus_types::internal_api::background::FmAnalysisStatus; use nexus_types::internal_api::background::fm_analysis as status; use nexus_types::inventory; @@ -217,8 +221,48 @@ impl FmAnalysis { (fm::analysis_input::Input, status::PreparationStatus), PreparationError, > { - let mut builder = - fm::analysis_input::Input::builder(parent_sitrep, inv)?; + // Load all external (U.2) zpools and project them down to FM's + // `InServiceDisk` view, filtering on `disk_policy = in_service` and a + // live (non-soft-deleted) physical_disk row. M.2 disks are not + // represented as control plane disks today, so the U.2-only filter + // on the underlying query matches reality. + // + // This is the executed view from the DB — flipped only after sagas / + // cleaners have actually drained resources, not while a planner is + // merely proposing changes. A faulty disk a planner proposes to + // expunge is still the diagnoser's concern until the control plane + // has actually moved on. + let zpools_and_disks = self + .datastore + .zpool_list_all_external_batched(opctx) + .await + .context("failed to load in-service control plane disks")?; + let mut in_service_disks_map = IdOrdMap::new(); + for (zpool, disk) in zpools_and_disks { + if disk.disk_policy != PhysicalDiskPolicy::InService { + continue; + } + in_service_disks_map + .insert_unique(InServiceDisk { + physical_disk_id: disk.id(), + zpool_id: zpool.id(), + sled_id: disk.sled_id.into(), + vendor: disk.vendor, + serial: disk.serial, + model: disk.model, + variant: disk.variant.into(), + }) + .expect( + "physical_disk.id is a primary key, so duplicates are \ + impossible", + ); + } + let in_service_disks = Arc::new(in_service_disks_map); + let mut builder = fm::analysis_input::Input::builder( + parent_sitrep, + inv, + in_service_disks, + )?; let mut errors = Vec::new(); self.load_new_ereports(opctx, &mut builder, &mut errors) .await diff --git a/nexus/src/app/background/tasks/fm_rendezvous.rs b/nexus/src/app/background/tasks/fm_rendezvous.rs index ef76c1db310..0e2696f4778 100644 --- a/nexus/src/app/background/tasks/fm_rendezvous.rs +++ b/nexus/src/app/background/tasks/fm_rendezvous.rs @@ -552,6 +552,7 @@ mod tests { alerts_requested: iddqd::IdOrdMap::new(), ereports: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), }; case1 .alerts_requested @@ -631,6 +632,7 @@ mod tests { alerts_requested: iddqd::IdOrdMap::new(), ereports: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), }; case2 .alerts_requested @@ -933,6 +935,7 @@ mod tests { ereports, alerts_requested: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), } }; @@ -1145,6 +1148,7 @@ mod tests { ereports, alerts_requested: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), } }; @@ -1254,6 +1258,7 @@ mod tests { ereports, alerts_requested: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), } }; @@ -1449,6 +1454,7 @@ mod tests { alerts_requested: iddqd::IdOrdMap::new(), ereports: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), }; case1 .support_bundles_requested @@ -1625,6 +1631,7 @@ mod tests { alerts_requested: iddqd::IdOrdMap::new(), ereports: iddqd::IdOrdMap::new(), support_bundles_requested: iddqd::IdOrdMap::new(), + facts: iddqd::IdOrdMap::new(), }; case.support_bundles_requested .insert_unique(fm::case::SupportBundleRequest { diff --git a/nexus/src/app/background/tasks/fm_sitrep_load.rs b/nexus/src/app/background/tasks/fm_sitrep_load.rs index a6afdcb93ee..b2dca967abd 100644 --- a/nexus/src/app/background/tasks/fm_sitrep_load.rs +++ b/nexus/src/app/background/tasks/fm_sitrep_load.rs @@ -10,7 +10,6 @@ use chrono::Utc; use futures::future::BoxFuture; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; -use nexus_types::fm::Sitrep; use nexus_types::fm::SitrepVersion; use nexus_types::internal_api::background::SitrepLoadStatus as Status; use serde_json::json; @@ -23,7 +22,7 @@ pub struct SitrepLoader { tx: watch::Sender>, } -pub type CurrentSitrep = Arc<(SitrepVersion, Sitrep)>; +pub use nexus_types::fm::CurrentSitrep; impl BackgroundTask for SitrepLoader { fn activate<'a>( @@ -192,6 +191,7 @@ mod test { use super::*; use crate::app::background::BackgroundTask; use nexus_db_queries::db::pub_test_utils::TestDatabase; + use nexus_types::fm::Sitrep; use nexus_types::fm::SitrepMetadata; use omicron_test_utils::dev; use omicron_uuid_kinds::CollectionUuid; diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 00f8dc27544..716a00b36de 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -306,7 +306,6 @@ pub struct Nexus { repo_depot_resolver: Box, /// Watch channel containing the currently-loaded fault management sitrep. - #[allow(dead_code)] sitrep_load_rx: watch::Receiver>, /// handle to pull update status data @@ -1129,6 +1128,14 @@ impl Nexus { &self.db_datastore } + pub(crate) fn sitrep_load_rx( + &self, + ) -> watch::Receiver> { + let mut rx = self.sitrep_load_rx.clone(); + rx.mark_unchanged(); + rx + } + pub(crate) fn samael_max_issue_delay(&self) -> Option { let mid = self.samael_max_issue_delay.lock().unwrap(); *mid diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index 1774bf35b28..e82ba37c2a6 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -532,6 +532,12 @@ impl nexus_test_interface::NexusServer for Server { self.apictx.context.nexus.inventory_load_rx() } + fn sitrep_load_rx( + &self, + ) -> watch::Receiver> { + self.apictx.context.nexus.sitrep_load_rx() + } + fn get_http_server_external_address(&self) -> SocketAddr { self.apictx.context.nexus.get_external_server_address().unwrap() } diff --git a/nexus/test-interface/src/lib.rs b/nexus/test-interface/src/lib.rs index 35f726740e1..53d254b44ca 100644 --- a/nexus/test-interface/src/lib.rs +++ b/nexus/test-interface/src/lib.rs @@ -88,6 +88,10 @@ pub trait NexusServer: Send + Sync + 'static { fn inventory_load_rx(&self) -> watch::Receiver>>; + fn sitrep_load_rx( + &self, + ) -> watch::Receiver>; + fn get_http_server_external_address(&self) -> SocketAddr; fn get_http_server_techport_address(&self) -> SocketAddr; fn get_http_server_internal_address(&self) -> SocketAddr; diff --git a/nexus/test-utils/src/nexus_test.rs b/nexus/test-utils/src/nexus_test.rs index 693aea88732..745ea7b31bc 100644 --- a/nexus/test-utils/src/nexus_test.rs +++ b/nexus/test-utils/src/nexus_test.rs @@ -200,6 +200,38 @@ impl ControlPlaneTestContext { } } + /// Wait until at least one fault management sitrep has been committed and + /// loaded. + /// + /// # Panics + /// + /// Panics if no sitrep is loaded within `timeout`. + pub async fn wait_for_at_least_one_sitrep(&self, timeout: Duration) { + let mut sitrep_rx = self.server.sitrep_load_rx(); + + match wait_for_watch_channel_condition( + &mut sitrep_rx, + async |sitrep| { + if sitrep.is_some() { + Ok(()) + } else { + Err(CondCheckError::<()>::NotYet) + } + }, + timeout, + ) + .await + { + Ok(()) => (), + Err(poll::Error::TimedOut(elapsed)) => { + panic!("no sitrep found within {elapsed:?}"); + } + Err(poll::Error::PermanentError(())) => { + unreachable!("check can only fail via timeout") + } + } + } + pub fn internal_client(&self) -> nexus_client::Client { nexus_client::Client::new( &format!("http://{}", self.internal_client.bind_address), diff --git a/nexus/types/output/analysis_input_report_empty.out b/nexus/types/output/analysis_input_report_empty.out index f221b3960a0..a479e87d36c 100644 --- a/nexus/types/output/analysis_input_report_empty.out +++ b/nexus/types/output/analysis_input_report_empty.out @@ -4,3 +4,5 @@ parent sitrep: inventory collection: bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb no new ereports since the parent sitrep no cases copied forward + +no in-service control plane disks diff --git a/nexus/types/output/analysis_input_report_same_inv.out b/nexus/types/output/analysis_input_report_same_inv.out index 5ed6b60d151..25631755f78 100644 --- a/nexus/types/output/analysis_input_report_same_inv.out +++ b/nexus/types/output/analysis_input_report_same_inv.out @@ -5,3 +5,5 @@ inventory collection: bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb --> same collection as parent sitrep no new ereports since the parent sitrep no cases copied forward + +no in-service control plane disks diff --git a/nexus/types/output/analysis_input_report_with_cases.out b/nexus/types/output/analysis_input_report_with_cases.out index f3af0fe55ff..a1fc877ef72 100644 --- a/nexus/types/output/analysis_input_report_with_cases.out +++ b/nexus/types/output/analysis_input_report_with_cases.out @@ -22,3 +22,5 @@ cases (2 total): closed in sitrep: 55555555-5555-5555-5555-555555555555 copied forwards because these ereports haven't been marked seen yet: * ereport dddddddd-dddd-dddd-dddd-dddddddddddd:2 + +no in-service control plane disks diff --git a/nexus/types/src/fm.rs b/nexus/types/src/fm.rs index fd2957704aa..1c4dba06532 100644 --- a/nexus/types/src/fm.rs +++ b/nexus/types/src/fm.rs @@ -12,6 +12,8 @@ pub mod ereport; pub use ereport::{Ereport, EreportId}; pub mod case; pub use case::Case; +pub mod fact; +pub use fact::{DiskFact, FactPayload, ZpoolUnhealthyFactPayload}; pub(crate) mod json_display; use case::AlertRequest; @@ -202,6 +204,10 @@ pub struct SitrepVersion { pub time_made_current: DateTime, } +/// The current sitrep paired with its [`SitrepVersion`] metadata, wrapped in an +/// `Arc` so it can be shared cheaply (e.g., across `watch` channels). +pub type CurrentSitrep = Arc<(SitrepVersion, Sitrep)>; + #[derive( Copy, Clone, @@ -217,4 +223,5 @@ pub struct SitrepVersion { #[strum(serialize_all = "snake_case")] pub enum DiagnosisEngineKind { PowerShelf, + PhysicalDisk, } diff --git a/nexus/types/src/fm/analysis_reports.rs b/nexus/types/src/fm/analysis_reports.rs index 249e26f1b8b..42b6c6537a4 100644 --- a/nexus/types/src/fm/analysis_reports.rs +++ b/nexus/types/src/fm/analysis_reports.rs @@ -9,7 +9,9 @@ use super::case; use super::ereport::EreportId; use super::json_display::fmt_json_value; use iddqd::IdOrdMap; -use omicron_uuid_kinds::{CaseUuid, CollectionUuid, SitrepUuid}; +use omicron_uuid_kinds::{ + CaseUuid, CollectionUuid, PhysicalDiskUuid, SitrepUuid, +}; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::collections::BTreeSet; @@ -227,6 +229,9 @@ pub struct InputReport { /// Cases which have closed, but which have been copied forwards as they /// contain ereports which have not yet been marked seen. pub closed_cases_copied_forward: BTreeMap, + /// All control-plane-managed physical disks visible to the diagnosis + /// engines for this analysis pass. + pub in_service_disks: BTreeSet, } #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] @@ -257,6 +262,7 @@ impl fmt::Display for InputReportMultilineDisplay<'_> { new_ereport_ids, open_cases, closed_cases_copied_forward, + in_service_disks, }, indent, } = self; @@ -357,6 +363,21 @@ impl fmt::Display for InputReportMultilineDisplay<'_> { writeln!(f, "{:indent$}no cases copied forward", "")?; } + if in_service_disks.is_empty() { + writeln!(f, "\n{:indent$}no in-service control plane disks", "")?; + } else { + writeln!( + f, + "\n{:indent$}in-service control plane disks ({} total):", + "", + in_service_disks.len() + )?; + let indent = indent + 2; + for disk_id in in_service_disks { + writeln!(f, "{:indent$}* disk {disk_id}", "")?; + } + } + Ok(()) } } @@ -441,6 +462,7 @@ mod tests { new_ereport_ids, open_cases, closed_cases_copied_forward, + in_service_disks: BTreeSet::new(), } } @@ -456,6 +478,7 @@ mod tests { new_ereport_ids: BTreeSet::new(), open_cases: BTreeMap::new(), closed_cases_copied_forward: BTreeMap::new(), + in_service_disks: BTreeSet::new(), } } @@ -474,6 +497,7 @@ mod tests { new_ereport_ids: BTreeSet::new(), open_cases: BTreeMap::new(), closed_cases_copied_forward: BTreeMap::new(), + in_service_disks: BTreeSet::new(), } } diff --git a/nexus/types/src/fm/case.rs b/nexus/types/src/fm/case.rs index 03e930e4d64..b6b8576e68f 100644 --- a/nexus/types/src/fm/case.rs +++ b/nexus/types/src/fm/case.rs @@ -2,14 +2,17 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. +use super::json_display::fmt_json_value; use crate::alert::AlertClass; use crate::fm::DiagnosisEngineKind; use crate::fm::Ereport; use crate::fm::EreportId; +use crate::fm::FactPayload; use crate::support_bundle::BundleDataSelection; use iddqd::{IdOrdItem, IdOrdMap}; use omicron_uuid_kinds::{ - AlertUuid, CaseEreportUuid, CaseUuid, SitrepUuid, SupportBundleUuid, + AlertUuid, CaseEreportUuid, CaseUuid, FactUuid, SitrepUuid, + SupportBundleUuid, }; use serde::{Deserialize, Serialize}; use std::fmt; @@ -24,6 +27,9 @@ pub struct Case { pub ereports: IdOrdMap, pub alerts_requested: IdOrdMap, pub support_bundles_requested: IdOrdMap, + /// Diagnosis-engine-derived facts attached to this case. See + /// [`Fact`] for semantics. + pub facts: IdOrdMap, } impl Case { @@ -159,6 +165,84 @@ impl CaseEreport { } } +/// A diagnosis-engine-derived fact attached to a [`Case`]. +/// +/// Facts are **immutable**: to "update" a fact, the diagnosis engine +/// removes the old one and adds a fresh one. As long as a fact's content +/// matches the engine's current view, the same fact is carried forward +/// across sitreps unchanged. +/// +/// The `payload` is a fully-typed [`FactPayload`] whose variant is owned by +/// the case's diagnosis engine (see [`Metadata::de`]). +/// +/// `Eq`/`PartialEq` derive over all fields, including the typed payload. +/// This is the equality the DB round-trip test needs. +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +pub struct Fact { + pub id: FactUuid, + /// The sitrep in which this fact was first added. Preserved + /// unchanged when the fact is carried forward into a child sitrep. + /// Debug-only. + pub created_sitrep_id: SitrepUuid, + pub payload: FactPayload, + pub comment: String, +} + +impl IdOrdItem for Fact { + type Key<'a> = &'a FactUuid; + fn key(&self) -> Self::Key<'_> { + &self.id + } + iddqd::id_upcast!(); +} + +impl Fact { + pub fn display_multiline( + &self, + indent: usize, + sitrep_id: Option, + ) -> impl fmt::Display + '_ { + struct DisplayFact<'a> { + fact: &'a Fact, + indent: usize, + sitrep_id: Option, + } + + impl fmt::Display for DisplayFact<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + const BULLET: &str = "* "; + const ADDED_IN: &str = "added in:"; + const COMMENT: &str = "comment:"; + const WIDTH: usize = const_max_len(&[ADDED_IN, COMMENT]); + + let &Self { + fact: Fact { id, created_sitrep_id, payload, comment }, + indent, + sitrep_id, + } = self; + let this_sitrep = |s| { + if Some(s) == sitrep_id { " <-- this sitrep" } else { "" } + }; + + writeln!(f, "{BULLET:>indent$}fact {id}")?; + writeln!( + f, + "{:>indent$}{ADDED_IN:indent$}{COMMENT: { ereports, alerts_requested, support_bundles_requested, + facts, }, indent, sitrep_id, @@ -286,6 +371,16 @@ impl fmt::Display for DisplayCase<'_> { } } + if !facts.is_empty() { + writeln!(f, "\n{:>indent$}facts:", "")?; + writeln!(f, "{:>indent$}------", "")?; + + let indent = indent + 2; + for fact in facts.iter() { + fact.display_multiline(indent, sitrep_id).fmt(f)?; + } + } + if !alerts_requested.is_empty() { writeln!(f, "\n{:>indent$}alerts requested:", "")?; writeln!(f, "{:>indent$}-----------------", "")?; @@ -373,12 +468,14 @@ mod tests { use super::*; use crate::fm::DiagnosisEngineKind; use crate::fm::ereport::EreportFilters; - use crate::inventory::SpType; + use crate::fm::{DiskFact, ZpoolUnhealthyFactPayload}; + use crate::inventory::{SpType, ZpoolHealth}; use crate::support_bundle::BundleDataSelection; use ereport_types::{Ena, EreportId}; use omicron_uuid_kinds::{ - AlertUuid, CaseUuid, EreporterRestartUuid, OmicronZoneUuid, SitrepUuid, - SupportBundleUuid, + AlertUuid, CaseUuid, CollectionUuid, EreporterRestartUuid, FactUuid, + OmicronZoneUuid, PhysicalDiskUuid, SitrepUuid, SupportBundleUuid, + ZpoolUuid, }; use std::str::FromStr; use std::sync::Arc; @@ -514,6 +611,34 @@ mod tests { }) .unwrap(); + let mut facts = IdOrdMap::new(); + facts + .insert_unique(Fact { + id: FactUuid::from_str("f00f00f0-0f00-4f00-8f00-f00f00f00f00") + .unwrap(), + created_sitrep_id, + payload: FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( + ZpoolUnhealthyFactPayload { + physical_disk_id: PhysicalDiskUuid::from_str( + "d15d15d1-5d15-4d15-8d15-d15d15d15d15", + ) + .unwrap(), + zpool_id: ZpoolUuid::from_str( + "200100f0-0100-4f00-8f00-f00f00f00f00", + ) + .unwrap(), + last_seen_health: ZpoolHealth::Degraded, + observed_in_inv: CollectionUuid::from_str( + "c0110011-c011-4011-8011-c0110011c011", + ) + .unwrap(), + time_observed: chrono::DateTime::::MIN_UTC, + }, + )), + comment: "made-up fact for display test".to_string(), + }) + .unwrap(); + // Create the case let case = Case { id: case_id, @@ -527,6 +652,7 @@ mod tests { ereports, alerts_requested, support_bundles_requested, + facts, }; eprintln!("example case display:"); diff --git a/nexus/types/src/fm/fact.rs b/nexus/types/src/fm/fact.rs new file mode 100644 index 00000000000..ae270e86b5f --- /dev/null +++ b/nexus/types/src/fm/fact.rs @@ -0,0 +1,77 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Typed payloads of diagnosis-engine-derived [`Fact`]s. +//! +//! Each diagnosis engine owns one variant of [`FactPayload`]. A [`Case`] +//! belongs to exactly one diagnosis engine (see [`Metadata::de`]), so every +//! fact on a case carries that engine's variant. +//! +//! [`Fact`]: super::case::Fact +//! [`Case`]: super::case::Case +//! [`Metadata::de`]: super::case::Metadata::de + +use crate::inventory::ZpoolHealth; +use chrono::{DateTime, Utc}; +use omicron_uuid_kinds::{CollectionUuid, PhysicalDiskUuid, ZpoolUuid}; +use serde::{Deserialize, Serialize}; + +/// The typed payload of a [`Fact`](super::case::Fact). +/// +/// One variant per diagnosis engine. The variant a fact carries always +/// matches its case's [`de`](super::case::Metadata::de); other engines and +/// shared FM code must not interpret another engine's variant. +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(tag = "engine", rename_all = "snake_case")] +pub enum FactPayload { + /// A fact owned by the physical-disk diagnosis engine. + PhysicalDisk(DiskFact), +} + +impl From for FactPayload { + fn from(fact: DiskFact) -> Self { + FactPayload::PhysicalDisk(fact) + } +} + +impl FactPayload { + /// The physical-disk payload, or `None` if this fact belongs to a + /// different diagnosis engine. + pub fn as_physical_disk(&self) -> Option<&DiskFact> { + #[allow(unreachable_patterns)] + match self { + FactPayload::PhysicalDisk(fact) => Some(fact), + _ => None, + } + } +} + +/// Per-fact state for the physical-disk diagnosis engine. +#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub enum DiskFact { + /// The zpool's most recently observed health is non-`Online`. + ZpoolUnhealthy(ZpoolUnhealthyFactPayload), +} + +/// Payload of a [`DiskFact::ZpoolUnhealthy`] fact. +#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] +pub struct ZpoolUnhealthyFactPayload { + /// The physical disk this fact (and its parent case) is about. + /// Every fact on a Disk case must agree on this value. + pub physical_disk_id: PhysicalDiskUuid, + /// The zpool whose health was observed. Kept for provenance — the + /// case is keyed by `physical_disk_id`, but knowing the exact zpool + /// makes the fact self-describing when read in isolation. + pub zpool_id: ZpoolUuid, + pub last_seen_health: ZpoolHealth, + /// Inventory collection that produced this observation. Recorded for + /// provenance only: the diagnosis engine never looks this collection + /// back up (it may well have been GC'd by the time anyone reads the + /// fact). If multiple `ZpoolUnhealthy` facts ever end up on the same + /// case, this lets a human reader see which inventory each came from. + pub observed_in_inv: CollectionUuid, + /// `time_done` of `observed_in_inv`. + pub time_observed: DateTime, +} diff --git a/nexus/types/src/in_service_disk.rs b/nexus/types/src/in_service_disk.rs new file mode 100644 index 00000000000..bc7814ce2cb --- /dev/null +++ b/nexus/types/src/in_service_disk.rs @@ -0,0 +1,41 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! "Currently in-service control-plane disks" — the executed view from +//! the `physical_disk` and `zpool` DB tables. +//! +//! This is distinct from the planned view in `BlueprintPhysicalDiskConfig`: +//! a disk is in this set only after the control plane has actually committed +//! to managing it (`physical_disk.disk_policy = 'in_service'`), not while a +//! planner is merely proposing to expunge or adopt it. Consumers that need +//! the *committed* view of which disks are part of the rack — fault +//! management diagnosers in particular — should read this rather than the +//! target blueprint. + +use crate::external_api::physical_disk::PhysicalDiskKind; +use iddqd::{IdOrdItem, id_upcast}; +use omicron_uuid_kinds::PhysicalDiskUuid; +use omicron_uuid_kinds::SledUuid; +use omicron_uuid_kinds::ZpoolUuid; + +/// One control-plane-managed physical disk, joined with its zpool and the +/// stable identity facts the DB already knows about it. +#[derive(Clone, Debug, PartialEq)] +pub struct InServiceDisk { + pub physical_disk_id: PhysicalDiskUuid, + pub zpool_id: ZpoolUuid, + pub sled_id: SledUuid, + pub vendor: String, + pub serial: String, + pub model: String, + pub variant: PhysicalDiskKind, +} + +impl IdOrdItem for InServiceDisk { + type Key<'a> = PhysicalDiskUuid; + fn key(&self) -> Self::Key<'_> { + self.physical_disk_id + } + id_upcast!(); +} diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index 9fccee9ef5f..2370cdd3ce1 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -45,7 +45,7 @@ use sled_agent_types_versions::latest::inventory::SingleMeasurementInventory; use sled_agent_types_versions::latest::inventory::SledCpuFamily; use sled_agent_types_versions::latest::inventory::SledRole; use sled_agent_types_versions::latest::inventory::SvcsEnabledNotOnlineResult; -use sled_agent_types_versions::latest::inventory::ZpoolHealth; +pub use sled_agent_types_versions::latest::inventory::ZpoolHealth; use sled_hardware_types::BaseboardId; use std::collections::BTreeMap; use std::collections::BTreeSet; diff --git a/nexus/types/src/lib.rs b/nexus/types/src/lib.rs index 36e945f58eb..9a5eeb77c06 100644 --- a/nexus/types/src/lib.rs +++ b/nexus/types/src/lib.rs @@ -35,6 +35,7 @@ pub mod deployment; pub mod external_api; pub mod fm; pub mod identity; +pub mod in_service_disk; pub mod instance; pub mod internal_api; pub mod inventory; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 308379c85c5..939b744188e 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -7620,7 +7620,8 @@ ON omicron.public.fm_sitrep_history (sitrep_id); CREATE TYPE IF NOT EXISTS omicron.public.diagnosis_engine AS ENUM ( - 'power_shelf' + 'power_shelf', + 'physical_disk' ); CREATE TABLE IF NOT EXISTS omicron.public.fm_case ( @@ -7647,6 +7648,63 @@ CREATE INDEX IF NOT EXISTS lookup_fm_cases_for_sitrep ON omicron.public.fm_case (sitrep_id); +-- Per-engine "facts" attached to a case. Each diagnosis engine persists its +-- facts in its own table (one table per engine), with a fact's content +-- represented as typed columns. The `fm_fact_physical_disk` table below holds +-- the physical-disk engine's facts. +CREATE TYPE IF NOT EXISTS omicron.public.fm_fact_physical_disk_kind AS ENUM ( + 'zpool_unhealthy' +); + +CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( + -- Stable UUID for this fact across sitreps. + id UUID NOT NULL, + -- Sitrep this row belongs to. + sitrep_id UUID NOT NULL, + -- UUID of the case this fact attaches to. + case_id UUID NOT NULL, + -- UUID of the sitrep in which this fact was first added. Preserved + -- unchanged when the fact is carried forward into a child sitrep, so + -- this can be used to tell at a glance how long a fact has been + -- attached to its case. Debug-only. + created_sitrep_id UUID NOT NULL, + -- Free-form, debug-only comment. + comment TEXT NOT NULL, + + -- Which physical-disk fact this row represents. The columns below are + -- populated according to this discriminant (see the CHECK constraint). + kind omicron.public.fm_fact_physical_disk_kind NOT NULL, + + -- Columns for a 'zpool_unhealthy' fact. NULL for any other kind. + physical_disk_id UUID, + zpool_id UUID, + last_seen_health omicron.public.inv_zpool_health, + observed_in_inv UUID, + time_observed TIMESTAMPTZ, + + PRIMARY KEY (sitrep_id, id), + + CONSTRAINT zpool_unhealthy_columns_present CHECK ( + (kind = 'zpool_unhealthy' + AND physical_disk_id IS NOT NULL + AND zpool_id IS NOT NULL + AND last_seen_health IS NOT NULL + AND observed_in_inv IS NOT NULL + AND time_observed IS NOT NULL) + OR + (kind != 'zpool_unhealthy' + AND physical_disk_id IS NULL + AND zpool_id IS NULL + AND last_seen_health IS NULL + AND observed_in_inv IS NULL + AND time_observed IS NULL) + ) +); + +CREATE INDEX IF NOT EXISTS + lookup_fm_fact_physical_disk_for_case +ON omicron.public.fm_fact_physical_disk (sitrep_id, case_id); + CREATE TABLE IF NOT EXISTS omicron.public.fm_ereport_in_case ( -- ID of this association. When an ereport is assigned to a case, that -- association is assigned a UUID. These are used primarily to aid in @@ -8623,7 +8681,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '261.0.0', NULL) + (TRUE, NOW(), NOW(), '262.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/fm-disk-de-and-facts/up1.sql b/schema/crdb/fm-disk-de-and-facts/up1.sql new file mode 100644 index 00000000000..602e91c8175 --- /dev/null +++ b/schema/crdb/fm-disk-de-and-facts/up1.sql @@ -0,0 +1 @@ +ALTER TYPE omicron.public.diagnosis_engine ADD VALUE IF NOT EXISTS 'physical_disk'; diff --git a/schema/crdb/fm-disk-de-and-facts/up2.sql b/schema/crdb/fm-disk-de-and-facts/up2.sql new file mode 100644 index 00000000000..3aad0937d4c --- /dev/null +++ b/schema/crdb/fm-disk-de-and-facts/up2.sql @@ -0,0 +1,3 @@ +CREATE TYPE IF NOT EXISTS omicron.public.fm_fact_physical_disk_kind AS ENUM ( + 'zpool_unhealthy' +); diff --git a/schema/crdb/fm-disk-de-and-facts/up3.sql b/schema/crdb/fm-disk-de-and-facts/up3.sql new file mode 100644 index 00000000000..c095cf8d564 --- /dev/null +++ b/schema/crdb/fm-disk-de-and-facts/up3.sql @@ -0,0 +1,44 @@ +CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( + -- Stable UUID for this fact across sitreps. + id UUID NOT NULL, + -- Sitrep this row belongs to. + sitrep_id UUID NOT NULL, + -- UUID of the case this fact attaches to. + case_id UUID NOT NULL, + -- UUID of the sitrep in which this fact was first added. Preserved + -- unchanged when the fact is carried forward into a child sitrep, so + -- this can be used to tell at a glance how long a fact has been + -- attached to its case. Debug-only. + created_sitrep_id UUID NOT NULL, + -- Free-form, debug-only comment. + comment TEXT NOT NULL, + + -- Which physical-disk fact this row represents. The columns below are + -- populated according to this discriminant (see the CHECK constraint). + kind omicron.public.fm_fact_physical_disk_kind NOT NULL, + + -- Columns for a 'zpool_unhealthy' fact. NULL for any other kind. + physical_disk_id UUID, + zpool_id UUID, + last_seen_health omicron.public.inv_zpool_health, + observed_in_inv UUID, + time_observed TIMESTAMPTZ, + + PRIMARY KEY (sitrep_id, id), + + CONSTRAINT zpool_unhealthy_columns_present CHECK ( + (kind = 'zpool_unhealthy' + AND physical_disk_id IS NOT NULL + AND zpool_id IS NOT NULL + AND last_seen_health IS NOT NULL + AND observed_in_inv IS NOT NULL + AND time_observed IS NOT NULL) + OR + (kind != 'zpool_unhealthy' + AND physical_disk_id IS NULL + AND zpool_id IS NULL + AND last_seen_health IS NULL + AND observed_in_inv IS NULL + AND time_observed IS NULL) + ) +); diff --git a/schema/crdb/fm-disk-de-and-facts/up4.sql b/schema/crdb/fm-disk-de-and-facts/up4.sql new file mode 100644 index 00000000000..83d3461270d --- /dev/null +++ b/schema/crdb/fm-disk-de-and-facts/up4.sql @@ -0,0 +1 @@ +CREATE INDEX IF NOT EXISTS lookup_fm_fact_physical_disk_for_case ON omicron.public.fm_fact_physical_disk (sitrep_id, case_id); diff --git a/schema/crdb/fm-disk-de-and-facts/up4.verify.sql b/schema/crdb/fm-disk-de-and-facts/up4.verify.sql new file mode 100644 index 00000000000..788cdeebc19 --- /dev/null +++ b/schema/crdb/fm-disk-de-and-facts/up4.verify.sql @@ -0,0 +1,2 @@ +-- DO NOT EDIT. Generated by test_migration_verification_files. +SELECT CAST(IF((SELECT true WHERE EXISTS (SELECT index_name FROM omicron.crdb_internal.table_indexes WHERE descriptor_name = 'fm_fact_physical_disk' AND index_name = 'lookup_fm_fact_physical_disk_for_case')),'true','Schema change verification failed: index lookup_fm_fact_physical_disk_for_case on table fm_fact_physical_disk does not exist') AS BOOL); diff --git a/test-utils/src/dev/test_cmds.rs b/test-utils/src/dev/test_cmds.rs index f254c06726e..60919f4311f 100644 --- a/test-utils/src/dev/test_cmds.rs +++ b/test-utils/src/dev/test_cmds.rs @@ -244,6 +244,19 @@ impl<'a> Redactor<'a> { self } + /// Replace any text matching `pattern` with ``. + /// + /// More flexible than [`Self::field`], which requires a fixed + /// `namevalue` shape. Use this when the entire variable substring + /// is itself a regex (e.g., the substring may take one of several + /// alternative forms across runs). + pub fn variable_regex(&mut self, name: &str, pattern: &str) -> &mut Self { + let re = regex::Regex::new(pattern).unwrap(); + let replacement = format!("<{}_REDACTED>", name.to_uppercase()); + self.extra_regex.push((re, replacement)); + self + } + /// Redact an entire indented section. /// /// This can be used if the shape of a section might change from run to run. diff --git a/uuid-kinds/src/lib.rs b/uuid-kinds/src/lib.rs index 4687e2d5710..304e122201a 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -60,6 +60,7 @@ impl_typed_uuid_kinds! { ExternalIp = {}, ExternalSubnet = {}, ExternalZpool = {}, + Fact = {}, FmdHostCase = {}, FmdResource = {}, Instance = {}, From bfedaa533e2e860dd5eef2517e3df88546f1a5af Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 3 Jun 2026 11:51:03 -0700 Subject: [PATCH 02/20] better errors --- nexus/db-model/src/fm/fact_physical_disk.rs | 31 +++++++++++++-------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index b6510590bff..42a84d904e8 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -107,29 +107,36 @@ impl FmFactPhysicalDisk { /// indicates a corrupt row (e.g. hand-edited) and yields an internal /// error rather than a panic. pub fn into_fact(self) -> Result { - let payload = match self.kind { + let kind = self.kind; + let payload = match kind { FmFactPhysicalDiskKind::ZpoolUnhealthy => { FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( ZpoolUnhealthyFactPayload { physical_disk_id: self .physical_disk_id - .ok_or_else(|| missing_column("physical_disk_id"))? + .ok_or_else(|| { + missing_column(kind, "physical_disk_id") + })? .into(), zpool_id: self .zpool_id - .ok_or_else(|| missing_column("zpool_id"))? + .ok_or_else(|| missing_column(kind, "zpool_id"))? .into(), last_seen_health: self .last_seen_health - .ok_or_else(|| missing_column("last_seen_health"))? + .ok_or_else(|| { + missing_column(kind, "last_seen_health") + })? .into(), observed_in_inv: self .observed_in_inv - .ok_or_else(|| missing_column("observed_in_inv"))? + .ok_or_else(|| { + missing_column(kind, "observed_in_inv") + })? .into(), - time_observed: self - .time_observed - .ok_or_else(|| missing_column("time_observed"))?, + time_observed: self.time_observed.ok_or_else(|| { + missing_column(kind, "time_observed") + })?, }, )) } @@ -143,10 +150,10 @@ impl FmFactPhysicalDisk { } } -fn missing_column(column: &str) -> Error { +fn missing_column(kind: FmFactPhysicalDiskKind, column: &str) -> Error { Error::internal_error(&format!( - "fm_fact_physical_disk row of kind 'zpool_unhealthy' has a NULL \ - {column} (violates the zpool_unhealthy_columns_present CHECK \ - constraint)" + "fm_fact_physical_disk row of kind {kind:?} has a NULL {column}, \ + violating the CHECK constraint requiring it to be non-NULL for \ + this kind" )) } From 1b5106a9217a35501886f03c2fb8ab9839f54bd7 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 3 Jun 2026 15:55:16 -0700 Subject: [PATCH 03/20] clearer constraints, a non-null column for physical_id --- nexus/db-model/src/fm/fact_physical_disk.rs | 15 ++++++--------- nexus/db-schema/src/schema.rs | 2 +- nexus/types/src/fm/fact.rs | 11 +++++++++++ schema/crdb/dbinit.sql | 18 +++++++----------- schema/crdb/fm-disk-de-and-facts/up3.sql | 18 +++++++----------- 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index 42a84d904e8..60e9ef25d6d 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -51,10 +51,13 @@ pub struct FmFactPhysicalDisk { /// Preserved unchanged when the fact is carried forward; debug-only. pub created_sitrep_id: DbTypedUuid, pub comment: String, + + /// The physical disk this fact is about. Common to every `kind`, so it is + /// always present (the column is `NOT NULL`). + pub physical_disk_id: DbTypedUuid, pub kind: FmFactPhysicalDiskKind, // Columns for the `ZpoolUnhealthy` kind. - pub physical_disk_id: Option>, pub zpool_id: Option>, pub last_seen_health: Option, pub observed_in_inv: Option>, @@ -80,8 +83,8 @@ impl FmFactPhysicalDisk { case_id: case_id.into(), created_sitrep_id: fact.created_sitrep_id.into(), comment: fact.comment.clone(), + physical_disk_id: disk_fact.physical_disk_id().into(), kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, - physical_disk_id: None, zpool_id: None, last_seen_health: None, observed_in_inv: None, @@ -90,7 +93,6 @@ impl FmFactPhysicalDisk { match disk_fact { DiskFact::ZpoolUnhealthy(p) => Self { kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, - physical_disk_id: Some(p.physical_disk_id.into()), zpool_id: Some(p.zpool_id.into()), last_seen_health: Some(p.last_seen_health.into()), observed_in_inv: Some(p.observed_in_inv.into()), @@ -112,12 +114,7 @@ impl FmFactPhysicalDisk { FmFactPhysicalDiskKind::ZpoolUnhealthy => { FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( ZpoolUnhealthyFactPayload { - physical_disk_id: self - .physical_disk_id - .ok_or_else(|| { - missing_column(kind, "physical_disk_id") - })? - .into(), + physical_disk_id: self.physical_disk_id.into(), zpool_id: self .zpool_id .ok_or_else(|| missing_column(kind, "zpool_id"))? diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index b1275bfe138..62a72c0bad8 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -3250,8 +3250,8 @@ table! { case_id -> Uuid, created_sitrep_id -> Uuid, comment -> Text, + physical_disk_id -> Uuid, kind -> crate::enums::FmFactPhysicalDiskKindEnum, - physical_disk_id -> Nullable, zpool_id -> Nullable, last_seen_health -> Nullable, observed_in_inv -> Nullable, diff --git a/nexus/types/src/fm/fact.rs b/nexus/types/src/fm/fact.rs index ae270e86b5f..714f6660c2b 100644 --- a/nexus/types/src/fm/fact.rs +++ b/nexus/types/src/fm/fact.rs @@ -55,6 +55,17 @@ pub enum DiskFact { ZpoolUnhealthy(ZpoolUnhealthyFactPayload), } +impl DiskFact { + /// The physical disk this fact is about. Common to every kind of disk + /// fact: a Disk case is keyed by its physical disk, and every fact on + /// the case agrees on this value. + pub fn physical_disk_id(&self) -> PhysicalDiskUuid { + match self { + DiskFact::ZpoolUnhealthy(p) => p.physical_disk_id, + } + } +} + /// Payload of a [`DiskFact::ZpoolUnhealthy`] fact. #[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)] pub struct ZpoolUnhealthyFactPayload { diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 939b744188e..b39b88d184a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -7671,12 +7671,16 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( -- Free-form, debug-only comment. comment TEXT NOT NULL, + -- The physical disk this fact is about. Common to every kind of + -- physical-disk fact (the case is keyed by it), so it is always present + -- regardless of `kind`. + physical_disk_id UUID NOT NULL, + -- Which physical-disk fact this row represents. The columns below are -- populated according to this discriminant (see the CHECK constraint). kind omicron.public.fm_fact_physical_disk_kind NOT NULL, -- Columns for a 'zpool_unhealthy' fact. NULL for any other kind. - physical_disk_id UUID, zpool_id UUID, last_seen_health omicron.public.inv_zpool_health, observed_in_inv UUID, @@ -7685,19 +7689,11 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( PRIMARY KEY (sitrep_id, id), CONSTRAINT zpool_unhealthy_columns_present CHECK ( - (kind = 'zpool_unhealthy' - AND physical_disk_id IS NOT NULL + kind = 'zpool_unhealthy' AND zpool_id IS NOT NULL AND last_seen_health IS NOT NULL AND observed_in_inv IS NOT NULL - AND time_observed IS NOT NULL) - OR - (kind != 'zpool_unhealthy' - AND physical_disk_id IS NULL - AND zpool_id IS NULL - AND last_seen_health IS NULL - AND observed_in_inv IS NULL - AND time_observed IS NULL) + AND time_observed IS NOT NULL ) ); diff --git a/schema/crdb/fm-disk-de-and-facts/up3.sql b/schema/crdb/fm-disk-de-and-facts/up3.sql index c095cf8d564..4249378b4a1 100644 --- a/schema/crdb/fm-disk-de-and-facts/up3.sql +++ b/schema/crdb/fm-disk-de-and-facts/up3.sql @@ -13,12 +13,16 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( -- Free-form, debug-only comment. comment TEXT NOT NULL, + -- The physical disk this fact is about. Common to every kind of + -- physical-disk fact (the case is keyed by it), so it is always present + -- regardless of `kind`. + physical_disk_id UUID NOT NULL, + -- Which physical-disk fact this row represents. The columns below are -- populated according to this discriminant (see the CHECK constraint). kind omicron.public.fm_fact_physical_disk_kind NOT NULL, -- Columns for a 'zpool_unhealthy' fact. NULL for any other kind. - physical_disk_id UUID, zpool_id UUID, last_seen_health omicron.public.inv_zpool_health, observed_in_inv UUID, @@ -27,18 +31,10 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( PRIMARY KEY (sitrep_id, id), CONSTRAINT zpool_unhealthy_columns_present CHECK ( - (kind = 'zpool_unhealthy' - AND physical_disk_id IS NOT NULL + kind = 'zpool_unhealthy' AND zpool_id IS NOT NULL AND last_seen_health IS NOT NULL AND observed_in_inv IS NOT NULL - AND time_observed IS NOT NULL) - OR - (kind != 'zpool_unhealthy' - AND physical_disk_id IS NULL - AND zpool_id IS NULL - AND last_seen_health IS NULL - AND observed_in_inv IS NULL - AND time_observed IS NULL) + AND time_observed IS NOT NULL ) ); From 4b665e79053be4b53f6fee5e08e36e4a86129ce4 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 10 Jun 2026 12:36:38 -0700 Subject: [PATCH 04/20] PR feedback: growable fact CHECK constraint, filter deleted disks, tolerate duplicate zpools - The per-kind CHECK constraint on fm_fact_physical_disk is now an implication (kind != 'zpool_unhealthy' OR columns present), so future fact kinds add their own constraint instead of rewriting this one - The fm_analysis in-service disk projection skips soft-deleted physical_disk rows, matching what its comment already claimed - A duplicate physical disk in the zpool listing now logs a warning and keeps the first zpool instead of panicking the background task --- nexus/db-model/src/fm/fact_physical_disk.rs | 6 ++-- nexus/src/app/background/tasks/fm_analysis.rs | 29 ++++++++++++++----- schema/crdb/dbinit.sql | 8 +++-- schema/crdb/fm-disk-de-and-facts/up3.sql | 8 +++-- 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index 60e9ef25d6d..2b85de0e1b4 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -34,8 +34,10 @@ impl_enum_type!( /// Diesel row for the `fm_fact_physical_disk` table. /// /// The payload columns are populated according to `kind`: a column is `Some` -/// iff it belongs to that `kind`'s payload. This is enforced in the database -/// by the per-kind CHECK constraints (e.g. `zpool_unhealthy_columns_present`). +/// if it belongs to that `kind`'s payload, and `None` otherwise (by +/// convention; the per-kind CHECK constraints, e.g. +/// `zpool_unhealthy_columns_present`, enforce only the "present for the +/// matching kind" direction so that future kinds may share columns). #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = fm_fact_physical_disk)] pub struct FmFactPhysicalDisk { diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 17cb3fddd48..f26352627f1 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -227,7 +227,7 @@ impl FmAnalysis { // represented as control plane disks today, so the U.2-only filter // on the underlying query matches reality. // - // This is the executed view from the DB — flipped only after sagas / + // This is the executed view from the DB, flipped only after sagas / // cleaners have actually drained resources, not while a planner is // merely proposing changes. A faulty disk a planner proposes to // expunge is still the diagnoser's concern until the control plane @@ -239,23 +239,36 @@ impl FmAnalysis { .context("failed to load in-service control plane disks")?; let mut in_service_disks_map = IdOrdMap::new(); for (zpool, disk) in zpools_and_disks { - if disk.disk_policy != PhysicalDiskPolicy::InService { + if disk.time_deleted().is_some() + || disk.disk_policy != PhysicalDiskPolicy::InService + { continue; } - in_service_disks_map + let physical_disk_id = disk.id(); + let zpool_id = zpool.id(); + if in_service_disks_map .insert_unique(InServiceDisk { - physical_disk_id: disk.id(), - zpool_id: zpool.id(), + physical_disk_id, + zpool_id, sled_id: disk.sled_id.into(), vendor: disk.vendor, serial: disk.serial, model: disk.model, variant: disk.variant.into(), }) - .expect( - "physical_disk.id is a primary key, so duplicates are \ - impossible", + .is_err() + { + // One live zpool per disk is a code-maintained invariant, + // not a schema constraint. Tolerate a violation rather than + // panicking the analysis task: keep the first zpool seen + // for the disk. + slog::warn!( + &opctx.log, + "multiple live zpools reference the same physical disk"; + "physical_disk_id" => %physical_disk_id, + "zpool_id" => %zpool_id, ); + } } let in_service_disks = Arc::new(in_service_disks_map); let mut builder = fm::analysis_input::Input::builder( diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 172fa967cbb..2490ec9573a 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -7758,12 +7758,16 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( PRIMARY KEY (sitrep_id, id), + -- Each variant validates that the columns it expects are present. + -- Future variants should add their own constraint like this one, + -- leaving existing constraints untouched. CONSTRAINT zpool_unhealthy_columns_present CHECK ( - kind = 'zpool_unhealthy' - AND zpool_id IS NOT NULL + kind != 'zpool_unhealthy' OR ( + zpool_id IS NOT NULL AND last_seen_health IS NOT NULL AND observed_in_inv IS NOT NULL AND time_observed IS NOT NULL + ) ) ); diff --git a/schema/crdb/fm-disk-de-and-facts/up3.sql b/schema/crdb/fm-disk-de-and-facts/up3.sql index 4249378b4a1..51692acc720 100644 --- a/schema/crdb/fm-disk-de-and-facts/up3.sql +++ b/schema/crdb/fm-disk-de-and-facts/up3.sql @@ -30,11 +30,15 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( PRIMARY KEY (sitrep_id, id), + -- Each variant validates that the columns it expects are present. + -- Future variants should add their own constraint like this one, + -- leaving existing constraints untouched. CONSTRAINT zpool_unhealthy_columns_present CHECK ( - kind = 'zpool_unhealthy' - AND zpool_id IS NOT NULL + kind != 'zpool_unhealthy' OR ( + zpool_id IS NOT NULL AND last_seen_health IS NOT NULL AND observed_in_inv IS NOT NULL AND time_observed IS NOT NULL + ) ) ); From 272b368a01feb0b6cbcbe36c13099656d63cdee7 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 10 Jun 2026 13:20:22 -0700 Subject: [PATCH 05/20] [fm] diagnosis engines read their Input from the SitrepBuilder SitrepBuilder::cases is seeded from the open cases of the Input the builder was constructed from, but diagnosis::analyze() previously took the Input as a separate argument; nothing tied the two together, and an engine handed a mismatched pair would panic looking up a case that was never seeded. Store the Input on the builder and have engines read it from there, making the mismatch unrepresentable. --- nexus/fm/src/analysis_input.rs | 1 + nexus/fm/src/builder.rs | 14 ++++++++++++++ nexus/fm/src/diagnosis/mod.rs | 8 ++------ nexus/fm/src/diagnosis/physical_disk.rs | 19 +++++++++---------- nexus/src/app/background/tasks/fm_analysis.rs | 2 +- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/nexus/fm/src/analysis_input.rs b/nexus/fm/src/analysis_input.rs index 2f8e135cbb2..8073e32493d 100644 --- a/nexus/fm/src/analysis_input.rs +++ b/nexus/fm/src/analysis_input.rs @@ -32,6 +32,7 @@ pub use nexus_types::fm::analysis_reports::InputReport as Report; /// is constructed, the inputs are immutable and cannot be modified. To /// construct a new `Input` as part of a preparaation phase, use /// [`Input::builder`]. +#[derive(Debug)] pub struct Input { parent_sitrep: Option>, inv: Arc, diff --git a/nexus/fm/src/builder.rs b/nexus/fm/src/builder.rs index d528b69c0a4..8245943239f 100644 --- a/nexus/fm/src/builder.rs +++ b/nexus/fm/src/builder.rs @@ -24,6 +24,11 @@ pub struct SitrepBuilder<'a> { pub parent_sitrep: Option<&'a fm::Sitrep>, pub sitrep_id: SitrepUuid, pub cases: case::AllCases, + /// The analysis input this builder was constructed from. `cases` is + /// seeded from this input's open cases, so diagnosis engines must read + /// their inputs from here rather than taking a separate `Input` argument + /// that might disagree. + input: &'a analysis_input::Input, closed_cases_copied_forward: &'a IdOrdMap, comment: String, } @@ -66,11 +71,20 @@ impl<'a> SitrepBuilder<'a> { inventory, parent_sitrep, comment: String::new(), + input: inputs, closed_cases_copied_forward, cases, } } + /// The analysis input this builder was constructed from. + /// + /// The returned reference borrows the input (lifetime `'a`), not the + /// builder, so callers may hold it while mutating the builder. + pub fn input(&self) -> &'a analysis_input::Input { + self.input + } + pub fn comment(&self) -> &str { &self.comment } diff --git a/nexus/fm/src/diagnosis/mod.rs b/nexus/fm/src/diagnosis/mod.rs index b62e676fb36..4a2d7d21856 100644 --- a/nexus/fm/src/diagnosis/mod.rs +++ b/nexus/fm/src/diagnosis/mod.rs @@ -9,15 +9,11 @@ //! so the dispatch order does not matter. use crate::SitrepBuilder; -use crate::analysis_input::Input; mod physical_disk; -pub fn analyze( - input: &Input, - builder: &mut SitrepBuilder<'_>, -) -> anyhow::Result<()> { - physical_disk::analyze(input, builder)?; +pub fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { + physical_disk::analyze(builder)?; Ok(()) } diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 3992c9cb46c..ab6bb1d4777 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -5,7 +5,6 @@ //! Disk diagnosis engine. use crate::SitrepBuilder; -use crate::analysis_input::Input; use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; use nexus_types::fm::DiagnosisEngineKind; use nexus_types::fm::{DiskFact, ZpoolUnhealthyFactPayload}; @@ -59,10 +58,10 @@ struct ParentCaseSummary { unhealthy_facts: IdOrdMap, } -pub(super) fn analyze( - input: &Input, - builder: &mut SitrepBuilder<'_>, -) -> anyhow::Result<()> { +pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { + // The input borrow has lifetime 'a, not a borrow of `builder`, so we may + // hold it while mutating the builder below. + let input = builder.input(); let inv_collection_id = input.inventory().id; let inv_time_done = input.inventory().time_done; @@ -172,10 +171,10 @@ pub(super) fn analyze( // is NOT a recovery signal: sled could be powered off, or // inventory could be lossy) for (case_id, summary) in &parent_cases { - let mut case_mut = builder - .cases - .case_mut(case_id) - .expect("case_id came from iterating builder.cases"); + let mut case_mut = builder.cases.case_mut(case_id).expect( + "builder.cases is seeded from the open cases of builder.input(), \ + which is where this case_id came from", + ); match in_service_health.get(&summary.physical_disk_id) { None => { case_mut.close(format!( @@ -392,7 +391,7 @@ mod tests { input, SitrepBuilderRng::from_seed("disk-analyze"), ); - analyze(input, &mut builder).expect("analyze ok"); + analyze(&mut builder).expect("analyze ok"); builder.build(OmicronZoneUuid::new_v4(), Utc::now()) } diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index f26352627f1..3bf66f5432c 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -345,7 +345,7 @@ impl FmAnalysis { ) -> status::AnalysisStatus { let start_time = Utc::now(); let mut sitrep_builder = fm::SitrepBuilder::new(&opctx.log, &inputs); - let result = fm::diagnosis::analyze(&inputs, &mut sitrep_builder); + let result = fm::diagnosis::analyze(&mut sitrep_builder); let end_time = Utc::now(); let (sitrep, report) = sitrep_builder.build(self.nexus_id, end_time); From 14c2a06fbc5a37b283df851400735f8412f9d455 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 10 Jun 2026 13:46:23 -0700 Subject: [PATCH 06/20] drop unused lookup_fm_fact_physical_disk_for_case index Facts are only read during sitrep load, which paginates over the (sitrep_id, id) primary key; nothing queries by case directly. Easy to re-add with a migration if omdb grows a case-scoped fact query. --- schema/crdb/dbinit.sql | 4 ---- schema/crdb/fm-disk-de-and-facts/up4.sql | 1 - schema/crdb/fm-disk-de-and-facts/up4.verify.sql | 2 -- 3 files changed, 7 deletions(-) delete mode 100644 schema/crdb/fm-disk-de-and-facts/up4.sql delete mode 100644 schema/crdb/fm-disk-de-and-facts/up4.verify.sql diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 2490ec9573a..34d17d8f6b4 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -7771,10 +7771,6 @@ CREATE TABLE IF NOT EXISTS omicron.public.fm_fact_physical_disk ( ) ); -CREATE INDEX IF NOT EXISTS - lookup_fm_fact_physical_disk_for_case -ON omicron.public.fm_fact_physical_disk (sitrep_id, case_id); - CREATE TABLE IF NOT EXISTS omicron.public.fm_ereport_in_case ( -- ID of this association. When an ereport is assigned to a case, that -- association is assigned a UUID. These are used primarily to aid in diff --git a/schema/crdb/fm-disk-de-and-facts/up4.sql b/schema/crdb/fm-disk-de-and-facts/up4.sql deleted file mode 100644 index 83d3461270d..00000000000 --- a/schema/crdb/fm-disk-de-and-facts/up4.sql +++ /dev/null @@ -1 +0,0 @@ -CREATE INDEX IF NOT EXISTS lookup_fm_fact_physical_disk_for_case ON omicron.public.fm_fact_physical_disk (sitrep_id, case_id); diff --git a/schema/crdb/fm-disk-de-and-facts/up4.verify.sql b/schema/crdb/fm-disk-de-and-facts/up4.verify.sql deleted file mode 100644 index 788cdeebc19..00000000000 --- a/schema/crdb/fm-disk-de-and-facts/up4.verify.sql +++ /dev/null @@ -1,2 +0,0 @@ --- DO NOT EDIT. Generated by test_migration_verification_files. -SELECT CAST(IF((SELECT true WHERE EXISTS (SELECT index_name FROM omicron.crdb_internal.table_indexes WHERE descriptor_name = 'fm_fact_physical_disk' AND index_name = 'lookup_fm_fact_physical_disk_for_case')),'true','Schema change verification failed: index lookup_fm_fact_physical_disk_for_case on table fm_fact_physical_disk does not exist') AS BOOL); From d2a676775e9a1f3fe7829e507960c2cff4842537 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 10 Jun 2026 13:50:48 -0700 Subject: [PATCH 07/20] [fm] disk diagnoser: index parent cases by disk Replace the per-disk linear scan of parent cases with a one-time inverse index. First case ID wins on (pathological) collisions, same as the scan it replaces. --- nexus/fm/src/diagnosis/physical_disk.rs | 26 +++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index ab6bb1d4777..f7b2658f8cb 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -161,6 +161,19 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { }) .collect(); + // Inverse index: which parent case is about which disk. Cases are + // per-disk, so a disk with two parent cases is pathological; keep the + // lowest case ID. + let mut case_by_disk: BTreeMap< + PhysicalDiskUuid, + (CaseUuid, &ParentCaseSummary), + > = BTreeMap::new(); + for (case_id, summary) in &parent_cases { + case_by_disk + .entry(summary.physical_disk_id) + .or_insert((*case_id, summary)); + } + // For each parent case, decide what to do based on its disk's current // state: // - disk no longer in service → close the case (expungement) @@ -210,17 +223,10 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { continue; } - let parent_for_disk = - parent_cases.iter().find_map(|(case_id, summary)| { - if summary.physical_disk_id == disk.physical_disk_id { - Some((*case_id, summary)) - } else { - None - } - }); + let parent_for_disk = case_by_disk.get(&disk.physical_disk_id).copied(); let case_id_for_fact = match parent_for_disk { - // Parent case already has an accurate fact — fully covered. + // Parent case already has an accurate fact; fully covered. Some((_, summary)) if summary .unhealthy_facts @@ -232,7 +238,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // Parent case exists; its stale facts were removed above. // Refresh under the same case. Some((case_id, _)) => case_id, - // No parent case for this disk — open one. + // No parent case for this disk; open one. None => { let mut new_case = builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk); From 82ef44fd5bd6e156df2eee547bfe7626d39b3d3c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 11 Jun 2026 12:49:48 -0700 Subject: [PATCH 08/20] simplify comments --- nexus/db-model/src/fm/fact_physical_disk.rs | 17 ++------ nexus/fm/src/analysis_input.rs | 4 +- nexus/fm/src/builder.rs | 6 +-- nexus/fm/src/builder/rng.rs | 5 --- nexus/fm/src/diagnosis/mod.rs | 3 +- nexus/fm/src/diagnosis/physical_disk.rs | 43 ++++++++----------- nexus/src/app/background/tasks/fm_analysis.rs | 7 +-- nexus/types/src/fm/case.rs | 3 -- nexus/types/src/fm/fact.rs | 2 +- nexus/types/src/in_service_disk.rs | 6 +-- 10 files changed, 32 insertions(+), 64 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index 2b85de0e1b4..f81df308b35 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -34,10 +34,7 @@ impl_enum_type!( /// Diesel row for the `fm_fact_physical_disk` table. /// /// The payload columns are populated according to `kind`: a column is `Some` -/// if it belongs to that `kind`'s payload, and `None` otherwise (by -/// convention; the per-kind CHECK constraints, e.g. -/// `zpool_unhealthy_columns_present`, enforce only the "present for the -/// matching kind" direction so that future kinds may share columns). +/// if it belongs to that `kind`'s payload, and `None` otherwise. #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = fm_fact_physical_disk)] pub struct FmFactPhysicalDisk { @@ -68,11 +65,7 @@ pub struct FmFactPhysicalDisk { impl FmFactPhysicalDisk { /// Build a row from a fact's shared metadata (`fact`) and its - /// already-dispatched physical-disk payload (`disk_fact`). - /// - /// Callers route each fact to its engine's table by matching on - /// [`fact.payload`](fm::case::Fact::payload) and pass the matched payload - /// here, so this never has to interpret another engine's payload. + /// physical-disk payload (`disk_fact`). pub fn from_sitrep( sitrep_id: impl Into>, case_id: impl Into>, @@ -106,10 +99,8 @@ impl FmFactPhysicalDisk { /// Reconstruct an in-memory fact from a row. /// - /// The payload columns the database's CHECK constraint guarantees are - /// non-NULL for this `kind` are unwrapped; a NULL where one is required - /// indicates a corrupt row (e.g. hand-edited) and yields an internal - /// error rather than a panic. + /// A NULL in a column the CHECK constraint requires for this `kind` + /// yields an internal error rather than a panic. pub fn into_fact(self) -> Result { let kind = self.kind; let payload = match kind { diff --git a/nexus/fm/src/analysis_input.rs b/nexus/fm/src/analysis_input.rs index 8073e32493d..93751409172 100644 --- a/nexus/fm/src/analysis_input.rs +++ b/nexus/fm/src/analysis_input.rs @@ -41,7 +41,6 @@ pub struct Input { new_ereports: IdOrdMap, open_cases: IdOrdMap, closed_cases_copied_forward: IdOrdMap, - /// All control plane managed disks in_service_disks: Arc>, } @@ -70,8 +69,7 @@ impl Input { } /// All control-plane-managed disks (`physical_disk.disk_policy = - /// in_service` in the DB), indexed by `physical_disk_id`. See the - /// field-level documentation on `Input::in_service_disks` for semantics. + /// in_service` in the DB), indexed by `physical_disk_id`. pub fn in_service_disks(&self) -> &IdOrdMap { &self.in_service_disks } diff --git a/nexus/fm/src/builder.rs b/nexus/fm/src/builder.rs index 8245943239f..2037fbe5ea8 100644 --- a/nexus/fm/src/builder.rs +++ b/nexus/fm/src/builder.rs @@ -24,10 +24,8 @@ pub struct SitrepBuilder<'a> { pub parent_sitrep: Option<&'a fm::Sitrep>, pub sitrep_id: SitrepUuid, pub cases: case::AllCases, - /// The analysis input this builder was constructed from. `cases` is - /// seeded from this input's open cases, so diagnosis engines must read - /// their inputs from here rather than taking a separate `Input` argument - /// that might disagree. + /// The analysis input this builder was constructed from; `cases` is + /// seeded from its open cases. input: &'a analysis_input::Input, closed_cases_copied_forward: &'a IdOrdMap, comment: String, diff --git a/nexus/fm/src/builder/rng.rs b/nexus/fm/src/builder/rng.rs index 9ba02eca5cc..86b6a1a7da9 100644 --- a/nexus/fm/src/builder/rng.rs +++ b/nexus/fm/src/builder/rng.rs @@ -61,11 +61,6 @@ impl SitrepBuilderRng { } } -/// Per-case child RNGs. Each `next_*` returns the next deterministic UUID -/// in its stream; collisions across calls are statistically impossible, but -/// callers in `builder/case.rs` still guard inserts with a `contains_key` -/// loop so that a future change here (e.g., reseeding mid-build) can't -/// silently corrupt a case. #[derive(Clone, Debug)] pub(super) struct CaseBuilderRng { ereport_assignment_rng: TypedUuidRng, diff --git a/nexus/fm/src/diagnosis/mod.rs b/nexus/fm/src/diagnosis/mod.rs index 4a2d7d21856..48a3c3a184c 100644 --- a/nexus/fm/src/diagnosis/mod.rs +++ b/nexus/fm/src/diagnosis/mod.rs @@ -5,8 +5,7 @@ //! Fault management diagnosis engines. //! //! Each submodule defines one diagnosis engine (DE). `analyze` dispatches to -//! each engine in turn; engines are deterministic and idempotent per RFD 603, -//! so the dispatch order does not matter. +//! each engine in turn; engines are deterministic and idempotent per RFD 603. use crate::SitrepBuilder; diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index f7b2658f8cb..794c0cdca1d 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -13,8 +13,7 @@ use omicron_uuid_kinds::{CaseUuid, FactUuid, PhysicalDiskUuid, ZpoolUuid}; use std::collections::BTreeMap; /// A [`DiskFact::ZpoolUnhealthy`] payload paired with the `FactUuid` it -/// lives under. Used to build in-memory indices over facts during -/// analysis; not serialized. +/// lives under. #[derive(Clone, Copy, Debug)] struct ZpoolUnhealthyFact { fact_id: FactUuid, @@ -59,8 +58,6 @@ struct ParentCaseSummary { } pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { - // The input borrow has lifetime 'a, not a borrow of `builder`, so we may - // hold it while mutating the builder below. let input = builder.input(); let inv_collection_id = input.inventory().id; let inv_time_done = input.inventory().time_done; @@ -79,8 +76,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // The current health snapshot for every in-service disk, keyed by // physical_disk_id. Absence from this index is a positive signal that // the control plane has moved on from the disk (expungement / - // decommissioning); see prepare_inputs in - // nexus/src/app/background/tasks/fm_analysis.rs. + // decommissioning); see the analysis task's input preparation. let in_service_health: IdOrdMap = input .in_service_disks() .iter() @@ -91,11 +87,9 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { }) .collect(); - // Index parent-forwarded Disk cases from the input — the state copied - // from the parent sitrep. - // Every case is about one physical disk; we derive the disk - // from its facts. Skip (with a warning) any case we can't safely - // interpret. + // Index the Disk cases copied forward from the parent sitrep. Every case + // is about one physical disk; we derive the disk from its facts. Skip + // (with a warning) any case we can't safely interpret. let parent_cases: BTreeMap = input .open_cases() .iter() @@ -287,9 +281,9 @@ mod tests { }; use std::sync::Arc; - /// Synthesize a synthetic in-service disk set from a list of zpool IDs. - /// Each zpool gets its own fresh `PhysicalDiskUuid` and dummy identity - /// facts — tests in this module only care about the zpool dimension. + /// Make an in-service disk set from a list of zpool IDs. Each zpool gets + /// its own fresh `PhysicalDiskUuid` and dummy identity facts; tests in + /// this module only care about the zpool dimension. fn mk_in_service( zpool_ids: impl IntoIterator, ) -> IdOrdMap { @@ -385,9 +379,8 @@ mod tests { input } - /// Run `disk::analyze` over an input and return the resulting Sitrep - /// along with the analysis report (whose log entries the close-comment - /// assertions in `closes_*` tests inspect). + /// Run `analyze` over an input and return the resulting Sitrep along + /// with the analysis report. fn run_analyze( log: &slog::Logger, input: &Input, @@ -455,7 +448,7 @@ mod tests { } } - /// Helper: collect (case, fact, DiskFact) triples for every fact on a + /// Collect (case, fact, DiskFact) triples for every fact on a /// physical-disk case in a sitrep. Optionally filtered to open cases /// only. fn disk_facts( @@ -590,9 +583,9 @@ mod tests { setup("disk_close_on_expungement"); let target = zpools[0]; set_health(&mut collection, target, ZpoolHealth::Degraded); - // Target is NOT in-service in this sitrep (just expunged). + // Target is NOT in-service in this sitrep (just expunged), so + // disk_id_for fabricates a stable PhysicalDiskUuid for it. let in_service = mk_in_service(zpools.iter().copied().skip(1)); - // Target isn't in the in-service set; fabricate a stable PhysicalDiskUuid. let target_disk_id = disk_id_for(&in_service, target); let parent_id = SitrepUuid::new_v4(); let parent = make_parent_with_disk_case( @@ -704,9 +697,9 @@ mod tests { logctx.cleanup_successful(); } - /// When the parent sitrep's fact content matches the diagnosis engine's current - /// observation, the fact carries forward with the same UUID — no - /// remove-and-readd churn. + /// When the parent sitrep's fact content matches the diagnosis engine's + /// current observation, the fact carries forward with the same UUID, + /// with no remove-and-readd churn. #[test] fn fact_uuid_stable_when_observation_unchanged() { let (logctx, mut collection, zpools) = setup("disk_fact_uuid_stable"); @@ -758,7 +751,7 @@ mod tests { /// When the parent's fact recorded a different `last_seen_health` than /// what we observe now, the diagnosis engine removes the stale fact and emits /// a fresh one (new UUID). The case stays open because the zpool is - /// still unhealthy — just with a different value. + /// still unhealthy, just with a different value. #[test] fn fact_uuid_rotates_when_observation_changes() { let (logctx, mut collection, zpools) = setup("disk_fact_uuid_rotates"); @@ -808,7 +801,7 @@ mod tests { } } // The case itself should still be the same one that was carried - // forward — only the fact rotated. + // forward; only the fact rotated. assert!(open[0].0.is_open()); logctx.cleanup_successful(); } diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 3bf66f5432c..e355583b159 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -227,11 +227,8 @@ impl FmAnalysis { // represented as control plane disks today, so the U.2-only filter // on the underlying query matches reality. // - // This is the executed view from the DB, flipped only after sagas / - // cleaners have actually drained resources, not while a planner is - // merely proposing changes. A faulty disk a planner proposes to - // expunge is still the diagnoser's concern until the control plane - // has actually moved on. + // See `nexus_types::in_service_disk` for why FM reads the executed + // DB view rather than the target blueprint. let zpools_and_disks = self .datastore .zpool_list_all_external_batched(opctx) diff --git a/nexus/types/src/fm/case.rs b/nexus/types/src/fm/case.rs index b6b8576e68f..d96fc723aed 100644 --- a/nexus/types/src/fm/case.rs +++ b/nexus/types/src/fm/case.rs @@ -174,9 +174,6 @@ impl CaseEreport { /// /// The `payload` is a fully-typed [`FactPayload`] whose variant is owned by /// the case's diagnosis engine (see [`Metadata::de`]). -/// -/// `Eq`/`PartialEq` derive over all fields, including the typed payload. -/// This is the equality the DB round-trip test needs. #[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] pub struct Fact { pub id: FactUuid, diff --git a/nexus/types/src/fm/fact.rs b/nexus/types/src/fm/fact.rs index 714f6660c2b..490ee7446f1 100644 --- a/nexus/types/src/fm/fact.rs +++ b/nexus/types/src/fm/fact.rs @@ -72,7 +72,7 @@ pub struct ZpoolUnhealthyFactPayload { /// The physical disk this fact (and its parent case) is about. /// Every fact on a Disk case must agree on this value. pub physical_disk_id: PhysicalDiskUuid, - /// The zpool whose health was observed. Kept for provenance — the + /// The zpool whose health was observed. Kept for provenance: the /// case is keyed by `physical_disk_id`, but knowing the exact zpool /// makes the fact self-describing when read in isolation. pub zpool_id: ZpoolUuid, diff --git a/nexus/types/src/in_service_disk.rs b/nexus/types/src/in_service_disk.rs index bc7814ce2cb..b3ce6c2a597 100644 --- a/nexus/types/src/in_service_disk.rs +++ b/nexus/types/src/in_service_disk.rs @@ -2,15 +2,15 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! "Currently in-service control-plane disks" — the executed view from +//! "Currently in-service control-plane disks": the executed view from //! the `physical_disk` and `zpool` DB tables. //! //! This is distinct from the planned view in `BlueprintPhysicalDiskConfig`: //! a disk is in this set only after the control plane has actually committed //! to managing it (`physical_disk.disk_policy = 'in_service'`), not while a //! planner is merely proposing to expunge or adopt it. Consumers that need -//! the *committed* view of which disks are part of the rack — fault -//! management diagnosers in particular — should read this rather than the +//! the *committed* view of which disks are part of the rack (fault +//! management diagnosers in particular) should read this rather than the //! target blueprint. use crate::external_api::physical_disk::PhysicalDiskKind; From 6f2e4a59af71db57cbb94afda7b0f982358d785d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 11 Jun 2026 13:49:46 -0700 Subject: [PATCH 09/20] [fm] disk diagnoser: close uninterpretable cases; deflake omdb test Hardening from a pre-review pass: - Include zpool_id in the "same observation" comparison for facts, so a zpool that is destroyed and recreated (still unhealthy) rotates the fact instead of carrying a stale zpool reference forward. - Close cases the diagnosis engine cannot interpret (foreign fact payload, facts disagreeing on the disk, no facts) and duplicate cases for the same disk, rather than carrying them forward unprocessable forever. Closing is safe for fault coverage: detection is independent of case bookkeeping, so a still-faulty disk gets a fresh well-formed case in the same analysis pass. Reasons are a typed enum (UninterpretableCase) surfaced in the close comment and warn logs. - In the omdb output test, drive the FM tasks to their steady state with explicit background task activations (analysis -> loader -> analysis -> rendezvous) instead of racing the watch-channel triggers. This makes the expected output deterministic for both fm_analysis and fm_rendezvous, and removes the sitrep_load_rx test plumbing (NexusServer trait method, accessors, wait helper) that existed only to support the old wait. --- dev-tools/omdb/tests/test_all_output.rs | 26 +- nexus/fm/src/diagnosis/physical_disk.rs | 570 +++++++++++++----- .../app/background/tasks/fm_sitrep_load.rs | 4 +- nexus/src/app/mod.rs | 9 +- nexus/src/lib.rs | 6 - nexus/test-interface/src/lib.rs | 4 - nexus/test-utils/src/nexus_test.rs | 29 - nexus/types/src/fm.rs | 4 - 8 files changed, 451 insertions(+), 201 deletions(-) diff --git a/dev-tools/omdb/tests/test_all_output.rs b/dev-tools/omdb/tests/test_all_output.rs index 42bde1b627f..1a43237539c 100644 --- a/dev-tools/omdb/tests/test_all_output.rs +++ b/dev-tools/omdb/tests/test_all_output.rs @@ -11,6 +11,7 @@ use dropshot::Method; use expectorate::assert_contents; use gateway_client::ClientInfo as _; use http::StatusCode; +use nexus_test_utils::background::activate_background_task; use nexus_test_utils::wait_for_producer; use nexus_test_utils::{OXIMETER_UUID, PRODUCER_UUID}; use nexus_test_utils_macros::nexus_test; @@ -187,11 +188,26 @@ async fn test_omdb_success_cases() { .wait_for_at_least_one_inventory_collection(Duration::from_secs(60)) .await; - // Wait until `fm_analysis` has committed at least one sitrep, so that the - // omdb snapshot for FM tasks is stable. (Otherwise sitrep IDs render as - // `None` or `Some(...)` depending on whether the task's natural cadence - // had landed by the time we sample it.) - cptestctx.wait_for_at_least_one_sitrep(Duration::from_secs(60)).await; + // Drive the FM task pipeline to its steady state, so that the omdb + // snapshot of each task's last completed activation is deterministic + // rather than racing the tasks' watch-channel triggers: + // + // 1. `fm_analysis` commits the first sitrep (unless its natural cadence + // already has). This run reports "committed new sitrep". + // 2. `fm_sitrep_loader` loads that sitrep and publishes it on the sitrep + // watch channel. + // 3. `fm_analysis` re-runs with the loaded sitrep as its parent and + // reports "no changes" -- the steady-state output asserted below. + // (No later activation ever commits another sitrep here: this + // environment has no in-service control plane disks and no + // consumable ereports, so every post-load analysis is a no-op.) + // 4. `fm_rendezvous` runs against the loaded sitrep, so its status shows + // the executed operations rather than "no FM situation report loaded". + let lockstep_client = &cptestctx.lockstep_client; + activate_background_task(lockstep_client, "fm_analysis").await; + activate_background_task(lockstep_client, "fm_sitrep_loader").await; + activate_background_task(lockstep_client, "fm_analysis").await; + activate_background_task(lockstep_client, "fm_rendezvous").await; let mut output = String::new(); diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 794c0cdca1d..401251c60e1 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -6,6 +6,7 @@ use crate::SitrepBuilder; use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; +use nexus_types::fm; use nexus_types::fm::DiagnosisEngineKind; use nexus_types::fm::{DiskFact, ZpoolUnhealthyFactPayload}; use nexus_types::inventory::ZpoolHealth; @@ -57,6 +58,67 @@ struct ParentCaseSummary { unhealthy_facts: IdOrdMap, } +/// Why a parent-forwarded Disk case could not be interpreted. +/// +/// Uninterpretable cases are closed by [`analyze`]: an open case this engine +/// cannot process would otherwise be carried forward into every future +/// sitrep with no path to closure. +#[derive(Debug, Eq, PartialEq, thiserror::Error)] +enum UninterpretableCase { + #[error( + "fact {fact_id} does not belong to the physical-disk diagnosis engine" + )] + ForeignFactPayload { fact_id: FactUuid }, + #[error( + "facts reference different physical disks ({expected} and {found}, \ + 1 expected)" + )] + DisagreeingDisks { expected: PhysicalDiskUuid, found: PhysicalDiskUuid }, + #[error("case has no facts, so the disk it concerns cannot be determined")] + NoFacts, +} + +/// Summarize one parent-forwarded Disk case, or explain why it cannot be +/// interpreted. +fn summarize_case( + case: &fm::Case, +) -> Result { + let mut unhealthy_facts: IdOrdMap = IdOrdMap::new(); + let mut case_disk_id: Option = None; + for fact in case.facts.iter() { + // Every fact on a physical-disk case must carry a physical-disk + // payload; a foreign payload is a data-model violation. + let Some(disk_fact) = fact.payload.as_physical_disk() else { + return Err(UninterpretableCase::ForeignFactPayload { + fact_id: fact.id, + }); + }; + match disk_fact { + DiskFact::ZpoolUnhealthy(payload) => { + let payload = *payload; + let disk_id = + *case_disk_id.get_or_insert(payload.physical_disk_id); + if disk_id != payload.physical_disk_id { + return Err(UninterpretableCase::DisagreeingDisks { + expected: disk_id, + found: payload.physical_disk_id, + }); + } + unhealthy_facts + .insert_unique(ZpoolUnhealthyFact { + fact_id: fact.id, + payload, + }) + .expect("fact ids are unique within a case"); + } + } + } + let Some(physical_disk_id) = case_disk_id else { + return Err(UninterpretableCase::NoFacts); + }; + Ok(ParentCaseSummary { physical_disk_id, unhealthy_facts }) +} + pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { let input = builder.input(); let inv_collection_id = input.inventory().id; @@ -88,76 +150,48 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { .collect(); // Index the Disk cases copied forward from the parent sitrep. Every case - // is about one physical disk; we derive the disk from its facts. Skip - // (with a warning) any case we can't safely interpret. - let parent_cases: BTreeMap = input + // is about one physical disk; we derive the disk from its facts. + let mut parent_cases = BTreeMap::::new(); + let mut uninterpretable = Vec::<(CaseUuid, UninterpretableCase)>::new(); + for case in input .open_cases() .iter() .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) - .filter_map(|c| { - let case_id = c.id; - let mut unhealthy_facts: IdOrdMap = - IdOrdMap::new(); - let mut case_disk_id: Option = None; - for fact in c.facts.iter() { - // Every fact on a physical-disk case must carry a - // physical-disk payload. A foreign payload is a data-model - // violation; skip the whole case rather than acting on - // inconsistent state. - let Some(disk_fact) = fact.payload.as_physical_disk() else { - slog::warn!( - &builder.log, - "skipping Disk case: fact payload does not belong to \ - the physical-disk diagnosis engine"; - "case_id" => %case_id, - "fact_id" => %fact.id, - ); - return None; - }; - match disk_fact { - DiskFact::ZpoolUnhealthy(payload) => { - let payload = *payload; - let disk_id = *case_disk_id - .get_or_insert(payload.physical_disk_id); - if disk_id != payload.physical_disk_id { - slog::warn!( - &builder.log, - "skipping Disk case: facts reference \ - different physical disks (1 expected)"; - "case_id" => %case_id, - "expected_physical_disk_id" => %disk_id, - "fact_physical_disk_id" => - %payload.physical_disk_id, - ); - return None; - } - unhealthy_facts - .insert_unique(ZpoolUnhealthyFact { - fact_id: fact.id, - payload, - }) - .expect("fact ids are unique within a case"); - } - } + { + match summarize_case(case) { + Ok(summary) => { + parent_cases.insert(case.id, summary); } - let Some(physical_disk_id) = case_disk_id else { + Err(reason) => { slog::warn!( &builder.log, - "skipping Disk case with no facts; cannot derive disk id"; - "case_id" => %case_id, + "closing uninterpretable Disk case"; + "case_id" => %case.id, + "reason" => %reason, ); - return None; - }; - Some(( - case_id, - ParentCaseSummary { physical_disk_id, unhealthy_facts }, - )) - }) - .collect(); + uninterpretable.push((case.id, reason)); + } + } + } + + // Close the cases we couldn't interpret, so they don't ride along as + // open-but-unprocessable in every future sitrep. This is safe with + // respect to fault coverage: detection below is independent of case + // bookkeeping, so if a closed case concerned a disk that is genuinely + // unhealthy and in service, a fresh, well-formed case is opened in this + // same pass. + for (case_id, reason) in uninterpretable { + builder + .cases + .case_mut(&case_id) + .expect("case came from builder.input()'s open cases") + .close(format!("cannot interpret case: {reason}")); + } // Inverse index: which parent case is about which disk. Cases are // per-disk, so a disk with two parent cases is pathological; keep the - // lowest case ID. + // lowest case ID and close the rest as duplicates. (A half-maintained + // duplicate would otherwise decay into an uninterpretable empty case.) let mut case_by_disk: BTreeMap< PhysicalDiskUuid, (CaseUuid, &ParentCaseSummary), @@ -167,18 +201,39 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { .entry(summary.physical_disk_id) .or_insert((*case_id, summary)); } + for (case_id, summary) in &parent_cases { + let (kept_case_id, _) = case_by_disk[&summary.physical_disk_id]; + if *case_id != kept_case_id { + slog::warn!( + &builder.log, + "closing duplicate Disk case"; + "case_id" => %case_id, + "kept_case_id" => %kept_case_id, + "physical_disk_id" => %summary.physical_disk_id, + ); + builder + .cases + .case_mut(case_id) + .expect("case came from builder.input()'s open cases") + .close(format!( + "duplicate of case {kept_case_id} for disk {}", + summary.physical_disk_id, + )); + } + } - // For each parent case, decide what to do based on its disk's current - // state: + // For each disk's surviving parent case, decide what to do based on the + // disk's current state: // - disk no longer in service → close the case (expungement) // - disk's zpool back to Online → close the case (recovery) - // - disk still unhealthy → drop any facts whose recorded health no - // longer matches; the matching loop below will re-add a fresh fact + // - disk still unhealthy → drop any facts whose recorded observation + // (zpool + health) no longer matches; the matching loop below will + // re-add a fresh fact // - disk in service but absent from inventory → leave alone (absence // is NOT a recovery signal: sled could be powered off, or // inventory could be lossy) - for (case_id, summary) in &parent_cases { - let mut case_mut = builder.cases.case_mut(case_id).expect( + for &(case_id, summary) in case_by_disk.values() { + let mut case_mut = builder.cases.case_mut(&case_id).expect( "builder.cases is seeded from the open cases of builder.input(), \ which is where this case_id came from", ); @@ -198,7 +253,9 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { continue; }; for fact_ref in summary.unhealthy_facts.iter() { - if fact_ref.payload.last_seen_health != current_health { + if fact_ref.payload.last_seen_health != current_health + || fact_ref.payload.zpool_id != snap.zpool_id + { case_mut.remove_fact(fact_ref.fact_id); } } @@ -208,7 +265,8 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // For each currently-faulty in-service disk: ensure a case exists // (reusing the parent-forwarded one for this disk if any) and add a - // fresh fact if one with this exact health isn't already present. + // fresh fact if one with this exact observation (zpool + health) isn't + // already present. for disk in in_service_health.iter() { let Some(current_health) = disk.zpool_health else { continue; @@ -222,10 +280,10 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { let case_id_for_fact = match parent_for_disk { // Parent case already has an accurate fact; fully covered. Some((_, summary)) - if summary - .unhealthy_facts - .iter() - .any(|f| f.payload.last_seen_health == current_health) => + if summary.unhealthy_facts.iter().any(|f| { + f.payload.last_seen_health == current_health + && f.payload.zpool_id == disk.zpool_id + }) => { continue; } @@ -394,45 +452,63 @@ mod tests { builder.build(OmicronZoneUuid::new_v4(), Utc::now()) } - fn make_parent_with_disk_case( + /// Make a `ZpoolUnhealthy` (Degraded) fact for the given disk and zpool. + fn make_degraded_fact( parent_sitrep_id: SitrepUuid, inv_collection_id: omicron_uuid_kinds::CollectionUuid, physical_disk_id: PhysicalDiskUuid, zpool_id: ZpoolUuid, - ) -> Sitrep { - let mut cases = iddqd::IdOrdMap::new(); - let case_id = omicron_uuid_kinds::CaseUuid::new_v4(); - let mut facts = iddqd::IdOrdMap::new(); - facts - .insert_unique(fm::case::Fact { - id: omicron_uuid_kinds::FactUuid::new_v4(), - created_sitrep_id: parent_sitrep_id, - payload: DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { - physical_disk_id, - zpool_id, - last_seen_health: ZpoolHealth::Degraded, - observed_in_inv: inv_collection_id, - time_observed: Utc::now(), - }) - .into(), - comment: format!("zpool {zpool_id} degraded"), - }) - .unwrap(); - cases - .insert_unique(fm::Case { - id: case_id, - metadata: fm::case::Metadata { - created_sitrep_id: parent_sitrep_id, - closed_sitrep_id: None, - de: DiagnosisEngineKind::PhysicalDisk, - comment: format!("zpool {zpool_id} degraded"), - }, - ereports: Default::default(), - alerts_requested: Default::default(), - support_bundles_requested: Default::default(), - facts, + ) -> fm::case::Fact { + fm::case::Fact { + id: omicron_uuid_kinds::FactUuid::new_v4(), + created_sitrep_id: parent_sitrep_id, + payload: DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id, + zpool_id, + last_seen_health: ZpoolHealth::Degraded, + observed_in_inv: inv_collection_id, + time_observed: Utc::now(), }) - .unwrap(); + .into(), + comment: format!("zpool {zpool_id} degraded"), + } + } + + /// Make an open `PhysicalDisk` case carrying the given facts. + fn make_disk_case( + case_id: omicron_uuid_kinds::CaseUuid, + parent_sitrep_id: SitrepUuid, + facts: impl IntoIterator, + ) -> fm::Case { + let mut fact_map = iddqd::IdOrdMap::new(); + for fact in facts { + fact_map.insert_unique(fact).unwrap(); + } + fm::Case { + id: case_id, + metadata: fm::case::Metadata { + created_sitrep_id: parent_sitrep_id, + closed_sitrep_id: None, + de: DiagnosisEngineKind::PhysicalDisk, + comment: "a disk case".to_string(), + }, + ereports: Default::default(), + alerts_requested: Default::default(), + support_bundles_requested: Default::default(), + facts: fact_map, + } + } + + /// Make a parent sitrep containing the given cases. + fn make_parent_sitrep( + parent_sitrep_id: SitrepUuid, + inv_collection_id: omicron_uuid_kinds::CollectionUuid, + cases: impl IntoIterator, + ) -> Sitrep { + let mut case_map = iddqd::IdOrdMap::new(); + for case in cases { + case_map.insert_unique(case).unwrap(); + } Sitrep { metadata: fm::SitrepMetadata { id: parent_sitrep_id, @@ -443,11 +519,31 @@ mod tests { next_inv_min_time_started: Utc::now(), comment: String::new(), }, - cases, + cases: case_map, ereports_by_id: Default::default(), } } + fn make_parent_with_disk_case( + parent_sitrep_id: SitrepUuid, + inv_collection_id: omicron_uuid_kinds::CollectionUuid, + physical_disk_id: PhysicalDiskUuid, + zpool_id: ZpoolUuid, + ) -> Sitrep { + let fact = make_degraded_fact( + parent_sitrep_id, + inv_collection_id, + physical_disk_id, + zpool_id, + ); + let case = make_disk_case( + omicron_uuid_kinds::CaseUuid::new_v4(), + parent_sitrep_id, + [fact], + ); + make_parent_sitrep(parent_sitrep_id, inv_collection_id, [case]) + } + /// Collect (case, fact, DiskFact) triples for every fact on a /// physical-disk case in a sitrep. Optionally filtered to open cases /// only. @@ -643,56 +739,180 @@ mod tests { logctx.cleanup_successful(); } - /// A parent Disk case with zero facts has no derivable disk ID, so - /// the diagnosis engine leaves it alone (carried forward unchanged). + /// A parent Disk case with zero facts has no derivable disk ID; the + /// engine closes it as uninterpretable rather than carrying an + /// unprocessable open case forward into every future sitrep. #[test] - fn empty_case_is_left_open() { - let (logctx, collection, _zpools) = setup("disk_empty_case_left_open"); + fn empty_case_is_closed() { + let (logctx, collection, _zpools) = setup("disk_empty_case_closed"); let in_service = mk_in_service(std::iter::empty()); let parent_sitrep_id = SitrepUuid::new_v4(); let empty_case_id = omicron_uuid_kinds::CaseUuid::new_v4(); - let mut parent_cases = iddqd::IdOrdMap::new(); - parent_cases - .insert_unique(fm::Case { - id: empty_case_id, - metadata: fm::case::Metadata { - created_sitrep_id: parent_sitrep_id, - closed_sitrep_id: None, - de: DiagnosisEngineKind::PhysicalDisk, - comment: "an open case with no facts".to_string(), - }, - ereports: Default::default(), - alerts_requested: Default::default(), - support_bundles_requested: Default::default(), - facts: Default::default(), - }) - .unwrap(); - let parent = Sitrep { - metadata: fm::SitrepMetadata { - id: parent_sitrep_id, - inv_collection_id: collection.id, - creator_id: OmicronZoneUuid::new_v4(), - parent_sitrep_id: None, - time_created: Utc::now(), - next_inv_min_time_started: Utc::now(), - comment: String::new(), - }, - cases: parent_cases, - ereports_by_id: Default::default(), - }; + let empty_case = make_disk_case(empty_case_id, parent_sitrep_id, []); + let parent = + make_parent_sitrep(parent_sitrep_id, collection.id, [empty_case]); let input = build_input(collection, Some(parent), in_service); - let (sitrep, _report) = run_analyze(&logctx.log, &input); + let (sitrep, report) = run_analyze(&logctx.log, &input); let case = sitrep .cases .iter() .find(|c| c.id == empty_case_id) .expect("empty case should still be in the output sitrep"); + assert!(!case.is_open(), "uninterpretable empty case should be closed",); + let report_str = format!("{}", report.display_multiline(0)); + assert!( + report_str.contains("cannot interpret case"), + "close comment should say the case was uninterpretable, got: \ + {report_str}", + ); + logctx.cleanup_successful(); + } + + /// Two open parent cases about the same disk: the engine keeps and + /// maintains the one with the lowest case ID, and closes the other as a + /// duplicate. (A half-maintained duplicate would otherwise decay into + /// an uninterpretable empty case.) + #[test] + fn duplicate_case_is_closed() { + let (logctx, mut collection, zpools) = setup("disk_duplicate_closed"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + + let id_a = omicron_uuid_kinds::CaseUuid::new_v4(); + let id_b = omicron_uuid_kinds::CaseUuid::new_v4(); + let (kept_id, dup_id) = + if id_a < id_b { (id_a, id_b) } else { (id_b, id_a) }; + let kept_fact = make_degraded_fact( + parent_id, + collection.id, + target_disk_id, + target, + ); + let kept_fact_id = kept_fact.id; + let dup_fact = make_degraded_fact( + parent_id, + collection.id, + target_disk_id, + target, + ); + let parent = make_parent_sitrep( + parent_id, + collection.id, + [ + make_disk_case(kept_id, parent_id, [kept_fact]), + make_disk_case(dup_id, parent_id, [dup_fact]), + ], + ); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, report) = run_analyze(&logctx.log, &input); + + let kept = sitrep.cases.get(&kept_id).expect("kept case present"); + let dup = sitrep.cases.get(&dup_id).expect("duplicate case present"); + assert!(kept.is_open(), "lowest-ID case should remain open"); + assert!(!dup.is_open(), "duplicate case should be closed"); + // The kept case's fact is still accurate (Degraded), so it should + // carry forward unchanged. + assert!( + kept.facts.contains_key(&kept_fact_id), + "kept case should retain its fact", + ); + // No third case should have been opened for this disk. + let disk_case_count = sitrep + .cases + .iter() + .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .count(); + assert_eq!(disk_case_count, 2); + let report_str = format!("{}", report.display_multiline(0)); assert!( - case.is_open(), - "empty case should be left open (no disk to verify)", + report_str.contains("duplicate of case"), + "close comment should call out the duplicate, got: {report_str}", + ); + logctx.cleanup_successful(); + } + + /// A case whose facts disagree about which disk they concern is closed + /// as uninterpretable. Because fault detection is independent of case + /// bookkeeping, the still-unhealthy disk gets a fresh, well-formed case + /// in the same pass. + #[test] + fn uninterpretable_case_is_replaced() { + let (logctx, mut collection, zpools) = + setup("disk_uninterpretable_replaced"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + let parent_id = SitrepUuid::new_v4(); + + // One fact about the target disk, one about an unrelated disk: the + // case is self-contradictory. + let corrupt_case_id = omicron_uuid_kinds::CaseUuid::new_v4(); + let corrupt_case = make_disk_case( + corrupt_case_id, + parent_id, + [ + make_degraded_fact( + parent_id, + collection.id, + target_disk_id, + target, + ), + make_degraded_fact( + parent_id, + collection.id, + PhysicalDiskUuid::new_v4(), + ZpoolUuid::new_v4(), + ), + ], + ); + let parent = + make_parent_sitrep(parent_id, collection.id, [corrupt_case]); + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, report) = run_analyze(&logctx.log, &input); + + let corrupt = sitrep + .cases + .get(&corrupt_case_id) + .expect("corrupt case should still be in the output sitrep"); + assert!( + !corrupt.is_open(), + "case with disagreeing facts should be closed", + ); + // The target disk is still Degraded and in service, so a fresh case + // should have been opened for it. + let open = disk_facts(&sitrep, true); + assert_eq!( + open.len(), + 1, + "expected exactly one open Disk fact (on the replacement case)", + ); + assert_ne!(open[0].0.id, corrupt_case_id); + match &open[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id, + zpool_id, + last_seen_health, + .. + }) => { + assert_eq!(*physical_disk_id, target_disk_id); + assert_eq!(*zpool_id, target); + assert_eq!(*last_seen_health, ZpoolHealth::Degraded); + } + } + let report_str = format!("{}", report.display_multiline(0)); + assert!( + report_str.contains("cannot interpret case"), + "close comment should say the case was uninterpretable, got: \ + {report_str}", ); logctx.cleanup_successful(); } @@ -805,4 +1025,68 @@ mod tests { assert!(open[0].0.is_open()); logctx.cleanup_successful(); } + + /// When the parent's fact references a different zpool than the one + /// currently backing the disk (the zpool was destroyed and recreated), + /// the fact rotates even though the observed health is unchanged. + /// Today's adoption flow mints a new physical disk UUID alongside a new + /// zpool, so this can't happen via normal control plane operation; this + /// pins the engine's behavior if that ever changes. + #[test] + fn fact_uuid_rotates_when_zpool_replaced() { + let (logctx, mut collection, zpools) = + setup("disk_fact_uuid_rotates_zpool_replaced"); + let target = zpools[0]; + set_health(&mut collection, target, ZpoolHealth::Degraded); + let in_service = mk_in_service(zpools.iter().copied()); + let target_disk_id = disk_id_for(&in_service, target); + // The parent's fact records the same disk and the same health + // (Degraded), but a zpool that no longer exists. + let old_zpool_id = ZpoolUuid::new_v4(); + let parent_id = SitrepUuid::new_v4(); + let parent = make_parent_with_disk_case( + parent_id, + collection.id, + target_disk_id, + old_zpool_id, + ); + let parent_fact_id = parent + .cases + .iter() + .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) + .expect("parent should have one Disk case") + .facts + .iter() + .next() + .expect("parent case should have one fact") + .id; + + let input = build_input(collection, Some(parent), in_service); + let (sitrep, _report) = run_analyze(&logctx.log, &input); + let open = disk_facts(&sitrep, true); + assert_eq!( + open.len(), + 1, + "expected exactly one open Disk fact (the refreshed one)", + ); + assert_ne!( + open[0].1.id, parent_fact_id, + "fact UUID should rotate because the disk's zpool changed", + ); + match &open[0].2 { + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + zpool_id, + last_seen_health, + .. + }) => { + assert_eq!( + *zpool_id, target, + "refreshed fact should reference the current zpool", + ); + assert_eq!(*last_seen_health, ZpoolHealth::Degraded); + } + } + assert!(open[0].0.is_open()); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/app/background/tasks/fm_sitrep_load.rs b/nexus/src/app/background/tasks/fm_sitrep_load.rs index b2dca967abd..a6afdcb93ee 100644 --- a/nexus/src/app/background/tasks/fm_sitrep_load.rs +++ b/nexus/src/app/background/tasks/fm_sitrep_load.rs @@ -10,6 +10,7 @@ use chrono::Utc; use futures::future::BoxFuture; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; +use nexus_types::fm::Sitrep; use nexus_types::fm::SitrepVersion; use nexus_types::internal_api::background::SitrepLoadStatus as Status; use serde_json::json; @@ -22,7 +23,7 @@ pub struct SitrepLoader { tx: watch::Sender>, } -pub use nexus_types::fm::CurrentSitrep; +pub type CurrentSitrep = Arc<(SitrepVersion, Sitrep)>; impl BackgroundTask for SitrepLoader { fn activate<'a>( @@ -191,7 +192,6 @@ mod test { use super::*; use crate::app::background::BackgroundTask; use nexus_db_queries::db::pub_test_utils::TestDatabase; - use nexus_types::fm::Sitrep; use nexus_types::fm::SitrepMetadata; use omicron_test_utils::dev; use omicron_uuid_kinds::CollectionUuid; diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 815409efa16..9863cbf7ffd 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -307,6 +307,7 @@ pub struct Nexus { repo_depot_resolver: Box, /// Watch channel containing the currently-loaded fault management sitrep. + #[allow(dead_code)] sitrep_load_rx: watch::Receiver>, /// handle to pull update status data @@ -1129,14 +1130,6 @@ impl Nexus { &self.db_datastore } - pub(crate) fn sitrep_load_rx( - &self, - ) -> watch::Receiver> { - let mut rx = self.sitrep_load_rx.clone(); - rx.mark_unchanged(); - rx - } - pub(crate) fn samael_max_issue_delay(&self) -> Option { let mid = self.samael_max_issue_delay.lock().unwrap(); *mid diff --git a/nexus/src/lib.rs b/nexus/src/lib.rs index c4308ed4f66..8293bd9b9fc 100644 --- a/nexus/src/lib.rs +++ b/nexus/src/lib.rs @@ -533,12 +533,6 @@ impl nexus_test_interface::NexusServer for Server { self.apictx.context.nexus.inventory_load_rx() } - fn sitrep_load_rx( - &self, - ) -> watch::Receiver> { - self.apictx.context.nexus.sitrep_load_rx() - } - fn get_http_server_external_address(&self) -> SocketAddr { self.apictx.context.nexus.get_external_server_address().unwrap() } diff --git a/nexus/test-interface/src/lib.rs b/nexus/test-interface/src/lib.rs index 53d254b44ca..35f726740e1 100644 --- a/nexus/test-interface/src/lib.rs +++ b/nexus/test-interface/src/lib.rs @@ -88,10 +88,6 @@ pub trait NexusServer: Send + Sync + 'static { fn inventory_load_rx(&self) -> watch::Receiver>>; - fn sitrep_load_rx( - &self, - ) -> watch::Receiver>; - fn get_http_server_external_address(&self) -> SocketAddr; fn get_http_server_techport_address(&self) -> SocketAddr; fn get_http_server_internal_address(&self) -> SocketAddr; diff --git a/nexus/test-utils/src/nexus_test.rs b/nexus/test-utils/src/nexus_test.rs index f1cd46402d9..496b66b9747 100644 --- a/nexus/test-utils/src/nexus_test.rs +++ b/nexus/test-utils/src/nexus_test.rs @@ -198,35 +198,6 @@ impl ControlPlaneTestContext { } } - /// Wait until at least one fault management sitrep has been committed and - /// loaded. - /// - /// # Panics - /// - /// Panics if no sitrep is loaded within `timeout`. - pub async fn wait_for_at_least_one_sitrep(&self, timeout: Duration) { - let mut sitrep_rx = self.server.sitrep_load_rx(); - - match wait_for_watch_channel_condition( - &mut sitrep_rx, - |sitrep| sitrep.is_some(), - timeout, - ) - .await - { - Ok(()) => (), - Err(poll::WatchChannelError::TimedOut(elapsed)) => { - panic!("no sitrep found within {elapsed:?}"); - } - Err(poll::WatchChannelError::SenderDropped) => { - panic!( - "sitrep watch channel sender dropped before a \ - sitrep was available" - ); - } - } - } - pub fn internal_client(&self) -> nexus_client::Client { nexus_client::Client::new( &format!("http://{}", self.internal_client.bind_address), diff --git a/nexus/types/src/fm.rs b/nexus/types/src/fm.rs index 1c4dba06532..f80d5fd9eff 100644 --- a/nexus/types/src/fm.rs +++ b/nexus/types/src/fm.rs @@ -204,10 +204,6 @@ pub struct SitrepVersion { pub time_made_current: DateTime, } -/// The current sitrep paired with its [`SitrepVersion`] metadata, wrapped in an -/// `Arc` so it can be shared cheaply (e.g., across `watch` channels). -pub type CurrentSitrep = Arc<(SitrepVersion, Sitrep)>; - #[derive( Copy, Clone, From 83c73ad6d0c18b5d32b3e99eeada3a941362ac54 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 12 Jun 2026 14:48:35 -0700 Subject: [PATCH 10/20] [fm] derive fact row kind from payload via exhaustive match Previously from_sitrep() set a default kind in the base row that every match arm had to remember to override; a future DiskFact variant that forgot would silently inherit the wrong discriminant. Route kind through an exhaustive match on the payload so that can't compile. --- nexus/db-model/src/fm/fact_physical_disk.rs | 32 ++++++++++++++------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index f81df308b35..2caf8c8a0d3 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -72,29 +72,31 @@ impl FmFactPhysicalDisk { fact: &fm::case::Fact, disk_fact: &DiskFact, ) -> Self { - let base = Self { + let mut row = Self { id: fact.id.into(), sitrep_id: sitrep_id.into(), case_id: case_id.into(), created_sitrep_id: fact.created_sitrep_id.into(), comment: fact.comment.clone(), physical_disk_id: disk_fact.physical_disk_id().into(), - kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, + kind: db_kind(disk_fact), zpool_id: None, last_seen_health: None, observed_in_inv: None, time_observed: None, }; + // Each arm populates the columns belonging to its `kind` and leaves + // every other payload column `None`. A column missed here is caught + // at insert time by the table's per-kind CHECK constraint. match disk_fact { - DiskFact::ZpoolUnhealthy(p) => Self { - kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, - zpool_id: Some(p.zpool_id.into()), - last_seen_health: Some(p.last_seen_health.into()), - observed_in_inv: Some(p.observed_in_inv.into()), - time_observed: Some(p.time_observed), - ..base - }, + DiskFact::ZpoolUnhealthy(p) => { + row.zpool_id = Some(p.zpool_id.into()); + row.last_seen_health = Some(p.last_seen_health.into()); + row.observed_in_inv = Some(p.observed_in_inv.into()); + row.time_observed = Some(p.time_observed); + } } + row } /// Reconstruct an in-memory fact from a row. @@ -140,6 +142,16 @@ impl FmFactPhysicalDisk { } } +/// The `kind` discriminant for a fact's payload. Exhaustive by construction: +/// adding a `DiskFact` variant will not compile until it is mapped here, so +/// `from_sitrep` can never write a row whose `kind` was defaulted rather than +/// derived from the payload. +fn db_kind(disk_fact: &DiskFact) -> FmFactPhysicalDiskKind { + match disk_fact { + DiskFact::ZpoolUnhealthy(_) => FmFactPhysicalDiskKind::ZpoolUnhealthy, + } +} + fn missing_column(kind: FmFactPhysicalDiskKind, column: &str) -> Error { Error::internal_error(&format!( "fm_fact_physical_disk row of kind {kind:?} has a NULL {column}, \ From 546fc5d6cd4501aa57be001dc2458b2574be2ce5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 09:50:01 -0700 Subject: [PATCH 11/20] [fm] tidy disk diagnoser per review feedback Remove the unused CaseBuilder::facts() accessor, collapse the three identical multi-line .expect() strings in analyze() to one short message, and drop the redundant case-level comment on a freshly opened disk case (its specifics already live in the fact added alongside it). --- nexus/fm/src/builder/case.rs | 6 ------ nexus/fm/src/diagnosis/physical_disk.rs | 20 +++++++------------- 2 files changed, 7 insertions(+), 19 deletions(-) diff --git a/nexus/fm/src/builder/case.rs b/nexus/fm/src/builder/case.rs index b2c6726942a..46bd7f40b26 100644 --- a/nexus/fm/src/builder/case.rs +++ b/nexus/fm/src/builder/case.rs @@ -279,12 +279,6 @@ impl CaseBuilder { } } - /// Iterate the facts currently attached to this case (including any that - /// were carried forward from the parent sitrep). - pub fn facts(&self) -> impl Iterator { - self.case.facts.iter() - } - pub fn add_ereport( &mut self, report: &Arc, diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 401251c60e1..907716cb45c 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -184,7 +184,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { builder .cases .case_mut(&case_id) - .expect("case came from builder.input()'s open cases") + .expect("case_id came from builder's open cases") .close(format!("cannot interpret case: {reason}")); } @@ -214,7 +214,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { builder .cases .case_mut(case_id) - .expect("case came from builder.input()'s open cases") + .expect("case_id came from builder's open cases") .close(format!( "duplicate of case {kept_case_id} for disk {}", summary.physical_disk_id, @@ -233,10 +233,10 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // is NOT a recovery signal: sled could be powered off, or // inventory could be lossy) for &(case_id, summary) in case_by_disk.values() { - let mut case_mut = builder.cases.case_mut(&case_id).expect( - "builder.cases is seeded from the open cases of builder.input(), \ - which is where this case_id came from", - ); + let mut case_mut = builder + .cases + .case_mut(&case_id) + .expect("case_id came from builder's open cases"); match in_service_health.get(&summary.physical_disk_id) { None => { case_mut.close(format!( @@ -292,13 +292,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { Some((case_id, _)) => case_id, // No parent case for this disk; open one. None => { - let mut new_case = - builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk); - new_case.set_comment(format!( - "physical disk {} unhealthy", - disk.physical_disk_id, - )); - new_case.id + builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk).id } }; From 271875579992dda0c7aec445a11f6ed3210738cf Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 10:26:55 -0700 Subject: [PATCH 12/20] [fm] split Fact into FactMetadata + payload Lets the per-DE row constructor take (metadata, payload) instead of a whole Fact plus a redundant DiskFact, removing the consistency footgun where the two could disagree. Mirrors Case's metadata/payload split. --- nexus/db-model/src/fm/fact_physical_disk.rs | 26 +++++++++--------- nexus/db-queries/src/db/datastore/fm.rs | 15 +++++++---- nexus/fm/src/builder/case.rs | 8 +++--- nexus/fm/src/diagnosis/physical_disk.rs | 23 +++++++++------- nexus/types/src/fm/case.rs | 30 ++++++++++++++++----- 5 files changed, 66 insertions(+), 36 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index 2caf8c8a0d3..d5d3820a384 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -6,9 +6,7 @@ //! //! Each physical-disk fact is stored as typed columns in the //! `fm_fact_physical_disk` table. The `kind` discriminant selects which payload -//! columns are populated; a CHECK constraint -//! (`zpool_unhealthy_columns_present`) enforces that the right columns are -//! non-NULL for each kind. See [`nexus_types::fm::DiskFact`] for semantics. +//! columns are populated. use crate::DbTypedUuid; use crate::impl_enum_type; @@ -16,6 +14,7 @@ use crate::inventory::InvZpoolHealth; use chrono::{DateTime, Utc}; use nexus_db_schema::schema::fm_fact_physical_disk; use nexus_types::fm; +use nexus_types::fm::case::FactMetadata; use nexus_types::fm::{DiskFact, FactPayload, ZpoolUnhealthyFactPayload}; use omicron_common::api::external::Error; use omicron_uuid_kinds::{ @@ -51,8 +50,7 @@ pub struct FmFactPhysicalDisk { pub created_sitrep_id: DbTypedUuid, pub comment: String, - /// The physical disk this fact is about. Common to every `kind`, so it is - /// always present (the column is `NOT NULL`). + /// The physical disk this fact is about. Common to every `kind`. pub physical_disk_id: DbTypedUuid, pub kind: FmFactPhysicalDiskKind, @@ -64,20 +62,20 @@ pub struct FmFactPhysicalDisk { } impl FmFactPhysicalDisk { - /// Build a row from a fact's shared metadata (`fact`) and its + /// Build a row from a fact's shared metadata (`metadata`) and its /// physical-disk payload (`disk_fact`). pub fn from_sitrep( sitrep_id: impl Into>, case_id: impl Into>, - fact: &fm::case::Fact, + metadata: &FactMetadata, disk_fact: &DiskFact, ) -> Self { let mut row = Self { - id: fact.id.into(), + id: metadata.id.into(), sitrep_id: sitrep_id.into(), case_id: case_id.into(), - created_sitrep_id: fact.created_sitrep_id.into(), - comment: fact.comment.clone(), + created_sitrep_id: metadata.created_sitrep_id.into(), + comment: metadata.comment.clone(), physical_disk_id: disk_fact.physical_disk_id().into(), kind: db_kind(disk_fact), zpool_id: None, @@ -134,10 +132,12 @@ impl FmFactPhysicalDisk { } }; Ok(fm::case::Fact { - id: self.id.into(), - created_sitrep_id: self.created_sitrep_id.into(), + metadata: FactMetadata { + id: self.id.into(), + created_sitrep_id: self.created_sitrep_id.into(), + comment: self.comment, + }, payload, - comment: self.comment, }) } } diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index 3aa71000b4b..8743f8733d2 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -547,7 +547,7 @@ impl DataStore { for row in batch { let case_id: CaseUuid = row.case_id.into(); let fact = row.into_fact()?; - let id = fact.id; + let id = fact.metadata.id; by_case .entry(case_id) .or_default() @@ -840,7 +840,10 @@ impl DataStore { fm::FactPayload::PhysicalDisk(disk_fact) => { physical_disk_facts.push( model::fm::FmFactPhysicalDisk::from_sitrep( - sitrep_id, case_id, fact, disk_fact, + sitrep_id, + case_id, + &fact.metadata, + disk_fact, ), ); } @@ -2281,8 +2284,11 @@ mod tests { let mut facts = iddqd::IdOrdMap::new(); facts .insert_unique(fm::case::Fact { - id: FactUuid::new_v4(), - created_sitrep_id: sitrep_id, + metadata: fm::case::FactMetadata { + id: FactUuid::new_v4(), + created_sitrep_id: sitrep_id, + comment: "a representative fact for case 1".to_string(), + }, payload: fm::FactPayload::PhysicalDisk( fm::DiskFact::ZpoolUnhealthy( fm::ZpoolUnhealthyFactPayload { @@ -2299,7 +2305,6 @@ mod tests { }, ), ), - comment: "a representative fact for case 1".to_string(), }) .unwrap(); diff --git a/nexus/fm/src/builder/case.rs b/nexus/fm/src/builder/case.rs index 46bd7f40b26..438f4932406 100644 --- a/nexus/fm/src/builder/case.rs +++ b/nexus/fm/src/builder/case.rs @@ -261,10 +261,12 @@ impl CaseBuilder { .kv("payload", &payload) .comment(comment.clone()); let fact = fm::case::Fact { - id, - created_sitrep_id: self.sitrep_id, + metadata: fm::case::FactMetadata { + id, + created_sitrep_id: self.sitrep_id, + comment, + }, payload, - comment, }; self.case.facts.insert_unique(fact).expect("UUID should be unused"); id diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 907716cb45c..bf6936d2193 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -90,7 +90,7 @@ fn summarize_case( // payload; a foreign payload is a data-model violation. let Some(disk_fact) = fact.payload.as_physical_disk() else { return Err(UninterpretableCase::ForeignFactPayload { - fact_id: fact.id, + fact_id: fact.metadata.id, }); }; match disk_fact { @@ -106,7 +106,7 @@ fn summarize_case( } unhealthy_facts .insert_unique(ZpoolUnhealthyFact { - fact_id: fact.id, + fact_id: fact.metadata.id, payload, }) .expect("fact ids are unique within a case"); @@ -454,8 +454,11 @@ mod tests { zpool_id: ZpoolUuid, ) -> fm::case::Fact { fm::case::Fact { - id: omicron_uuid_kinds::FactUuid::new_v4(), - created_sitrep_id: parent_sitrep_id, + metadata: fm::case::FactMetadata { + id: omicron_uuid_kinds::FactUuid::new_v4(), + created_sitrep_id: parent_sitrep_id, + comment: format!("zpool {zpool_id} degraded"), + }, payload: DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { physical_disk_id, zpool_id, @@ -464,7 +467,6 @@ mod tests { time_observed: Utc::now(), }) .into(), - comment: format!("zpool {zpool_id} degraded"), } } @@ -788,7 +790,7 @@ mod tests { target_disk_id, target, ); - let kept_fact_id = kept_fact.id; + let kept_fact_id = kept_fact.metadata.id; let dup_fact = make_degraded_fact( parent_id, collection.id, @@ -938,6 +940,7 @@ mod tests { .iter() .next() .expect("parent case should have one fact") + .metadata .id; let input = build_input(collection, Some(parent), in_service); @@ -945,7 +948,7 @@ mod tests { let open = disk_facts(&sitrep, true); assert_eq!(open.len(), 1, "expected exactly one open Disk fact"); assert_eq!( - open[0].1.id, parent_fact_id, + open[0].1.metadata.id, parent_fact_id, "fact UUID should be stable across sitreps when the \ observation hasn't changed", ); @@ -990,6 +993,7 @@ mod tests { .iter() .next() .expect("parent case should have one fact") + .metadata .id; let input = build_input(collection, Some(parent), in_service); @@ -1001,7 +1005,7 @@ mod tests { "expected exactly one open Disk fact (the refreshed one)", ); assert_ne!( - open[0].1.id, parent_fact_id, + open[0].1.metadata.id, parent_fact_id, "fact UUID should rotate because last_seen_health changed", ); match &open[0].2 { @@ -1053,6 +1057,7 @@ mod tests { .iter() .next() .expect("parent case should have one fact") + .metadata .id; let input = build_input(collection, Some(parent), in_service); @@ -1064,7 +1069,7 @@ mod tests { "expected exactly one open Disk fact (the refreshed one)", ); assert_ne!( - open[0].1.id, parent_fact_id, + open[0].1.metadata.id, parent_fact_id, "fact UUID should rotate because the disk's zpool changed", ); match &open[0].2 { diff --git a/nexus/types/src/fm/case.rs b/nexus/types/src/fm/case.rs index 6410eee3009..686ef49eec0 100644 --- a/nexus/types/src/fm/case.rs +++ b/nexus/types/src/fm/case.rs @@ -176,19 +176,28 @@ impl CaseEreport { /// the case's diagnosis engine (see [`Metadata::de`]). #[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] pub struct Fact { + #[serde(flatten)] + pub metadata: FactMetadata, + pub payload: FactPayload, +} + +/// The diagnosis-engine-agnostic part of a [`Fact`]: everything that is not +/// the typed [`payload`](Fact::payload). Every diagnosis engine's facts share +/// these fields. +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +pub struct FactMetadata { pub id: FactUuid, /// The sitrep in which this fact was first added. Preserved /// unchanged when the fact is carried forward into a child sitrep. /// Debug-only. pub created_sitrep_id: SitrepUuid, - pub payload: FactPayload, pub comment: String, } impl IdOrdItem for Fact { type Key<'a> = &'a FactUuid; fn key(&self) -> Self::Key<'_> { - &self.id + &self.metadata.id } iddqd::id_upcast!(); } @@ -213,7 +222,12 @@ impl Fact { const WIDTH: usize = const_max_len(&[ADDED_IN, COMMENT]); let &Self { - fact: Fact { id, created_sitrep_id, payload, comment }, + fact: + Fact { + metadata: + FactMetadata { id, created_sitrep_id, comment }, + payload, + }, indent, sitrep_id, } = self; @@ -619,9 +633,14 @@ mod tests { let mut facts = IdOrdMap::new(); facts .insert_unique(Fact { - id: FactUuid::from_str("f00f00f0-0f00-4f00-8f00-f00f00f00f00") + metadata: FactMetadata { + id: FactUuid::from_str( + "f00f00f0-0f00-4f00-8f00-f00f00f00f00", + ) .unwrap(), - created_sitrep_id, + created_sitrep_id, + comment: "made-up fact for display test".to_string(), + }, payload: FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( ZpoolUnhealthyFactPayload { physical_disk_id: PhysicalDiskUuid::from_str( @@ -640,7 +659,6 @@ mod tests { time_observed: chrono::DateTime::::MIN_UTC, }, )), - comment: "made-up fact for display test".to_string(), }) .unwrap(); From 0ada45de69421ce033ecf7a40fde22cd5d4172a0 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 11:41:03 -0700 Subject: [PATCH 13/20] [fm] disk diagnoser: clearer names for parsing a case observed_health -> observed_zpool_health (keeps the load-bearing 'observed' semantic), summarize_case -> parse_case (it validates and rejects, not just condenses), and ParentCaseSummary -> ParsedDiskCase to match. --- nexus/fm/src/diagnosis/physical_disk.rs | 57 ++++++++++++------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index bf6936d2193..13ab573d685 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -47,9 +47,10 @@ impl IdOrdItem for DiskHealthSnapshot { id_upcast!(); } -/// Per-case summary built from a case's facts. Each Disk case is about a -/// single physical disk; every fact on the case must reference that disk. -struct ParentCaseSummary { +/// A parent-forwarded Disk case, parsed into the form this engine acts on. +/// Each Disk case is about a single physical disk; every fact on the case +/// must reference that disk. +struct ParsedDiskCase { /// The physical disk this case is about. physical_disk_id: PhysicalDiskUuid, /// All `ZpoolUnhealthy` facts on this case. Normally one; pathological @@ -78,11 +79,9 @@ enum UninterpretableCase { NoFacts, } -/// Summarize one parent-forwarded Disk case, or explain why it cannot be -/// interpreted. -fn summarize_case( - case: &fm::Case, -) -> Result { +/// Parse one parent-forwarded Disk case into a [`ParsedDiskCase`], or explain +/// why it cannot be interpreted. +fn parse_case(case: &fm::Case) -> Result { let mut unhealthy_facts: IdOrdMap = IdOrdMap::new(); let mut case_disk_id: Option = None; for fact in case.facts.iter() { @@ -116,7 +115,7 @@ fn summarize_case( let Some(physical_disk_id) = case_disk_id else { return Err(UninterpretableCase::NoFacts); }; - Ok(ParentCaseSummary { physical_disk_id, unhealthy_facts }) + Ok(ParsedDiskCase { physical_disk_id, unhealthy_facts }) } pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { @@ -127,7 +126,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // Index every zpool we observed in this inventory, so we can distinguish // "saw it, it's Online" from "didn't see it at all" when looking up by // an in-service disk's zpool below. - let observed_health: BTreeMap = input + let observed_zpool_health: BTreeMap = input .inventory() .sled_agents .iter() @@ -145,22 +144,22 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { .map(|d| DiskHealthSnapshot { physical_disk_id: d.physical_disk_id, zpool_id: d.zpool_id, - zpool_health: observed_health.get(&d.zpool_id).copied(), + zpool_health: observed_zpool_health.get(&d.zpool_id).copied(), }) .collect(); // Index the Disk cases copied forward from the parent sitrep. Every case // is about one physical disk; we derive the disk from its facts. - let mut parent_cases = BTreeMap::::new(); + let mut parent_cases = BTreeMap::::new(); let mut uninterpretable = Vec::<(CaseUuid, UninterpretableCase)>::new(); for case in input .open_cases() .iter() .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) { - match summarize_case(case) { - Ok(summary) => { - parent_cases.insert(case.id, summary); + match parse_case(case) { + Ok(parsed_case) => { + parent_cases.insert(case.id, parsed_case); } Err(reason) => { slog::warn!( @@ -194,22 +193,22 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // duplicate would otherwise decay into an uninterpretable empty case.) let mut case_by_disk: BTreeMap< PhysicalDiskUuid, - (CaseUuid, &ParentCaseSummary), + (CaseUuid, &ParsedDiskCase), > = BTreeMap::new(); - for (case_id, summary) in &parent_cases { + for (case_id, parsed_case) in &parent_cases { case_by_disk - .entry(summary.physical_disk_id) - .or_insert((*case_id, summary)); + .entry(parsed_case.physical_disk_id) + .or_insert((*case_id, parsed_case)); } - for (case_id, summary) in &parent_cases { - let (kept_case_id, _) = case_by_disk[&summary.physical_disk_id]; + for (case_id, parsed_case) in &parent_cases { + let (kept_case_id, _) = case_by_disk[&parsed_case.physical_disk_id]; if *case_id != kept_case_id { slog::warn!( &builder.log, "closing duplicate Disk case"; "case_id" => %case_id, "kept_case_id" => %kept_case_id, - "physical_disk_id" => %summary.physical_disk_id, + "physical_disk_id" => %parsed_case.physical_disk_id, ); builder .cases @@ -217,7 +216,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { .expect("case_id came from builder's open cases") .close(format!( "duplicate of case {kept_case_id} for disk {}", - summary.physical_disk_id, + parsed_case.physical_disk_id, )); } } @@ -232,16 +231,16 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // - disk in service but absent from inventory → leave alone (absence // is NOT a recovery signal: sled could be powered off, or // inventory could be lossy) - for &(case_id, summary) in case_by_disk.values() { + for &(case_id, parsed_case) in case_by_disk.values() { let mut case_mut = builder .cases .case_mut(&case_id) .expect("case_id came from builder's open cases"); - match in_service_health.get(&summary.physical_disk_id) { + match in_service_health.get(&parsed_case.physical_disk_id) { None => { case_mut.close(format!( "disk {} no longer in service", - summary.physical_disk_id, + parsed_case.physical_disk_id, )); } Some(snap) if snap.zpool_health == Some(ZpoolHealth::Online) => { @@ -252,7 +251,7 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { let Some(current_health) = snap.zpool_health else { continue; }; - for fact_ref in summary.unhealthy_facts.iter() { + for fact_ref in parsed_case.unhealthy_facts.iter() { if fact_ref.payload.last_seen_health != current_health || fact_ref.payload.zpool_id != snap.zpool_id { @@ -279,8 +278,8 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { let case_id_for_fact = match parent_for_disk { // Parent case already has an accurate fact; fully covered. - Some((_, summary)) - if summary.unhealthy_facts.iter().any(|f| { + Some((_, parsed_case)) + if parsed_case.unhealthy_facts.iter().any(|f| { f.payload.last_seen_health == current_health && f.payload.zpool_id == disk.zpool_id }) => From 39e4228c756286885644afe2c8c9d43812a72cd5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 11:51:14 -0700 Subject: [PATCH 14/20] [fm] disk diagnoser: build case_by_disk in a single pass The duplicate-case detection re-scanned parent_cases and re-looked-up case_by_disk to find the cases that weren't kept. Since parent_cases iterates ascending by CaseUuid, the keep-lowest / close-the-rest decision can be made in one pass with the Entry API. --- nexus/fm/src/diagnosis/physical_disk.rs | 53 +++++++++++++------------ 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 13ab573d685..52e33fa0e5d 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -12,6 +12,7 @@ use nexus_types::fm::{DiskFact, ZpoolUnhealthyFactPayload}; use nexus_types::inventory::ZpoolHealth; use omicron_uuid_kinds::{CaseUuid, FactUuid, PhysicalDiskUuid, ZpoolUuid}; use std::collections::BTreeMap; +use std::collections::btree_map::Entry; /// A [`DiskFact::ZpoolUnhealthy`] payload paired with the `FactUuid` it /// lives under. @@ -188,36 +189,38 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { } // Inverse index: which parent case is about which disk. Cases are - // per-disk, so a disk with two parent cases is pathological; keep the - // lowest case ID and close the rest as duplicates. (A half-maintained - // duplicate would otherwise decay into an uninterpretable empty case.) + // per-disk, so a disk with two parent cases is pathological. `parent_cases` + // iterates ascending by CaseUuid, so the first case we see for a disk is + // the lowest-ID one: keep it, and close any later case for the same disk as + // a duplicate. (A half-maintained duplicate would otherwise decay into an + // uninterpretable empty case.) let mut case_by_disk: BTreeMap< PhysicalDiskUuid, (CaseUuid, &ParsedDiskCase), > = BTreeMap::new(); for (case_id, parsed_case) in &parent_cases { - case_by_disk - .entry(parsed_case.physical_disk_id) - .or_insert((*case_id, parsed_case)); - } - for (case_id, parsed_case) in &parent_cases { - let (kept_case_id, _) = case_by_disk[&parsed_case.physical_disk_id]; - if *case_id != kept_case_id { - slog::warn!( - &builder.log, - "closing duplicate Disk case"; - "case_id" => %case_id, - "kept_case_id" => %kept_case_id, - "physical_disk_id" => %parsed_case.physical_disk_id, - ); - builder - .cases - .case_mut(case_id) - .expect("case_id came from builder's open cases") - .close(format!( - "duplicate of case {kept_case_id} for disk {}", - parsed_case.physical_disk_id, - )); + match case_by_disk.entry(parsed_case.physical_disk_id) { + Entry::Vacant(slot) => { + slot.insert((*case_id, parsed_case)); + } + Entry::Occupied(kept) => { + let (kept_case_id, _) = *kept.get(); + slog::warn!( + &builder.log, + "closing duplicate Disk case"; + "case_id" => %case_id, + "kept_case_id" => %kept_case_id, + "physical_disk_id" => %parsed_case.physical_disk_id, + ); + builder + .cases + .case_mut(case_id) + .expect("case_id came from builder's open cases") + .close(format!( + "duplicate of case {kept_case_id} for disk {}", + parsed_case.physical_disk_id, + )); + } } } From 4b6e9284f1a5e5aadc33c04d72278529bf05ea4e Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 11:54:40 -0700 Subject: [PATCH 15/20] [fm] disk diagnoser: separate case-closing from fact reconciliation Fact reconciliation was split across two loops over two collections: the first loop (over parent cases) removed stale facts, the second (over faulty in-service disks) added fresh ones, coupled by a 'removed above' comment. Make the first loop closing-only and let the second own all fact state for a disk (remove stale + add fresh) in one place. --- nexus/fm/src/diagnosis/physical_disk.rs | 105 +++++++++++------------- 1 file changed, 46 insertions(+), 59 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 52e33fa0e5d..c08a0d1d18f 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -224,16 +224,11 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { } } - // For each disk's surviving parent case, decide what to do based on the - // disk's current state: - // - disk no longer in service → close the case (expungement) - // - disk's zpool back to Online → close the case (recovery) - // - disk still unhealthy → drop any facts whose recorded observation - // (zpool + health) no longer matches; the matching loop below will - // re-add a fresh fact - // - disk in service but absent from inventory → leave alone (absence - // is NOT a recovery signal: sled could be powered off, or - // inventory could be lossy) + // Close the surviving parent case for any disk that has recovered or left + // service. A still-faulty disk's facts are reconciled in the next loop; a + // disk in service but absent from this inventory is left alone (absence is + // NOT a recovery signal: the sled could be powered off, or the collection + // could be lossy). for &(case_id, parsed_case) in case_by_disk.values() { let mut case_mut = builder .cases @@ -248,27 +243,18 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { } Some(snap) if snap.zpool_health == Some(ZpoolHealth::Online) => { case_mut - .close(format!("zpool {} back to Online", snap.zpool_id,)); - } - Some(snap) => { - let Some(current_health) = snap.zpool_health else { - continue; - }; - for fact_ref in parsed_case.unhealthy_facts.iter() { - if fact_ref.payload.last_seen_health != current_health - || fact_ref.payload.zpool_id != snap.zpool_id - { - case_mut.remove_fact(fact_ref.fact_id); - } - } + .close(format!("zpool {} back to Online", snap.zpool_id)); } + // Faulty or absent from inventory: leave open; the next loop + // reconciles the faulty ones and leaves the absent ones untouched. + Some(_) => {} } } - // For each currently-faulty in-service disk: ensure a case exists - // (reusing the parent-forwarded one for this disk if any) and add a - // fresh fact if one with this exact observation (zpool + health) isn't - // already present. + // For each currently-faulty in-service disk, ensure its case carries + // exactly one fact matching the current observation: reuse the + // parent-forwarded case if any (dropping any stale facts), otherwise open + // a fresh case. for disk in in_service_health.iter() { let Some(current_health) = disk.zpool_health else { continue; @@ -277,41 +263,42 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { continue; } - let parent_for_disk = case_by_disk.get(&disk.physical_disk_id).copied(); - - let case_id_for_fact = match parent_for_disk { - // Parent case already has an accurate fact; fully covered. - Some((_, parsed_case)) - if parsed_case.unhealthy_facts.iter().any(|f| { - f.payload.last_seen_health == current_health - && f.payload.zpool_id == disk.zpool_id - }) => - { - continue; - } - // Parent case exists; its stale facts were removed above. - // Refresh under the same case. - Some((case_id, _)) => case_id, - // No parent case for this disk; open one. - None => { - builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk).id + let mut case_mut = match case_by_disk.get(&disk.physical_disk_id) { + Some(&(case_id, parsed_case)) => { + let mut case_mut = builder + .cases + .case_mut(&case_id) + .expect("case_id came from builder's open cases"); + let mut has_match = false; + for fact in parsed_case.unhealthy_facts.iter() { + if fact.payload.zpool_id == disk.zpool_id + && fact.payload.last_seen_health == current_health + { + // An accurate fact is already present; keep it. + has_match = true; + } else { + // Stale observation; drop it. + case_mut.remove_fact(fact.fact_id); + } + } + if has_match { + continue; + } + case_mut } + None => builder.cases.open_case(DiagnosisEngineKind::PhysicalDisk), }; - builder - .cases - .case_mut(&case_id_for_fact) - .expect("case_id came from this fn") - .add_fact( - DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { - physical_disk_id: disk.physical_disk_id, - zpool_id: disk.zpool_id, - last_seen_health: current_health, - observed_in_inv: inv_collection_id, - time_observed: inv_time_done, - }), - format!("zpool {} health={current_health}", disk.zpool_id,), - ); + case_mut.add_fact( + DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { + physical_disk_id: disk.physical_disk_id, + zpool_id: disk.zpool_id, + last_seen_health: current_health, + observed_in_inv: inv_collection_id, + time_observed: inv_time_done, + }), + format!("zpool {} health={current_health}", disk.zpool_id), + ); } Ok(()) From e731c5de53fb9e09a6d675dd0fdaa3cc0e16406c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 17:23:58 -0700 Subject: [PATCH 16/20] [fm] disk diagnoser tests: name the disk-fact triple, dedup fact-id lookup Replace the (case, fact, DiskFact) tuple returned by disk_facts with a named DiskFactRef so call sites read .case/.fact/.disk_fact instead of .0/.1/.2, and extract the repeated parent-fact-id chain into a sole_disk_fact_id helper. Also fix a stale doc comment (identity fields, not facts). --- nexus/fm/src/diagnosis/physical_disk.rs | 104 +++++++++++------------- 1 file changed, 46 insertions(+), 58 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index c08a0d1d18f..c20ffa3d4f8 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -323,8 +323,9 @@ mod tests { use std::sync::Arc; /// Make an in-service disk set from a list of zpool IDs. Each zpool gets - /// its own fresh `PhysicalDiskUuid` and dummy identity facts; tests in - /// this module only care about the zpool dimension. + /// its own fresh `PhysicalDiskUuid` and dummy identity fields + /// (vendor/serial/model); tests in this module only care about the zpool + /// dimension. fn mk_in_service( zpool_ids: impl IntoIterator, ) -> IdOrdMap { @@ -529,13 +530,18 @@ mod tests { make_parent_sitrep(parent_sitrep_id, inv_collection_id, [case]) } - /// Collect (case, fact, DiskFact) triples for every fact on a - /// physical-disk case in a sitrep. Optionally filtered to open cases - /// only. - fn disk_facts( - sitrep: &Sitrep, - open_only: bool, - ) -> Vec<(&fm::Case, &fm::case::Fact, DiskFact)> { + /// A physical-disk fact found in a sitrep, paired with the case it lives + /// on and its decoded [`DiskFact`] payload. + #[derive(Debug)] + struct DiskFactRef<'a> { + case: &'a fm::Case, + fact: &'a fm::case::Fact, + disk_fact: DiskFact, + } + + /// Collect every physical-disk fact in a sitrep, with its case and decoded + /// payload. Optionally filtered to open cases only. + fn disk_facts(sitrep: &Sitrep, open_only: bool) -> Vec> { sitrep .cases .iter() @@ -543,12 +549,24 @@ mod tests { .filter(|c| !open_only || c.is_open()) .flat_map(|c| { c.facts.iter().filter_map(move |f| { - f.payload.as_physical_disk().map(|d| (c, f, d.clone())) + f.payload.as_physical_disk().map(|d| DiskFactRef { + case: c, + fact: f, + disk_fact: d.clone(), + }) }) }) .collect() } + /// The fact UUID of the one physical-disk fact on the one physical-disk + /// case in `sitrep`. Panics unless there is exactly one. + fn sole_disk_fact_id(sitrep: &Sitrep) -> omicron_uuid_kinds::FactUuid { + let facts = disk_facts(sitrep, false); + assert_eq!(facts.len(), 1, "expected exactly one physical-disk fact"); + facts[0].fact.metadata.id + } + #[test] fn opens_on_degraded_in_service() { let (logctx, mut collection, zpools) = @@ -562,7 +580,7 @@ mod tests { let (sitrep, _report) = run_analyze(&logctx.log, &input); let facts = disk_facts(&sitrep, true); assert_eq!(facts.len(), 1); - match &facts[0].2 { + match &facts[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { physical_disk_id, zpool_id, @@ -615,7 +633,7 @@ mod tests { let (sitrep, _report) = run_analyze(&logctx.log, &input); let open_cases = disk_facts(&sitrep, true); assert_eq!(open_cases.len(), 1); - match &open_cases[0].2 { + match &open_cases[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { zpool_id, .. @@ -646,7 +664,7 @@ mod tests { let all = disk_facts(&sitrep, false); assert_eq!(all.len(), 1); assert!( - !all[0].0.is_open(), + !all[0].case.is_open(), "case should be closed when zpool returns to Online", ); let report_str = format!("{}", report.display_multiline(0)); @@ -681,7 +699,7 @@ mod tests { let all = disk_facts(&sitrep, false); assert_eq!(all.len(), 1); assert!( - !all[0].0.is_open(), + !all[0].case.is_open(), "case should be closed when zpool's disk is expunged", ); let report_str = format!("{}", report.display_multiline(0)); @@ -716,7 +734,7 @@ mod tests { let all = disk_facts(&sitrep, false); assert_eq!(all.len(), 1); assert!( - all[0].0.is_open(), + all[0].case.is_open(), "case should remain open when its zpool is absent from the \ current inventory collection (sled could be down or inventory \ is lossy)", @@ -880,8 +898,8 @@ mod tests { 1, "expected exactly one open Disk fact (on the replacement case)", ); - assert_ne!(open[0].0.id, corrupt_case_id); - match &open[0].2 { + assert_ne!(open[0].case.id, corrupt_case_id); + match &open[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { physical_disk_id, zpool_id, @@ -920,28 +938,18 @@ mod tests { target, ); // Capture the parent's fact UUID for the target zpool. - let parent_fact_id = parent - .cases - .iter() - .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) - .expect("parent should have one Disk case") - .facts - .iter() - .next() - .expect("parent case should have one fact") - .metadata - .id; + let parent_fact_id = sole_disk_fact_id(&parent); let input = build_input(collection, Some(parent), in_service); let (sitrep, _report) = run_analyze(&logctx.log, &input); let open = disk_facts(&sitrep, true); assert_eq!(open.len(), 1, "expected exactly one open Disk fact"); assert_eq!( - open[0].1.metadata.id, parent_fact_id, + open[0].fact.metadata.id, parent_fact_id, "fact UUID should be stable across sitreps when the \ observation hasn't changed", ); - match &open[0].2 { + match &open[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { zpool_id, last_seen_health, @@ -973,17 +981,7 @@ mod tests { target_disk_id, target, ); - let parent_fact_id = parent - .cases - .iter() - .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) - .expect("parent should have one Disk case") - .facts - .iter() - .next() - .expect("parent case should have one fact") - .metadata - .id; + let parent_fact_id = sole_disk_fact_id(&parent); let input = build_input(collection, Some(parent), in_service); let (sitrep, _report) = run_analyze(&logctx.log, &input); @@ -994,10 +992,10 @@ mod tests { "expected exactly one open Disk fact (the refreshed one)", ); assert_ne!( - open[0].1.metadata.id, parent_fact_id, + open[0].fact.metadata.id, parent_fact_id, "fact UUID should rotate because last_seen_health changed", ); - match &open[0].2 { + match &open[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { zpool_id, last_seen_health, @@ -1009,7 +1007,7 @@ mod tests { } // The case itself should still be the same one that was carried // forward; only the fact rotated. - assert!(open[0].0.is_open()); + assert!(open[0].case.is_open()); logctx.cleanup_successful(); } @@ -1037,17 +1035,7 @@ mod tests { target_disk_id, old_zpool_id, ); - let parent_fact_id = parent - .cases - .iter() - .find(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) - .expect("parent should have one Disk case") - .facts - .iter() - .next() - .expect("parent case should have one fact") - .metadata - .id; + let parent_fact_id = sole_disk_fact_id(&parent); let input = build_input(collection, Some(parent), in_service); let (sitrep, _report) = run_analyze(&logctx.log, &input); @@ -1058,10 +1046,10 @@ mod tests { "expected exactly one open Disk fact (the refreshed one)", ); assert_ne!( - open[0].1.metadata.id, parent_fact_id, + open[0].fact.metadata.id, parent_fact_id, "fact UUID should rotate because the disk's zpool changed", ); - match &open[0].2 { + match &open[0].disk_fact { DiskFact::ZpoolUnhealthy(ZpoolUnhealthyFactPayload { zpool_id, last_seen_health, @@ -1074,7 +1062,7 @@ mod tests { assert_eq!(*last_seen_health, ZpoolHealth::Degraded); } } - assert!(open[0].0.is_open()); + assert!(open[0].case.is_open()); logctx.cleanup_successful(); } } From f9479fcd0205427347ea264be1e1f6dccac50885 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 15 Jun 2026 17:59:34 -0700 Subject: [PATCH 17/20] [fm] address review feedback from eliza on #10541 Exhaustively destructure FactMetadata in from_sitrep so a new field fails to compile until it is mapped to a column. remove_fact now takes a comment (logged on removal) and logs the removed fact's payload; it warns if asked to remove a fact that does not exist. Drop a redundant 'Empty today' line from known_ereport_classes' doc. Close uninterpretable Disk cases inline in the parse loop instead of collecting them into a vec and looping again (the iterator borrows the input, not the builder), which also drops the now-redundant separate warn. Reframe the duplicate-case tie-break comment: keeping the lowest-ID case is arbitrary-but-deterministic, not 'correct'. --- nexus/db-model/src/fm/fact_physical_disk.rs | 9 ++-- nexus/fm/src/builder/case.rs | 28 +++++++++-- nexus/fm/src/diagnosis/mod.rs | 3 -- nexus/fm/src/diagnosis/physical_disk.rs | 54 ++++++++++----------- 4 files changed, 54 insertions(+), 40 deletions(-) diff --git a/nexus/db-model/src/fm/fact_physical_disk.rs b/nexus/db-model/src/fm/fact_physical_disk.rs index d5d3820a384..2c1518f0077 100644 --- a/nexus/db-model/src/fm/fact_physical_disk.rs +++ b/nexus/db-model/src/fm/fact_physical_disk.rs @@ -70,12 +70,15 @@ impl FmFactPhysicalDisk { metadata: &FactMetadata, disk_fact: &DiskFact, ) -> Self { + // Destructure exhaustively: a new `FactMetadata` field will fail to + // compile here until it is mapped to a column. + let FactMetadata { id, created_sitrep_id, comment } = metadata; let mut row = Self { - id: metadata.id.into(), + id: (*id).into(), sitrep_id: sitrep_id.into(), case_id: case_id.into(), - created_sitrep_id: metadata.created_sitrep_id.into(), - comment: metadata.comment.clone(), + created_sitrep_id: (*created_sitrep_id).into(), + comment: comment.clone(), physical_disk_id: disk_fact.physical_disk_id().into(), kind: db_kind(disk_fact), zpool_id: None, diff --git a/nexus/fm/src/builder/case.rs b/nexus/fm/src/builder/case.rs index 438f4932406..ab6a02ef4ec 100644 --- a/nexus/fm/src/builder/case.rs +++ b/nexus/fm/src/builder/case.rs @@ -273,11 +273,29 @@ impl CaseBuilder { } /// Remove a fact from this case. The fact will not be carried forward - /// into the next sitrep. - pub fn remove_fact(&mut self, id: FactUuid) { - if self.case.facts.remove(&id).is_some() { - slog::info!(&self.log, "removed a fact"; "fact_id" => %id); - self.report_log.entry("removed fact").kv("fact_id", id); + /// into the next sitrep. `comment` records why it was removed. + pub fn remove_fact(&mut self, id: FactUuid, comment: impl ToString) { + let comment = comment.to_string(); + if let Some(fact) = self.case.facts.remove(&id) { + slog::info!( + &self.log, + "removed a fact"; + "fact_id" => %id, + "payload" => ?fact.payload, + "comment" => %comment, + ); + self.report_log + .entry("removed fact") + .kv("fact_id", id) + .kv("payload", &fact.payload) + .comment(comment); + } else { + slog::warn!( + &self.log, + "tried to remove a fact that does not exist"; + "fact_id" => %id, + "comment" => %comment, + ); } } diff --git a/nexus/fm/src/diagnosis/mod.rs b/nexus/fm/src/diagnosis/mod.rs index 48a3c3a184c..30e210779e3 100644 --- a/nexus/fm/src/diagnosis/mod.rs +++ b/nexus/fm/src/diagnosis/mod.rs @@ -19,9 +19,6 @@ pub fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { /// Ereport classes that any diagnosis engine in this build of Nexus knows /// how to consume. The background task uses this to filter loaded ereports. /// -/// Empty today: Grow this list alongside FM analysis as new classes gain -/// ereport support. -/// /// **NULL-class ereports are intentionally excluded by the loader's SQL /// filter** (`class = ANY(...)` never matches NULL). If FM analysis ever /// needs to handle the "couldn't extract a class" or "reporter doesn't know diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index c20ffa3d4f8..5832c1f9d6b 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -152,7 +152,6 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // Index the Disk cases copied forward from the parent sitrep. Every case // is about one physical disk; we derive the disk from its facts. let mut parent_cases = BTreeMap::::new(); - let mut uninterpretable = Vec::<(CaseUuid, UninterpretableCase)>::new(); for case in input .open_cases() .iter() @@ -163,37 +162,30 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { parent_cases.insert(case.id, parsed_case); } Err(reason) => { - slog::warn!( - &builder.log, - "closing uninterpretable Disk case"; - "case_id" => %case.id, - "reason" => %reason, - ); - uninterpretable.push((case.id, reason)); + // Close the cases we couldn't interpret, so they don't ride + // along as open-but-unprocessable in every future sitrep. This + // is safe with respect to fault coverage: detection below is + // independent of case bookkeeping, so if a closed case + // concerned a disk that is genuinely unhealthy and in service, + // a fresh, well-formed case is opened in this same pass. + builder + .cases + .case_mut(&case.id) + .expect("case_id came from builder's open cases") + .close(format!("cannot interpret case: {reason}")); } } } - // Close the cases we couldn't interpret, so they don't ride along as - // open-but-unprocessable in every future sitrep. This is safe with - // respect to fault coverage: detection below is independent of case - // bookkeeping, so if a closed case concerned a disk that is genuinely - // unhealthy and in service, a fresh, well-formed case is opened in this - // same pass. - for (case_id, reason) in uninterpretable { - builder - .cases - .case_mut(&case_id) - .expect("case_id came from builder's open cases") - .close(format!("cannot interpret case: {reason}")); - } - - // Inverse index: which parent case is about which disk. Cases are - // per-disk, so a disk with two parent cases is pathological. `parent_cases` - // iterates ascending by CaseUuid, so the first case we see for a disk is - // the lowest-ID one: keep it, and close any later case for the same disk as - // a duplicate. (A half-maintained duplicate would otherwise decay into an - // uninterpretable empty case.) + // Inverse index: which parent case is about which disk. Cases are per-disk, + // so a disk with two parent cases is already pathological. We keep one and + // close the rest as duplicates; which one we keep is arbitrary. + // `parent_cases` iterates ascending by CaseUuid, so we deterministically + // keep the lowest-ID case, but the ID ordering carries no meaning here + // (keeping the oldest would be nicer, but that needs sitrep version numbers + // we don't thread through here). Closing the duplicate matters: a + // half-maintained one would otherwise decay into an uninterpretable empty + // case. let mut case_by_disk: BTreeMap< PhysicalDiskUuid, (CaseUuid, &ParsedDiskCase), @@ -278,7 +270,11 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { has_match = true; } else { // Stale observation; drop it. - case_mut.remove_fact(fact.fact_id); + case_mut.remove_fact( + fact.fact_id, + "stale zpool observation, superseded by current \ + inventory", + ); } } if has_match { From 0604e5e48d43a5682f6fd074f07bff2ec37270f5 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 16 Jun 2026 16:34:14 -0700 Subject: [PATCH 18/20] [fm] omdb test: redact racy fm_sitrep_gc orphan counts The fm_sitrep_gc task's "orphaned sitreps deleted" and "orphaned fm_sitrep_analysis_report rows deleted" counts are timing-dependent. When two fm_analysis activations overlap (e.g. the boot-time activation and the explicit drive in the test), both can insert a first sitrep before either is made current; the loser's sitrep and its stashed analysis report are inserted but orphaned (fm_sitrep_insert's ParentNotCurrent path), then deleted by a later GC pass. Whether that race occurred before the omdb snapshot is non-deterministic, which made test_omdb_success_cases flaky. Redact both counts. --- dev-tools/omdb/tests/successes.out | 8 ++++---- dev-tools/omdb/tests/test_all_output.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index a4c8c0886d1..ee1f67b7393 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -755,7 +755,7 @@ task: "fm_sitrep_gc" last completed activation: , triggered by started at (s ago) and ran for ms batch size: 1000 - orphaned sitreps deleted: 0 + orphaned sitreps deleted: batches: 1 orphaned fm_alert_request rows deleted: 0 batches: 1 @@ -765,7 +765,7 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_fact_physical_disk rows deleted: 0 batches: 1 - orphaned fm_sitrep_analysis_report rows deleted: 0 + orphaned fm_sitrep_analysis_report rows deleted: batches: 1 orphaned fm_support_bundle_request rows deleted: 0 batches: 1 @@ -1453,7 +1453,7 @@ task: "fm_sitrep_gc" last completed activation: , triggered by started at (s ago) and ran for ms batch size: 1000 - orphaned sitreps deleted: 0 + orphaned sitreps deleted: batches: 1 orphaned fm_alert_request rows deleted: 0 batches: 1 @@ -1463,7 +1463,7 @@ task: "fm_sitrep_gc" batches: 1 orphaned fm_fact_physical_disk rows deleted: 0 batches: 1 - orphaned fm_sitrep_analysis_report rows deleted: 0 + orphaned fm_sitrep_analysis_report rows deleted: batches: 1 orphaned fm_support_bundle_request rows deleted: 0 batches: 1 diff --git a/dev-tools/omdb/tests/test_all_output.rs b/dev-tools/omdb/tests/test_all_output.rs index 625ccef5f57..777a9c6047f 100644 --- a/dev-tools/omdb/tests/test_all_output.rs +++ b/dev-tools/omdb/tests/test_all_output.rs @@ -389,6 +389,19 @@ async fn test_omdb_success_cases() { r" --> (same collection as parent sitrep|different from parent sitrep \(collection [-a-f0-9]+\))", ); + // The `fm_sitrep_gc` task's orphan counts are racy. When two `fm_analysis` + // activations overlap (e.g. the boot-time activation and the explicit drive + // above), both can insert a first sitrep before either is made current; the + // loser's sitrep and its stashed analysis report are inserted but orphaned + // (see `DataStore::fm_sitrep_insert`'s `ParentNotCurrent` path), and a + // later GC pass deletes them. Whether that race happened before this + // snapshot is timing-dependent, so redact both counts. Other child tables + // stay zero: with no faults, orphaned sitreps carry no cases, facts, + // ereports, or bundles. + redactor + .field("orphaned sitreps deleted:", r"\d+") + .field("orphaned fm_sitrep_analysis_report rows deleted:", r"\d+"); + // The `sp_ereport_ingester` task's output depends on how many simulated // sled agents ahppen to register with Nexus before its first execution. // These redactions work around the issue described in From 8e687364935e50a7489ad858a66ca46ff40d8686 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 17 Jun 2026 11:08:28 -0700 Subject: [PATCH 19/20] [fm] address PR review feedback - physical_disk: dedup parent Disk cases via a BiHashMap keyed by both case id and physical disk id, making one-case-per-disk a structural invariant; drop the redundant duplicate-close warn (close() logs it) - fm_analysis: surface the duplicate-zpool anomaly in the input report warnings, and extract load_in_service_disks - case: render Fact comments as // comments, matching the Case metadata display convention - analysis_reports: exercise the in-service-disks display branch in the golden test --- nexus/fm/src/builder/case.rs | 5 +- nexus/fm/src/diagnosis/physical_disk.rs | 128 +++++++++--------- nexus/src/app/background/tasks/fm_analysis.rs | 48 +++++-- .../analysis_input_report_with_cases.out | 4 +- nexus/types/src/fm/analysis_reports.rs | 15 +- nexus/types/src/fm/case.rs | 9 +- 6 files changed, 123 insertions(+), 86 deletions(-) diff --git a/nexus/fm/src/builder/case.rs b/nexus/fm/src/builder/case.rs index ab6a02ef4ec..f7389877341 100644 --- a/nexus/fm/src/builder/case.rs +++ b/nexus/fm/src/builder/case.rs @@ -233,8 +233,9 @@ impl CaseBuilder { self.case.metadata.comment = comment.to_string(); } - /// Emit a new fact under this case. The fact's UUID is freshly - /// allocated from the case's deterministic RNG. + /// Emit a new fact under this case. + /// + /// Returns the newly generated fact UUID. pub fn add_fact( &mut self, payload: impl Into, diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 5832c1f9d6b..5f86b4bab44 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -5,14 +5,13 @@ //! Disk diagnosis engine. use crate::SitrepBuilder; -use iddqd::{IdOrdItem, IdOrdMap, id_upcast}; +use iddqd::{BiHashItem, BiHashMap, IdOrdItem, IdOrdMap, bi_upcast, id_upcast}; use nexus_types::fm; use nexus_types::fm::DiagnosisEngineKind; use nexus_types::fm::{DiskFact, ZpoolUnhealthyFactPayload}; use nexus_types::inventory::ZpoolHealth; use omicron_uuid_kinds::{CaseUuid, FactUuid, PhysicalDiskUuid, ZpoolUuid}; use std::collections::BTreeMap; -use std::collections::btree_map::Entry; /// A [`DiskFact::ZpoolUnhealthy`] payload paired with the `FactUuid` it /// lives under. @@ -52,6 +51,8 @@ impl IdOrdItem for DiskHealthSnapshot { /// Each Disk case is about a single physical disk; every fact on the case /// must reference that disk. struct ParsedDiskCase { + /// The case this was parsed from. + case_id: CaseUuid, /// The physical disk this case is about. physical_disk_id: PhysicalDiskUuid, /// All `ZpoolUnhealthy` facts on this case. Normally one; pathological @@ -60,6 +61,22 @@ struct ParsedDiskCase { unhealthy_facts: IdOrdMap, } +/// A `ParsedDiskCase` is indexed by both the case it came from and the disk it +/// concerns. Keying on `physical_disk_id` makes "at most one case per disk" an +/// invariant of the map itself: inserting a second case for a disk already +/// present fails (see [`analyze`]). +impl BiHashItem for ParsedDiskCase { + type K1<'a> = CaseUuid; + type K2<'a> = PhysicalDiskUuid; + fn key1(&self) -> Self::K1<'_> { + self.case_id + } + fn key2(&self) -> Self::K2<'_> { + self.physical_disk_id + } + bi_upcast!(); +} + /// Why a parent-forwarded Disk case could not be interpreted. /// /// Uninterpretable cases are closed by [`analyze`]: an open case this engine @@ -116,7 +133,7 @@ fn parse_case(case: &fm::Case) -> Result { let Some(physical_disk_id) = case_disk_id else { return Err(UninterpretableCase::NoFacts); }; - Ok(ParsedDiskCase { physical_disk_id, unhealthy_facts }) + Ok(ParsedDiskCase { case_id: case.id, physical_disk_id, unhealthy_facts }) } pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { @@ -149,70 +166,49 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { }) .collect(); - // Index the Disk cases copied forward from the parent sitrep. Every case - // is about one physical disk; we derive the disk from its facts. - let mut parent_cases = BTreeMap::::new(); + // Index the Disk cases from the parent sitrep, keyed by both the case id + // and the physical disk the case concerns. + // + // Keying on the disk makes "at most one case per disk" an invariant of the + // map: `insert_unique` rejects a second case for a disk already present. + // A disk with two parent cases is pathological; we keep the first one + // inserted and close the rest as duplicates. + let mut cases = BiHashMap::::new(); for case in input .open_cases() .iter() .filter(|c| c.metadata.de == DiagnosisEngineKind::PhysicalDisk) { - match parse_case(case) { - Ok(parsed_case) => { - parent_cases.insert(case.id, parsed_case); - } + let parsed_case = match parse_case(case) { + Ok(parsed_case) => parsed_case, Err(reason) => { // Close the cases we couldn't interpret, so they don't ride - // along as open-but-unprocessable in every future sitrep. This - // is safe with respect to fault coverage: detection below is - // independent of case bookkeeping, so if a closed case - // concerned a disk that is genuinely unhealthy and in service, - // a fresh, well-formed case is opened in this same pass. + // along as open-but-unprocessable in every future sitrep. builder .cases .case_mut(&case.id) .expect("case_id came from builder's open cases") .close(format!("cannot interpret case: {reason}")); + continue; } - } - } + }; - // Inverse index: which parent case is about which disk. Cases are per-disk, - // so a disk with two parent cases is already pathological. We keep one and - // close the rest as duplicates; which one we keep is arbitrary. - // `parent_cases` iterates ascending by CaseUuid, so we deterministically - // keep the lowest-ID case, but the ID ordering carries no meaning here - // (keeping the oldest would be nicer, but that needs sitrep version numbers - // we don't thread through here). Closing the duplicate matters: a - // half-maintained one would otherwise decay into an uninterpretable empty - // case. - let mut case_by_disk: BTreeMap< - PhysicalDiskUuid, - (CaseUuid, &ParsedDiskCase), - > = BTreeMap::new(); - for (case_id, parsed_case) in &parent_cases { - match case_by_disk.entry(parsed_case.physical_disk_id) { - Entry::Vacant(slot) => { - slot.insert((*case_id, parsed_case)); - } - Entry::Occupied(kept) => { - let (kept_case_id, _) = *kept.get(); - slog::warn!( - &builder.log, - "closing duplicate Disk case"; - "case_id" => %case_id, - "kept_case_id" => %kept_case_id, - "physical_disk_id" => %parsed_case.physical_disk_id, - ); - builder - .cases - .case_mut(case_id) - .expect("case_id came from builder's open cases") - .close(format!( - "duplicate of case {kept_case_id} for disk {}", - parsed_case.physical_disk_id, - )); - } + let disk_id = parsed_case.physical_disk_id; + if cases.insert_unique(parsed_case).is_err() { + // A case for this disk is already present; keep it and close this + // one. The disk id is the only key that can collide here, since + // case ids are unique across `open_cases()`. + let kept_case_id = cases + .get2(&disk_id) + .expect("insert_unique failed, so a case for this disk exists") + .case_id; + builder + .cases + .case_mut(&case.id) + .expect("case_id came from builder's open cases") + .close(format!( + "duplicate of case {kept_case_id} for disk {disk_id}", + )); } } @@ -221,10 +217,10 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // disk in service but absent from this inventory is left alone (absence is // NOT a recovery signal: the sled could be powered off, or the collection // could be lossy). - for &(case_id, parsed_case) in case_by_disk.values() { + for parsed_case in cases.iter() { let mut case_mut = builder .cases - .case_mut(&case_id) + .case_mut(&parsed_case.case_id) .expect("case_id came from builder's open cases"); match in_service_health.get(&parsed_case.physical_disk_id) { None => { @@ -248,6 +244,11 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { // parent-forwarded case if any (dropping any stale facts), otherwise open // a fresh case. for disk in in_service_health.iter() { + // Only modify "facts" for disk cases where the zpool health is known, + // and it's not "Online" already. + // + // For all other cases, the zpool is healthy, so we don't have + // a case anymore. let Some(current_health) = disk.zpool_health else { continue; }; @@ -255,13 +256,17 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { continue; } - let mut case_mut = match case_by_disk.get(&disk.physical_disk_id) { - Some(&(case_id, parsed_case)) => { + let mut case_mut = match cases.get2(&disk.physical_disk_id) { + Some(parsed_case) => { let mut case_mut = builder .cases - .case_mut(&case_id) + .case_mut(&parsed_case.case_id) .expect("case_id came from builder's open cases"); let mut has_match = false; + // Although we currently expect only one unhealthy fact for + // each case, the schema does allow for more than one. + // We iterate over all facts of these type to invalidate + // all the facts which are out-of-date. for fact in parsed_case.unhealthy_facts.iter() { if fact.payload.zpool_id == disk.zpool_id && fact.payload.last_seen_health == current_health @@ -771,9 +776,10 @@ mod tests { } /// Two open parent cases about the same disk: the engine keeps and - /// maintains the one with the lowest case ID, and closes the other as a - /// duplicate. (A half-maintained duplicate would otherwise decay into - /// an uninterpretable empty case.) + /// maintains the first one it encounters and closes the other as a + /// duplicate. Because `open_cases()` is ordered by case id, the survivor is + /// deterministically the lowest-ID case. (A half-maintained duplicate would + /// otherwise decay into an uninterpretable empty case.) #[test] fn duplicate_case_is_closed() { let (logctx, mut collection, zpools) = setup("disk_duplicate_closed"); diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index d056bac7221..20f8fa6d175 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -234,6 +234,31 @@ impl FmAnalysis { (fm::analysis_input::Input, status::PreparationStatus), PreparationError, > { + let mut warnings = Vec::new(); + + let in_service_disks = + Arc::new(self.load_in_service_disks(opctx, &mut warnings).await?); + + let mut builder = fm::analysis_input::Input::builder( + parent_sitrep, + inv, + in_service_disks, + )?; + self.load_new_ereports(opctx, &mut builder, &mut warnings) + .await + .context("failed to load new ereports")?; + + let (input, report) = builder.build(); + Ok((input, status::PreparationStatus { warnings, report })) + } + + /// Load all in-service control plane disks, projected down to FM's + /// [`InServiceDisk`] view. + async fn load_in_service_disks( + &self, + opctx: &OpContext, + warnings: &mut Vec, + ) -> anyhow::Result> { // Load all external (U.2) zpools and project them down to FM's // `InServiceDisk` view, filtering on `disk_policy = in_service` and a // live (non-soft-deleted) physical_disk row. M.2 disks are not @@ -247,7 +272,7 @@ impl FmAnalysis { .zpool_list_all_external_batched(opctx) .await .context("failed to load in-service control plane disks")?; - let mut in_service_disks_map = IdOrdMap::new(); + let mut in_service_disks = IdOrdMap::new(); for (zpool, disk) in zpools_and_disks { if disk.time_deleted().is_some() || disk.disk_policy != PhysicalDiskPolicy::InService @@ -256,7 +281,7 @@ impl FmAnalysis { } let physical_disk_id = disk.id(); let zpool_id = zpool.id(); - if in_service_disks_map + if in_service_disks .insert_unique(InServiceDisk { physical_disk_id, zpool_id, @@ -278,21 +303,14 @@ impl FmAnalysis { "physical_disk_id" => %physical_disk_id, "zpool_id" => %zpool_id, ); + warnings.push(format!( + "multiple live zpools reference physical disk \ + {physical_disk_id} (kept first seen, ignored zpool \ + {zpool_id})" + )); } } - let in_service_disks = Arc::new(in_service_disks_map); - let mut builder = fm::analysis_input::Input::builder( - parent_sitrep, - inv, - in_service_disks, - )?; - let mut warnings = Vec::new(); - self.load_new_ereports(opctx, &mut builder, &mut warnings) - .await - .context("failed to load new ereports")?; - - let (input, report) = builder.build(); - Ok((input, status::PreparationStatus { warnings, report })) + Ok(in_service_disks) } async fn load_new_ereports( diff --git a/nexus/types/output/analysis_input_report_with_cases.out b/nexus/types/output/analysis_input_report_with_cases.out index d47f769f343..66b4ea38b50 100644 --- a/nexus/types/output/analysis_input_report_with_cases.out +++ b/nexus/types/output/analysis_input_report_with_cases.out @@ -21,4 +21,6 @@ cases (2 total): copied forwards because these ereports haven't been marked seen yet: * ereport dddddddd-dddd-dddd-dddd-dddddddddddd:2 -no in-service control plane disks +in-service control plane disks (2 total): + * disk 11111111-1111-1111-1111-111111111111 + * disk 22222222-2222-2222-2222-222222222222 diff --git a/nexus/types/src/fm/analysis_reports.rs b/nexus/types/src/fm/analysis_reports.rs index 16a54e94da5..d5e2929a754 100644 --- a/nexus/types/src/fm/analysis_reports.rs +++ b/nexus/types/src/fm/analysis_reports.rs @@ -386,7 +386,8 @@ mod tests { use super::*; use ereport_types::{Ena, EreportId}; use omicron_uuid_kinds::{ - CaseUuid, CollectionUuid, EreporterRestartUuid, SitrepUuid, + CaseUuid, CollectionUuid, EreporterRestartUuid, PhysicalDiskUuid, + SitrepUuid, }; use std::str::FromStr; @@ -452,6 +453,16 @@ mod tests { }, ); + let mut in_service_disks = BTreeSet::new(); + in_service_disks.insert( + PhysicalDiskUuid::from_str("11111111-1111-1111-1111-111111111111") + .unwrap(), + ); + in_service_disks.insert( + PhysicalDiskUuid::from_str("22222222-2222-2222-2222-222222222222") + .unwrap(), + ); + InputReport { parent_sitrep_id: Some(parent_sitrep_id), parent_inv_id: Some(parent_inv_id), @@ -459,7 +470,7 @@ mod tests { new_ereport_ids, open_cases, closed_cases_copied_forward, - in_service_disks: BTreeSet::new(), + in_service_disks, } } diff --git a/nexus/types/src/fm/case.rs b/nexus/types/src/fm/case.rs index 686ef49eec0..b0717b954a0 100644 --- a/nexus/types/src/fm/case.rs +++ b/nexus/types/src/fm/case.rs @@ -217,9 +217,6 @@ impl Fact { impl fmt::Display for DisplayFact<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { const BULLET: &str = "* "; - const ADDED_IN: &str = "added in:"; - const COMMENT: &str = "comment:"; - const WIDTH: usize = const_max_len(&[ADDED_IN, COMMENT]); let &Self { fact: @@ -236,13 +233,15 @@ impl Fact { }; writeln!(f, "{BULLET:>indent$}fact {id}")?; + if !comment.is_empty() { + writeln!(f, "{:>indent$}// {comment}", "")?; + } writeln!( f, - "{:>indent$}{ADDED_IN:indent$}added in: {created_sitrep_id}{}", "", this_sitrep(*created_sitrep_id), )?; - writeln!(f, "{:>indent$}{COMMENT: Date: Wed, 17 Jun 2026 16:46:17 -0700 Subject: [PATCH 20/20] [fm] use DuplicateItem error to read kept case, avoiding a second lookup --- nexus/fm/src/diagnosis/physical_disk.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 5f86b4bab44..202752f70f7 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -193,21 +193,19 @@ pub(super) fn analyze(builder: &mut SitrepBuilder<'_>) -> anyhow::Result<()> { } }; - let disk_id = parsed_case.physical_disk_id; - if cases.insert_unique(parsed_case).is_err() { + if let Err(dup) = cases.insert_unique(parsed_case) { // A case for this disk is already present; keep it and close this // one. The disk id is the only key that can collide here, since - // case ids are unique across `open_cases()`. - let kept_case_id = cases - .get2(&disk_id) - .expect("insert_unique failed, so a case for this disk exists") - .case_id; + // case ids are unique across `open_cases()`, so there is exactly + // one duplicate. + let kept = dup.duplicates()[0]; builder .cases .case_mut(&case.id) .expect("case_id came from builder's open cases") .close(format!( - "duplicate of case {kept_case_id} for disk {disk_id}", + "duplicate of case {} for disk {}", + kept.case_id, kept.physical_disk_id, )); } }