PendingMgsUpdate slot id should be a u16 (#8398)

davepacheco · web-flow · commit 682caa3e0d27 · 2025-07-01T16:38:42.000Z
diff --git a/dev-tools/reconfigurator-cli/src/lib.rs b/dev-tools/reconfigurator-cli/src/lib.rs
@@ -1317,7 +1317,7 @@ fn cmd_blueprint_edit(
             let update = PendingMgsUpdate {
                 baseboard_id: baseboard_id.clone(),
                 sp_type: sp.sp_type,
-                slot_id: u32::from(sp.sp_slot),
+                slot_id: sp.sp_slot,
                 details,
                 artifact_hash,
                 artifact_version,
diff --git a/dev-tools/reconfigurator-sp-updater/src/main.rs b/dev-tools/reconfigurator-sp-updater/src/main.rs
@@ -188,7 +188,7 @@ impl Inventory {
 struct SpInfo {
     baseboard_id: Arc<BaseboardId>,
     sp_type: SpType,
-    sp_slot_id: u32,
+    sp_slot_id: u16,
 }
 
 impl Inventory {
@@ -213,16 +213,21 @@ 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, s))
+                .map(|s| (sp_id.type_, sp_slot, s))
         })
         .collect::<Vec<Result<_, _>>>()
         .await
         .into_iter()
         .filter_map(|r| match r {
-            Ok((sp_id, v)) => Some((sp_id, v.into_inner())),
+            Ok((sp_type, sp_slot, v)) => {
+                Some((sp_type, sp_slot, v.into_inner()))
+            }
             Err(error) => {
                 warn!(
                     log,
@@ -236,17 +241,14 @@ impl Inventory {
 
         let sps_by_serial = sp_infos
             .into_iter()
-            .map(|(sp_id, sp_state)| {
+            .map(|(sp_type, sp_slot, sp_state)| {
                 let baseboard_id = Arc::new(BaseboardId {
                     serial_number: sp_state.serial_number,
                     part_number: sp_state.model,
                 });
                 let serial_number = baseboard_id.serial_number.clone();
-                let sp_info = SpInfo {
-                    baseboard_id,
-                    sp_type: sp_id.type_,
-                    sp_slot_id: sp_id.slot,
-                };
+                let sp_info =
+                    SpInfo { baseboard_id, sp_type, sp_slot_id: sp_slot };
                 (serial_number, sp_info)
             })
             .collect();
diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs
@@ -1273,7 +1273,7 @@ impl BpPendingMgsUpdateSp {
         PendingMgsUpdate {
             baseboard_id,
             sp_type: self.sp_type.into(),
-            slot_id: u32::from(**self.sp_slot),
+            slot_id: **self.sp_slot,
             artifact_hash: self.artifact_sha256.into(),
             artifact_version: (*self.artifact_version).clone(),
             details: PendingMgsUpdateDetails::Sp {
diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs
@@ -517,19 +517,13 @@ impl DataStore {
                             } => continue,
                         };
 
-                    // slot_ids fit into a u16 in practice.  This will be
-                    // enforced at compile time shortly.  See
-                    // oxidecomputer/omicron#8378.
-                    let update_slot_id = u16::try_from(update.slot_id)
-                        .expect("slot id to fit into u16");
-
                     let db_blueprint_id = DbTypedUuid::from(blueprint_id)
                         .into_sql::<diesel::sql_types::Uuid>(
                     );
                     let db_sp_type =
                         SpType::from(update.sp_type).into_sql::<SpTypeEnum>();
                     let db_slot_id =
-                        SpMgsSlot::from(SqlU16::from(update_slot_id))
+                        SpMgsSlot::from(SqlU16::from(update.slot_id))
                             .into_sql::<diesel::sql_types::Int4>();
                     let db_artifact_hash =
                         ArtifactHash::from(update.artifact_hash)
@@ -2813,7 +2807,7 @@ mod tests {
         builder.pending_mgs_update_insert(PendingMgsUpdate {
             baseboard_id: baseboard_id.clone(),
             sp_type: sp.sp_type,
-            slot_id: u32::from(sp.sp_slot),
+            slot_id: sp.sp_slot,
             details: PendingMgsUpdateDetails::Sp {
                 expected_active_version: "1.0.0".parse().unwrap(),
                 expected_inactive_version: ExpectedVersion::NoValidVersion,
diff --git a/nexus/mgs-updates/src/common_sp_update.rs b/nexus/mgs-updates/src/common_sp_update.rs
@@ -297,7 +297,7 @@ pub enum PrecheckError {
     )]
     WrongDevice {
         sp_type: SpType,
-        slot_id: u32,
+        slot_id: u16,
         expected_part: String,
         expected_serial: String,
         found_part: String,
diff --git a/nexus/mgs-updates/src/driver_update.rs b/nexus/mgs-updates/src/driver_update.rs
@@ -56,7 +56,7 @@ pub(crate) struct SpComponentUpdate {
     pub log: slog::Logger,
     pub component: SpComponent,
     pub target_sp_type: SpType,
-    pub target_sp_slot: u32,
+    pub target_sp_slot: u16,
     pub firmware_slot: u16,
     pub update_id: SpUpdateUuid,
 }
@@ -242,7 +242,7 @@ pub(crate) async fn apply_update(
                 client
                     .sp_component_update(
                         sp_type,
-                        sp_slot,
+                        u32::from(sp_slot),
                         component,
                         sp_update.firmware_slot,
                         &sp_update.update_id.as_untyped_uuid(),
@@ -450,7 +450,11 @@ 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, sp_slot, component)
+                    .sp_component_update_status(
+                        sp_type,
+                        u32::from(sp_slot),
+                        component,
+                    )
                     .await?;
 
                 debug!(
@@ -549,7 +553,12 @@ 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, sp_slot, component, &arg)
+                .sp_component_update_abort(
+                    sp_type,
+                    u32::from(sp_slot),
+                    component,
+                    &arg,
+                )
                 .await
         })
         .await
@@ -712,7 +721,7 @@ mod test {
         gwtestctx: &GatewayTestContext,
         artifacts: &TestArtifacts,
         sp_type: SpType,
-        slot_id: u32,
+        slot_id: u16,
         artifact_hash: &ArtifactHash,
         expected_result: UpdateCompletedHow,
     ) {
@@ -796,7 +805,7 @@ mod test {
         gwtestctx: &GatewayTestContext,
         artifacts: &TestArtifacts,
         sp_type: SpType,
-        slot_id: u32,
+        slot_id: u16,
         artifact_hash: &ArtifactHash,
         expected_result: UpdateCompletedHow,
     ) {
diff --git a/nexus/mgs-updates/src/rot_updater.rs b/nexus/mgs-updates/src/rot_updater.rs
@@ -217,7 +217,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater {
             // Verify that the device is the one we think it is.
             let state = mgs_clients
             .try_all_serially(log, move |mgs_client| async move {
-                mgs_client.sp_get(update.sp_type, update.slot_id).await
+                mgs_client.sp_get(update.sp_type, u32::from(update.slot_id)).await
             })
             .await?
             .into_inner();
@@ -276,7 +276,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater {
                 mgs_client
                     .sp_component_caboose_get(
                         update.sp_type,
-                        update.slot_id,
+                        u32::from(update.slot_id),
                         &SpComponent::ROT.to_string(),
                         active.to_u16(),
                     )
@@ -326,7 +326,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater {
                     mgs_client
                         .sp_component_caboose_get(
                             update.sp_type,
-                            update.slot_id,
+                            u32::from(update.slot_id),
                             &SpComponent::ROT.to_string(),
                             expected_active_slot.slot().toggled().to_u16(),
                         )
@@ -415,7 +415,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater {
                 mgs_client
                     .sp_component_active_slot_set(
                         update.sp_type,
-                        update.slot_id,
+                        u32::from(update.slot_id),
                         &SpComponent::ROT.to_string(),
                         persist,
                         &SpComponentFirmwareSlot { slot: inactive_slot }
@@ -426,7 +426,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater {
                 mgs_client
                     .sp_component_reset(
                         update.sp_type,
-                        update.slot_id,
+                        u32::from(update.slot_id),
                         &SpComponent::ROT.to_string(),
                     )
                     .await?;
diff --git a/nexus/mgs-updates/src/sp_updater.rs b/nexus/mgs-updates/src/sp_updater.rs
@@ -164,7 +164,9 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater {
             // Verify that the device is the one we think it is.
             let state = mgs_clients
                 .try_all_serially(log, move |mgs_client| async move {
-                    mgs_client.sp_get(update.sp_type, update.slot_id).await
+                    mgs_client
+                        .sp_get(update.sp_type, u32::from(update.slot_id))
+                        .await
                 })
                 .await?
                 .into_inner();
@@ -188,7 +190,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater {
                     mgs_client
                         .sp_component_caboose_get(
                             update.sp_type,
-                            update.slot_id,
+                            u32::from(update.slot_id),
                             &SpComponent::SP_ITSELF.to_string(),
                             0,
                         )
@@ -240,7 +242,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater {
                     mgs_client
                         .sp_component_caboose_get(
                             update.sp_type,
-                            update.slot_id,
+                            u32::from(update.slot_id),
                             &SpComponent::SP_ITSELF.to_string(),
                             1,
                         )
@@ -300,7 +302,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater {
                 mgs_client
                     .sp_component_reset(
                         update.sp_type,
-                        update.slot_id,
+                        u32::from(update.slot_id),
                         &SpComponent::SP_ITSELF.to_string(),
                     )
                     .await?;
diff --git a/nexus/mgs-updates/src/test_util/sp_test_state.rs b/nexus/mgs-updates/src/test_util/sp_test_state.rs
@@ -56,8 +56,9 @@ impl SpTestState {
     pub async fn load(
         mgs_client: &gateway_client::Client,
         sp_type: SpType,
-        sp_slot: u32,
+        sp_slot: u16,
     ) -> Result<SpTestState, GatewayClientError> {
+        let sp_slot = u32::from(sp_slot);
         let caboose_sp_active = mgs_client
             .sp_component_caboose_get(
                 sp_type,
diff --git a/nexus/mgs-updates/src/test_util/updates.rs b/nexus/mgs-updates/src/test_util/updates.rs
@@ -67,7 +67,7 @@ pub struct UpdateDescription<'a> {
 
     // Update parameters
     pub sp_type: SpType,
-    pub slot_id: u32,
+    pub slot_id: u16,
     pub artifact_hash: &'a ArtifactHash,
 
     // Overrides
@@ -266,7 +266,7 @@ pub struct InProgressAttempt {
 
     // Parameters of the update itself
     sp_type: SpType,
-    slot_id: u32,
+    slot_id: u16,
     deployed_caboose: hubtools::Caboose,
 
     // Status of the driver
@@ -385,7 +385,7 @@ pub struct FinishedUpdateAttempt {
 impl FinishedUpdateAttempt {
     async fn new(
         sp_type: SpType,
-        slot_id: u32,
+        slot_id: u16,
         sp1: SpTestState,
         deployed_caboose: hubtools::Caboose,
         result: Result<UpdateCompletedHow, ApplyUpdateError>,
diff --git a/nexus/reconfigurator/planning/src/mgs_updates/mod.rs b/nexus/reconfigurator/planning/src/mgs_updates/mod.rs
@@ -245,7 +245,7 @@ fn mgs_update_status(
             update,
         );
         Ok(MgsUpdateStatus::Impossible)
-    } else if u32::from(sp_info.sp_slot) != update.slot_id {
+    } else if sp_info.sp_slot != update.slot_id {
         warn!(
             log,
             "baseboard with in-progress SP update has moved";
@@ -470,7 +470,7 @@ fn try_make_update_sp(
     Some(PendingMgsUpdate {
         baseboard_id: baseboard_id.clone(),
         sp_type: sp_info.sp_type,
-        slot_id: u32::from(sp_info.sp_slot),
+        slot_id: sp_info.sp_slot,
         details: PendingMgsUpdateDetails::Sp {
             expected_active_version,
             expected_inactive_version,
@@ -567,7 +567,7 @@ mod test {
     /// - switch 1: sidecar-c
     /// - psc 0: psc-b
     /// - psc 1: psc-c
-    fn test_config() -> BTreeMap<(SpType, u32), (&'static str, &'static str)> {
+    fn test_config() -> BTreeMap<(SpType, u16), (&'static str, &'static str)> {
         BTreeMap::from([
             ((SpType::Sled, 0), ("sled_0", "gimlet-d")),
             ((SpType::Sled, 1), ("sled_1", "gimlet-e")),
@@ -672,7 +672,7 @@ mod test {
     // `inactive_version` in the inactive slot.
     fn make_collection(
         active_version: ArtifactVersion,
-        active_version_exceptions: &BTreeMap<(SpType, u32), ArtifactVersion>,
+        active_version_exceptions: &BTreeMap<(SpType, u16), ArtifactVersion>,
         inactive_version: ExpectedVersion,
     ) -> Collection {
         let mut builder = nexus_inventory::CollectionBuilder::new(
@@ -711,7 +711,7 @@ mod test {
             };
 
             let baseboard_id = builder
-                .found_sp_state("test", sp_type, sp_slot, sp_state)
+                .found_sp_state("test", sp_type, u32::from(sp_slot), sp_state)
                 .unwrap();
             let active_version = active_version_exceptions
                 .get(&(sp_type, sp_slot))
@@ -1147,7 +1147,7 @@ mod test {
     }
 
     fn verify_one_sp_update(
-        expected_updates: &mut BTreeMap<(SpType, u32), (&str, ArtifactHash)>,
+        expected_updates: &mut BTreeMap<(SpType, u16), (&str, ArtifactHash)>,
         update: &PendingMgsUpdate,
     ) {
         let sp_type = update.sp_type;
diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs
@@ -1224,7 +1224,7 @@ pub struct PendingMgsUpdate {
     /// what type of baseboard this is
     pub sp_type: SpType,
     /// last known MGS slot (cubby number) of the baseboard
-    pub slot_id: u32,
+    pub slot_id: u16,
 
     /// component-specific details of the pending update
     pub details: PendingMgsUpdateDetails,
@@ -1243,7 +1243,7 @@ impl slog::KV for PendingMgsUpdate {
         slog::KV::serialize(&self.baseboard_id, record, serializer)?;
         serializer
             .emit_str(Key::from("sp_type"), &format!("{:?}", self.sp_type))?;
-        serializer.emit_u32(Key::from("sp_slot"), self.slot_id)?;
+        serializer.emit_u16(Key::from("sp_slot"), self.slot_id)?;
         slog::KV::serialize(&self.details, record, serializer)?;
         serializer.emit_str(
             Key::from("artifact_hash"),
diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json
@@ -5928,7 +5928,7 @@
           "slot_id": {
             "description": "last known MGS slot (cubby number) of the baseboard",
             "type": "integer",
-            "format": "uint32",
+            "format": "uint16",
             "minimum": 0
           },
           "sp_type": {
