[fm] always record the physical location of an ereport reporter

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

@@ -292,13 +292,8 @@ async fn cmd_db_ereport_info(
     println!("    {COLLECTOR_ID:>WIDTH$}: {collector_id}");
     match Reporter::try_from(reporter) {
         Err(err) => eprintln!("{err}"),
-        Ok(Reporter::Sp { sp_type, slot }) => {
-            println!(
-                "    {REPORTER:>WIDTH$}: {sp_type:?} {slot} (service processor)"
-            )
-        }
-        Ok(Reporter::HostOs { sled }) => {
-            println!("    {REPORTER:>WIDTH$}: sled {sled:?} (host OS)");
+        Ok(reporter) => {
+            println!("    {REPORTER:>WIDTH$}: {reporter}");
         }
     }
     println!("    {RESTART_ID:>WIDTH$}: {restart_id}");
@@ -361,8 +356,8 @@ async fn cmd_db_ereporters(
                     dsl::restart_id,
                     dsl::reporter,
                     dsl::sled_id,
-                    dsl::sp_slot,
-                    dsl::sp_type,
+                    dsl::slot,
+                    dsl::slot_type,
                     dsl::serial_number,
                     dsl::part_number
                 ))
@@ -379,7 +374,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 +384,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

@@ -102,32 +102,20 @@ pub struct Ereport {
 #[derive(Copy, Clone, Debug, Insertable, Queryable, Selectable)]
 #[diesel(table_name = ereport)]
 pub struct Reporter {
+    /// Whether this ereport was generated by SP firmware or the host OS.
     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>,
+    /// The type of slot occupied by the reporter (sled, switch, power shelf).
+    pub slot_type: SpType,
+    /// The slot number of the reporter.
+    pub slot: 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.
+    /// EreporterType::Host`), and always NULL for SP ereports. This is
+    /// enforced by the `reporter_identity_validity` CHECK constraint.
     pub sled_id: Option<DbTypedUuid<SledKind>>,
 }
 
@@ -222,58 +210,44 @@ 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 {
-                reporter: EreporterType::Host,
-                sled_id: Some(sled.into()),
-                sp_type: None,
-                sp_slot: None,
-            },
-            types::Reporter::Sp { sp_type, slot } => Self {
-                reporter: EreporterType::Sp,
-                sp_type: Some(sp_type.into()),
-                sp_slot: Some(slot.into()),
-                sled_id: None,
-            },
+        let types::Reporter { slot_type, slot, kind } = reporter;
+        let (reporter, sled_id) = match kind {
+            types::ReporterKind::Sp => (EreporterType::Sp, None),
+            types::ReporterKind::HostOs { sled } => {
+                (EreporterType::Host, Some(sled.into()))
+            }
+        };
+        Self {
+            reporter,
+            slot_type: slot_type.into(),
+            slot: slot.into(),
+            sled_id,
         }
     }
 }
 
 impl TryFrom<Reporter> for types::Reporter {
     type Error = Error;
     fn try_from(reporter: Reporter) -> Result<Self, Self::Error> {
-        match reporter {
-            Reporter {
-                reporter: EreporterType::Sp,
-                sp_type: Some(sp_type),
-                sp_slot: Some(slot),
-                ..
-            } => Ok(Self::Sp {
-                sp_type: sp_type.into(),
-                slot: crate::SqlU16::from(slot).0,
-            }),
-            Reporter {
-                reporter: EreporterType::Sp, sp_type, sp_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:?}",
-                ),
-            }),
-            Reporter {
-                reporter: EreporterType::Host,
-                sled_id: Some(id),
-                ..
-            } => Ok(Self::HostOs { sled: id.into() }),
-            Reporter {
-                reporter: EreporterType::Host, 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",
-            )),
-        }
+        let Reporter { reporter, slot_type, slot, sled_id } = reporter;
+        let slot_type = slot_type.into();
+        let slot = crate::SqlU16::from(slot).0;
+        let kind = match reporter {
+            EreporterType::Sp => types::ReporterKind::Sp,
+            EreporterType::Host => {
+                let sled = sled_id
+                    .ok_or_else(|| {
+                        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",
+                        )
+                    })?
+                    .into();
+                types::ReporterKind::HostOs { sled }
+            }
+        };
+        Ok(types::Reporter { slot_type, slot, kind })
     }
 }

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-slots-non-null"),
         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

@@ -9,6 +9,7 @@ use crate::authz;
 use crate::context::OpContext;
 use crate::db::datastore::RunnableQuery;
 use crate::db::model::Ereport;
+use crate::db::model::EreporterType;
 use crate::db::model::SpMgsSlot;
 use crate::db::model::SpType;
 use crate::db::model::SqlU16;
