[nexus] Add part to service bundle ereport paths (#8767)

In #8739, I added code for collecting ereports into support bundles
which stores the ereport JSON in directories for each sled/switch/PSC
serial number from which an ereport was received. Unfortunately, I
failed failed to consider that the version 1 Oxide serial numbers are
only unique within the namespace of a particular part, and not globally
--- so (for example) a switch and a compute sled may have colliding
serials. This means that the current code could incorrectly group
ereports reported by two totally different devices. While the ereport
JSON files _do_ contain additional information that disambiguates this
(it includes includes the part number, as well as MGS metadata with the
SP type for SP ereports), and restart IDs are additionally capable of
distinguishing between reporters, putting ereports from two different
systems within the same directory still has the potential to be quite
misleading.

Thus, this branch changes the paths for ereports to include the part
number as well as the serial number, in the format:

```
{part_number}-{serial_number}/{restart_id}/{ENA}.json
```

In order to include part numbers for host OS ereports, I decided to add
a part number column to the `host_ereport` table as well. Initially, I
had opted not to do this, as I was thinking that, since `host_ereport`
includes a sled UUID, we could just join with the `sled` table to get
the part number. However, it occurred to me that ereports may be
received from a sled that's later expunged from the rack, and the `sled`
record for the sled may eventually be deleted, so such a join would
fail. We might retain such ereports past the lifetime of the sled in the
rack. So, I thought it was better to always include the part number in
the ereport record.

I've added a migration that attempts to backfill the
`host_ereport.part_number` column from the `sled` table for existing
host OS ereport records. In practice, this won't do anything, since
we're not collecting them yet,but it seemed nice to have. Sadly, the
column had to be left nullable, since we may theoretically encounter an
ereport with a sled UUID that points to an already-deleted sled record,
but...whatever. Since there aren't currently any host OS ereport records
anyway, this shouldn't happen, and we'll just handle the nullability;
this isn't terrible as we must already do so for SP ereport records.

Fixes #8765

Author: hawkw

nexus/db-model/src/ereport.rs

@@ -117,14 +117,15 @@ impl From<HostEreport> for Ereport {
             sled_id,
             class,
             report,
+            part_number,
         } = host_report;
         Ereport {
             id: EreportId { restart_id: restart_id.into(), ena: ena.into() },
             metadata: EreportMetadata {
                 time_collected,
                 time_deleted,
                 collector_id: collector_id.into(),
-                part_number: None, // TODO
+                part_number,
                 serial_number: Some(sled_serial),
                 class,
             },
@@ -253,4 +254,10 @@ pub struct HostEreport {
     pub class: Option<String>,
 
     pub report: serde_json::Value,
+
+    // It's a shame this has to be nullable, while the serial is not. However,
+    // this field was added in a migration, and we have to be able to handle the
+    // case where a sled record was hard-deleted when backfilling the ereport
+    // table's part_number column. Sad.
+    pub part_number: Option<String>,
 }

nexus/db-model/src/schema_versions.rs

@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: Version = Version::new(176, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(177, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(177, "add-host-ereport-part-number"),
         KnownVersion::new(176, "audit-log"),
         KnownVersion::new(175, "inv-host-phase-1-active-slot"),
         KnownVersion::new(174, "add-tuf-rot-by-sign"),

nexus/db-schema/src/schema.rs

@@ -2664,6 +2664,8 @@ table! {
         class -> Nullable<Text>,
 
         report -> Jsonb,
+
+        part_number -> Nullable<Text>,
     }
 }
 

nexus/src/app/background/tasks/support_bundle_collector.rs

@@ -1081,32 +1081,41 @@ impl BackgroundTask for SupportBundleCollector {
 }
 
 async fn write_ereport(ereport: Ereport, dir: &Utf8Path) -> anyhow::Result<()> {
-    // Here's where we construct the file path for each ereport JSON file, given
-    // the top-lebel ereport directory path.  Each ereport is stored in a
-    // subdirectory for the serial number of the system that produced the
-    // ereport.  These paths take the following form:
+    // Here's where we construct the file path for each ereport JSON file,
+    // given the top-level ereport directory path.  Each ereport is stored in a
+    // subdirectory for the part and serial numbers of the system that produced
+    // the ereport.  Part numbers must be included in addition to serial
+    // numbers, as the v1 serial scheme only guarantees uniqueness within a
+    // part number.  These paths take the following form:
     //
-    //    {serial_number}/{restart_id}/{ENA}.json
+    //   {part-number}-{serial_number}/{restart_id}/{ENA}.json
     //
-    // We can assume that the restart ID and serial number consist only of
+    // We can assume that the restart ID and ENA consist only of
     // filesystem-safe characters, as the restart ID is known to be a UUID, and
-    // the ENA is just an integer.  For the serial number, which we don't have
-    // full control over --- it came from the ereport metadata --- we must
-    // check that it doesn't contain any characters unsuitable for use in a
-    // filesystem path.
-    let sn = &ereport
+    // the ENA is just an integer.  For the serial and part numbers, which
+    // Nexus doesn't have full control over --- it came from the ereport
+    // metadata --- we must check that it doesn't contain any characters
+    // unsuitable for use in a filesystem path.
+    let pn = ereport
+        .metadata
+        .part_number
+        .as_deref()
+        // If the part or serial numbers contain any unsavoury characters, it
+        // goes in the `unknown_serial` hole! Note that the alleged serial
+        // number from the ereport will still be present in the JSON as a
+        // string, so we're not *lying* about what was received; we're just
+        // giving up on using it in the path.
+        .filter(|&s| is_fs_safe_single_path_component(s))
+        .unwrap_or("unknown_part");
+    let sn = ereport
         .metadata
         .serial_number
         .as_deref()
-        // If the serial number contains any unsavoury characters, it goes in
-        // the `unknown_serial` hole!  Note that the alleged serial number from
-        // the ereport will still be present in the JSON as a string, so we're
-        // not *lying* about what was received; we're just giving up on using it
-        // in the path.
         .filter(|&s| is_fs_safe_single_path_component(s))
         .unwrap_or("unknown_serial");
+
     let dir = dir
-        .join(sn)
+        .join(format!("{pn}-{sn}"))
         // N.B. that we call `into_untyped_uuid()` here, as the `Display`
         // implementation for a typed UUID appends " (ereporter_restart)", which
         // we don't want.
@@ -1116,11 +1125,11 @@ async fn write_ereport(ereport: Ereport, dir: &Utf8Path) -> anyhow::Result<()> {
         .with_context(|| format!("failed to create directory '{dir}'"))?;
     let file_path = dir.join(format!("{}.json", ereport.id.ena));
     let json = serde_json::to_vec(&ereport).with_context(|| {
-        format!("failed to serialize ereport '{sn}:{}'", ereport.id)
+        format!("failed to serialize ereport {pn}:{sn}/{}", ereport.id)
     })?;
     tokio::fs::write(&file_path, json)
         .await
-        .with_context(|| format!("failed to write {file_path}'"))
+        .with_context(|| format!("failed to write '{file_path}'"))
 }
 
 // Takes a directory "dir", and zips the contents into a single zipfile.
@@ -1701,6 +1710,7 @@ mod test {
                         collector_id: OmicronZoneUuid::new_v4().into(),
                         sled_id: sled_id.into(),
                         sled_serial: HOST_SERIAL.to_string(),
+                        part_number: Some(GIMLET_PN.to_string()),
                         class: Some("ereport.fake.whatever".to_string()),
                         report: serde_json::json!({"hello_world": true}),
                     },
@@ -1712,6 +1722,7 @@ mod test {
                         collector_id: OmicronZoneUuid::new_v4().into(),
                         sled_id: sled_id.into(),
                         sled_serial: HOST_SERIAL.to_string(),
+                        part_number: Some(GIMLET_PN.to_string()),
                         class: Some("ereport.fake.whatever.thingy".to_string()),
                         report: serde_json::json!({"goodbye_world": false}),
                     },
@@ -1734,6 +1745,7 @@ mod test {
                         collector_id: OmicronZoneUuid::new_v4().into(),
                         sled_id: sled_id.into(),
                         sled_serial: HOST_SERIAL.to_string(),
+                        part_number: Some(GIMLET_PN.to_string()),
                         class: Some("ereport.something.hostos_related".to_string()),
                         report: serde_json::json!({"illumos": "very yes", "whatever": 42}),
                     },

schema/crdb/add-host-ereport-part-number/up1.sql

@@ -0,0 +1,2 @@
+ALTER TABLE omicron.public.host_ereport
+    ADD COLUMN IF NOT EXISTS part_number STRING(63);

schema/crdb/add-host-ereport-part-number/up2.sql

@@ -0,0 +1,8 @@
+-- backfill host OS ereports part_number column by querying the sled table. if
+-- there isn't a sled for the ereport's sled UUID in the sled table (i.e. if the
+-- sled record was hard deleted), leave it null.
+SET LOCAL disallow_full_table_scans = off;
+UPDATE omicron.public.host_ereport
+    SET part_number = sled.part_number
+    FROM sled
+    WHERE host_ereport.sled_id = sled.id;

schema/crdb/dbinit.sql

@@ -6460,6 +6460,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.host_ereport (
      */
     report JSONB NOT NULL,
 
+    part_number STRING(63),
+
     PRIMARY KEY (restart_id, ena)
 );
 
@@ -6495,7 +6497,7 @@ INSERT INTO omicron.public.db_metadata (
     version,
     target_version
 ) VALUES
-    (TRUE, NOW(), NOW(), '176.0.0', NULL)
+    (TRUE, NOW(), NOW(), '177.0.0', NULL)
 ON CONFLICT DO NOTHING;
 
 COMMIT;