Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nexus/db-model/src/fm/case.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ impl CaseMetadata {
de,
comment,
alerts_requested: _,
support_bundles_requested: _,
ereports: _,
} = case;
Self {
Expand Down
4 changes: 4 additions & 0 deletions nexus/db-queries/src/db/datastore/fm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ impl DataStore {
comment,
ereports,
alerts_requested,
support_bundles_requested: iddqd::IdOrdMap::new(),
}
}));
}
Expand Down Expand Up @@ -1552,6 +1553,7 @@ mod tests {
de,
ereports,
alerts_requested,
support_bundles_requested: _,
} = case;
let case_id = id;
let Some(expected) = this.cases.get(&case_id) else {
Expand Down Expand Up @@ -1770,6 +1772,7 @@ mod tests {
de: fm::DiagnosisEngineKind::PowerShelf,
ereports,
alerts_requested,
support_bundles_requested: iddqd::IdOrdMap::new(),
comment: "my cool case".to_string(),
}
};
Expand Down Expand Up @@ -1802,6 +1805,7 @@ mod tests {
de: fm::DiagnosisEngineKind::PowerShelf,
ereports,
alerts_requested,
support_bundles_requested: iddqd::IdOrdMap::new(),
comment: "break in case of emergency".to_string(),
}
};
Expand Down
1 change: 1 addition & 0 deletions nexus/fm/src/builder/case.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ impl AllCases {
comment: String::new(),
ereports: Default::default(),
alerts_requested: Default::default(),
support_bundles_requested: Default::default(),
};
entry.insert(CaseBuilder::new(
&self.log, sitrep_id, case, case_rng,
Expand Down
2 changes: 2 additions & 0 deletions nexus/src/app/background/tasks/fm_rendezvous.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ mod tests {
de: fm::DiagnosisEngineKind::PowerShelf,
alerts_requested: iddqd::IdOrdMap::new(),
ereports: iddqd::IdOrdMap::new(),
support_bundles_requested: iddqd::IdOrdMap::new(),
comment: "my great case".to_string(),
};
case1
Expand Down Expand Up @@ -305,6 +306,7 @@ mod tests {
de: fm::DiagnosisEngineKind::PowerShelf,
alerts_requested: iddqd::IdOrdMap::new(),
ereports: iddqd::IdOrdMap::new(),
support_bundles_requested: iddqd::IdOrdMap::new(),
comment: "my other great case".to_string(),
};
case2
Expand Down
104 changes: 103 additions & 1 deletion nexus/types/src/fm/case.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
use crate::alert::AlertClass;
use crate::fm::DiagnosisEngineKind;
use crate::fm::Ereport;
use crate::support_bundle::BundleDataSelection;
use iddqd::{IdOrdItem, IdOrdMap};
use omicron_uuid_kinds::{AlertUuid, CaseEreportUuid, CaseUuid, SitrepUuid};
use omicron_uuid_kinds::{
AlertUuid, CaseEreportUuid, CaseUuid, SitrepUuid, SupportBundleUuid,
};
use serde::{Deserialize, Serialize};
use std::fmt;
use std::sync::Arc;
Expand All @@ -21,6 +24,7 @@ pub struct Case {

pub ereports: IdOrdMap<CaseEreport>,
pub alerts_requested: IdOrdMap<AlertRequest>,
pub support_bundles_requested: IdOrdMap<SupportBundleRequest>,

pub comment: String,
}
Expand Down Expand Up @@ -88,6 +92,22 @@ impl iddqd::IdOrdItem for AlertRequest {
iddqd::id_upcast!();
}

#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
pub struct SupportBundleRequest {
pub id: SupportBundleUuid,
pub requested_sitrep_id: SitrepUuid,
pub data_selection: Option<BundleDataSelection>,
}

impl iddqd::IdOrdItem for SupportBundleRequest {
type Key<'a> = &'a SupportBundleUuid;
fn key(&self) -> Self::Key<'_> {
&self.id
}

iddqd::id_upcast!();
}

struct DisplayCase<'a> {
case: &'a Case,
indent: usize,
Expand Down Expand Up @@ -120,6 +140,7 @@ impl fmt::Display for DisplayCase<'_> {
ereports,
comment,
alerts_requested,
support_bundles_requested,
},
indent,
sitrep_id,
Expand Down Expand Up @@ -234,6 +255,47 @@ impl fmt::Display for DisplayCase<'_> {
}
}

