Use topo instead of devinfo for disks, make location required

installinator/src/hardware.rs

@@ -7,6 +7,7 @@ use anyhow::Result;
 use anyhow::anyhow;
 use anyhow::ensure;
 use sled_agent_types::disk::DiskVariant;
+use sled_hardware::DiskBayOccupant;
 use sled_hardware::ExternalDisks;
 use sled_hardware::HardwareManager;
 use sled_hardware::SledMode;
@@ -33,6 +34,25 @@ impl Hardware {
         )
         .map_err(|err| anyhow!("failed to create HardwareManager: {err}"))?;
 
+        // An M.2 we cannot read would be left out of the install, so fail the
+        // scan and let the caller retry.
+        let unreadable_m2s: Vec<String> = hardware
+            .disk_bays()
+            .into_values()
+            .filter(|bay| bay.kind == DiskVariant::M2)
+            .filter_map(|bay| match bay.occupant {
+                DiskBayOccupant::Unreadable { error, .. } => {
+                    Some(format!("{}: {error}", bay.location))
+                }
+                _ => None,
+            })
+            .collect();
+        ensure!(
+            unreadable_m2s.is_empty(),
+            "could not read M.2 bay(s): {}",
+            unreadable_m2s.join("; "),
+        );
+
         let disks: Vec<RawDisk> =
             hardware.disks().into_values().map(|disk| disk.into()).collect();
 

sled-agent/config-reconciler/src/disks_common.rs

@@ -30,9 +30,9 @@ pub(crate) fn update_properties_from_raw_disk(
     }
 
     // The only properties we expect to change are the firmware metadata and
-    // the chassis location (which may be learned after the disk was adopted).
-    // Update those and check again; if they're still not equal, something
-    // weird is going on. At least log a warning.
+    // the chassis location, which is re-read from the hardware topology on
+    // every poll. Update those and check again; if they're still not equal,
+    // something weird is going on. At least log a warning.
     let mut disk = disk.clone();
     disk.update_mutable_properties(raw_disk);
     if *raw_disk == RawDisk::from(disk.clone()) {
@@ -68,14 +68,12 @@ mod tests {
     use sled_hardware::PooledDisk;
     use sled_hardware::UnparsedDisk;
 
-    // A disk adopted before its chassis location was known must pick up the
-    // location once the raw disk reports it, and be considered unchanged
-    // afterwards.
+    // A change to a disk's chassis location reported by the hardware
+    // topology must be copied onto the adopted disk, which is then
+    // considered unchanged.
     #[test]
-    fn location_learned_after_adoption_is_propagated() {
-        let logctx = dev::test_setup_log(
-            "location_learned_after_adoption_is_propagated",
-        );
+    fn location_change_is_propagated() {
+        let logctx = dev::test_setup_log("location_change_is_propagated");
 
         let identity = DiskIdentity {
             vendor: "test".into(),
@@ -91,32 +89,30 @@ mod tests {
             pcie_slot: 0,
             identity: identity.clone(),
             is_boot_disk: false,
-            location: None,
+            location: "N2".to_string(),
             partitions: vec![],
             zpool_name: ZpoolName::External(ExternalZpoolUuid::new_v4()),
             firmware: firmware.clone(),
         });
 
-        // The raw disk matches what was adopted, except that it now carries a
-        // chassis location.
-        let raw_disk = RawDisk::Real(
-            UnparsedDisk::new(
-                "/test-devfs".into(),
-                None,
-                0,
-                DiskVariant::U2,
-                identity,
-                false,
-                firmware,
-            )
-            .with_location(Some("N3".to_string())),
-        );
+        // The raw disk matches what was adopted, except for its chassis
+        // location.
+        let raw_disk = RawDisk::Real(UnparsedDisk::new(
+            "/test-devfs".into(),
+            None,
+            0,
+            DiskVariant::U2,
+            identity,
+            false,
+            firmware,
+            "N3".to_string(),
+        ));
 
         let updated = assert_matches!(
             update_properties_from_raw_disk(&adopted, &raw_disk, &logctx.log),
             MaybeUpdatedDisk::Updated(disk) => disk
         );
-        assert_eq!(updated.location(), Some("N3"));
+        assert_eq!(updated.location(), "N3");
         assert_eq!(RawDisk::from(updated.clone()), raw_disk);
 
         // A second pass sees nothing left to update.

sled-agent/config-reconciler/src/internal_disks.rs

@@ -996,7 +996,7 @@ mod tests {
                 pcie_slot: raw_disk.pcie_slot(),
                 identity: raw_disk.identity().clone(),
                 is_boot_disk: raw_disk.is_boot_disk(),
-                location: raw_disk.location().map(str::to_string),
+                location: raw_disk.location().to_string(),
                 partitions: vec![],
                 zpool_name: ZpoolName::Internal(InternalZpoolUuid::new_v4()),
                 firmware: raw_disk.firmware().clone(),
@@ -1026,6 +1026,7 @@ mod tests {
             },
             false,
             DiskFirmware::new(0, None, false, 1, vec![]),
+            sled_storage::disk::synthetic_disk_location(variant, 0),
         ))
     }
 

sled-agent/config-reconciler/src/raw_disks.rs

@@ -112,7 +112,7 @@ impl RawDisksSender {
                     identity: disk.identity().clone(),
                     variant: disk.variant(),
                     pcie_slot: disk.pcie_slot(),
-                    location: disk.location().map(str::to_string),
+                    location: Some(disk.location().to_string()),
                     active_firmware_slot: firmware.active_slot(),
                     next_active_firmware_slot: firmware.next_active_slot(),
                     number_of_firmware_slots: firmware.number_of_slots(),
@@ -191,6 +191,7 @@ mod tests {
                     let variant = DiskVariant::U2;
                     let firmware =
                         DiskFirmware::new(0, None, false, 255, Vec::new());
+                    let location = "N0".to_string();
 
                     match kind {
                         ArbitraryDiskKind::Real => {
@@ -202,6 +203,7 @@ mod tests {
                                 identity,
                                 is_boot_disk,
                                 firmware,
+                                location,
                             ))
                         }
                         ArbitraryDiskKind::Synthetic => {
@@ -211,7 +213,7 @@ mod tests {
                                 variant,
                                 pcie_slot: slot,
                                 firmware,
-                                location: "N0".to_string(),
+                                location,
                             })
                         }
                     }

sled-agent/config-reconciler/src/reconciler_task/external_disks.rs

@@ -806,7 +806,7 @@ mod tests {
                 pcie_slot: raw_disk.pcie_slot(),
                 identity: raw_disk.identity().clone(),
                 is_boot_disk: raw_disk.is_boot_disk(),
-                location: raw_disk.location().map(str::to_string),
+                location: raw_disk.location().to_string(),
                 partitions: vec![],
                 zpool_name: ZpoolName::new_external(pool_id),
                 firmware: raw_disk.firmware().clone(),
@@ -840,6 +840,7 @@ mod tests {
             },
             false,
             DiskFirmware::new(0, None, false, 1, vec![]),
+            sled_storage::disk::synthetic_disk_location(variant, 0),
         ))
     }
 

