nexus-types: add SupportBundleRequest type and Case field by mergeconflict · Pull Request #10090 · oxidecomputer/omicron

mergeconflict · 2026-03-18T22:16:22Z

The core thing this PR does is:

Introduce a new SupportBundleRequest type to fm::case (analogous to @hawkw's AlertRequest), and
Introduce a new support_bundles_requested field to Case (analogous to alerts_requested).

There's a lot of ✨ceremony✨ around this small change, though:

DisplayCase has a sophisticated impl Display that formats the stuff it contains. So we need Display impls for all the things SupportBundleRequest transitively contains (BundleData, etc.). Here's what this comes out looking like:

support bundles requested:
-------------------------
* bundle a9b8c7d6-e5f4-3210-fedc-ba0987654321
  requested in: bba63ba7-bf6b-45f4-b241-d13ccd07fe1c <-- this sitrep
  data:         all (default)

* bundle d1a2b3c4-e5f6-7890-abcd-ef1234567890
  requested in: ea7affb0-36eb-4a9a-b9bd-22e00f1bcc04
  data:         host_info(all), sp_dumps, reconfigurator, ereports(classes: hw.pwr.*)

Since Case is serde::Serialize/Deserialize, we also need to add derives to SupportBundleRequest and all the things it transitively contains.
Lastly, since we want to have some sort of reasonable round-trip test for the serialization / deserialization, we use proptest to generate arbitrary inputs. This requires Arbitrary impls (or functions that return Strategy) for all the things too.

jgallagher · 2026-03-19T02:09:52Z

nexus/types/src/fm/ereport.rs

    pub only_classes: Vec<String>,
 }

+impl fmt::Display for EreportFilters {


With apologies for the drive-by style nit on a draft PR: I'd discourage implementing fmt::Display for types like this, based primarily on this discussion in the std lib docs, particularly:

Because a type can only have one Display implementation, it is often preferable to only implement Display when there is a single most “obvious” way that values can be formatted as text.

This has gotten us into some trouble with other types in the past where "type implements Display, so I can easily convert it to a String, so now I can store a String when I should've been storing a stronger type". That doesn't seem particularly likely in this case, but I'd still recommend moving this to a more explicit method or type. Over in Reconfigurator land we have a bunch of helper types that themselves implement Display, but that's their only purpose; e.g.,

omicron/nexus/types/src/deployment.rs

Lines 518 to 522 in afe86cb

/// Return a struct that can be displayed to present information about the

/// blueprint.

pub fn display(&self) -> BlueprintDisplay<'_> {

BlueprintDisplay { blueprint: self }

}

No worries for the drive-by, thanks for looking! I've moved it out of draft now, and I'll figure out how to address your comment tomorrow.

Yeah, you may notice a similar pattern with the Case pretty-printer actually being a CaseDisplay displayer type --- we also use that to pass through the requested indentation level.

I will note that the display implementation for FM types here is largely in service of OMDB (and perhaps in future other human-facing dev tools), and I will be quite sad if any of us end up trying to round-trip this stuff through a String and back again. But, I think the displayer thingy is generally a good idea, especially if we want to turn this into a bulleted list or something and need to also pass through an indentation or similar.

…ield Add the SupportBundleRequest type to fm::case and the support_bundles_requested field to Case. Add Display impls for BundleData, BundleDataSelection, SledSelection, and EreportFilters so that case formatting can show the full data selection for each requested support bundle. Add Serialize/Deserialize derives to the support bundle selection types (BundleDataSelection, BundleData, BundleDataCategory, SledSelection, EreportFilters) since SupportBundleRequest contains Option<BundleDataSelection>. Add proptest Arbitrary impls for BundleDataSelection and EreportFilters (in test_utils modules) and a proptest-based serde round-trip test for BundleDataSelection. Add test_case_display coverage for support bundle requests, exercising both data_selection: None and Some with parameterized categories.

hawkw · 2026-03-19T02:53:12Z

I think that either this branch or mergeconflict/fm-sb-type-relocation (?) needs to be rebased/merge main, as it looks like there are a lot of unrelated changes in the diff here.

hawkw · 2026-03-19T03:02:05Z

nexus/types/src/fm/case.rs

+                        writeln!(
+                            f,
+                            "{:>indent$}{DATA:<WIDTH$} {selection}\n",
+                            "",
+                        )?;


how long do you think this list is liable to be? i always try my best to make my omdb command output 80-column-terminal friendly, because i am spiritually very old or something. i won't hold you to this particular bit of mental illness unless you find it entertaining, but if this is liable to be quite long, we might want to bite the bullet and turn it into a multi-line bulleted list or something?

hawkw · 2026-03-19T03:02:42Z

nexus/types/src/fm/case.rs

            })
            .unwrap();