if !support_bundles_requested.is_empty() {
writeln!(f, "\n{:>indent$}support bundles requested:", "")?;
writeln!(f, "{:>indent$}-------------------------", "")?;

let indent = indent + 2;
for SupportBundleRequest {
id,
requested_sitrep_id,
data_selection,
} in support_bundles_requested.iter()
{
const REQUESTED_IN: &str = "requested in:";
const DATA: &str = "data:";
const WIDTH: usize = const_max_len(&[REQUESTED_IN, DATA]);

writeln!(f, "{BULLET:>indent$}bundle {id}",)?;
writeln!(
f,
"{:>indent$}{REQUESTED_IN:<WIDTH$} {requested_sitrep_id}{}",
"",
this_sitrep(*requested_sitrep_id)
)?;
match data_selection {
None => {
writeln!(
f,
"{:>indent$}{DATA:<WIDTH$} all (default)\n",
"",
)?;
}
Some(selection) => {
writeln!(
f,
"{:>indent$}{DATA:<WIDTH$} {selection}\n",
"",
)?;
Comment on lines +289 to +293
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}
}
}

writeln!(f)?;

Ok(())
Expand All @@ -248,6 +310,7 @@ mod tests {
use ereport_types::{Ena, EreportId};
use omicron_uuid_kinds::{
AlertUuid, CaseUuid, EreporterRestartUuid, OmicronZoneUuid, SitrepUuid,
SupportBundleUuid,
};
use std::str::FromStr;
use std::sync::Arc;
Expand Down Expand Up @@ -276,6 +339,12 @@ mod tests {
let alert2_id =
AlertUuid::from_str("8a6f88ef-c436-44a9-b4cb-cae91d7306c9")
.unwrap();
let bundle1_id =
SupportBundleUuid::from_str("d1a2b3c4-e5f6-7890-abcd-ef1234567890")
.unwrap();
let bundle2_id =
SupportBundleUuid::from_str("a9b8c7d6-e5f4-3210-fedc-ba0987654321")
.unwrap();

// Create some ereports
let mut ereports = IdOrdMap::new();
Expand Down Expand Up @@ -349,6 +418,38 @@ mod tests {
})
.unwrap();

use crate::support_bundle::{BundleData, BundleDataSelection};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let mut bundle1_data = BundleDataSelection::new();
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()
},
));
Comment on lines +423 to +435
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


let mut support_bundles_requested = IdOrdMap::new();
support_bundles_requested
.insert_unique(SupportBundleRequest {
id: bundle1_id,
requested_sitrep_id: created_sitrep_id,
data_selection: Some(bundle1_data),
})
.unwrap();
support_bundles_requested
.insert_unique(SupportBundleRequest {
id: bundle2_id,
requested_sitrep_id: closed_sitrep_id,
data_selection: None,
})
.unwrap();

// Create the case
let case = Case {
id: case_id,
Expand All @@ -357,6 +458,7 @@ mod tests {
de: DiagnosisEngineKind::PowerShelf,
ereports,
alerts_requested,
support_bundles_requested,
comment: "Power shelf rectifier added and removed here :-)"
.to_string(),
};
Expand Down
88 changes: 87 additions & 1 deletion nexus/types/src/fm/ereport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ fn get_sp_metadata_string(
}

/// A set of filters for fetching ereports.
#[derive(Clone, Debug, Default, Eq, PartialEq)]
#[derive(Clone, Debug, Default, Eq, PartialEq, Serialize, Deserialize)]
pub struct EreportFilters {
/// If present, include only ereports that were collected at the specified
/// timestamp or later.
Expand All @@ -240,6 +240,55 @@ pub struct EreportFilters {
pub only_classes: Vec<String>,
}

impl fmt::Display for EreportFilters {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use itertools::Itertools;

// Writes a semicolon-separated part to the formatter, tracking whether
// we've written anything yet.
let mut empty = true;
let mut fmt_part =
|f: &mut fmt::Formatter, args: fmt::Arguments| -> fmt::Result {
if !empty {
write!(f, "; ")?;
}
empty = false;
f.write_fmt(args)
};

if let Some(start) = &self.start_time {
fmt_part(f, format_args!("start: {start}"))?;
}
if let Some(end) = &self.end_time {
fmt_part(f, format_args!("end: {end}"))?;
}
if !self.only_serials.is_empty() {
fmt_part(
f,
format_args!(
"serials: {}",
self.only_serials.iter().format(", ")
),
)?;
}
if !self.only_classes.is_empty() {
fmt_part(
f,
format_args!(
"classes: {}",
self.only_classes.iter().format(", ")
),
)?;
}

// If no filters are set, display "none" rather than empty output.
if empty {
write!(f, "none")?;
}
Ok(())
}
}

impl EreportFilters {
pub fn check_time_range(&self) -> Result<(), Error> {
if let (Some(start), Some(end)) = (self.start_time, self.end_time) {
Expand All @@ -253,3 +302,40 @@ impl EreportFilters {
Ok(())
}
}

#[cfg(test)]
pub(crate) mod test_utils {
use super::*;
use proptest::prelude::*;

fn arb_datetime() -> impl Strategy<Value = DateTime<Utc>> {
// Generate timestamps in a reasonable range (2020-2030).
(1577836800i64..1893456000i64)
.prop_map(|secs| DateTime::from_timestamp(secs, 0).unwrap())
}

impl Arbitrary for EreportFilters {
type Parameters = ();
type Strategy = BoxedStrategy<Self>;

fn arbitrary_with(_: Self::Parameters) -> Self::Strategy {
(
prop::option::of(arb_datetime()),
prop::option::of(arb_datetime()),
prop::collection::vec(".*", 0..=3),
prop::collection::vec(".*", 0..=3),
)
.prop_map(
|(start_time, end_time, only_serials, only_classes)| {
EreportFilters {
start_time,
end_time,
only_serials,
only_classes,
}
},
)
.boxed()
}
}
}
Loading
Loading