[fm] Add disk diagnoser with typed fact tables by smklein · Pull Request #10541 · oxidecomputer/omicron

smklein · 2026-06-03T18:42:12Z

This PR provides an alternative to #10460 , using strongly-typed "enum-ish" tables for each DE's set of facts, rather than a JSON blob. It should be mutually exclusive with that PR (we either do JSON, or we don't).

The first fault management diagnosis engine: opens a case for any non-Online zpool whose backing physical disk is currently in service in the control plane, and closes it on recovery or expungement. The engine also closes any case it cannot interpret (foreign fact payload, facts disagreeing on the disk, no facts) and any duplicate case for a disk, rather than carrying unprocessable cases forward forever; this is safe for fault coverage because a still-faulty disk gets a fresh well-formed case in the same analysis pass.

Supporting infrastructure introduced along the way:

DiagnosisEngineKind::PhysicalDisk variant (Rust + DB enum)
fm_fact_physical_disk child table for per-engine state, with typed columns selected by a kind discriminant and a per-kind CHECK constraint (one case has 0..N immutable facts; stable UUIDs across sitreps; participates in copy-forward + GC like other sitrep child tables)
CaseBuilder::{add_fact, remove_fact, facts} API
InServiceDisk nexus-types projection consumed by FM, populated from the existing zpool_list_all_external_batched datastore method with policy filtering done in the background task

The omdb output test drives the FM tasks to a deterministic steady state with explicit background task activations (analysis -> loader -> analysis -> rendezvous) rather than waiting on watch channels.

smklein · 2026-06-03T18:49:52Z

-                    )
-                    .execute_async(&conn)
-                    .await?;
+                    // Delete every child row by sitrep_id. Driving this off


This looks a little involved, but it's worth noting: This is only used in tests? So doing some one-time complexity to avoid a need for future updates on each new fact type seems like a nice tradeoff.

…lerate 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

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.

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.

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.

smklein · 2026-06-10T21:14:32Z

+    /// 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,


So: I'm shoving this in here because we previously called analyze with SitrepBuilder and input as two distinct arguments.

Realistically, they would come from the same input, but I want to prevent them from ever diverging, so... now they're both part of SitrepBuilder.

(This is a very tiny change but it does change the API of analyze)

hm, i think this is good, i was passing them separately because i wanted to be able to mutably borrow the SitrepBuilder while also immutably borrowing the input. it looks like because you're handing out a borrow of the input that is not tied to the lifetime of the builder, we are hopefully fine here?

smklein · 2026-06-10T21:15:53Z

+    -- UUID of the case this fact attaches to.
+    case_id UUID NOT NULL,


originally I had an index for "lookup fact by case ID" but I actually removed it; it's not necessary with how we do sitrep loading currently.

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.

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.

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).

smklein · 2026-06-15T17:30:17Z