sled-hardware/src/disk.rs

@@ -217,11 +217,11 @@ pub struct UnparsedDisk {
     identity: DiskIdentity,
     is_boot_disk: bool,
     firmware: DiskFirmware,
-    #[serde(default)]
-    location: Option<String>,
+    location: String,
 }
 
 impl UnparsedDisk {
+    #[expect(clippy::too_many_arguments)]
     pub fn new(
         devfs_path: Utf8PathBuf,
         dev_path: Option<Utf8PathBuf>,
@@ -230,6 +230,7 @@ impl UnparsedDisk {
         identity: DiskIdentity,
         is_boot_disk: bool,
         firmware: DiskFirmware,
+        location: String,
     ) -> Self {
         Self {
             paths: DiskPaths { devfs_path, dev_path },
@@ -238,16 +239,10 @@ impl UnparsedDisk {
             identity,
             is_boot_disk,
             firmware,
-            location: None,
+            location,
         }
     }
 
-    /// Records where this disk sits in the chassis. See [`Self::location`].
-    pub fn with_location(mut self, location: Option<String>) -> Self {
-        self.location = location;
-        self
-    }
-
     pub fn paths(&self) -> &DiskPaths {
         &self.paths
     }
@@ -268,16 +263,19 @@ impl UnparsedDisk {
         self.is_boot_disk
     }
 
-    /// The PCIe physical slot number of the bridge above this disk.
+    /// The PCIe physical slot number of the bridge behind this disk's bay.
     ///
-    /// This is the `physical-slot#` property of the parent `pcieb` device.
-    /// It identifies the disk's position in the board's PCIe topology and is
-    /// board-specific: the same U.2 bay has a different number on Gimlet and
-    /// Cosmo. It is not the location label printed on the chassis.
+    /// This is the `binding/slot` property of the bay in the hardware
+    /// topology, the same number the `pcieb` device reports as
+    /// `physical-slot#`. It identifies the bay's position in the board's PCIe
+    /// topology and is board-specific: the same U.2 bay has a different
+    /// number on Gimlet and Cosmo. It is not the location label printed on
+    /// the chassis.
     ///
     /// Although **thus far** it has been stable, this value may also change
-    /// in subsequent host OS versions. For stability, the value of "location"
-    /// should be preferred.
+    /// in subsequent host OS versions. It is kept as the key inventory uses
+    /// for a disk and to join a disk to its firmware. For anything else,
+    /// prefer [`Self::location`].
     pub fn pcie_slot(&self) -> i64 {
         self.pcie_slot
     }
@@ -286,12 +284,11 @@ impl UnparsedDisk {
     /// 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, and is the value to show someone who has to find the drive. It
-    /// comes from libtopo and is best-effort: `None` means the topology had
-    /// no label for the disk or could not be read. A missing location never
-    /// prevents the disk from being used.
-    pub fn location(&self) -> Option<&str> {
-        self.location.as_deref()
+    /// sled, and is the value to show someone who has to find the drive. The
+    /// topology is where sled-hardware finds disks in the first place, so
+    /// every disk has one.
+    pub fn location(&self) -> &str {
+        &self.location
     }
 
     pub fn firmware(&self) -> &DiskFirmware {
@@ -326,7 +323,7 @@ pub struct PooledDisk {
     pub zpool_name: ZpoolName,
     pub firmware: DiskFirmware,
     /// See [`UnparsedDisk::location`].
-    pub location: Option<String>,
+    pub location: String,
 }
 
 impl PooledDisk {