[fm] everyone gets a (nullable) slot (#10127)

This is a smallish refactor to the database schema for the `ereport`
table. Presently, we have `sp_type` and `sp_slot` columns, both of which
are nullable, and a `CHECK` constraint that enforces they are non-NULL
for SP ereports and NULL for host OS ereports. This was done because at
the time, I figured we could just use the sled ID column to go find the
location for host OS ereports. It turns out that this is somewhat more
fraught than I had hoped --- especially if a sled bounces or is removed
from the control plane, in which case these records don't make sense
anymore --- and it's much easier to just always record location for a
reporter when inserting the ereport. Thus, this branch changes the
schema a bit to:

1. rename the columns a bit so they're no longer SP-specific
2. make `slot_type` (nee `sp_type`) non-NULL, since even if we don't
know the sled number for a host ereport, we will always know it came
from a sled, by construction
3. change the `CHECK` constraint to allow host OS ereports to say what
slot the sled was in
4. change up some of the model and domain types to always have a slot
number

This closes #10096, which was my previous attempt to mess around with
the schema for representing ereport physical locations. Unlike that
branch, we leave the slot number nullable here
for host OS ereports, since (as discussed [here][1]) there may be
periods of time during which a sled-agent is known to the control plane,
but we don't actually know the physical location of the sled that
sled-agent corresponds to. We hope this won't happen very often, but we
must be able to represent it rather than being forced to discard any
ereports from such sleds.

There's a bit of a complex migration that attempts to backfill the
`slot` column for host OS ereports if we can find the sled UUID in the
inventory. In practice, this will not actually end up doing anything,
since...there's no code for actually collecting host OS ereports and
putting them in CRDB, so there aren't actually going to be any existing
records in need of backfilling outside of a couple integration tests.
But it felt nicer to *try* to populate this information anyway, I guess
because I'm a big dweeb or something.

[1]:
#10096 (comment)

Author: hawkw

dev-tools/omdb/src/bin/omdb/db/ereport.rs

@@ -256,6 +256,7 @@ async fn cmd_db_ereport_info(
     const CLASS: &str = "class";
     const REPORTER: &str = "reported by";
     const RESTART_ID: &str = "restart ID";
+    const SLED_ID: &str = "  sled ID";
     const PART_NUMBER: &str = "  part number";
     const SERIAL_NUMBER: &str = "  serial number";
     const WIDTH: usize = const_max_len(&[
@@ -264,6 +265,7 @@ async fn cmd_db_ereport_info(
         TIME_DELETED,
         COLLECTOR_ID,
         REPORTER,
+        SLED_ID,
         PART_NUMBER,
         SERIAL_NUMBER,
     ]);
@@ -297,8 +299,15 @@ async fn cmd_db_ereport_info(
                 "    {REPORTER:>WIDTH$}: {sp_type:?} {slot} (service processor)"
             )
         }
-        Ok(Reporter::HostOs { sled }) => {
-            println!("    {REPORTER:>WIDTH$}: sled {sled:?} (host OS)");
+        Ok(Reporter::HostOs { sled, slot }) => {
+            if let Some(slot) = slot {
+                println!("    {REPORTER:>WIDTH$}: sled {slot} (host OS)");
+            } else {
+                println!(
+                    "    {REPORTER:>WIDTH$}: <unknown sled slot> (host OS)"
+                );
+            }
+            println!("    {SLED_ID:>WIDTH$}: {sled:?}")
         }
     }
     println!("    {RESTART_ID:>WIDTH$}: {restart_id}");
@@ -361,8 +370,8 @@ async fn cmd_db_ereporters(
                     dsl::restart_id,
                     dsl::reporter,
                     dsl::sled_id,
-                    dsl::sp_slot,
-                    dsl::sp_type,
+                    dsl::slot_type,
+                    dsl::slot,
                     dsl::serial_number,
                     dsl::part_number
                 ))
@@ -379,7 +388,7 @@ async fn cmd_db_ereporters(
             if let Some(slot) = slot {
                 if slot_type.is_some() {
                     query = query
-                        .filter(dsl::sp_slot.eq(db::model::SqlU16::new(slot)));
+                        .filter(dsl::slot.eq(db::model::SqlU16::new(slot)));
                 } else {
                     anyhow::bail!(
                         "cannot filter reporters by slot without a value for `--type`"
@@ -389,7 +398,7 @@ async fn cmd_db_ereporters(
 
             if let Some(slot_type) = slot_type {
                 query = query
-                    .filter(dsl::sp_type.eq(slot_type));
+                    .filter(dsl::slot_type.eq(slot_type));
             }
 
             if let Some(serial) = serial {

nexus/db-model/src/ereport.rs

@@ -104,31 +104,30 @@ pub struct Ereport {
 pub struct Reporter {
     pub reporter: EreporterType,
 
-    //
-    // The physical location of the reporting SP.
-    //
-    /// SP location: the type of SP slot (sled, switch, power shelf).
-    ///
-    /// For SP ereports (i.e. those with `reporter == EreporterType::Sp`) this
-    /// is never NULL, which is enforced by the `reporter_identity_validity`
-    /// CHECK constraint. This is because SPs are indexed by their physical
-    /// location when requesting ereports through MGS.
-    pub sp_type: Option<SpType>,
-    /// SP location: the slot number.
-    ///
-    /// For SP ereports (i.e. those with `reporter == EreporterType::Sp`) this
-    /// is never NULL, which is enforced by the `reporter_identity_validity`
-    /// CHECK constraint. This is because SPs are indexed by their physical
-    /// location when requesting ereports through MGS.
-    pub sp_slot: Option<SpMgsSlot>,
-
     /// For host OS ereports, the sled UUID of the sled-agent from which this
     /// ereport was received.
     ///
     /// This is never NULL for host OS ereports (i.e. those with `reporter ==
     /// EreporterType::Host`). This is enforced by the
     /// `reporter_identity_validity` CHECK constraint.
     pub sled_id: Option<DbTypedUuid<SledKind>>,
+
+    //
+    // The physical location of the reporter
+    //
+    /// Reporter location: the type of the physical slot (sled, switch, power
+    /// shelf).
+    pub slot_type: SpType,
+    /// Reporter location: the slot number.
+    ///
+    /// For SP ereports (i.e. those with `reporter == EreporterType::Sp`) this
+    /// is never NULL, which is enforced by the `reporter_identity_validity`
+    /// CHECK constraint. This is because SPs are indexed by their physical
+    /// location when requesting ereports through MGS. For host OS ereports,
+    /// this may be NULL, as it is possible for a sled-agent to be part of the
+    /// control plane before its location is included in an inventory
+    /// collection.
+    pub slot: Option<SpMgsSlot>,
 }
 
 impl Ereport {
@@ -223,17 +222,17 @@ impl TryFrom<Ereport> for types::Ereport {
 impl From<types::Reporter> for Reporter {
     fn from(reporter: types::Reporter) -> Self {
         match reporter {
-            types::Reporter::HostOs { sled } => Self {
+            types::Reporter::HostOs { sled, slot } => Self {
                 reporter: EreporterType::Host,
                 sled_id: Some(sled.into()),
-                sp_type: None,
-                sp_slot: None,
+                slot_type: SpType::Sled,
+                slot: slot.map(SpMgsSlot::from),
             },
             types::Reporter::Sp { sp_type, slot } => Self {
                 reporter: EreporterType::Sp,
-                sp_type: Some(sp_type.into()),
-                sp_slot: Some(slot.into()),
                 sled_id: None,
+                slot_type: sp_type.into(),
+                slot: Some(slot.into()),
             },
         }
     }
@@ -245,35 +244,53 @@ impl TryFrom<Reporter> for types::Reporter {
         match reporter {
             Reporter {
                 reporter: EreporterType::Sp,
-                sp_type: Some(sp_type),
-                sp_slot: Some(slot),
+                slot_type,
+                slot: Some(slot),
                 ..
             } => Ok(Self::Sp {
-                sp_type: sp_type.into(),
+                sp_type: slot_type.into(),
                 slot: crate::SqlU16::from(slot).0,
             }),
             Reporter {
-                reporter: EreporterType::Sp, sp_type, sp_slot, ..
+                reporter: EreporterType::Sp, slot_type, slot, ..
             } => Err(Error::InternalError {
                 internal_message: format!(
                     "the 'reporter_identity_validity' CHECK constraint \
                      should enforce that ereports with reporter='sp' have \
-                     a non-NULL SP type and slot, but this ereport has \
-                     sp_type={sp_type:?} and sp_slot={sp_slot:?}",
+                     a non-NULL `slot`, but this ereport has \
+                     slot_type={slot_type:?} and slot={slot:?}",
                 ),
             }),
             Reporter {
                 reporter: EreporterType::Host,
                 sled_id: Some(id),
+                slot_type: SpType::Sled,
+                slot,
                 ..
-            } => Ok(Self::HostOs { sled: id.into() }),
+            } => Ok(Self::HostOs {
+                sled: id.into(),
+                slot: slot.map(|slot| crate::SqlU16::from(slot).0),
+            }),
             Reporter {
-                reporter: EreporterType::Host, sled_id: None, ..
+                reporter: EreporterType::Host,
+                slot_type: SpType::Sled,
+                sled_id: None,
+                ..
             } => Err(Error::internal_error(
                 "the 'reporter_identity_validity' CHECK constraint \
                      should enforce that ereports with reporter='host' \
                      have a non-NULL sled_id, but this ereport does not",
             )),
+            Reporter { reporter: EreporterType::Host, slot_type, .. } => {
+                Err(Error::InternalError {
+                    internal_message: format!(
+                        "the 'reporter_identity_validity' CHECK constraint \
+                        should  enforce that ereports with reporter='host' \
+                        have slot_type='sled', but this ereport has \
+                        slot_type={slot_type:?}",
+                    ),
+                })
+            }
         }
     }
 }

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(241, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(242, 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(242, "ereport-everyone-gets-a-slot"),
         KnownVersion::new(241, "audit-log-incomplete-timeout"),
         KnownVersion::new(240, "multicast-drop-mvlan"),
         KnownVersion::new(239, "fm-alert-request"),

nexus/db-queries/src/db/datastore/ereport.rs

@@ -197,8 +197,8 @@ impl DataStore {
             .group_by((
                 dsl::restart_id,
                 dsl::reporter,
-                dsl::sp_slot,
-                dsl::sp_type,
+                dsl::slot_type,
+                dsl::slot,
                 dsl::sled_id,
             ))
             .select((
@@ -236,7 +236,7 @@ impl DataStore {
                     .get_result_async(conn)
                     .await
             }
-            fm::Reporter::HostOs { sled } => {
+            fm::Reporter::HostOs { sled, .. } => {
                 Self::host_latest_ereport_id_query(sled)
                     .get_result_async(conn)
                     .await
@@ -258,9 +258,9 @@ impl DataStore {
     ) -> impl RunnableQuery<EreportIdTuple> {
         dsl::ereport
             .filter(
-                dsl::sp_type
+                dsl::slot_type
                     .eq(sp_type)
-                    .and(dsl::sp_slot.eq(slot))
+                    .and(dsl::slot.eq(slot))
                     .and(dsl::time_deleted.is_null()),
             )
             .order_by((dsl::time_collected.desc(), dsl::ena.desc()))

nexus/db-schema/src/schema.rs

@@ -2869,9 +2869,9 @@ table! {
         report -> Jsonb,
 
         reporter -> crate::enums::EreporterTypeEnum,
-        sp_type -> Nullable<crate::enums::SpTypeEnum>,
-        sp_slot -> Nullable<Int4>,
         sled_id -> Nullable<Uuid>,
+        slot_type -> crate::enums::SpTypeEnum,
+        slot -> Nullable<Int4>,
     }
 }
 

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

@@ -587,10 +587,11 @@ mod test {
     async fn make_fake_ereports(datastore: &DataStore, opctx: &OpContext) {
         const SP_SERIAL: &str = "BRM42000069";
         const HOST_SERIAL: &str = "BRM66600042";
+        const SLED_SLOT: u16 = 8;
         const GIMLET_PN: &str = "9130000019";
         // Make some SP ereports...
         let sp_restart_id = EreporterRestartUuid::new_v4();
-        datastore.ereports_insert(&opctx, Reporter::Sp { sp_type: SpType::Sled, slot: 8}, vec![
+        datastore.ereports_insert(&opctx, Reporter::Sp { sp_type: SpType::Sled, slot: SLED_SLOT}, vec![
             EreportData {
                 id: EreportId { restart_id: sp_restart_id, ena: ereport_types::Ena(1) },
                 time_collected: chrono::Utc::now(),
@@ -648,7 +649,10 @@ mod test {
         datastore
             .ereports_insert(
                 &opctx,
-                Reporter::HostOs { sled: SledUuid::new_v4() },
+                Reporter::HostOs {
+                    sled: SledUuid::new_v4(),
+                    slot: Some(SLED_SLOT),
+                },
                 vec![
                     EreportData {
                         id: EreportId {
@@ -681,7 +685,7 @@ mod test {
         datastore
             .ereports_insert(
                 &opctx,
-                Reporter::HostOs { sled: SledUuid::new_v4() },
+                Reporter::HostOs { sled: SledUuid::new_v4(), slot: Some(SLED_SLOT) },
                 vec![
                     EreportData {
                         id: EreportId { restart_id: EreporterRestartUuid::new_v4(), ena:  ereport_types::Ena(1) },