Skip to content

Commit 4e138cb

Browse files
hawkwhawkw
committed
[nexus] refactor SP ereport processing into nexus-types
This commit moves the code that processes raw JSON representations of SP ereports from the `ereport_ingester` background task to `nexus_types::fm::ereport`. While a production system will only perform this processing when an ereport is received from a service processor via MGS in the `ereprot_ingester` task, I'd like to be able to also invoke the same logic with hard-coded example JSON ereports for test purposes. Currently, the test code for diagnosis engines in #9346 implemented its own version of this which was slightly different from the ereport_ingester version, so I've moved it to here so that we can use the same logic when processing test inputs.
1 parent 60f4124 commit 4e138cb

File tree

2 files changed

+128
-100
lines changed

2 files changed

+128
-100
lines changed

‎nexus/src/app/background/tasks/ereport_ingester.rs‎

Lines changed: 7 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
use crate::app::background::BackgroundTask;
1313
use chrono::Utc;
1414
use ereport_types::Ena;
15-
use ereport_types::Ereport;
1615
use ereport_types::EreportId;
1716
use futures::future::BoxFuture;
1817
use internal_dns_types::names::ServiceName;
@@ -245,76 +244,19 @@ impl Ingester {
245244
} else {
246245
status.get_or_insert_default().requests += 1;
247246
}
248-
247+
let time_collected = Utc::now();
249248
let received = reports.items.len();
250249
let status = status.get_or_insert_default();
251250
status.ereports_received += received;
252251

253252
let db_ereports = reports.items.into_iter().map(|ereport| {
254-
const MISSING_VPD: &str =
255-
" (perhaps the SP doesn't know its own VPD?)";
256-
let part_number = get_sp_metadata_string(
257-
"baseboard_part_number",
258-
&ereport,
259-
&restart_id,
253+
EreportData::from_sp_ereport(
260254
&opctx.log,
261-
MISSING_VPD,
262-
);
263-
let serial_number = get_sp_metadata_string(
264-
"baseboard_serial_number",
265-
&ereport,
266-
&restart_id,
267-
&opctx.log,
268-
MISSING_VPD,
269-
);
270-
let ena = ereport.ena;
271-
let class = ereport
272-
.data
273-
// "k" (for "kind") is used as an abbreviation of
274-
// "class" to save 4 bytes of ereport.
275-
.get("k")
276-
.or_else(|| ereport.data.get("class"));
277-
let class = match (class, ereport.data.get("lost")) {
278-
(Some(serde_json::Value::String(class)), _) => {
279-
Some(class.to_string())
280-
}
281-
(Some(v), _) => {
282-
slog::warn!(
283-
&opctx.log,
284-
"malformed ereport: value for 'k'/'class' \
285-
should be a string, but found: {v:?}";
286-
"ena" => ?ena,
287-
"restart_id" => ?restart_id,
288-
);
289-
None
290-
}
291-
// This is a loss record! I know this!
292-
(None, Some(serde_json::Value::Null)) => {
293-
Some("ereport.data_loss.possible".to_string())
294-
}
295-
(None, Some(serde_json::Value::Number(_))) => {
296-
Some("ereport.data_loss.certain".to_string())
297-
}
298-
(None, _) => {
299-
slog::warn!(
300-
&opctx.log,
301-
"ereport missing 'k'/'class' key";
302-
"ena" => ?ena,
303-
"restart_id" => ?restart_id,
304-
);
305-
None
306-
}
307-
};
308-
309-
EreportData {
310-
id: EreportId { restart_id, ena },
311-
time_collected: Utc::now(),
312-
collector_id: self.nexus_id,
313-
part_number,
314-
serial_number,
315-
class,
316-
report: serde_json::Value::Object(ereport.data),
317-
}
255+
restart_id,
256+
ereport,
257+
time_collected,
258+
self.nexus_id,
259+
)
318260
});
319261
let created = match self
320262
.datastore
@@ -427,41 +369,6 @@ impl Ingester {
427369
}
428370
}
429371

