Make disk location required in sled-agent inventory and Nexus

nexus/db-model/src/inventory.rs

@@ -2675,6 +2675,9 @@ pub struct InvPhysicalDisk {
     pub serial: String,
     pub variant: PhysicalDiskKind,
     /// The location of the disk in the chassis, as identified by libtopo.
+    ///
+    /// Always set on insert. NULL in rows written by Nexus versions that
+    /// predate this column.
     pub location: Option<String>,
 }
 
@@ -2692,7 +2695,7 @@ impl InvPhysicalDisk {
             model: disk.identity.model,
             serial: disk.identity.serial,
             variant: disk.variant.into(),
-            location: disk.location,
+            location: Some(disk.location),
         }
     }
 }

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

@@ -3460,6 +3460,7 @@ impl DataStore {
                 SledUuid,
                 Vec<nexus_types::inventory::PhysicalDisk>,
             >::new();
+            let mut nskipped = 0;
             let mut paginator = Paginator::new(
                 batch_size,
                 dropshot::PaginationOrder::Ascending,
@@ -3481,6 +3482,13 @@ impl DataStore {
                     p.found_batch(&batch, &|row| (row.sled_id, row.pcie_slot));
                 for disk in batch {
                     let sled_id = disk.sled_id.into();
+                    // Rows written by Nexus versions that predate the
+                    // `location` column are NULL. These disks are left out
+                    // of the collection.
+                    let Some(location) = disk.location else {
+                        nskipped += 1;
+                        continue;
+                    };
                     let firmware = disk_firmware
                         .get(&sled_id)
                         .and_then(|lookup| lookup.get(&disk.pcie_slot))
@@ -3495,12 +3503,20 @@ impl DataStore {
                             },
                             variant: disk.variant.into(),
                             pcie_slot: disk.pcie_slot,
-                            location: disk.location,
+                            location,
                             firmware: firmware.clone(),
                         },
                     );
                 }
             }
+            if nskipped > 0 {
+                warn!(
+                    &opctx.log,
+                    "skipped inventory disks without a location";
+                    "collection_id" => %id,
+                    "nskipped" => nskipped,
+                );
+            }
             disks
         };
 
@@ -6836,6 +6852,58 @@ mod test {
         logctx.cleanup_successful();
     }
 
+    // A disk row written before sled-agent always reported a location has a
+    // NULL location. Reading its collection leaves that disk out and keeps
+    // the rest.
+    #[tokio::test]
+    async fn test_disk_without_location_is_skipped() {
+        let logctx =
+            dev::test_setup_log("test_disk_without_location_is_skipped");
+        let db = TestDatabase::new_with_datastore(&logctx.log).await;
+        let (opctx, datastore) = (db.opctx(), db.datastore());
+
+        let Representative { builder, .. } = representative();
+        let collection = builder.build();
+        datastore
+            .inventory_insert_collection(&opctx, &collection)
+            .await
+            .expect("failed to insert collection");
+
+        let sled = collection
+            .sled_agents
+            .iter()
+            .find(|sa| sa.disks.len() > 1)
+            .expect("a sled agent with more than one disk");
+        let dropped = &sled.disks[0];
+
+        {
+            use nexus_db_schema::schema::inv_physical_disk::dsl;
+            let conn = datastore.pool_connection_for_tests().await.unwrap();
+            let updated = diesel::update(dsl::inv_physical_disk)
+                .filter(
+                    dsl::inv_collection_id.eq(to_db_typed_uuid(collection.id)),
+                )
+                .filter(dsl::sled_id.eq(to_db_typed_uuid(sled.sled_id)))
+                .filter(dsl::slot.eq(dropped.pcie_slot))
+                .set(dsl::location.eq(None::<String>))
+                .execute_async(&*conn)
+                .await
+                .unwrap();
+            assert_eq!(updated, 1);
+        }
+
+        let read = datastore
+            .inventory_collection_read(&opctx, collection.id)
+            .await
+            .expect("collection with a NULL disk location reads back");
+        let read_disks = &read.sled_agents.get(&sled.sled_id).unwrap().disks;
+        let expected: Vec<_> = sled.disks[1..].to_vec();
+        assert_eq!(*read_disks, expected);
+
+        db.terminate().await;
+        logctx.cleanup_successful();
+    }
+
     // Sanity check that we correctly receive, write, and then read an inventory
     // collection with multiple zones sharing an SNAT IP address.
     //

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

@@ -1154,7 +1154,7 @@ mod test {
             },
             variant: DiskVariant::U2,
             pcie_slot: slot,
-            location: None,
+            location: format!("N{slot}"),
             active_firmware_slot: 1,
             next_active_firmware_slot: None,
             number_of_firmware_slots: 1,
@@ -1183,7 +1183,10 @@ mod test {
             },
             variant: inv_phys_disk.variant.into(),
             pcie_slot: inv_phys_disk.pcie_slot,