+impl FmFactPhysicalDisk {
+    /// Build a row from a fact's shared metadata (`fact`) and its
+    /// physical-disk payload (`disk_fact`).
+    pub fn from_sitrep(


I'm sorta following the convention of the other fm-based models here (they all use from_sitrep), but I kinda think this method should maybe just be called "new"?

IDK; might change this in a follow-up.

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.

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.

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.

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.

hawkw

I haven't read through the entire PR yet, but I had one high-level design thought I wanted to bring up now to get your take on. Plus, some smaller notes.

hawkw · 2026-06-15T18:36:15Z

+    // 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".


This rocks, thank you!

hawkw · 2026-06-15T18:36:49Z

+    // 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]+\))",


urgh, I wanted to suggest we wrap this line, but it's a raw string, so that's hard. never mind.

hawkw · 2026-06-15T18:41:14Z

+    pub id: DbTypedUuid<FactKind>,
+    /// 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<SitrepKind>,
+    pub case_id: DbTypedUuid<CaseKind>,
+    /// Sitrep in which this fact was first added.
+    ///
+    /// Preserved unchanged when the fact is carried forward; debug-only.
+    pub created_sitrep_id: DbTypedUuid<SitrepKind>,
+    pub comment: String,


extremely not a blocker for this PR, but it occurs to me that these fields are going to be present on every fact table and also on the preexisting non-DE-specific "case objects" (i.e. alert requests, support bundle requests, and so on). I wonder if we might eventually want to have a macro or something similar to the existing #[derive(Asset)] and #[derive(Resource)] macros in nexus-db-macros for these structures.

For one thing, this could generate a fact_metadata() accessor automagically, and maybe it could also eventually be something we could hang the sitrep GC stuff off of, rather than having to manually add the GCed tables in the db-queries file.

I may try to hack something like that together if I have some spare time, or someone else might want to.

I think this is reasonable idea - also, prioritization-wise, I do really want to get this merged with some DB schema first.

I really want "many DEs, some of which have multiple facts" before investing the time in macros, just to have extra validation that we won't need to churn. I do think those macros will make it easier for developers to add facts, but the "total number of DEs and facts" seems very small right now, so I'd prefer to "wait a little, and do it with high confidence" rather than do it too early.

yeah, for sure!

hawkw · 2026-06-15T19:14:32Z

+    pub kind: FmFactPhysicalDiskKind,
+
+    // Columns for the `ZpoolUnhealthy` kind.
+    pub zpool_id: Option<DbTypedUuid<ZpoolKind>>,
+    pub last_seen_health: Option<InvZpoolHealth>,
+    pub observed_in_inv: Option<DbTypedUuid<CollectionKind>>,
+    pub time_observed: Option<DateTime<Utc>>,


As I understand it, the design here is that we would allow a DE to record multiple distinct fact types in a case by creating multiple records in the fact table for that DE associated with the case, and each of those records would map to exactly one variant of a Rust enum when the sitrep is loaded. I'm starting to wonder whether we really need to do this, or if there might be a simpler approach. Do we really anticipate that we will need to have this one-to-many relationship between a case and its facts, or could we do something where instead of having a "fact record", we have a "facts record" for each case?

Here, if we wanted to add a different kind of fact to the "physical disk" diagnosis engine, we would do something like adding a new variant of FmFactPhysicalDiskKind (say, "PcieLinkDown" or something), and then adding a bunch of nullable fields to the facts table, and having the DE insert a new fact record to represent the "PcieLinkDown" fact. But...what if we just had one record for each case that could represent both the unhealthy zpool facts and the PCIe link down facts? If the DE wanted to be able to have Rust types for each set of facts, you could imagine doing something like:

impl FmFactsPhysicalDisk { pub fn as_zpool_unhealthy(&self) -> Option<ZpoolUnhealthy> { let Self { zpool_id: Some(zpool_id), last_seen_health: Some(last_seen_health), unhealthy_zpool_observed_in_inv: Some(observed_in_inv), unhealthy_zpool_time_observed: Some(time_observed), ... // ignore unrelated fields } = self else { return None; }; Some(ZpoolUnhealthy { zpool_id, last_seen_health, observed_in_inv, time_observed, }) } pub fn as_pcie_link_down(&self) -> Option<PcieLinkDown> { let Self { link_down_whatever_fields_this_has: Some(whatever), link_down_other_field: Some(other_field), ... // ignore unrelated fields } = self else { return None; }; Some(PcieLinkDown { whatever, other_field, }) } }

We could still have an enum with variants for all those structs, too, if we wanted to, and could do something like having an iterator over each kind of fact the case contains, but I'm not sure if we would need that.

And, in some cases, the facts we want to represent may be simple enough that we don't actually need all this ceremony and they're just single columns that may or may not be present in a record for a particular case. This is the most attractive part of this approach, IMO --- if a DE just wants to be able to store a few different facts that are each representable by a single column, it can just add them to the schema for its table without having to go through all the ceremony of messing around with enum variants and stuff.

This might end up making things a bit easier, and prevents us from being able to represent some of the nonsensical cases we currently can (such as a case with several different zpool_unhealthy facts for the same zpool).

On the other hand, there may also be future DEs that do need the one-to-many fact relationship --- for example, you might want to be able to record that various crucible volumes were all impacted by an unhealthy physical disk. In that case, we might be able to represent that by using SQL arrays to store the multiple instances of the same fact in one record, like impacted_crucible_volume_ids UUID ARRAY or something. This might get messy if we wanted these one-to-many facts to store multiple fields per fact, but maybe there's a nice way to do that as well? Here, we don't care about the main advantage of representing one-to-many relationships as separate tables, because we're not going to be doing a bunch of JOINs on sitrep data; we're just loading the whole thing in one go.

Basically, I think the crux of what I'm getting at here is whether we actually need "fact tables" to represent a case's DE-specific facts, or if all we really need are "case tables" that extend a case with one record in a DE-specific table. This isn't a strongly-held belief and I'm very open to being convinced we do need the additional expressiveness of multiple fact records per case, but I wanted to bring it up so we could weigh some of the pros and cons.

Per a Matrix conversation with @smklein, I think we've determined that SQL arrays are not going to be sufficiently expressive for a lot of the multiple fact cases we may want to be able to represent, so we're going to continue with the current thing.

I think my motivation towards "have a one to many relationship between cases and facts" is that I suspect, especially for more complex cases, we'll need to build up a variety of facts over time to actually determine whether the case should be transformed into an active problem. I don't really know, yet, how complex this is going to get, but IMO the flexibility of: "you want to be able to add arbitrary data points as facts in the DB, and then re-assemble them in memory to analyze the current case status" seems like it should be possible.

There are also some limitations with ARRAY types in CRDB specifically, they warn:

The ARRAY data type is useful for ensuring compatibility with ORMs and other tools. However, if such compatibility is not a concern, it's more flexible to design your schema with normalized tables.

Which doesn't mean they're always bad! just that... there may be footguns we're dropping for ourselves if we want them to support arbitrary data this way. For example, they cannot support nesting, and not all types can be used in arrays.

hawkw · 2026-06-15T22:33:15Z

+    /// Reconstruct an in-memory fact from a row.
+    ///
+    /// 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<fm::case::Fact, Error> {
+        let kind = self.kind;
+        let payload = match kind {
+            FmFactPhysicalDiskKind::ZpoolUnhealthy => {
+                FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy(
+                    ZpoolUnhealthyFactPayload {
+                        physical_disk_id: self.physical_disk_id.into(),
+                        zpool_id: self
+                            .zpool_id
+                            .ok_or_else(|| missing_column(kind, "zpool_id"))?
+                            .into(),
+                        last_seen_health: self
+                            .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(kind, "observed_in_inv")
+                            })?
+                            .into(),
+                        time_observed: self.time_observed.ok_or_else(|| {
+                            missing_column(kind, "time_observed")
+                        })?,
+                    },
+                ))
+            }
+        };
+        Ok(fm::case::Fact {
+            metadata: FactMetadata {
+                id: self.id.into(),
+                created_sitrep_id: self.created_sitrep_id.into(),
+                comment: self.comment,
+            },
+            payload,
+        })
+    }


someday, i bet we'll be able to figure out a way to derive all this from an enum, but let's save that for later. :)

