MGS SpIdentifier slot could be a u16 (#8490)

Author: davepacheco

dev-tools/omdb/src/bin/omdb/mgs.rs

@@ -195,7 +195,7 @@ fn show_sp_ids(sp_ids: &[SpIdentifier]) -> Result<(), anyhow::Error> {
     struct SpIdRow {
         #[tabled(rename = "TYPE")]
         type_: &'static str,
-        slot: u32,
+        slot: u16,
     }
 
     impl From<&SpIdentifier> for SpIdRow {
@@ -221,7 +221,7 @@ fn show_sps_from_ignition(
     struct IgnitionRow {
         #[tabled(rename = "TYPE")]
         type_: &'static str,
-        slot: u32,
+        slot: u16,
         system_type: String,
     }
 
@@ -269,7 +269,7 @@ fn show_sp_states(
     struct SpStateRow<'a> {
         #[tabled(rename = "TYPE")]
         type_: &'static str,
-        slot: u32,
+        slot: u16,
         model: String,
         serial: String,
         rev: u32,

dev-tools/omdb/src/bin/omdb/mgs/sensors.rs

@@ -26,7 +26,7 @@ pub(crate) struct SensorsArgs {
 
     /// restrict to specified sled(s)
     #[clap(long, use_value_delimiter = true)]
-    pub sled: Vec<u32>,
+    pub sled: Vec<u16>,
 
     /// exclude sleds rather than include them
     #[clap(long, short)]
@@ -256,7 +256,7 @@ struct SpInfo {
 async fn sp_info(
     mgs_client: gateway_client::Client,
     type_: SpType,
-    slot: u32,
+    slot: u16,
 ) -> Result<SpInfo, anyhow::Error> {
     let mut devices = MultiMap::new();
     let mut timestamps = vec![];
@@ -429,7 +429,7 @@ fn sp_info_csv<R: std::io::Read>(
             }
         };
 
-        let slot = parts[1].parse::<u32>().or_else(|_| {
+        let slot = parts[1].parse::<u16>().or_else(|_| {
             bail!("invalid slot in \"{field}\"");
         })?;
 

dev-tools/reconfigurator-sp-updater/src/main.rs

@@ -213,13 +213,10 @@ impl Inventory {
             }),
         )
         .then(async move |sp_id| {
-            let sp_slot = u16::try_from(sp_id.slot).with_context(|| {
-                format!("sp slot number is out of range: {sp_id:?}")
-            })?;
             c.sp_get(sp_id.type_, sp_id.slot)
                 .await
                 .with_context(|| format!("fetching info about SP {:?}", sp_id))
-                .map(|s| (sp_id.type_, sp_slot, s))
+                .map(|s| (sp_id.type_, sp_id.slot, s))
         })
         .collect::<Vec<Result<_, _>>>()
         .await

gateway-test-utils/src/setup.rs

@@ -151,11 +151,11 @@ pub async fn test_setup_with_config(
             port_description.location.get(&expected_location).unwrap();
         let (sp_addr, sp_ereport_addr) = match target_sp.typ {
             SpType::Switch => {
-                let switch = &simrack.sidecars[target_sp.slot];
+                let switch = &simrack.sidecars[usize::from(target_sp.slot)];
                 (switch.local_addr(sp_port), switch.local_ereport_addr(sp_port))
             }
             SpType::Sled => {
-                let sled = &simrack.gimlets[target_sp.slot];
+                let sled = &simrack.gimlets[usize::from(target_sp.slot)];
                 (sled.local_addr(sp_port), sled.local_ereport_addr(sp_port))
             }
             SpType::Power => todo!(),

gateway-types/src/component.rs

@@ -43,8 +43,8 @@ pub enum SpType {
 pub struct SpIdentifier {
     #[serde(rename = "type")]
     pub typ: SpType,
-    #[serde(deserialize_with = "deserializer_u32_from_string")]
-    pub slot: u32,
+    #[serde(deserialize_with = "deserializer_u16_from_string")]
+    pub slot: u16,
 }
 
 impl fmt::Display for SpIdentifier {
@@ -59,29 +59,29 @@ impl fmt::Display for SpIdentifier {
 // trying to deserialize the flattened struct as a map of strings to strings,
 // which breaks on `slot` (but not on `typ` for reasons I don't entirely
 // understand). We can work around by using an enum that allows either `String`
-// or `u32` (which gets us past the serde map of strings), and then parsing the
-// string into a u32 ourselves (which gets us to the `slot` we want). More
+// or `u16` (which gets us past the serde map of strings), and then parsing the
+// string into a u16 ourselves (which gets us to the `slot` we want). More
 // background: https://github.com/serde-rs/serde/issues/1346
-fn deserializer_u32_from_string<'de, D>(
+fn deserializer_u16_from_string<'de, D>(
     deserializer: D,
-) -> Result<u32, D::Error>
+) -> Result<u16, D::Error>
 where
     D: serde::Deserializer<'de>,
 {
     use serde::de::{self, Unexpected};
 
     #[derive(Debug, Deserialize)]
     #[serde(untagged)]
-    enum StringOrU32 {
+    enum StringOrU16 {
         String(String),
-        U32(u32),
+        U16(u16),
     }
 
-    match StringOrU32::deserialize(deserializer)? {
-        StringOrU32::String(s) => s
+    match StringOrU16::deserialize(deserializer)? {
+        StringOrU16::String(s) => s
             .parse()
-            .map_err(|_| de::Error::invalid_type(Unexpected::Str(&s), &"u32")),
-        StringOrU32::U32(n) => Ok(n),
+            .map_err(|_| de::Error::invalid_type(Unexpected::Str(&s), &"u16")),
+        StringOrU16::U16(n) => Ok(n),
     }
 }
 

gateway/src/management_switch.rs

@@ -89,34 +89,24 @@ fn default_ereport_listen_port() -> u16 {
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Deserialize, Serialize)]
 pub struct SpIdentifier {
     pub typ: SpType,
-    pub slot: usize,
+    pub slot: u16,
 }
 
 impl SpIdentifier {
-    pub fn new(typ: SpType, slot: usize) -> Self {
+    pub fn new(typ: SpType, slot: u16) -> Self {
         Self { typ, slot }
     }
 }
 
 impl From<gateway_types::component::SpIdentifier> for SpIdentifier {
     fn from(id: gateway_types::component::SpIdentifier) -> Self {
-        Self {
-            typ: id.typ.into(),
-            // id.slot may come from an untrusted source, but usize >= 32 bits
-            // on any platform that will run this code, so unwrap is fine
-            slot: usize::try_from(id.slot).unwrap(),
-        }
+        Self { typ: id.typ.into(), slot: id.slot }
     }
 }
 
 impl From<SpIdentifier> for gateway_types::component::SpIdentifier {
     fn from(id: SpIdentifier) -> Self {
-        Self {
-            typ: id.typ.into(),
-            // id.slot comes from a trusted source (crate::management_switch)
-            // and will not exceed u32::MAX
-            slot: u32::try_from(id.slot).unwrap(),
-        }
+        Self { typ: id.typ.into(), slot: id.slot }
     }
 }
 

gateway/src/metrics.rs

@@ -648,7 +648,7 @@ impl SpPoller {
                         hubris_archive_id: Cow::Owned(
                             hubris_archive_id.clone(),
                         ),
-                        slot: self.spid.slot as u32,
+                        slot: u32::from(self.spid.slot),
                         component_kind: Cow::Owned(dev.device),
                         component_id,
                         description: Cow::Owned(dev.description),

nexus/inventory/src/builder.rs

@@ -188,26 +188,9 @@ impl CollectionBuilder {
         &mut self,
         source: &str,
         sp_type: SpType,
-        slot: u32,
+        sp_slot: u16,
         sp_state: SpState,
     ) -> Option<Arc<BaseboardId>> {
-        // Much ado about very little: MGS reports that "slot" is a u32, though
-        // in practice this seems very unlikely to be bigger than a u8.  (How
-        // many slots can there be within one rack?)  The database only supports
-        // signed integers, so if we assumed this really could span the range of
-        // a u32, we'd need to store it in an i64.  Instead, assume here that we
-        // can stick it into a u16 (which still seems generous).  This will
-        // allow us to store it into an Int32 in the database.
-        let Ok(sp_slot) = u16::try_from(slot) else {
-            self.found_error(InventoryError::from(anyhow!(
-                "MGS {:?}: SP {:?} slot {}: slot number did not fit into u16",
-                source,
-                sp_type,
-                slot
-            )));
-            return None;
-        };
-
         // Normalize the baseboard id: i.e., if we've seen this baseboard
         // before, use the same baseboard id record.  Otherwise, make a new one.
         let baseboard = Self::normalize_item(
@@ -580,7 +563,6 @@ mod test {
     use super::now_db_precision;
     use crate::examples::Representative;
     use crate::examples::representative;
-    use crate::examples::sp_state;
     use base64::Engine;
     use base64::engine::general_purpose::STANDARD as BASE64_STANDARD;
     use gateway_client::types::PowerState;
@@ -1089,15 +1071,6 @@ mod test {
             .unwrap();
         assert_eq!(sled1_bb, sled1_bb_dup);
 
-        // report an SP with an impossible slot number
-        let sled2_sp = builder.found_sp_state(
-            "fake MGS 1",
-            SpType::Sled,
-            u32::from(u16::MAX) + 1,
-            sp_state("1"),
-        );
-        assert_eq!(sled2_sp, None);
-
         // report SP caboose for an unknown baseboard
         let bogus_baseboard = BaseboardId {
             part_number: String::from("p1"),
@@ -1309,17 +1282,7 @@ mod test {
                 .is_none()
         );
 
-        // We should see an error.
-        assert_eq!(
-            collection
-                .errors
-                .iter()
-                .map(|e| format!("{:#}", e))
-                .collect::<Vec<_>>(),
-            vec![
-                "MGS \"fake MGS 1\": SP Sled slot 65536: \
-                slot number did not fit into u16"
-            ]
-        );
+        // We should see no errors.
+        assert!(collection.errors.is_empty());
     }
 }

nexus/mgs-updates/src/common_sp_update.rs

@@ -69,7 +69,7 @@ pub trait SpComponentUpdater {
     fn target_sp_type(&self) -> SpType;
 
     /// The slot number of the target SP.
-    fn target_sp_slot(&self) -> u32;
+    fn target_sp_slot(&self) -> u16;
 
     /// The target firmware slot for the component.
     fn firmware_slot(&self) -> u16;

nexus/mgs-updates/src/driver_update.rs

@@ -242,7 +242,7 @@ pub(crate) async fn apply_update(
                 client
                     .sp_component_update(
                         sp_type,
-                        u32::from(sp_slot),
+                        sp_slot,
                         component,
                         sp_update.firmware_slot,
                         &sp_update.update_id.as_untyped_uuid(),
@@ -450,11 +450,7 @@ async fn wait_for_delivery(
         let status = mgs_clients
             .try_all_serially(log, |client| async move {
                 let update_status = client
-                    .sp_component_update_status(
-                        sp_type,
-                        u32::from(sp_slot),
-                        component,
-                    )
+                    .sp_component_update_status(sp_type, sp_slot, component)
                     .await?;
 
                 debug!(
@@ -553,12 +549,7 @@ async fn abort_update(
         .try_all_serially(log, |mgs_client| async move {
             let arg = UpdateAbortBody { id: update_id };
             mgs_client
-                .sp_component_update_abort(
-                    sp_type,
-                    u32::from(sp_slot),
-                    component,
-                    &arg,
-                )
+                .sp_component_update_abort(sp_type, sp_slot, component, &arg)
                 .await
         })
         .await