[fm] Filter ereports during preparation by 'known-to-Nexus' by smklein · Pull Request #10349 · oxidecomputer/omicron

smklein · 2026-04-30T20:47:06Z

Before this PR, the preparation phase of FM loaded "all unmarked ereports". Unfortunately, if this set contains any ereports not-known to Nexus (which is, today, all of them) this set will grow in an unbounded fashion. This is problematic, as the only good FM subsystem is a stable FM subsystem.

To mitigate: this PR adds a set of "known ereport classes", as a big static array. Once we begin parsing / using these ereport classes, we can (and should!) add to this list. Thus: we'll only pull in ereports we can actually use.

Although this leaves the FM machinery up and running (and continuing to scan) it should also mean that we can continue to work on FM incrementally: if we even add a single "ereport class" which we want to query, we should be able to start parsing them, marking them "seen", and we'll have appropriate backpressure on the loading side of things.

Additionally, this PR adds omdb db ereport classes, which will help us analyze the set of known/unknown ereports. This has output like the following (though my sample contents here are, admittedly, totally made up):

$ omdb db ereport classes
  KNOWN-TO-NEXUS CLASS                                TOTAL UNMARKED
  no             ereport.cpu.amd.l3_cache_correctable 2681  2681
  no             ereport.fan.over_temp                2674  2674
  no             ereport.psu.under_voltage            2323  2323
  no             ereport.dimm.ce_threshold            1947  1947
  no             ereport.nic.link_flap                1937  1937
  no             ereport.thermal.fan_stuck            1832  1832
  no             ereport.smbus.timeout                1729  1729
  no             ereport.data_loss.possible           1466  1466
  no             ereport.data_loss.certain            1439  1439
  yes            ereport.example.handled              50    0
  excluded       (NULL)                               12    12

  Known classes with no rows in the database:
    ereport.example.never_seen

Fixes #10348

hawkw · 2026-04-30T21:23:55Z

Additionally, this PR adds omdb db ereport classes, which will help us analyze the set of known/unknown ereports. This has output like the following (though my sample contents here are, admittedly, totally made up):

neat!

hawkw

overall i like this approach a lot! i had a bunch of annoying but ultimately unimportant nitpicks.

hawkw · 2026-04-30T22:04:59Z