hawkw · 2026-06-15T22:33:45Z

+        // (... gonna punt on that until we actually have a couple different
+        // tables to query from).


reasonable!

hawkw · 2026-06-15T22:36:47Z

+    /// 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) {


it might be kind of nice to add a comment to this, so that you can also log why you removed the fact? not a huge deal though.

Done, that seems like a good idea. f9479fc

hawkw · 2026-06-15T22:37:47Z

+        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);


take it or leave it: perhaps we could change this to use if let Some(fact) = self.case.facts.remove(&id) { and then also include the kind of fact that was removed in the log?

f9479fc

Sounds good, we're logging the payload of the fact now (via slog and in the report log)

hawkw · 2026-06-15T22:39:34Z

+/// Empty today: Grow this list alongside FM analysis as new classes gain
+/// ereport support.


honestly i would probably remove this completely from the doc comment

Suggested change

/// Empty today: Grow this list alongside FM analysis as new classes gain

/// ereport support.

Agreed, cutting this out

hawkw · 2026-06-15T22:41:49Z

+                slog::warn!(
+                    &builder.log,
+                    "closing uninterpretable Disk case";
+                    "case_id" => %case.id,
+                    "reason" => %reason,
+                );


do we need to log this separately here? it will also get logged when we actually go and close the case a couple lines later.

hawkw · 2026-06-15T22:45:29Z

+    let mut parent_cases = BTreeMap::<CaseUuid, ParsedDiskCase>::new();
+    let mut uninterpretable = Vec::<(CaseUuid, UninterpretableCase)>::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);
+            }
+            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.
+    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}"));
+    }