@@ -235,8 +236,8 @@ impl DataStore {
             .group_by((
                 dsl::restart_id,
                 dsl::reporter,
-                dsl::sp_slot,
-                dsl::sp_type,
+                dsl::slot,
+                dsl::slot_type,
                 dsl::sled_id,
             ))
             .select((
@@ -266,15 +267,15 @@ impl DataStore {
         conn: &async_bb8_diesel::Connection<DbConnection>,
         reporter: fm::Reporter,
     ) -> Result<Option<EreportId>, Error> {
-        let result = match reporter {
-            fm::Reporter::Sp { sp_type, slot } => {
-                let sp_type = sp_type.into();
-                let slot = SpMgsSlot::from(SqlU16::new(slot));
-                Self::sp_latest_ereport_id_query(sp_type, slot)
+        let result = match reporter.kind {
+            fm::ReporterKind::Sp => {
+                let slot_type = reporter.slot_type.into();
+                let slot = SpMgsSlot::from(SqlU16::new(reporter.slot));
+                Self::sp_latest_ereport_id_query(slot_type, slot)
                     .get_result_async(conn)
                     .await
             }
-            fm::Reporter::HostOs { sled } => {
+            fm::ReporterKind::HostOs { sled } => {
                 Self::host_latest_ereport_id_query(sled)
                     .get_result_async(conn)
                     .await
@@ -291,14 +292,15 @@ impl DataStore {
     }
 
     fn sp_latest_ereport_id_query(
-        sp_type: SpType,
+        slot_type: SpType,
         slot: SpMgsSlot,
     ) -> impl RunnableQuery<EreportIdTuple> {
         dsl::ereport
             .filter(
-                dsl::sp_type
-                    .eq(sp_type)
-                    .and(dsl::sp_slot.eq(slot))
+                dsl::reporter
+                    .eq(EreporterType::Sp)
+                    .and(dsl::slot_type.eq(slot_type))
+                    .and(dsl::slot.eq(slot))
                     .and(dsl::time_deleted.is_null()),
             )
             .order_by((dsl::time_collected.desc(), dsl::ena.desc()))
@@ -311,8 +313,9 @@ impl DataStore {
     ) -> impl RunnableQuery<EreportIdTuple> {
         dsl::ereport
             .filter(
-                dsl::sled_id
-                    .eq(sled_id.into_untyped_uuid())
+                dsl::reporter
+                    .eq(EreporterType::Host)
+                    .and(dsl::sled_id.eq(sled_id.into_untyped_uuid()))
                     .and(dsl::time_deleted.is_null()),
             )
             .order_by((dsl::time_collected.desc(), dsl::ena.desc()))
@@ -577,9 +580,10 @@ mod tests {
         datastore
             .ereports_insert(
                 &opctx,
-                fm::Reporter::Sp {
-                    sp_type: nexus_types::inventory::SpType::Sled,
+                fm::Reporter {
+                    slot_type: nexus_types::inventory::SpType::Sled,
                     slot: 19,
+                    kind: fm::ReporterKind::Sp,
                 },
                 vec![ereport.clone()],
             )

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

@@ -1044,7 +1044,7 @@ mod tests {
     use ereport_types;
     use nexus_types::alert::AlertClass;
     use nexus_types::fm;
-    use nexus_types::fm::ereport::{EreportData, Reporter};
+    use nexus_types::fm::ereport::{EreportData, Reporter, ReporterKind};
     use omicron_test_utils::dev;
     use omicron_uuid_kinds::CollectionUuid;
     use omicron_uuid_kinds::OmicronZoneUuid;
@@ -1718,9 +1718,10 @@ mod tests {
         };
 
         // Insert the ereports
-        let reporter = Reporter::Sp {
-            sp_type: nexus_types::inventory::SpType::Sled,
+        let reporter = Reporter {
+            slot_type: nexus_types::inventory::SpType::Sled,
             slot: 0,
+            kind: ReporterKind::Sp,
         };
 
         datastore

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 -> Int4,
     }
 }
 

nexus/fm/src/test_util.rs

@@ -6,7 +6,7 @@ use crate::builder::SitrepBuilderRng;
 use chrono::Utc;
 use nexus_reconfigurator_planning::example;
 use nexus_types::fm::ereport::{
-    Ena, Ereport, EreportData, EreportId, Reporter,
+    Ena, Ereport, EreportData, EreportId, Reporter, ReporterKind,
 };
 use omicron_test_utils::dev;
 use omicron_uuid_kinds::EreporterRestartKind;
@@ -135,8 +135,8 @@ pub fn mk_ereport(
     time_collected: chrono::DateTime<Utc>,
     json: serde_json::Map<String, serde_json::Value>,
 ) -> Ereport {
-    let data = match reporter {
-        Reporter::Sp { .. } => {
+    let data = match reporter.kind {
+        ReporterKind::Sp => {
             let raw = ereport_types::Ereport { ena: id.ena, data: json };
             EreportData::from_sp_ereport(
                 log,
@@ -146,7 +146,7 @@ pub fn mk_ereport(
                 collector_id,
             )
         }
-        Reporter::HostOs { .. } => {
+        ReporterKind::HostOs { .. } => {
             todo!(
                 "eliza: when we get around to actually ingesting host ereport \
                  JSON, figure out what the field names for serial and part \

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

@@ -200,7 +200,11 @@ impl Ingester {
         slot: u16,
     ) -> Option<EreporterStatus> {
         // Fetch the latest ereport from this SP.
-        let reporter = nexus_types::fm::ereport::Reporter::Sp { sp_type, slot };
+        let reporter = nexus_types::fm::ereport::Reporter {
+            slot_type: sp_type,
+            slot,
+            kind: nexus_types::fm::ereport::ReporterKind::Sp,
+        };
         let latest =
             match self.datastore.latest_ereport_id(&opctx, reporter).await {
                 Ok(latest) => latest,