-            location: inv_phys_disk.location.clone(),
+            location: inv_phys_disk
+                .location
+                .clone()
+                .expect("disk has a location"),
             firmware: PhysicalDiskFirmware::Nvme(NvmeFirmware {
                 active_slot: inv_disk.active_firmware_slot,
                 next_active_slot: inv_disk.next_active_firmware_slot,

nexus/inventory/src/builder.rs

@@ -1365,12 +1365,12 @@ mod test {
         assert_eq!(sled1_agent.disks.len(), 4);
         assert_eq!(sled1_agent.disks[0].identity.vendor, "memetendo");
         assert_eq!(sled1_agent.disks[0].pcie_slot, 0);
-        assert_eq!(sled1_agent.disks[0].location.as_deref(), Some("N0"));
+        assert_eq!(sled1_agent.disks[0].location, "N0");
         assert_eq!(sled1_agent.disks[3].identity.vendor, "macrohard");
         assert_eq!(sled1_agent.disks[3].identity.model, "box");
         assert_eq!(sled1_agent.disks[3].identity.serial, "XXIV");
         assert_eq!(sled1_agent.disks[3].pcie_slot, 18);
-        assert_eq!(sled1_agent.disks[3].location.as_deref(), Some("M.2 West"));
+        assert_eq!(sled1_agent.disks[3].location, "M.2 West");
 
         let sled4_agent =
             collection.sled_agents.get(&sled_agent_id_extra).unwrap();

nexus/inventory/src/examples.rs

@@ -544,7 +544,7 @@ pub fn representative() -> Representative {
             },
             variant: DiskVariant::U2,
             pcie_slot: 0,
-            location: Some("N0".to_string()),
+            location: "N0".to_string(),
             active_firmware_slot: 1,
             next_active_firmware_slot: None,
             number_of_firmware_slots: 1,
@@ -559,7 +559,7 @@ pub fn representative() -> Representative {
             },
             variant: DiskVariant::U2,
             pcie_slot: 1,
-            location: Some("N1".to_string()),
+            location: "N1".to_string(),
             active_firmware_slot: 1,
             next_active_firmware_slot: None,
             number_of_firmware_slots: 1,
@@ -574,7 +574,7 @@ pub fn representative() -> Representative {
             },
             variant: DiskVariant::U2,
             pcie_slot: 2,
-            location: Some("N2".to_string()),
+            location: "N2".to_string(),
             active_firmware_slot: 1,
             next_active_firmware_slot: None,
             number_of_firmware_slots: 1,
@@ -590,7 +590,7 @@ pub fn representative() -> Representative {
             },
             variant: DiskVariant::M2,
             pcie_slot: 18,
-            location: Some("M.2 West".to_string()),
+            location: "M.2 West".to_string(),
             active_firmware_slot: 1,
             next_active_firmware_slot: None,
             number_of_firmware_slots: 1,

nexus/reconfigurator/planning/src/system.rs

@@ -1569,7 +1569,7 @@ impl Sled {
                         identity: disk.disk_identity.clone(),
                         variant: DiskVariant::U2,
                         pcie_slot: i64::try_from(i).unwrap(),
-                        location: Some(format!("N{i}")),
+                        location: format!("N{i}"),
                         active_firmware_slot: 1,
                         next_active_firmware_slot: None,
                         number_of_firmware_slots: 1,

nexus/types/src/inventory.rs

@@ -1079,13 +1079,9 @@ pub struct PhysicalDisk {
     /// hardware topology: "N5" for a U.2 bay, "M.2 East" for a boot device.
     ///
     /// This is the operator-facing position, matching what is printed on the
-    /// sled. It is best-effort: `None` means the sled's topology had no label
-    /// for the disk or could not be read.
-    //
-    // Older serialized state had no such field; treating its absence as
-    // `None` accurately represents that nothing recorded a location.
-    #[serde(default)]
-    pub location: Option<String>,
+    /// sled. Sled-agent finds disks through the topology, so every disk has
+    /// one.
+    pub location: String,
     pub firmware: PhysicalDiskFirmware,
 }
 
@@ -1302,10 +1298,10 @@ impl IdOrdItem for DiscoveredSled {
 mod tests {
     use super::*;
 
-    // Reconfigurator state files written before `PhysicalDisk` gained
-    // `location` (and renamed `slot` to `pcie_slot`) must still deserialize.
+    // Reconfigurator state files that call `pcie_slot` by its old name,
+    // `slot`, still deserialize. Ones from before `location` existed do not.
     #[test]
-    fn physical_disk_loads_older_serialized_form() {
+    fn physical_disk_older_serialized_forms() {
         let disk = PhysicalDisk {
             identity: DiskIdentity {
                 vendor: "vendor".to_string(),
@@ -1314,11 +1310,10 @@ mod tests {
             },
             variant: PhysicalDiskKind::U2,
             pcie_slot: 5,
-            location: Some("N5".to_string()),
+            location: "N5".to_string(),
             firmware: PhysicalDiskFirmware::Unknown,
         };
 
-        // The current form round-trips and uses the new field name.
         let mut current = serde_json::to_value(&disk).unwrap();
         assert_eq!(current["pcie_slot"], 5);
         assert_eq!(current["location"], "N5");
@@ -1327,15 +1322,17 @@ mod tests {
             disk
         );
 
-        // Rewrite it into the older form: `slot` instead of `pcie_slot`, and
-        // no `location` at all.
         let object = current.as_object_mut().unwrap();
         let slot = object.remove("pcie_slot").unwrap();
         object.insert("slot".to_string(), slot);
-        object.remove("location").unwrap();
+        assert_eq!(
+            serde_json::from_value::<PhysicalDisk>(current.clone()).unwrap(),
+            disk
+        );
 
-        let loaded: PhysicalDisk = serde_json::from_value(current).unwrap();
-        assert_eq!(loaded, PhysicalDisk { location: None, ..disk });
+        current.as_object_mut().unwrap().remove("location").unwrap();
+        serde_json::from_value::<PhysicalDisk>(current)
+            .expect_err("a disk without a location should not load");
     }
 
     #[test]

nexus/types/src/inventory/display.rs

@@ -852,9 +852,8 @@ fn display_sleds(
         }
         for disk in disks {
             let PhysicalDisk { identity, variant, location, .. } = disk;
-            let loc = location.as_deref().unwrap_or("unknown location");
             let mut indent2 = IndentWriter::new("  ", &mut indented);
-            writeln!(indent2, "{variant:?}: {identity:?} in {loc}")?;
+            writeln!(indent2, "{variant:?}: {identity:?} in {location}")?;
         }
 
         if !zpools.is_empty() {