actually wait a sec, why do we need to do this weird little dance of ptutign all the uninterpretable cases in a vec and then looping over it and closing them? at first glance I thought this was necessary because we we were immutably borrowing the cases to make the iterator and needed to mutate them here, but...the iterator immutably borrows the cases in the inputs, not the SitrepBuidler, which is what we're mutating. so why can't this just be:

Suggested change

let mut parent_cases = BTreeMap::<CaseUuid, ParsedDiskCase>::new();

let mut uninterpretable = Vec::<(CaseUuid, UninterpretableCase)>::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);

}

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.

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}"));

}

let mut parent_cases = BTreeMap::<CaseUuid, ParsedDiskCase>::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);

}

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.

builder

.cases

.case_mut(&case.id)

.expect("case_id came from builder's open cases")

.close(format!("cannot interpret case: {reason}"));

}

}

}

agreed, this was an artifact of some earlier iteration. but one less vec seems good.

done in f9479fc , almost exactly as you proposed

hawkw · 2026-06-15T22:48:58Z

+    // 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.)


hm, deciding which case has priority based on uuid ordering seems a bit unfortunately arbitrary. it kinda seems like it would be nicer to keep the oldest one, but determining that would require a bigger change to get the sitrep version numbers in here so we could compare them... 🤷‍♀️

at the end of the day, i think making this decision arbitrarily based on version numbers is probably not terrible, but the comment makes it feel like it's the correct behavior, whcih...'im not convinced about.

I've re-worded this a bit to make it clear it's arbitrary. I'm not even sure "oldest" should win! This whole thing seems dependent on "case opening logic not seeing already-open cases", which seems buggy from the get-go

hawkw · 2026-06-15T23:10:54Z

~~Okay, so this time, the CI failure is due to a timeout waiting for the Nexus handoff, which I really don't think is my fault?~~

EDIT: my bad, this was meant to be a comment on #10541, and it was also wrong, since the build failure was actually my fault there... 😅

…ookup 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).

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'.

Brings the disk diagnoser's typed-fact-table work (and main) into the saga branch. The merge propagates the Fact -> FactMetadata + payload split and the new remove_fact(comment) signature into the saga engine, its db-model row (fm_fact_saga), the datastore, and their tests so the tree builds; the saga-specific cleanups mirroring #10541 follow in a separate commit.

Mirror the disk diagnoser cleanups in the saga engine, now that both share the FactMetadata/payload split: - summarize_case -> parse_case (it validates and rejects, not just condenses) and ParentSagaCase -> ParsedSagaCase to match. - observed -> observed_sagas. - Close uninterpretable cases inline in the parse loop instead of collecting them into a vec and looping again. - Build case_for_saga in a single pass with the Entry API, closing duplicate cases inline; reframe the tie-break comment as arbitrary-but-deterministic rather than 'correct'. - Separate case-closing from fact reconciliation: the first pass over surviving cases is now closing-only, and the second pass owns all of a saga's fact state (drop duplicate + stale facts, add fresh) in one place. A new covered() helper folds the three per-kind match arms. - FmFactSaga::from_sitrep takes (metadata, payload) and destructures FactMetadata exhaustively, so a new field fails to compile until it is mapped to a column. - Tests: replace the (Fact, SagaFact) tuple from saga_facts with a named SagaFactRef, and extract the repeated sole-fact lookup into a sole_saga_fact_id helper.

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.