430-
/// Attempt to extract a VPD metadata from an SP ereport, logging a warning if
431-
/// it's missing. We still want to keep such ereports, as the error condition
432-
/// could be that the SP couldn't determine the metadata field, but it's
433-
/// uncomfortable, so we ought to complain about it.
434-
fn get_sp_metadata_string(
435-
key: &str,
436-
ereport: &Ereport,
437-
restart_id: &EreporterRestartUuid,
438-
log: &slog::Logger,
439-
extra_context: &'static str,
440-
) -> Option<String> {
441-
match ereport.data.get(key) {
442-
Some(serde_json::Value::String(s)) => Some(s.clone()),
443-
Some(v) => {
444-
slog::warn!(
445-
&log,
446-
"malformed ereport: value for '{key}' should be a string, \
447-
but found: {v:?}";
448-
"ena" => ?ereport.ena,
449-
"restart_id" => ?restart_id,
450-
);
451-
None
452-
}
453-
None => {
454-
slog::warn!(
455-
&log,
456-
"ereport missing '{key}'{extra_context}";
457-
"ena" => ?ereport.ena,
458-
"restart_id" => ?restart_id,
459-
);
460-
None
461-
}
462-
}
463-
}
464-
465372
struct EreportQueryParams {
466373
committed_ena: Option<Ena>,
467374
start_ena: Option<Ena>,

‎nexus/types/src/fm/ereport.rs‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
77
use crate::inventory::SpType;
88
use chrono::{DateTime, Utc};
9+
use omicron_uuid_kinds::EreporterRestartUuid;
910
use omicron_uuid_kinds::OmicronZoneUuid;
1011
use omicron_uuid_kinds::SledUuid;
1112
use serde::Deserialize;
@@ -57,6 +58,91 @@ pub struct EreportData {
5758
pub report: serde_json::Value,
5859
}
5960

61+
impl EreportData {
62+
/// Interpret a service processor ereport from a raw JSON blobule, plus the
63+
/// restart ID and collection metadata.
64+
///
65+
/// This conversion is lossy; if some information is not present in the raw
66+
/// ereport JSON, such as the SP's VPD identity, we log a warning, rather
67+
/// than returning an error, and leave those fields empty in the returned
68+
/// `EreportData`. This is because, if we receive an ereport that is
69+
/// incomplete, we would still like to preserve whatever information *is*
70+
/// present, rather than throwing the whole thing away. Thus, this function
71+
/// also takes a `slog::Logger` for logging warnings if some expected fields
72+
/// are not present or malformed.
73+
pub fn from_sp_ereport(
74+
log: &slog::Logger,
75+
restart_id: EreporterRestartUuid,
76+
ereport: ereport_types::Ereport,
77+
time_collected: DateTime<Utc>,
78+
collector_id: OmicronZoneUuid,
79+
) -> Self {
80+
const MISSING_VPD: &str = " (perhaps the SP doesn't know its own VPD?)";
81+
let part_number = get_sp_metadata_string(
82+
"baseboard_part_number",
83+
&ereport,
84+
&restart_id,
85+
&log,
86+
MISSING_VPD,
87+
);
88+
let serial_number = get_sp_metadata_string(
89+
"baseboard_serial_number",
90+
&ereport,
91+
&restart_id,
92+
&log,
93+
MISSING_VPD,
94+
);
95+
let ena = ereport.ena;
96+
let class = ereport
97+
.data
98+
// "k" (for "kind") is used as an abbreviation of
99+
// "class" to save 4 bytes of ereport.
100+
.get("k")
101+
.or_else(|| ereport.data.get("class"));
102+
let class = match (class, ereport.data.get("lost")) {
103+
(Some(serde_json::Value::String(class)), _) => {
104+
Some(class.to_string())
105+
}
106+
(Some(v), _) => {
107+
slog::warn!(
108+
&log,
109+
"malformed ereport: value for 'k'/'class' \
110+
should be a string, but found: {v:?}";
111+
"ena" => ?ena,
112+
"restart_id" => ?restart_id,
113+
);
114+
None
115+
}
116+
// This is a loss record! I know this!
117+
(None, Some(serde_json::Value::Null)) => {
118+
Some("ereport.data_loss.possible".to_string())
119+
}
120+
(None, Some(serde_json::Value::Number(_))) => {
121+
Some("ereport.data_loss.certain".to_string())
122+
}
123+
(None, _) => {
124+
slog::warn!(
125+
&log,
126+
"ereport missing 'k'/'class' key";
127+
"ena" => ?ena,
128+
"restart_id" => ?restart_id,
129+
);
130+
None
131+
}
132+
};
133+
134+
EreportData {
135+
id: EreportId { restart_id, ena },
136+
time_collected,
137+
collector_id,
138+
part_number,
139+
serial_number,
140+
class,
141+
report: serde_json::Value::Object(ereport.data),
142+
}
143+
}
144+
}
145+
60146
/// Describes the source of an ereport.
61147
#[derive(
62148
Copy,
@@ -93,3 +179,38 @@ impl fmt::Display for Reporter {
93179
}
94180
}
95181
}
182+
183+
/// Attempt to extract a VPD metadata from an SP ereport, logging a warning if
184+
/// it's missing. We still want to keep such ereports, as the error condition
185+
/// could be that the SP couldn't determine the metadata field, but it's
186+
/// uncomfortable, so we ought to complain about it.
187+
fn get_sp_metadata_string(
188+
key: &str,
189+
ereport: &ereport_types::Ereport,
190+
restart_id: &EreporterRestartUuid,
191+
log: &slog::Logger,
192+
extra_context: &'static str,
193+
) -> Option<String> {
194+
match ereport.data.get(key) {
195+
Some(serde_json::Value::String(s)) => Some(s.clone()),
196+
Some(v) => {
197+
slog::warn!(
198+
&log,
199+
"malformed ereport: value for '{key}' should be a string, \
200+
but found: {v:?}";
201+
"ena" => ?ereport.ena,
202+
"restart_id" => ?restart_id,
203+
);
204+
None
205+
}
206+
None => {
207+
slog::warn!(
208+
&log,
209+
"ereport missing '{key}'{extra_context}";
210+
"ena" => ?ereport.ena,
211+
"restart_id" => ?restart_id,
212+
);
213+
None
214+
}
215+
}
216+
}

0 commit comments

Comments
 (0)