+/// a few hundred entries; past ~1000 entries, prefer either prefix matching
+/// (`class LIKE 'ereport.cpu.amd.%'`) or a `known_ereport_class` lookup
+/// table joined into the query. Revisit this if the list grows that large.
+pub fn known_ereport_classes() -> &'static [&'static str] {


it feels a bit weird to me that this is in nexus_types::fm rather than in nexus_fm, which is where the diagnosis engine implementations will end up living? i would kind of expect this list to eventually be composed by concatenating lists of ereport classes from each DE module or something...

I intentionally put it here so that omdb could depend on it without adding a new dependency on nexus_fm

How strong is your preference to pull it out? Would you prefer omdb to have that direct dependency?

oh, hmm. well, i feel in the future this list is probably gonna end up coming from nexus-fm, unless we're happy just having one big list that any future change has to remember to update. i would really much rather break it out by DE, but 🤷‍♀️

Here's a thought: what if, rather than relying on omdb and Nexus having been built from the same commit, we made this a lockstep API that omdb could use to ask Nexus for the list of which ereports its DEs care about? That breaks the build-time dependency issue and also avoids lying when the omdb and Nexus versions are different. It's more work, though.

Done in c5c53c2, with a fall-back to just showing "?" if we don't know.

It seems pretty straightforward to do this, and the benefit of "not getting omdb/nexus versions mixed up" seems worth it.

hawkw · 2026-05-01T02:32:47Z

+            f.write_str(match self {
+                Self::Yes => "yes",
+                Self::No => "no",
+                Self::NullClass => "NullClass",


ew. i feel like this could just be

Suggested change

Self::NullClass => "NullClass",

Self::NullClass => "-",

or

Suggested change

Self::NullClass => "NullClass",

Self::NullClass => "n/a",

especially 'cause the column ought to be only 3 characters wide otherwise

Fixed in 5c5d500

hawkw · 2026-05-01T02:33:55Z

+    // The variable-length `class` column goes last so that wrapping on a
+    // narrow terminal doesn't disrupt the fixed-width numeric columns.


feels like it ought to be a comment on the field...

Sure, moved in 5c5d500

hawkw · 2026-05-01T02:34:50Z

+    #[derive(Tabled)]
+    #[tabled(rename_all = "SCREAMING_SNAKE_CASE")]
+    struct ClassRow<'a> {
+        #[tabled(rename = "KNOWN-TO-OMDB")]


everything else is SCREAMING_SNAKE_CASE rather than SCREAMING-KEBAB-CASE...and i might just say "KNOWN" to save characters honestly

hawkw · 2026-05-01T02:36:52Z

+        "Note: KNOWN-TO-OMDB reflects which classes have a diagnosis engine \
+         in THIS omdb\nbinary; the currently-deployed Nexus may differ if it \


weird phrasing here, the omdb binary does not technically have any diagnosis engines in it. it would be correct to say "have a diagnosis engine in Nexus as of the control plane build that produced this omdb binary" or something. i would also like it if this said what version this OMDB is, but i wont stress too hard about that.

also normally i think "note:" is lowercase in other omdb commands

5c5d500 , though I also added a lockstep API in c5c53c2

(hopefully this removes the need to print the omdb version?)

hawkw · 2026-05-01T02:38:37Z

+            "ereport.positronic-brain.example",
+            "ereport.spinning-blades.example",


hawkw · 2026-05-01T02:40:17Z

+            .select(db::model::Ereport::as_select())
+            .load_async(&*conn)
+            .await
+            .expect("failed to list unseen ereports")


this doesn't end up running afoul of the full scan rule, does it?

I don't think so? I'm pretty sure we'd be blowing up with the "FULL SCAN NOT ALLOWED" issue if it was

(also, FYI, this is test-only)

hawkw · 2026-05-01T02:41:35Z

+/// a few hundred entries; past ~1000 entries, prefer either prefix matching
+/// (`class LIKE 'ereport.cpu.amd.%'`) or a `known_ereport_class` lookup
+/// table joined into the query. Revisit this if the list grows that large.
+pub fn known_ereport_classes() -> &'static [&'static str] {


oh, hmm. well, i feel in the future this list is probably gonna end up coming from nexus-fm, unless we're happy just having one big list that any future change has to remember to update. i would really much rather break it out by DE, but 🤷‍♀️

mergeconflict · 2026-05-01T13:30:20Z

I'm thinking about this a bit... I wonder if, in the immediate term, we should just do the simplest thing possible to avoid blowing up the logs -- you mentioned "disable the sitrep planner altogether" in #10348, which I'm guessing would be a super easy PR to land and cherry-pick into 19.x if needed.

Then, that would buy us a bit of time to think about the allowlist/filter design a bit more. My initial thought on that is that, rather than having a static array of ereport types that we know how to handle, each diagnosis engine present in the system should provide the set of ereports that it knows how to consume. We'd take these sets and union them together (on startup I guess) to create the full allowlist. That places the burden of proof of "no unhandled ereports" locally with the thing that handles them, rather than being something off to the side that we'd have to remember to update. What do you think?

Having said that, this PR totally looks good to merge (modulo expectorate) if it's the direction we want to go. Thanks for jumping on this so quickly!

hawkw · 2026-05-01T13:43:05Z

I'm thinking about this a bit... I wonder if, in the immediate term, we should just do the simplest thing possible to avoid blowing up the logs -- you mentioned "disable the sitrep planner altogether" in #10348, which I'm guessing would be a super easy PR to land and cherry-pick into 19.x if needed.

My preference is to move forwards with this change rather than disabling, because that creates a separate "un disabling" step we have to take once things are fixed, and I've become wary of using the config file in cases where it isn't actually necessary. And, if we disable the fm_analysis task entirely, we still have the issue that, if we turn it on once we add a DE that cares about a small set of ereports, we're still potentially loading a bunch of other ones we don't know what to do with. Adding the list of ereports that DEs care about is something we would probably end up doing anyway even if we did reach for the blunt instrument first.

Then, that would buy us a bit of time to think about the allowlist/filter design a bit more. My initial thought on that is that, rather than having a static array of ereport types that we know how to handle, each diagnosis engine present in the system should provide the set of ereports that it knows how to consume. We'd take these sets and union them together (on startup I guess) to create the full allowlist. That places the burden of proof of "no unhandled ereports" locally with the thing that handles them, rather than being something off to the side that we'd have to remember to update. What do you think?

This is certainly my preference. But I think it makes sense to first land the changes to the queries and such that allow them to take a list of desired ereports, and then, once we have more of a framework for implementing individual DEs, we would change how the list of desired ereports classes is constructed by looking at the enabled DEs. But, we would still use the same queries and so forth in that world, too; we'd just be changing where the list of classes comes from.

I would encourage us to think of the static array in this PR as a placeholder.

smklein · 2026-05-01T19:18:31Z

Then, that would buy us a bit of time to think about the allowlist/filter design a bit more. My initial thought on that is that, rather than having a static array of ereport types that we know how to handle, each diagnosis engine present in the system should provide the set of ereports that it knows how to consume. We'd take these sets and union them together (on startup I guess) to create the full allowlist. That places the burden of proof of "no unhandled ereports" locally with the thing that handles them, rather than being something off to the side that we'd have to remember to update. What do you think?

So, I'm down with this direction - and I've moved the set of "known ereports" into nexus-fm -- but I kinda want to punt on something this clever until we actually have a few ereports that we've started processing. (Don't want to generalize too early here).

I've used https://docs.rs/linkme/latest/linkme/ before to achieve something similar

I would encourage us to think of the static array in this PR as a placeholder.

Agreed; though I think it's probably fine to use it for N < ~5

smklein · 2026-05-01T19:49:42Z

Also, for completeness: this can't be cherry-picked because it contains a schema change.

#10360 is the preferred (temporary) solution, which I'll revert as a part of getting this PR (#10349) merged.

smklein added 3 commits April 30, 2026 13:39

[fm] Filter ereports during preparation by 'known-to-Nexus'

042e3ee

fmt

7f38b1c

fix broken intra-doc link in known_ereport_classes

008623c

smklein marked this pull request as ready for review April 30, 2026 22:02

smklein requested review from hawkw and mergeconflict and removed request for hawkw April 30, 2026 22:02

hawkw reviewed Apr 30, 2026

View reviewed changes

smklein added 11 commits April 30, 2026 16:08

omdb counts with named struct

8dd8218

clarify source-of-truth for ereport knowledge in omdb

1904067

column order, less string cloning

43af897

explicit enum for KnownToOmdb

08ab0c8

less 'handler', more 'consumption by DE'

bea3edd

s/planner/FM analysis

1d50b6e

fewer doc comments

d85d45f

aasimovication

42e6428

partial index check to test utils

94aa4d6

show known classes in background task output

5f74c4d

fmt because of course

31e0da6

hawkw reviewed May 1, 2026

View reviewed changes

smklein added 4 commits May 1, 2026 09:45

fix omdb output

05543ed

text changes

5c5d500

Add lockstep API

c5c53c2

updating docs

ce60f4a

smklein mentioned this pull request May 1, 2026

[fm] Temporarily disable fm_analysis background task #10358

Closed

mergeconflict approved these changes May 1, 2026

View reviewed changes

		// The variable-length `class` column goes last so that wrapping on a
		// narrow terminal doesn't disrupt the fixed-width numeric columns.

		"Note: KNOWN-TO-OMDB reflects which classes have a diagnosis engine \
		in THIS omdb\nbinary; the currently-deployed Nexus may differ if it \

		"ereport.positronic-brain.example",
		"ereport.spinning-blades.example",

Conversation

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

Uh oh!

Uh oh!

hawkw commented Apr 30, 2026

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergeconflict commented May 1, 2026

Uh oh!

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

Uh oh!

Uh oh!

smklein commented May 1, 2026

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smklein commented Apr 30, 2026 •

edited

Loading

hawkw commented May 1, 2026 •

edited

Loading

smklein commented May 1, 2026 •

edited

Loading