hawkw

Okay, I feel good about this design, and I also feel pretty good about the DE implementation. This is a lot of code, and I had a lot more comments about fairly minor things, but I feel like I'm onboard with the design and we can merge this whenever you feel good about it. Hopefully some of my last round of comments are also helpful!

hawkw · 2026-06-16T18:15:31Z

+    /// 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,


hm, i think this is good, i was passing them separately because i wanted to be able to mutably borrow the SitrepBuilder while also immutably borrowing the input. it looks like because you're handing out a borrow of the input that is not tied to the lifetime of the builder, we are hopefully fine here?

hawkw · 2026-06-17T00:02:42Z

    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:                             <ORPHANED_FMSITREPANALYSISREPORT_ROWS_DELETED_REDACTED>


lol FMSITREPANALYSISREPORT

hawkw · 2026-06-17T00:06:17Z

+    // Columns for the `ZpoolUnhealthy` kind.
+    pub zpool_id: Option<DbTypedUuid<ZpoolKind>>,
+    pub last_seen_health: Option<InvZpoolHealth>,
+    pub observed_in_inv: Option<DbTypedUuid<CollectionKind>>,
+    pub time_observed: Option<DateTime<Utc>>,


much as it pains me to suggest this, I wonder somewhat if we might want to begin a convention of prefixing all of these with the fact kind name (so here, zpool_unhealthy_) to make it more obvious what thing they correspond to? on the other hand it feels so unpleasant that i regret suggesting this.

asdf I'm on the fence about this. I understand "why", but this could result in some obnoxiously long column names, where we're already guarded by the CONSTRAINT + in-memory reconstructed types.

hawkw · 2026-06-17T00:07:31Z

+                        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!",
+                        );


no one would blame you for panicking here, i think. but this is a bit nicer.

hawkw · 2026-06-17T00:44:49Z

+/// 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),
+}
+
+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 {
+    /// 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<Utc>,
+}


eventually i imagine we might want to stick each DE's facts in a separate module to avoid merge conflicts when multiple people are working on separate DEs. not a big deal yet though.

hawkw · 2026-06-17T00:45:48Z

+//! "Currently in-service control-plane disks": the executed view from
+//! the `physical_disk` and `zpool` DB tables.


"executed view" is a somewhat weird phrase to me, but I think I get what you're getting at.

- 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

hawkw · 2026-06-17T23:28:36Z

I just want to say that it feels really good to see this one land. Thank you again for all your hard work @smklein!

hawkw · 2026-06-17T23:30:26Z

+            // 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;


FWIW, the DuplicateItem error that insert_unique returns will also let you access the key of the the existing cases, so you don't actually have to look it up here.

Done in c4ff887

Adds the a fault management diagnosis engine: the problematic saga diagnoser. ## What it does Opens a case (keyed by saga ID) for any unfinished saga that needs attention. This PR uses three fact kinds: - **`NotProgressing`**: a live (running or unwinding) saga has recorded no node event for longer than `STALE_SAGA_THRESHOLD` (this is 30 minutes, but we can set it to whatever). - **`OwnerNotCurrentGeneration`**: the saga's `current_sec` is a Nexus that will never advance it, either quiesced (an older generation that handed off) or expunged (no `db_metadata_nexus` record). - **`Abandoned`**: Nexus permanently gave up on recovering the saga. ## Inputs The preparation phase builds an `ObservedSaga` projection, the saga analog of #10541's `InServiceDisk`: all unfinished sagas (running, unwinding, or abandoned), joined with each saga's latest node-event time and its owner's classification against `db_metadata_nexus`. This is the executed view read directly from the database, and it is included in the analysis input report so "why did FM (not) flag this saga" is answerable from a sitrep. ## Case lifecycle The two live-saga facts may coexist on one case; as conditions change, facts are removed and re-added under the same case (with stable fact UUIDs when the observation is unchanged). The case closes when the saga completes, or when every condition clears (progress resumes, owner re-adopted), and a recurrence later opens a fresh case. An abandoned saga's case never closes on its own; it persists, carrying the `Abandoned` fact, until remediation removes the saga row. Actually remediating an `Abandoned` saga seems like a case-by-case resolution, so I'm not doing it in this PR! ## Context - #10531: dogfood had sagas stuck in running/unwinding with no Nexus actually executing them. The determination there was that these should be surfaced by FM (#10530), decoupled from the update health check. This engine is that surfacing mechanism. - #10581: Nexus will explicitly abandon sagas it cannot recover for non-transient reasons. The `Abandoned` fact kind exists so that landing #10581 escalates these cases instead of silently closing them. Fixes #10530