+        use crate::support_bundle::{BundleData, BundleDataSelection};


considered unimportant, but personally i find having the imports way down here a bit weird...

hawkw · 2026-03-19T03:04:03Z

nexus/types/src/fm/case.rs

+        bundle1_data.insert(BundleData::Reconfigurator);
+        bundle1_data.insert(BundleData::SpDumps);
+        bundle1_data.insert(BundleData::HostInfo(
+            std::collections::HashSet::from([
+                crate::support_bundle::SledSelection::All,
+            ]),
+        ));
+        bundle1_data.insert(BundleData::Ereports(
+            crate::fm::ereport::EreportFilters {
+                only_classes: vec!["hw.pwr.*".to_string()],
+                ..Default::default()
+            },
+        ));


hmm --- not for this PR, necessarily, but I wonder if, as we start actually making these in more places, we ought to add a builder-type API to BundleDataSelection to construct complicated ones with somewhat less noise?

hawkw · 2026-03-19T03:07:27Z

nexus/types/src/fm/ereport.rs

    pub only_classes: Vec<String>,
 }

+impl fmt::Display for EreportFilters {


Yeah, you may notice a similar pattern with the Case pretty-printer actually being a CaseDisplay displayer type --- we also use that to pass through the requested indentation level.

I will note that the display implementation for FM types here is largely in service of OMDB (and perhaps in future other human-facing dev tools), and I will be quite sad if any of us end up trying to round-trip this stuff through a String and back again. But, I think the displayer thingy is generally a good idea, especially if we want to turn this into a bulleted list or something and need to also pass through an indentation or similar.

hawkw · 2026-03-19T03:08:42Z

nexus/types/src/support_bundle.rs

+        match self {
+            Self::Reconfigurator => write!(f, "reconfigurator"),
+            Self::HostInfo(sleds) => {
+                write!(f, "host_info({})", sleds.iter().format(", "))


does sleds.iter().format(", ") allocate a whole String? personally i don't really like when Display impls allocate but maybe this is overly persnickety of me.

mergeconflict marked this pull request as draft March 18, 2026 22:21

mergeconflict changed the base branch from main to mergeconflict/fm-sb-type-relocation March 18, 2026 22:21

mergeconflict force-pushed the mergeconflict/fm-sb-type-relocation branch from d3b67af to d5d60b3 Compare March 18, 2026 22:48

mergeconflict force-pushed the mergeconflict/fm-sb-types branch 2 times, most recently from 2baa491 to c2191f0 Compare March 18, 2026 22:58

mergeconflict force-pushed the mergeconflict/fm-sb-type-relocation branch 2 times, most recently from 08b5f84 to d87449e Compare March 18, 2026 23:14

mergeconflict force-pushed the mergeconflict/fm-sb-types branch from c2191f0 to e725e86 Compare March 18, 2026 23:14

mergeconflict mentioned this pull request Mar 18, 2026

Move bundle selection types and EreportFilters to nexus-types #10089

Open

mergeconflict force-pushed the mergeconflict/fm-sb-type-relocation branch from d87449e to 7a5224c Compare March 19, 2026 00:00

mergeconflict force-pushed the mergeconflict/fm-sb-types branch from e725e86 to beef2af Compare March 19, 2026 02:03

jgallagher reviewed Mar 19, 2026

View reviewed changes

mergeconflict requested review from hawkw and smklein March 19, 2026 02:20

mergeconflict marked this pull request as ready for review March 19, 2026 02:22

mergeconflict requested a review from jgallagher March 19, 2026 02:23

mergeconflict force-pushed the mergeconflict/fm-sb-types branch from beef2af to 1f1c514 Compare March 19, 2026 02:29

mergeconflict force-pushed the mergeconflict/fm-sb-types branch from 1f1c514 to ad04192 Compare March 19, 2026 02:51

mergeconflict force-pushed the mergeconflict/fm-sb-type-relocation branch from 7a5224c to 3a6ea71 Compare March 19, 2026 02:51

hawkw reviewed Mar 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nexus-types: add SupportBundleRequest type and Case field#10090

nexus-types: add SupportBundleRequest type and Case field#10090
mergeconflict wants to merge 1 commit intomergeconflict/fm-sb-type-relocationfrom
mergeconflict/fm-sb-types

mergeconflict commented Mar 18, 2026 •

edited

Loading

Uh oh!

jgallagher Mar 19, 2026

Uh oh!

mergeconflict Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

hawkw commented Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

hawkw Mar 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// Return a struct that can be displayed to present information about the
	/// blueprint.
	pub fn display(&self) -> BlueprintDisplay<'_> {
	BlueprintDisplay { blueprint: self }
	}

Conversation

mergeconflict commented Mar 18, 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!

hawkw commented Mar 19, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mergeconflict commented Mar 18, 2026 •

edited

Loading