[fm] Add disk diagnoser with typed fact tables

6e094f4

smklein commented Jun 3, 2026

View reviewed changes

smklein added 7 commits June 3, 2026 11:51

better errors

bfedaa5

clearer constraints, a non-null column for physical_id

1b5106a

merge

ae1ec41

drop unused lookup_fm_fact_physical_disk_for_case index

14c2a06

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.

[fm] disk diagnoser: index parent cases by disk

d2a6767

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.

smklein marked this pull request as ready for review June 10, 2026 21:12

smklein commented Jun 10, 2026

View reviewed changes

smklein mentioned this pull request Jun 11, 2026

[fm] Add saga diagnosis engine #10592

Merged

smklein added 3 commits June 11, 2026 12:50

simplify comments

82ef44f

merge

cebfc8c

smklein requested review from hawkw and mergeconflict and removed request for hawkw June 11, 2026 22:22

smklein added 2 commits June 12, 2026 14:57

smklein commented Jun 15, 2026

View reviewed changes

smklein added 2 commits June 15, 2026 11:00

[fm] split Fact into FactMetadata + payload

2718755

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.

[fm] disk diagnoser: clearer names for parsing a case

0ada45d

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.

smklein force-pushed the fm-disk-diagnoser-typed branch from 069e424 to 0ada45d Compare June 15, 2026 18:44

smklein added 2 commits June 15, 2026 11:51

hawkw reviewed Jun 15, 2026

View reviewed changes

smklein added 2 commits June 15, 2026 17:23

smklein mentioned this pull request Jun 16, 2026

test failed in CI: integration_tests::instances::test_instance_migrate #10627

Closed

merge

221dcc2

smklein added 2 commits June 16, 2026 13:35

merge

3c5545d

hawkw approved these changes Jun 17, 2026

View reviewed changes

smklein enabled auto-merge (squash) June 17, 2026 23:05

smklein mentioned this pull request Jun 17, 2026

[fm] Add simple disk diagnoser based on zpool health #10460

Closed

hawkw reviewed Jun 17, 2026

View reviewed changes

hawkw disabled auto-merge June 17, 2026 23:31

[fm] use DuplicateItem error to read kept case, avoiding a second lookup

c4ff887

smklein enabled auto-merge (squash) June 17, 2026 23:47

smklein merged commit b18659c into main Jun 18, 2026
18 checks passed

smklein deleted the fm-disk-diagnoser-typed branch June 18, 2026 01:52

hawkw mentioned this pull request Jul 20, 2026

Miscellaneous Cleanups For The FM Subsystem #10869

Open

6 tasks

hawkw mentioned this pull request Nov 3, 2026

want "SMF services in maintenance" under active problems #11675

Open

		-- UUID of the case this fact attaches to.
		case_id UUID NOT NULL,

		// (... gonna punt on that until we actually have a couple different
		// tables to query from).

		/// Empty today: Grow this list alongside FM analysis as new classes gain
		/// ereport support.

		//! "Currently in-service control-plane disks": the executed view from
		//! the `physical_disk` and `zpool` DB tables.

Conversation

smklein commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein commented Jun 3, 2026 •

edited

Loading

hawkw commented Jun 15, 2026 •

edited

Loading