From d2cfd201dcc8f09102dbd3c88321983397fdf327 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Sep 2026 15:05:32 -0700 Subject: [PATCH 01/19] Add libtopo dependency to sled-hardware Adds the libtopo crate (0.1.0, crates.io) as an illumos-only dependency of sled-hardware, ahead of using it to report each disk's chassis location label from the topology snapshot. No behavior change yet. libtopo depends on illumos-nvpair 0.4, so the workspace pin moves from 0.3.0 to 0.4.0. The 0.4 release is purely additive (it adds OwnedNvList), and oxlog, the only in-tree user, compiles unchanged. fmd-adm is bumped to its current main revision, which differs from the previous pin only by the same illumos-nvpair 0.4 bump, so the workspace ends up with a single copy of illumos-nvpair. The Cargo.lock change is limited to these packages. Cargo's resolver also wanted to flip several unrelated wide-range dependencies (windows-sys, socket2, heck, itertools) to newer versions already in the lock; those were reverted to keep the diff scoped. Verified with cargo check on Linux (oxlog, sled-hardware, omdb) and on illumos (sled-hardware, oxlog, omdb, sled-agent), cargo xtask clippy, cargo xtask check-workspace-deps, and cargo hakari generate (no workspace-hack changes). --- Cargo.lock | 27 +++++++++++++++++++++++---- Cargo.toml | 7 ++++--- sled-hardware/Cargo.toml | 1 + 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e96cddced28..facc6ebfa8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3999,7 +3999,7 @@ dependencies = [ [[package]] name = "fmd-adm" version = "0.3.0" -source = "git+https://github.com/oxidecomputer/fmd-adm?rev=846361bf0a698a8c7efefd97b2828b9aa74858c4#846361bf0a698a8c7efefd97b2828b9aa74858c4" +source = "git+https://github.com/oxidecomputer/fmd-adm?rev=5b57d8dd36f96778d0c067ec9647ddde9b58ace7#5b57d8dd36f96778d0c067ec9647ddde9b58ace7" dependencies = [ "fmd-adm-sys", "illumos-nvpair", @@ -4011,7 +4011,7 @@ dependencies = [ [[package]] name = "fmd-adm-sys" version = "0.4.1" -source = "git+https://github.com/oxidecomputer/fmd-adm?rev=846361bf0a698a8c7efefd97b2828b9aa74858c4#846361bf0a698a8c7efefd97b2828b9aa74858c4" +source = "git+https://github.com/oxidecomputer/fmd-adm?rev=5b57d8dd36f96778d0c067ec9647ddde9b58ace7#5b57d8dd36f96778d0c067ec9647ddde9b58ace7" [[package]] name = "fnv" @@ -5560,9 +5560,9 @@ dependencies = [ [[package]] name = "illumos-nvpair" -version = "0.3.0" +version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "66fe4718e52c3654c4e6c6501ce8387d334acd0910b7b7bb3a554aad0262d09f" +checksum = "bde3bf36c186fe406f2ecdc3c9c7fdc7b4ab56babb098006b058bd2d822a5aec" dependencies = [ "illumos-nvpair-sys", ] @@ -6578,6 +6578,24 @@ dependencies = [ "escape8259", ] +[[package]] +name = "libtopo" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08099e1e02473bb103f1ad4bfa9c81c04ebf7a638573c72959817a0347040022" +dependencies = [ + "illumos-nvpair", + "illumos-nvpair-sys", + "libtopo-sys", + "thiserror 2.0.18", +] + +[[package]] +name = "libtopo-sys" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e9c1776be01e57714dfe8247395585d69479444508dad3f4e35270801152001" + [[package]] name = "libxml" version = "0.3.3" @@ -14718,6 +14736,7 @@ dependencies = [ "libc", "libefi-illumos", "libnvme", + "libtopo", "macaddr", "omicron-common", "omicron-test-utils", diff --git a/Cargo.toml b/Cargo.toml index 339882b9dea..90883a825f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -534,8 +534,8 @@ filetime = "0.2.26" flate2 = "1.1.2" float-ord = "0.3.2" flume = "0.11.1" -fmd-adm = { git = "https://github.com/oxidecomputer/fmd-adm", rev = "846361bf0a698a8c7efefd97b2828b9aa74858c4" } -fmd-adm-sys = { git = "https://github.com/oxidecomputer/fmd-adm", rev = "846361bf0a698a8c7efefd97b2828b9aa74858c4" } +fmd-adm = { git = "https://github.com/oxidecomputer/fmd-adm", rev = "5b57d8dd36f96778d0c067ec9647ddde9b58ace7" } +fmd-adm-sys = { git = "https://github.com/oxidecomputer/fmd-adm", rev = "5b57d8dd36f96778d0c067ec9647ddde9b58ace7" } foreign-types = "0.3.2" fs-err = "3.1.1" futures = "0.3.31" @@ -589,7 +589,7 @@ hyper-util = "0.1.16" hyper-rustls = "0.27.7" hyper-staticfile = "0.10.1" iddqd = { version = "0.4.2", features = ["daft", "serde", "schemars08"] } -illumos-nvpair = "0.3.0" +illumos-nvpair = "0.4.0" illumos-nvpair-sys = "0.2.0" illumos-utils = { path = "illumos-utils" } iana-time-zone = "0.1.63" @@ -619,6 +619,7 @@ libc = "0.2.174" linkme = "0.3" libipcc = { git = "https://github.com/oxidecomputer/ipcc-rs", rev = "524eb8f125003dff50b9703900c6b323f00f9e1b" } libnvme = { git = "https://github.com/oxidecomputer/libnvme", rev = "dd5bb221d327a1bc9287961718c3c10d6bd37da0" } +libtopo = "0.1.0" linear-map = "1.2.0" live-tests-macros = { path = "live-tests/macros" } lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" } diff --git a/sled-hardware/Cargo.toml b/sled-hardware/Cargo.toml index 5ed13d64e73..e04a5da4514 100644 --- a/sled-hardware/Cargo.toml +++ b/sled-hardware/Cargo.toml @@ -36,6 +36,7 @@ omicron-workspace-hack.workspace = true illumos-devinfo = { git = "https://github.com/oxidecomputer/illumos-devinfo", branch = "main" } libefi-illumos = { git = "https://github.com/oxidecomputer/libefi-illumos", branch = "master" } libnvme.workspace = true +libtopo.workspace = true zone.workspace = true [dev-dependencies] From 91ff18c07d3e9315e369eead6af44f737c9e1540 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Sep 2026 17:43:04 -0700 Subject: [PATCH 02/19] Rename disk "slot" to "pcie_slot" in sled-hardware and Nexus types The value sled-hardware reads from the "physical-slot#" property of the pcieb bridge above each NVMe device is the PCIe Physical Slot Number. It is internal to the board's PCIe topology and board-specific: the same ten U.2 bays are numbered 0x00-0x09 on Gimlet and 0x20-0x29 on Cosmo. Calling it "slot" invited reading it as the bay number printed on the chassis, which is what led to #11258. This renames the field and accessors on UnparsedDisk, PooledDisk, RawDisk, Disk, RawSyntheticDisk, and nexus_types::inventory::PhysicalDisk to pcie_slot, renames the per-board range helpers on OxideSled to match, and documents what the number is and is not. Callers in sled-storage, the config reconciler, installinator, and the Nexus inventory datastore follow the rename. Deliberately unchanged: the sled-agent API type InventoryDisk and the inv_physical_disk / inv_nvme_disk_firmware columns, which will move together with the API version bump and schema migration that add the chassis location. NVMe firmware slot fields are a different concept and keep their names. Inventory display output is unchanged. Test coverage: pure rename with no behavior change; verified by cargo check --workspace --all-targets on Linux and cargo check --all-targets of sled-hardware, sled-storage, sled-agent-config-reconciler, omicron-sled-agent, and installinator on illumos, plus cargo xtask clippy and cargo fmt --check. --- installinator/src/write.rs | 10 ++-- nexus/db-model/src/inventory.rs | 2 +- .../db-queries/src/db/datastore/inventory.rs | 4 +- .../src/db/datastore/physical_disk.rs | 2 +- nexus/types/src/inventory.rs | 11 ++++- nexus/types/src/inventory/display.rs | 4 +- .../config-reconciler/src/internal_disks.rs | 4 +- sled-agent/config-reconciler/src/raw_disks.rs | 4 +- .../src/reconciler_task/external_disks.rs | 2 +- sled-agent/types/versions/src/impls/disk.rs | 5 +- sled-hardware/src/disk.rs | 21 +++++--- sled-hardware/src/illumos/mod.rs | 49 +++++++++++-------- sled-hardware/types/src/lib.rs | 18 +++++-- sled-storage/src/disk.rs | 28 ++++++----- 14 files changed, 101 insertions(+), 63 deletions(-) diff --git a/installinator/src/write.rs b/installinator/src/write.rs index 4ac93f88f61..17f843b822a 100644 --- a/installinator/src/write.rs +++ b/installinator/src/write.rs @@ -122,10 +122,10 @@ impl WriteDestination { let mut drives = BTreeMap::new(); for disk in hardware.m2_disks() { - let Ok(slot) = M2Slot::try_from(disk.slot()) else { + let Ok(slot) = M2Slot::try_from(disk.pcie_slot()) else { warn!( log, "skipping M.2 drive with unexpected slot number"; - "slot" => disk.slot(), + "pcie_slot" => disk.pcie_slot(), ); continue; }; @@ -136,7 +136,7 @@ impl WriteDestination { log, "found target M.2 disk"; "identity" => ?disk.identity(), "path" => disk.devfs_path().as_str(), - "slot" => disk.slot(), + "pcie_slot" => disk.pcie_slot(), "boot_image_path" => path.as_str(), "zpool" => %disk.zpool_name(), ); @@ -165,7 +165,7 @@ impl WriteDestination { log, "skipping duplicate M.2 drive entry"; "identity" => ?disk.identity(), "path" => disk.devfs_path().as_str(), - "slot" => disk.slot(), + "pcie_slot" => disk.pcie_slot(), "boot_image_path" => path.as_str(), "zpool" => %disk.zpool_name(), ); @@ -178,7 +178,7 @@ impl WriteDestination { log, "found M.2 disk but failed to find boot image path"; "identity" => ?disk.identity(), "path" => disk.devfs_path().as_str(), - "slot" => disk.slot(), + "pcie_slot" => disk.pcie_slot(), "boot_image_path_err" => err, "zpool" => %disk.zpool_name(), ); diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 75d9ce96045..b9b127ae404 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -2375,7 +2375,7 @@ impl InvPhysicalDisk { Self { inv_collection_id: inv_collection_id.into(), sled_id: sled_id.into(), - slot: disk.slot, + slot: disk.pcie_slot, vendor: disk.identity.vendor, model: disk.identity.model, serial: disk.identity.serial, diff --git a/nexus/db-queries/src/db/datastore/inventory.rs b/nexus/db-queries/src/db/datastore/inventory.rs index 6fbab9c177b..915d0894cae 100644 --- a/nexus/db-queries/src/db/datastore/inventory.rs +++ b/nexus/db-queries/src/db/datastore/inventory.rs @@ -210,7 +210,7 @@ impl DataStore { InvNvmeDiskFirmware::new( collection_id, sled_agent.sled_id, - disk.slot, + disk.pcie_slot, firmware, ) .map_err(|e| { @@ -3154,7 +3154,7 @@ impl DataStore { serial: disk.serial, }, variant: disk.variant.into(), - slot: disk.slot, + pcie_slot: disk.slot, firmware: firmware.clone(), }, ); diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index 864dee85844..35a80fa9007 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -1183,7 +1183,7 @@ mod test { serial: inv_phys_disk.serial.clone(), }, variant: inv_phys_disk.variant.into(), - slot: inv_phys_disk.slot, + pcie_slot: inv_phys_disk.slot, firmware: PhysicalDiskFirmware::Nvme(NvmeFirmware { active_slot: inv_disk.active_firmware_slot, next_active_slot: inv_disk.next_active_firmware_slot, diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index 40617f2dc81..ed3acafef42 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -607,7 +607,14 @@ pub struct PhysicalDisk { // always show up in the inventory. pub identity: DiskIdentity, pub variant: PhysicalDiskKind, - pub slot: i64, + /// PCIe physical slot number of the bridge above this disk, as reported + /// by sled-agent. + /// + /// This is the `physical-slot#` of the parent `pcieb` device. It is + /// internal to the board's PCIe topology and board-specific: the same + /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the + /// location label printed on the chassis. + pub pcie_slot: i64, pub firmware: PhysicalDiskFirmware, } @@ -616,7 +623,7 @@ impl From for PhysicalDisk { PhysicalDisk { identity: disk.identity, variant: disk.variant.into(), - slot: disk.slot, + pcie_slot: disk.slot, firmware: PhysicalDiskFirmware::Nvme(NvmeFirmware { active_slot: disk.active_firmware_slot, next_active_slot: disk.next_active_firmware_slot, diff --git a/nexus/types/src/inventory/display.rs b/nexus/types/src/inventory/display.rs index e82a94277b3..ff13e25141d 100644 --- a/nexus/types/src/inventory/display.rs +++ b/nexus/types/src/inventory/display.rs @@ -666,9 +666,9 @@ fn display_sleds( writeln!(indented, "physical disks:")?; } for disk in disks { - let PhysicalDisk { identity, variant, slot, .. } = disk; + let PhysicalDisk { identity, variant, pcie_slot, .. } = disk; let mut indent2 = IndentWriter::new(" ", &mut indented); - writeln!(indent2, "{variant:?}: {identity:?} in {slot}")?; + writeln!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?; } if !zpools.is_empty() { diff --git a/sled-agent/config-reconciler/src/internal_disks.rs b/sled-agent/config-reconciler/src/internal_disks.rs index d2c2cfd68f8..968c846a69a 100644 --- a/sled-agent/config-reconciler/src/internal_disks.rs +++ b/sled-agent/config-reconciler/src/internal_disks.rs @@ -544,7 +544,7 @@ impl From<&'_ Disk> for InternalDiskDetails { is_boot_disk: disk.is_boot_disk(), }, zpool_id, - slot: M2Slot::try_from(disk.slot()).ok(), + slot: M2Slot::try_from(disk.pcie_slot()).ok(), boot_image_raw_devfs_path: disk .boot_image_devfs_path(true) .map_err(Arc::new), @@ -993,7 +993,7 @@ mod tests { devfs_path: "/fake-disk".into(), dev_path: None, }, - slot: raw_disk.slot(), + pcie_slot: raw_disk.pcie_slot(), identity: raw_disk.identity().clone(), is_boot_disk: raw_disk.is_boot_disk(), partitions: vec![], diff --git a/sled-agent/config-reconciler/src/raw_disks.rs b/sled-agent/config-reconciler/src/raw_disks.rs index 61335ba830a..695e17c3a02 100644 --- a/sled-agent/config-reconciler/src/raw_disks.rs +++ b/sled-agent/config-reconciler/src/raw_disks.rs @@ -111,7 +111,7 @@ impl RawDisksSender { InventoryDisk { identity: disk.identity().clone(), variant: disk.variant(), - slot: disk.slot(), + slot: disk.pcie_slot(), active_firmware_slot: firmware.active_slot(), next_active_firmware_slot: firmware.next_active_slot(), number_of_firmware_slots: firmware.number_of_slots(), @@ -208,7 +208,7 @@ mod tests { path, identity, variant, - slot, + pcie_slot: slot, firmware, }) } diff --git a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs index 314c68ffd2c..6f6ab98713a 100644 --- a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs +++ b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs @@ -1118,7 +1118,7 @@ mod tests { devfs_path: "/fake-disk".into(), dev_path: None, }, - slot: raw_disk.slot(), + pcie_slot: raw_disk.pcie_slot(), identity: raw_disk.identity().clone(), is_boot_disk: raw_disk.is_boot_disk(), partitions: vec![], diff --git a/sled-agent/types/versions/src/impls/disk.rs b/sled-agent/types/versions/src/impls/disk.rs index 64b39c3fa9a..636b48a9527 100644 --- a/sled-agent/types/versions/src/impls/disk.rs +++ b/sled-agent/types/versions/src/impls/disk.rs @@ -81,8 +81,9 @@ impl TryFrom for M2Slot { fn try_from(value: i64) -> Result { match value { - // Gimlet should have 2 M.2 drives: drive A is assigned slot 17, and - // drive B is assigned slot 18. + // Gimlet and Cosmo have two M.2 drives. The value here is the + // PCIe physical slot number of the bridge above the drive: + // drive A is behind slot 0x11 and drive B behind slot 0x12. 17 => Ok(Self::A), 18 => Ok(Self::B), _ => bail!("unexpected M.2 slot {value}"), diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index 99d28656310..c3c397dc320 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -212,7 +212,7 @@ impl DiskFirmware { )] pub struct UnparsedDisk { paths: DiskPaths, - slot: i64, + pcie_slot: i64, variant: DiskVariant, identity: DiskIdentity, is_boot_disk: bool, @@ -223,7 +223,7 @@ impl UnparsedDisk { pub fn new( devfs_path: Utf8PathBuf, dev_path: Option, - slot: i64, + pcie_slot: i64, variant: DiskVariant, identity: DiskIdentity, is_boot_disk: bool, @@ -231,7 +231,7 @@ impl UnparsedDisk { ) -> Self { Self { paths: DiskPaths { devfs_path, dev_path }, - slot, + pcie_slot, variant, identity, is_boot_disk, @@ -259,8 +259,14 @@ impl UnparsedDisk { self.is_boot_disk } - pub fn slot(&self) -> i64 { - self.slot + /// The PCIe physical slot number of the bridge above this disk. + /// + /// 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. + pub fn pcie_slot(&self) -> i64 { + self.pcie_slot } pub fn firmware(&self) -> &DiskFirmware { @@ -285,7 +291,8 @@ impl UnparsedDisk { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct PooledDisk { pub paths: DiskPaths, - pub slot: i64, + /// See [`UnparsedDisk::pcie_slot`]. + pub pcie_slot: i64, pub identity: DiskIdentity, pub is_boot_disk: bool, pub partitions: Vec, @@ -328,7 +335,7 @@ impl PooledDisk { Ok(Self { paths: unparsed_disk.paths, - slot: unparsed_disk.slot, + pcie_slot: unparsed_disk.pcie_slot, identity: unparsed_disk.identity, is_boot_disk: unparsed_disk.is_boot_disk, partitions, diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index 05641b76f99..cbd2c039650 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -50,8 +50,8 @@ enum Error { #[error("Invalid value for boot-storage-unit property: {0}")] InvalidBootStorageUnitValue(i64), - #[error("Unrecognized slot for device {slot}")] - UnrecognizedSlot { slot: i64 }, + #[error("Unrecognized PCIe physical slot for device {pcie_slot}")] + UnrecognizedPcieSlot { pcie_slot: i64 }, #[error("Expected property {name} to have type {ty}")] UnexpectedPropertyType { name: String, ty: String }, @@ -208,27 +208,30 @@ impl HardwareView { } } -fn slot_to_disk_variant(sled: OxideSled, slot: i64) -> Option { - let u2_slots = sled.u2_disk_slots(); - let m2_slots = sled.m2_disk_slots(); - if u2_slots.contains(&slot) { +fn pcie_slot_to_disk_variant( + sled: OxideSled, + pcie_slot: i64, +) -> Option { + let u2_slots = sled.u2_pcie_slots(); + let m2_slots = sled.m2_pcie_slots(); + if u2_slots.contains(&pcie_slot) { Some(DiskVariant::U2) - } else if m2_slots.contains(&slot) { + } else if m2_slots.contains(&pcie_slot) { Some(DiskVariant::M2) } else { None } } -fn slot_is_boot_disk( +fn pcie_slot_is_boot_disk( sled: OxideSled, - slot: i64, + pcie_slot: i64, boot_storage_unit: BootStorageUnit, ) -> bool { - let slots = sled.bootdisk_slots(); + let slots = sled.bootdisk_pcie_slots(); match boot_storage_unit { - BootStorageUnit::A => slots[0] == slot, - BootStorageUnit::B => slots[1] == slot, + BootStorageUnit::A => slots[0] == pcie_slot, + BootStorageUnit::B => slots[1] == pcie_slot, } } @@ -450,14 +453,20 @@ fn poll_blkdev_node( // We expect that the parent of the "nvme" device is a "pcieb" driver. let pcieb_node = get_parent_node(&nvme_node, "pcieb")?; - // The "pcieb" device needs to have a physical slot for us to understand - // what type of disk it is. - let slot = i64_from_property( + // The "pcieb" device's PCIe physical slot number tells us which bay or + // M.2 socket the disk is in, and therefore what type of disk it is. The + // numbering is board-specific and internal to the PCIe topology; it is + // not the location label printed on the chassis. + let pcie_slot = i64_from_property( &find_properties(&pcieb_node, ["physical-slot#"])?[0], )?; - let Some(variant) = slot_to_disk_variant(sled, slot) else { - warn!(log, "Slot# {slot} is not recognized as a disk: {devfs_path}"); - return Err(Error::UnrecognizedSlot { slot }); + let Some(variant) = pcie_slot_to_disk_variant(sled, pcie_slot) else { + warn!( + log, + "PCIe physical slot {pcie_slot} is not recognized as a disk: \ + {devfs_path}" + ); + return Err(Error::UnrecognizedPcieSlot { pcie_slot }); }; let nvme = Nvme::new()?; @@ -490,10 +499,10 @@ fn poll_blkdev_node( let disk = UnparsedDisk::new( Utf8PathBuf::from(&devfs_path), dev_path, - slot, + pcie_slot, variant, device_id.clone(), - slot_is_boot_disk(sled, slot, boot_storage_unit), + pcie_slot_is_boot_disk(sled, pcie_slot, boot_storage_unit), firmware.clone(), ); disks.insert(device_id, disk); diff --git a/sled-hardware/types/src/lib.rs b/sled-hardware/types/src/lib.rs index 9e3ed2053f6..de2fe77f020 100644 --- a/sled-hardware/types/src/lib.rs +++ b/sled-hardware/types/src/lib.rs @@ -44,20 +44,32 @@ impl OxideSled { } } - pub fn m2_disk_slots(&self) -> RangeInclusive { + /// PCIe physical slot numbers of the M.2 boot devices. + /// + /// These are the `physical-slot#` values reported by the `pcieb` bridge + /// above each device. They are internal to the board's PCIe topology + /// and are not the labels printed on the chassis. + pub fn m2_pcie_slots(&self) -> RangeInclusive { match self { Self::Gimlet | Self::Cosmo => 0x11..=0x12, } } - pub fn u2_disk_slots(&self) -> RangeInclusive { + /// PCIe physical slot numbers of the U.2 bays. + /// + /// Same caveat as [`Self::m2_pcie_slots`]: these are board-internal PCIe + /// slot numbers, not chassis positions. Gimlet numbers its U.2 bays 0x00 + /// through 0x09 and Cosmo numbers the same ten bays 0x20 through 0x29. + pub fn u2_pcie_slots(&self) -> RangeInclusive { match self { Self::Gimlet => 0x00..=0x09, Self::Cosmo => 0x20..=0x29, } } - pub fn bootdisk_slots(&self) -> [i64; 2] { + /// PCIe physical slot numbers of boot storage units A and B, in that + /// order. + pub fn bootdisk_pcie_slots(&self) -> [i64; 2] { match self { Self::Gimlet | Self::Cosmo => [0x11, 0x12], } diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index 245a2ce5e68..134a52250f3 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -42,7 +42,7 @@ pub struct SyntheticDisk { // This shouldn't happen in prod, and is an unlikely test-only scenario, but // we'd still like to protect against it, since it could confuse the inventory // system. -const SYNTHETIC_SLOT_OFFSET: i64 = 1024; +const SYNTHETIC_PCIE_SLOT_OFFSET: i64 = 1024; // A generic name for the firmware in slot1 of an NVMe device. // @@ -100,7 +100,9 @@ pub struct RawSyntheticDisk { pub path: Utf8PathBuf, pub identity: DiskIdentity, pub variant: DiskVariant, - pub slot: i64, + /// A fake PCIe physical slot number, offset so it cannot collide with a + /// real disk. See [`UnparsedDisk::pcie_slot`]. + pub pcie_slot: i64, pub firmware: DiskFirmware, } @@ -110,18 +112,18 @@ impl RawSyntheticDisk { pub fn new_with_length>( vdev: P, length: u64, - slot: i64, + pcie_slot: i64, ) -> Result { let file = std::fs::File::create(vdev.as_ref())?; file.set_len(length)?; - Self::load(vdev, slot) + Self::load(vdev, pcie_slot) } /// Treats a file at path `vdev` as a synthetic disk. The file /// should already exist, and have the desired length. pub fn load>( vdev: P, - slot: i64, + pcie_slot: i64, ) -> Result { let path = vdev.as_ref(); let Some(file) = path.file_name() else { @@ -158,7 +160,7 @@ impl RawSyntheticDisk { path: path.into(), identity, variant, - slot: slot + SYNTHETIC_SLOT_OFFSET, + pcie_slot: pcie_slot + SYNTHETIC_PCIE_SLOT_OFFSET, firmware, }) } @@ -228,10 +230,10 @@ impl RawDisk { } } - pub fn slot(&self) -> i64 { + pub fn pcie_slot(&self) -> i64 { match self { - Self::Real(disk) => disk.slot(), - Self::Synthetic(disk) => disk.slot, + Self::Real(disk) => disk.pcie_slot(), + Self::Synthetic(disk) => disk.pcie_slot, } } @@ -374,10 +376,10 @@ impl Disk { } } - pub fn slot(&self) -> i64 { + pub fn pcie_slot(&self) -> i64 { match self { - Self::Real(disk) => disk.slot, - Self::Synthetic(disk) => disk.raw.slot, + Self::Real(disk) => disk.pcie_slot, + Self::Synthetic(disk) => disk.raw.pcie_slot, } } @@ -406,7 +408,7 @@ impl From for RawDisk { Disk::Real(pooled_disk) => RawDisk::Real(UnparsedDisk::new( pooled_disk.paths.devfs_path, pooled_disk.paths.dev_path, - pooled_disk.slot, + pooled_disk.pcie_slot, pooled_disk.zpool_name.kind().into(), pooled_disk.identity, pooled_disk.is_boot_disk, From 2417ea555322b3b83cb6c8c9c5b0e2cacf87af4c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 10 Sep 2026 18:10:17 -0700 Subject: [PATCH 03/19] Report each disk's chassis location from the hardware topology sled-hardware now asks libtopo where each NVMe controller sits in the chassis and records the platform's label ("N5" for a U.2 bay, "M.2 East" for a boot device) as a new location field on UnparsedDisk and PooledDisk. This is the operator-facing position printed on the sled, which the PCIe physical slot number that sled-agent already collects is not. The lookup mirrors nvmeadm: walk the hc scheme, find each nvme node by its io/instance property, and take its own label or, failing that, its parent's label when the parent is a bay or slot node. The devinfo nvme driver instance that sled-hardware already reads is the join key, now carried as an NvmeInstance newtype so it cannot be confused with the PCIe slot or NVMe firmware slots. A topo snapshot takes roughly half a second on a sled and the device tree is polled every five seconds, so DiskLocationCache remembers what topo said about each controller. Topo is consulted on the first poll after boot and again only when a controller appears that it has not been asked about. Controllers topo had no label for, and outright topo failures, are retried at most once a minute. A controller's location cannot change while the system is up (the instance number is fixed by devinfo path and the label by the static platform topology map), so learned labels are kept for the life of the process. Location is best-effort. A missing label, or a topo that cannot be read, is logged and leaves the field None; it never prevents a disk from being used. Synthetic and hardcoded disks are always None. The serde default on the field keeps existing sled-agent configs that list hardcoded disks deserializing. Nothing consumes the field yet; the sled-agent inventory API and the Nexus schema pick it up in the next commit. Test coverage: unit tests in sled-hardware::disk_location cover the cache decisions against a stand-in for the topo snapshot: first sight consults topo, steady state does not, unlabelled controllers are retried with the throttle, a new controller bypasses the throttle, and a topo failure keeps known labels and is throttled. The topo walk itself is illumos-only and was validated against a dogfood Gimlet and Cosmo by the libtopo disk_locations example it is ported from. Also cargo check --workspace --all-targets on Linux, cargo check --all-targets of sled-hardware, sled-storage, sled-agent-config-reconciler, omicron-sled-agent, and installinator on illumos, the sled-agent-config-reconciler test suite, cargo xtask clippy, and cargo fmt --check. --- .../config-reconciler/src/internal_disks.rs | 1 + .../src/reconciler_task/external_disks.rs | 1 + sled-hardware/src/disk.rs | 30 ++ sled-hardware/src/disk_location.rs | 338 ++++++++++++++++++ sled-hardware/src/illumos/mod.rs | 73 +++- sled-hardware/src/illumos/topo.rs | 110 ++++++ sled-hardware/src/lib.rs | 1 + sled-storage/src/disk.rs | 37 +- 8 files changed, 577 insertions(+), 14 deletions(-) create mode 100644 sled-hardware/src/disk_location.rs create mode 100644 sled-hardware/src/illumos/topo.rs diff --git a/sled-agent/config-reconciler/src/internal_disks.rs b/sled-agent/config-reconciler/src/internal_disks.rs index 968c846a69a..586e4009fa6 100644 --- a/sled-agent/config-reconciler/src/internal_disks.rs +++ b/sled-agent/config-reconciler/src/internal_disks.rs @@ -996,6 +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), partitions: vec![], zpool_name: ZpoolName::Internal(InternalZpoolUuid::new_v4()), firmware: raw_disk.firmware().clone(), diff --git a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs index 6f6ab98713a..6c0fcbc9978 100644 --- a/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs +++ b/sled-agent/config-reconciler/src/reconciler_task/external_disks.rs @@ -1121,6 +1121,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), partitions: vec![], zpool_name: ZpoolName::new_external(pool_id), firmware: raw_disk.firmware().clone(), diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index c3c397dc320..c73f6262508 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -217,6 +217,9 @@ pub struct UnparsedDisk { identity: DiskIdentity, is_boot_disk: bool, firmware: DiskFirmware, + /// See [`Self::location`]. + #[serde(default)] + location: Option, } impl UnparsedDisk { @@ -236,9 +239,16 @@ impl UnparsedDisk { identity, is_boot_disk, firmware, + location: None, } } + /// Records where this disk sits in the chassis. See [`Self::location`]. + pub fn with_location(mut self, location: Option) -> Self { + self.location = location; + self + } + pub fn paths(&self) -> &DiskPaths { &self.paths } @@ -269,6 +279,23 @@ impl UnparsedDisk { self.pcie_slot } + /// Where this disk sits in the chassis, as labelled by the platform's + /// 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() + } + + #[cfg(target_os = "illumos")] + pub(crate) fn set_location(&mut self, location: Option) { + self.location = location; + } + pub fn firmware(&self) -> &DiskFirmware { &self.firmware } @@ -300,6 +327,8 @@ pub struct PooledDisk { // disk. pub zpool_name: ZpoolName, pub firmware: DiskFirmware, + /// See [`UnparsedDisk::location`]. + pub location: Option, } impl PooledDisk { @@ -341,6 +370,7 @@ impl PooledDisk { partitions, zpool_name, firmware: unparsed_disk.firmware, + location: unparsed_disk.location, }) } } diff --git a/sled-hardware/src/disk_location.rs b/sled-hardware/src/disk_location.rs new file mode 100644 index 00000000000..6cc44eb021b --- /dev/null +++ b/sled-hardware/src/disk_location.rs @@ -0,0 +1,338 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Chassis locations of disks, learned from the platform hardware topology. +//! +//! The illumos fault management topology (libtopo) knows which bay or M.2 +//! socket each NVMe controller sits in and labels it the way the chassis +//! does: "N5" for a U.2 bay, "M.2 East" for a boot device. This module holds +//! the platform-independent parts of asking for that label: the key that +//! joins devinfo's view of a controller to topo's, and a cache that decides +//! when topo needs to be consulted at all. + +use slog::{Logger, debug, warn}; +use std::collections::HashMap; +use std::fmt; +use std::time::{Duration, Instant}; + +/// Instance number of an `nvme` driver node, the `N` in `nvme`. +/// +/// devinfo reports it on the `nvme` node and topo reports it as the +/// `io/instance` property of its `nvme` node, so it is the key that joins the +/// two views of a controller. It is unrelated to the PCIe physical slot +/// number and to NVMe firmware slots. +#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] +pub struct NvmeInstance(i32); + +/// An instance number outside the range devinfo and libnvme use, which is +/// zero through `i32::MAX`. +#[derive(Debug, thiserror::Error)] +#[error("invalid nvme driver instance number: {0}")] +pub struct InvalidNvmeInstance(pub i64); + +impl NvmeInstance { + /// The instance number as devinfo and libnvme represent it. + pub fn as_i32(self) -> i32 { + self.0 + } +} + +impl TryFrom for NvmeInstance { + type Error = InvalidNvmeInstance; + + /// From a devinfo instance number, which is signed but never negative. + fn try_from(value: i32) -> Result { + if value < 0 { + Err(InvalidNvmeInstance(value.into())) + } else { + Ok(Self(value)) + } + } +} + +impl TryFrom for NvmeInstance { + type Error = InvalidNvmeInstance; + + /// From a topo `io/instance` property, which is unsigned. + fn try_from(value: u32) -> Result { + i32::try_from(value) + .map(Self) + .map_err(|_| InvalidNvmeInstance(value.into())) + } +} + +impl fmt::Display for NvmeInstance { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "nvme{}", self.0) + } +} + +/// Remembers what the platform topology said about each NVMe controller, so +/// that topo, whose snapshot takes hundreds of milliseconds on a sled, is +/// consulted only when a controller appears that it has not been asked about. +/// +/// A controller's location cannot change while the system is up. Its instance +/// number is fixed by its devinfo path, and its label comes from the +/// platform's static topology map. A label, once learned, is therefore kept +/// for the life of the process. +#[derive(Debug, Default)] +pub struct DiskLocationCache { + /// What topo said about each controller the last time it was asked. + /// `None` means it was asked and had no label, or topo could not be read. + /// A controller absent from the map has never been asked about. + labels: HashMap>, + /// When topo was last consulted. Throttles re-asking about `None` + /// entries; never delays the first question about a new controller. + last_attempt: Option, +} + +impl DiskLocationCache { + /// Minimum time between repeated attempts to resolve a controller that + /// topo previously had no label for. + pub const RETRY_INTERVAL: Duration = Duration::from_secs(60); + + pub fn new() -> Self { + Self::default() + } + + /// Consults topo, through `read_labels`, if any controller in `present` + /// calls for it: immediately for a controller never asked about, and at + /// most once per [`Self::RETRY_INTERVAL`] for controllers topo previously + /// had no label for. Afterwards every controller in `present` has an + /// entry, so the steady state of repeated polls with the same controllers + /// never touches topo. + /// + /// `read_labels` returns the label of every controller topo has one for. + /// On success it replaces the cache wholesale. On failure the labels + /// already known are kept and the failure is logged. + pub fn refresh_if_needed( + &mut self, + log: &Logger, + present: &[NvmeInstance], + now: Instant, + read_labels: F, + ) where + F: FnOnce() -> Result, E>, + E: fmt::Display, + { + if !self.should_refresh(present, now) { + return; + } + self.last_attempt = Some(now); + + let read_ok = match read_labels() { + Ok(labels) => { + debug!( + log, + "read disk locations from hardware topology"; + "count" => labels.len(), + ); + self.labels = + labels.into_iter().map(|(i, l)| (i, Some(l))).collect(); + true + } + Err(err) => { + warn!( + log, + "failed to read disk locations from hardware topology"; + "err" => %err, + ); + false + } + }; + + for instance in present { + let entry = self.labels.entry(*instance).or_insert(None); + if entry.is_none() && read_ok { + warn!( + log, + "hardware topology has no location label for disk \ + controller"; + "nvme_instance" => %instance, + ); + } + } + } + + fn should_refresh(&self, present: &[NvmeInstance], now: Instant) -> bool { + let mut retry_unlabelled = false; + for instance in present { + match self.labels.get(instance) { + None => return true, + Some(None) => retry_unlabelled = true, + Some(Some(_)) => {} + } + } + retry_unlabelled + && self.last_attempt.is_none_or(|last| { + now.duration_since(last) >= Self::RETRY_INTERVAL + }) + } + + /// The chassis location label of a controller, if topo had one. + pub fn location(&self, instance: NvmeInstance) -> Option<&str> { + self.labels.get(&instance).and_then(|label| label.as_deref()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use omicron_test_utils::dev::test_setup_log; + use std::cell::Cell; + + fn inst(n: i32) -> NvmeInstance { + NvmeInstance::try_from(n).unwrap() + } + + fn labels(pairs: &[(i32, &str)]) -> HashMap { + pairs.iter().map(|(n, l)| (inst(*n), l.to_string())).collect() + } + + #[test] + fn nvme_instance_conversions() { + assert_eq!(inst(0).as_i32(), 0); + assert_eq!(inst(17).as_i32(), 17); + assert!(NvmeInstance::try_from(-1i32).is_err()); + assert_eq!(NvmeInstance::try_from(3u32).unwrap(), inst(3)); + assert!(NvmeInstance::try_from(u32::MAX).is_err()); + assert_eq!(inst(5).to_string(), "nvme5"); + } + + #[test] + fn first_sight_consults_topo_then_steady_state_does_not() { + let logctx = test_setup_log( + "first_sight_consults_topo_then_steady_state_does_not", + ); + let mut cache = DiskLocationCache::new(); + let calls = Cell::new(0); + let t0 = Instant::now(); + let present = [inst(0), inst(1)]; + + cache.refresh_if_needed(&logctx.log, &present, t0, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) + }); + assert_eq!(calls.get(), 1); + assert_eq!(cache.location(inst(0)), Some("N0")); + assert_eq!(cache.location(inst(1)), Some("N1")); + + // Same controllers, an hour later: nothing to ask. + let later = t0 + Duration::from_secs(3600); + cache.refresh_if_needed(&logctx.log, &present, later, || { + calls.set(calls.get() + 1); + Ok::<_, String>(HashMap::new()) + }); + assert_eq!(calls.get(), 1); + assert_eq!(cache.location(inst(0)), Some("N0")); + logctx.cleanup_successful(); + } + + #[test] + fn unlabelled_controller_is_retried_with_throttle() { + let logctx = + test_setup_log("unlabelled_controller_is_retried_with_throttle"); + let mut cache = DiskLocationCache::new(); + let calls = Cell::new(0); + let t0 = Instant::now(); + let present = [inst(0)]; + + // Topo knows nothing about this controller. + cache.refresh_if_needed(&logctx.log, &present, t0, || { + calls.set(calls.get() + 1); + Ok::<_, String>(HashMap::new()) + }); + assert_eq!(calls.get(), 1); + assert_eq!(cache.location(inst(0)), None); + + // Too soon to ask again. + let soon = t0 + Duration::from_secs(30); + cache.refresh_if_needed(&logctx.log, &present, soon, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0")])) + }); + assert_eq!(calls.get(), 1); + assert_eq!(cache.location(inst(0)), None); + + // After the retry interval the question is asked again. + let retry = t0 + DiskLocationCache::RETRY_INTERVAL; + cache.refresh_if_needed(&logctx.log, &present, retry, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0")])) + }); + assert_eq!(calls.get(), 2); + assert_eq!(cache.location(inst(0)), Some("N0")); + logctx.cleanup_successful(); + } + + #[test] + fn new_controller_bypasses_throttle() { + let logctx = test_setup_log("new_controller_bypasses_throttle"); + let mut cache = DiskLocationCache::new(); + let calls = Cell::new(0); + let t0 = Instant::now(); + + cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { + calls.set(calls.get() + 1); + Ok::<_, String>(HashMap::new()) + }); + assert_eq!(calls.get(), 1); + + // A controller never seen before shows up well inside the retry + // interval; it is asked about right away. + let soon = t0 + Duration::from_secs(5); + cache.refresh_if_needed(&logctx.log, &[inst(0), inst(1)], soon, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(1, "N1")])) + }); + assert_eq!(calls.get(), 2); + assert_eq!(cache.location(inst(0)), None); + assert_eq!(cache.location(inst(1)), Some("N1")); + logctx.cleanup_successful(); + } + + #[test] + fn topo_failure_keeps_known_labels_and_is_throttled() { + let logctx = + test_setup_log("topo_failure_keeps_known_labels_and_is_throttled"); + let mut cache = DiskLocationCache::new(); + let calls = Cell::new(0); + let t0 = Instant::now(); + + cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0")])) + }); + assert_eq!(calls.get(), 1); + + // A new controller appears but topo is broken. + let t1 = t0 + Duration::from_secs(1); + let present = [inst(0), inst(1)]; + cache.refresh_if_needed(&logctx.log, &present, t1, || { + calls.set(calls.get() + 1); + Err::, _>("topo exploded") + }); + assert_eq!(calls.get(), 2); + assert_eq!(cache.location(inst(0)), Some("N0")); + assert_eq!(cache.location(inst(1)), None); + + // The failure is throttled like any other unresolved controller. + let t2 = t1 + Duration::from_secs(2); + cache.refresh_if_needed(&logctx.log, &present, t2, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) + }); + assert_eq!(calls.get(), 2); + assert_eq!(cache.location(inst(1)), None); + + let t3 = t1 + DiskLocationCache::RETRY_INTERVAL; + cache.refresh_if_needed(&logctx.log, &present, t3, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) + }); + assert_eq!(calls.get(), 3); + assert_eq!(cache.location(inst(1)), Some("N1")); + logctx.cleanup_successful(); + } +} diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index cbd2c039650..cc316eb9924 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -7,6 +7,7 @@ use crate::ExternalDisks; use crate::HardwareView; use crate::TofinoSnapshot; use crate::TofinoView; +use crate::disk_location::{DiskLocationCache, NvmeInstance}; use crate::{DendriteAsic, SledMode, UnparsedDisk}; use camino::Utf8PathBuf; use gethostname::gethostname; @@ -22,9 +23,12 @@ use slog::info; use slog::o; use slog::warn; use std::collections::{HashMap, HashSet}; +use std::time::Instant; use tokio::sync::watch; use uuid::Uuid; +mod topo; + mod gpt; mod partitions; mod sysconf; @@ -47,6 +51,13 @@ enum Error { #[error("Node {node} missing device property {name}")] MissingDeviceProperty { node: String, name: String }, + #[error("Node {node} has an invalid nvme instance")] + InvalidNvmeInstance { + node: String, + #[source] + err: crate::disk_location::InvalidNvmeInstance, + }, + #[error("Invalid value for boot-storage-unit property: {0}")] InvalidBootStorageUnitValue(i64), @@ -128,6 +139,7 @@ impl HardwareSnapshot { fn new( log: &Logger, external_disks: &ExternalDisks, + location_cache: &mut DiskLocationCache, ) -> Result { let mut device_info = DevInfo::new_force_load().map_err(Error::DevInfo)?; @@ -160,6 +172,7 @@ impl HardwareSnapshot { let mut disks = HashMap::new(); match external_disks { ExternalDisks::DetectPhysical => { + let mut nvme_instances = HashMap::new(); let mut node_walker = device_info.walk_driver("blkdev"); while let Some(node) = node_walker.next().transpose().map_err(Error::DevInfo)? @@ -168,10 +181,32 @@ impl HardwareSnapshot { &log, sled_type, &mut disks, + &mut nvme_instances, node, boot_storage_unit, )?; } + + // Now that we know which controllers are present, ask the + // hardware topology where they are. The cache decides whether + // topo actually needs to be consulted this time around. + let present: Vec = + nvme_instances.values().copied().collect(); + location_cache.refresh_if_needed( + log, + &present, + Instant::now(), + || topo::read_disk_locations(log), + ); + for (identity, instance) in &nvme_instances { + if let Some(disk) = disks.get_mut(identity) { + disk.set_location( + location_cache + .location(*instance) + .map(str::to_string), + ); + } + } } ExternalDisks::Hardcoded { vdevs: _, disks: hardcoded_disks } => { @@ -389,6 +424,7 @@ fn poll_blkdev_node( log: &Logger, sled: OxideSled, disks: &mut HashMap, + nvme_instances: &mut HashMap, node: Node<'_>, boot_storage_unit: BootStorageUnit, ) -> Result<(), Error> { @@ -430,6 +466,10 @@ fn poll_blkdev_node( let nvme_instance = nvme_node .instance() .ok_or(Error::MissingNvmeDevinfoInstance { node: node.node_name() })?; + let nvme_instance = + NvmeInstance::try_from(nvme_instance).map_err(|err| { + Error::InvalidNvmeInstance { node: node.node_name(), err } + })?; let vendor_id = i64_from_property(&find_properties(&nvme_node, ["vendor-id"])?[0])?; @@ -470,7 +510,8 @@ fn poll_blkdev_node( }; let nvme = Nvme::new()?; - let controller = Controller::init_by_instance(&nvme, nvme_instance)?; + let controller = + Controller::init_by_instance(&nvme, nvme_instance.as_i32())?; let controller_lock = match controller.try_read_lock() { libnvme::controller::TryLockResult::Ok(locked) => locked, // We should only hit this if something in the system has locked the @@ -505,6 +546,9 @@ fn poll_blkdev_node( pcie_slot_is_boot_disk(sled, pcie_slot, boot_storage_unit), firmware.clone(), ); + // The location is filled in once every controller has been found; see + // `HardwareSnapshot::new`. + nvme_instances.insert(device_id.clone(), nvme_instance); disks.insert(device_id, disk); Ok(()) } @@ -550,9 +594,11 @@ fn poll_device_tree( log: &Logger, hardware_view_tx: &watch::Sender, external_disks: &ExternalDisks, + location_cache: &mut DiskLocationCache, ) -> Result<(), Error> { // Construct a view of hardware by walking the device tree. - let polled_hw = match HardwareSnapshot::new(log, external_disks) { + let polled_hw = HardwareSnapshot::new(log, external_disks, location_cache); + let polled_hw = match polled_hw { Ok(polled_hw) => polled_hw, Err(e) => { @@ -768,9 +814,15 @@ fn hardware_tracking_task( log: Logger, hardware_view_tx: watch::Sender, external_disks: ExternalDisks, + mut location_cache: DiskLocationCache, ) { loop { - match poll_device_tree(&log, &hardware_view_tx, &external_disks) { + match poll_device_tree( + &log, + &hardware_view_tx, + &external_disks, + &mut location_cache, + ) { // We've already warned about `NotAnOxideSled` by this point, // so let's not spam the logs. Ok(_) | Err(Error::NotAnOxideSled(_)) => (), @@ -864,7 +916,13 @@ impl HardwareManager { // This mitigates issues where the Sled Agent could try to propagate // an "empty" view of hardware to other consumers before the first // query. - match poll_device_tree(&log, &hardware_view_tx, &external_disks) { + let mut location_cache = DiskLocationCache::new(); + match poll_device_tree( + &log, + &hardware_view_tx, + &external_disks, + &mut location_cache, + ) { Ok(_) => (), // Allow non-sled devices to proceed with a "null" view of // hardware, otherwise they won't be able to start. @@ -891,7 +949,12 @@ impl HardwareManager { }); std::thread::spawn(move || { - hardware_tracking_task(log, hardware_view_tx, external_disks); + hardware_tracking_task( + log, + hardware_view_tx, + external_disks, + location_cache, + ); }); Ok(Self { hardware_view_rx }) diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs new file mode 100644 index 00000000000..131b083e9c8 --- /dev/null +++ b/sled-hardware/src/illumos/topo.rs @@ -0,0 +1,110 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Reading disk chassis locations from the illumos hardware topology. + +use crate::disk_location::NvmeInstance; +use libtopo::{Node, PropValue, Scheme, TopoHdl, WalkAction}; +use slog::{Logger, debug, warn}; +use std::collections::HashMap; +use std::time::Instant; + +// Node and property names from . +const NVME: &str = "nvme"; +const BAY: &str = "bay"; +const SLOT: &str = "slot"; +const TOPO_PGROUP_IO: &str = "io"; +const TOPO_IO_INSTANCE: &str = "instance"; + +/// Takes a fresh topology snapshot and returns the chassis label of every +/// NVMe controller that has one, keyed by driver instance. +/// +/// This mirrors what `nvmeadm list -L` does. Each `nvme` node in the `hc` +/// scheme is located by its `io/instance` property. Its label is its own +/// `protocol/label` if it has one, else its parent's label if the parent is +/// a `bay` (a U.2 bay) or a `slot` (an M.2 socket). Controllers with neither +/// are omitted from the result. +/// +/// A new handle is opened on every call. Taking a second snapshot on the same +/// handle is unsafe in libtopo (illumos issue 18110), and taking the snapshot +/// is what enumerates the hardware. +pub(super) fn read_disk_locations( + log: &Logger, +) -> Result, libtopo::Error> { + let start = Instant::now(); + let hdl = TopoHdl::open()?; + let snap = hdl.snapshot()?; + + let mut labels = HashMap::new(); + snap.walk(Scheme::Hc, |node| { + if node.name() != NVME { + return Ok(WalkAction::Continue); + } + + let instance = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { + Ok(PropValue::UInt32(value)) => match NvmeInstance::try_from(value) + { + Ok(instance) => instance, + Err(err) => { + warn!( + log, + "ignoring nvme topology node with unusable instance"; + "err" => %err, + ); + return Ok(WalkAction::Continue); + } + }, + Ok(other) => { + warn!( + log, + "ignoring nvme topology node whose io/instance is not a \ + uint32"; + "value" => ?other, + ); + return Ok(WalkAction::Continue); + } + Err(_) => { + debug!( + log, + "nvme topology node has no io/instance property"; + "node_instance" => node.instance(), + ); + return Ok(WalkAction::Continue); + } + }; + + let label = label_of(&node).or_else(|| { + node.parent() + .filter(|parent| { + let name = parent.name(); + name == BAY || name == SLOT + }) + .and_then(|parent| label_of(&parent)) + }); + match label { + Some(label) => { + labels.insert(instance, label); + } + None => debug!( + log, + "nvme topology node has no location label"; + "nvme_instance" => %instance, + ), + } + Ok(WalkAction::Continue) + })?; + + debug!( + log, + "read disk locations from hardware topology"; + "count" => labels.len(), + "elapsed" => ?start.elapsed(), + ); + Ok(labels) +} + +/// A node's label, treating "no label" and an empty label alike. +fn label_of(node: &Node<'_>) -> Option { + node.label().ok().filter(|label| !label.is_empty()) +} diff --git a/sled-hardware/src/lib.rs b/sled-hardware/src/lib.rs index d0ad7db5e6b..f407b0fa4a8 100644 --- a/sled-hardware/src/lib.rs +++ b/sled-hardware/src/lib.rs @@ -23,6 +23,7 @@ cfg_if::cfg_if! { pub mod cleanup; pub mod disk; pub use disk::*; +pub mod disk_location; pub mod underlay; // The type of networking 'ASIC' the Dendrite service is expected to manage diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index 134a52250f3..07662c2d4c0 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -237,6 +237,14 @@ impl RawDisk { } } + /// See [`UnparsedDisk::location`]. + pub fn location(&self) -> Option<&str> { + match self { + Self::Real(disk) => disk.location(), + Self::Synthetic(_) => None, + } + } + pub fn firmware(&self) -> &DiskFirmware { match self { RawDisk::Real(unparsed) => unparsed.firmware(), @@ -383,6 +391,14 @@ impl Disk { } } + /// See [`UnparsedDisk::location`]. + pub fn location(&self) -> Option<&str> { + match self { + Self::Real(disk) => disk.location.as_deref(), + Self::Synthetic(_) => None, + } + } + pub fn update_firmware_metadata(&mut self, raw_disk: &RawDisk) { match self { Disk::Real(pooled_disk) => { @@ -405,15 +421,18 @@ impl Disk { impl From for RawDisk { fn from(disk: Disk) -> RawDisk { match disk { - Disk::Real(pooled_disk) => RawDisk::Real(UnparsedDisk::new( - pooled_disk.paths.devfs_path, - pooled_disk.paths.dev_path, - pooled_disk.pcie_slot, - pooled_disk.zpool_name.kind().into(), - pooled_disk.identity, - pooled_disk.is_boot_disk, - pooled_disk.firmware, - )), + Disk::Real(pooled_disk) => RawDisk::Real( + UnparsedDisk::new( + pooled_disk.paths.devfs_path, + pooled_disk.paths.dev_path, + pooled_disk.pcie_slot, + pooled_disk.zpool_name.kind().into(), + pooled_disk.identity, + pooled_disk.is_boot_disk, + pooled_disk.firmware, + ) + .with_location(pooled_disk.location), + ), Disk::Synthetic(synthetic_disk) => { RawDisk::Synthetic(synthetic_disk.raw) } From 1f8a88b9013a08e66ce602602feb95387dfc53b3 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 08:53:07 -0700 Subject: [PATCH 04/19] Carry disk chassis location through the sled-agent API into inventory Sled Agent API version 54 (ADD_DISK_LOCATION_TO_INVENTORY) adds a location field to InventoryDisk and renames its slot field to pcie_slot, so callers can tell the chassis label apart from the PCIe physical slot number. Older clients keep receiving the previous shape through the usual conversion chain. Nexus records the label as a new nullable location column on inv_physical_disk (schema 300), shows it in the inventory display when present, and prints it in omdb's physical-disks table. The inventory column that holds the PCIe slot number keeps its name "slot" in the database, since columns cannot be renamed idempotently; the Rust field is pcie_slot via a diesel column_name attribute, and the dbinit.sql comments now say what the number actually is. The simulated sled-agent reports no location. Nothing in the external API changes here; the unadopted-disk view continues to expose the PCIe slot number under the name slot and is addressed separately. Test coverage: the OpenAPI manager check confirms version 54 is the only new document and blessed versions are untouched; the diff against version 53 is exactly the InventoryDisk field changes. The schema suite in omicron-nexus passes, including idempotent re-application of the new migration and the dbinit-versus-migrations comparison. The nexus-db-model migration verification test confirms no backfill verification file is needed for a nullable column. Inventory and physical-disk datastore tests in nexus-db-queries pass against a test CockroachDB. Also cargo check --workspace --all-targets on Linux, cargo check --all-targets of the sled-agent crates on illumos, the sled-agent-config-reconciler and sled-hardware suites, cargo xtask clippy, and cargo fmt --check. --- dev-tools/omdb/src/bin/omdb/db.rs | 7 +- nexus/db-model/src/inventory.rs | 21 ++- nexus/db-model/src/schema_versions.rs | 3 +- .../db-queries/src/db/datastore/inventory.rs | 14 +- .../src/db/datastore/physical_disk.rs | 6 +- nexus/db-schema/src/schema.rs | 2 + nexus/inventory/src/examples.rs | 12 +- nexus/reconfigurator/planning/src/system.rs | 3 +- nexus/src/external_api/http_entrypoints.rs | 2 +- nexus/types/src/inventory.rs | 12 +- nexus/types/src/inventory/display.rs | 9 +- .../sled-agent-53.0.0-84b2d6.json.gitstub | 1 + ...2d6.json => sled-agent-54.0.0-c5ef76.json} | 12 +- openapi/sled-agent/sled-agent-latest.json | 2 +- schema/crdb/dbinit.sql | 21 ++- schema/crdb/inv-physical-disk-location/up.sql | 1 + sled-agent/api/src/lib.rs | 22 ++- sled-agent/config-reconciler/src/raw_disks.rs | 3 +- sled-agent/rack-setup/src/plan/service.rs | 3 +- sled-agent/rack-setup/src/service.rs | 3 +- sled-agent/src/sim/sled_agent.rs | 3 +- .../inventory.rs | 158 ++++++++++++++++++ .../src/add_disk_location_to_inventory/mod.rs | 12 ++ sled-agent/types/versions/src/latest.rs | 5 +- sled-agent/types/versions/src/lib.rs | 2 + 25 files changed, 294 insertions(+), 45 deletions(-) create mode 100644 openapi/sled-agent/sled-agent-53.0.0-84b2d6.json.gitstub rename openapi/sled-agent/{sled-agent-53.0.0-84b2d6.json => sled-agent-54.0.0-c5ef76.json} (99%) create mode 100644 schema/crdb/inv-physical-disk-location/up.sql create mode 100644 sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs create mode 100644 sled-agent/types/versions/src/add_disk_location_to_inventory/mod.rs diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index d02b22d6ce1..01177142c7f 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -7581,7 +7581,9 @@ async fn cmd_db_inventory_physical_disks( struct DiskRow { inv_collection_id: Uuid, sled_id: Uuid, - slot: i64, + pcie_slot: i64, + #[tabled(display_with = "display_option_blank")] + location: Option, vendor: String, model: String, serial: String, @@ -7634,7 +7636,8 @@ async fn cmd_db_inventory_physical_disks( DiskRow { inv_collection_id: disk.inv_collection_id.into_untyped_uuid(), sled_id: disk.sled_id.into_untyped_uuid(), - slot: disk.slot, + pcie_slot: disk.pcie_slot, + location: disk.location.clone(), vendor: disk.vendor, model: disk.model.clone(), serial: disk.serial.clone(), diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index b9b127ae404..89920e801dc 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -2359,11 +2359,17 @@ impl From for SvcEnabledNotOnlineState { pub struct InvPhysicalDisk { pub inv_collection_id: DbTypedUuid, pub sled_id: DbTypedUuid, - pub slot: i64, + /// See [`nexus_types::inventory::PhysicalDisk::pcie_slot`]. The column + /// keeps its original name because columns cannot be renamed + /// idempotently. + #[diesel(column_name = slot)] + pub pcie_slot: i64, pub vendor: String, pub model: String, pub serial: String, pub variant: PhysicalDiskKind, + /// See [`nexus_types::inventory::PhysicalDisk::location`]. + pub location: Option, } impl InvPhysicalDisk { @@ -2375,11 +2381,12 @@ impl InvPhysicalDisk { Self { inv_collection_id: inv_collection_id.into(), sled_id: sled_id.into(), - slot: disk.pcie_slot, + pcie_slot: disk.pcie_slot, vendor: disk.identity.vendor, model: disk.identity.model, serial: disk.identity.serial, variant: disk.variant.into(), + location: disk.location, } } } @@ -2410,7 +2417,9 @@ pub enum InvNvmeDiskFirmwareError { pub struct InvNvmeDiskFirmware { inv_collection_id: DbTypedUuid, sled_id: DbTypedUuid, - slot: i64, + /// See [`InvPhysicalDisk::pcie_slot`]. + #[diesel(column_name = slot)] + pcie_slot: i64, active_slot: SqlU8, next_active_slot: Option, number_of_slots: SqlU8, @@ -2496,7 +2505,7 @@ impl InvNvmeDiskFirmware { Ok(Self { inv_collection_id: inv_collection_id.into(), sled_id: sled_id.into(), - slot: sled_slot, + pcie_slot: sled_slot, active_slot: firmware.active_slot.into(), next_active_slot: firmware.next_active_slot.map(|nas| nas.into()), number_of_slots: firmware.number_of_slots.into(), @@ -2538,8 +2547,8 @@ impl InvNvmeDiskFirmware { self.sled_id } - pub fn slot(&self) -> i64 { - self.slot + pub fn pcie_slot(&self) -> i64 { + self.pcie_slot } pub fn number_of_slots(&self) -> SqlU8 { diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index aebf8c9a43e..63284467888 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/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(299, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(300, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock> = 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(300, "inv-physical-disk-location"), KnownVersion::new(299, "blueprint-pruner-config"), KnownVersion::new(298, "blueprint-zone-multiple-external-ips"), KnownVersion::new(297, "inventory-zone-multiple-external-ips"), diff --git a/nexus/db-queries/src/db/datastore/inventory.rs b/nexus/db-queries/src/db/datastore/inventory.rs index 915d0894cae..102b460a398 100644 --- a/nexus/db-queries/src/db/datastore/inventory.rs +++ b/nexus/db-queries/src/db/datastore/inventory.rs @@ -3092,14 +3092,15 @@ impl DataStore { .map_err(|e| { public_error_from_diesel(e, ErrorHandler::Server) })?; - paginator = - p.found_batch(&batch, &|row| (row.sled_id(), row.slot())); + paginator = p.found_batch(&batch, &|row| { + (row.sled_id(), row.pcie_slot()) + }); for firmware in batch { disk_firmware .entry(firmware.sled_id().into()) .or_default() .insert( - firmware.slot(), + firmware.pcie_slot(), nexus_types::inventory::PhysicalDiskFirmware::from( firmware, ), @@ -3138,12 +3139,12 @@ impl DataStore { public_error_from_diesel(e, ErrorHandler::Server) })?; paginator = - p.found_batch(&batch, &|row| (row.sled_id, row.slot)); + p.found_batch(&batch, &|row| (row.sled_id, row.pcie_slot)); for disk in batch { let sled_id = disk.sled_id.into(); let firmware = disk_firmware .get(&sled_id) - .and_then(|lookup| lookup.get(&disk.slot)) + .and_then(|lookup| lookup.get(&disk.pcie_slot)) .unwrap_or(&nexus_types::inventory::PhysicalDiskFirmware::Unknown); disks.entry(sled_id).or_default().push( @@ -3154,7 +3155,8 @@ impl DataStore { serial: disk.serial, }, variant: disk.variant.into(), - pcie_slot: disk.slot, + pcie_slot: disk.pcie_slot, + location: disk.location, firmware: firmware.clone(), }, ); diff --git a/nexus/db-queries/src/db/datastore/physical_disk.rs b/nexus/db-queries/src/db/datastore/physical_disk.rs index 35a80fa9007..a163680e6e8 100644 --- a/nexus/db-queries/src/db/datastore/physical_disk.rs +++ b/nexus/db-queries/src/db/datastore/physical_disk.rs @@ -1155,7 +1155,8 @@ mod test { model: "model".to_string(), }, variant: DiskVariant::U2, - slot, + pcie_slot: slot, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, @@ -1183,7 +1184,8 @@ mod test { serial: inv_phys_disk.serial.clone(), }, variant: inv_phys_disk.variant.into(), - pcie_slot: inv_phys_disk.slot, + pcie_slot: inv_phys_disk.pcie_slot, + location: inv_phys_disk.location.clone(), firmware: PhysicalDiskFirmware::Nvme(NvmeFirmware { active_slot: inv_disk.active_firmware_slot, next_active_slot: inv_disk.next_active_firmware_slot, diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index df6783939fc..b785077fec4 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -2078,6 +2078,8 @@ table! { serial -> Text, variant -> crate::enums::PhysicalDiskKindEnum, + + location -> Nullable, } } diff --git a/nexus/inventory/src/examples.rs b/nexus/inventory/src/examples.rs index d8aeb034af5..911d176024a 100644 --- a/nexus/inventory/src/examples.rs +++ b/nexus/inventory/src/examples.rs @@ -487,7 +487,8 @@ pub fn representative() -> Representative { serial: "XXIV".to_string(), }, variant: DiskVariant::M2, - slot: 0, + pcie_slot: 0, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, @@ -502,7 +503,8 @@ pub fn representative() -> Representative { serial: "0001".to_string(), }, variant: DiskVariant::U2, - slot: 1, + pcie_slot: 1, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, @@ -516,7 +518,8 @@ pub fn representative() -> Representative { serial: "0002".to_string(), }, variant: DiskVariant::U2, - slot: 2, + pcie_slot: 2, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, @@ -530,7 +533,8 @@ pub fn representative() -> Representative { serial: "5".to_string(), }, variant: DiskVariant::U2, - slot: 3, + pcie_slot: 3, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index a2ad5061786..22a09ed064b 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -1535,7 +1535,8 @@ impl Sled { .map(|(i, disk)| InventoryDisk { identity: disk.disk_identity.clone(), variant: DiskVariant::U2, - slot: i64::try_from(i).unwrap(), + pcie_slot: i64::try_from(i).unwrap(), + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 1046148aca1..993324e2b97 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -6903,7 +6903,7 @@ impl NexusExternalApi for NexusExternalApiImpl { .into_iter() .map(|d| UnadoptedPhysicalDisk { sled_id: d.sled_id.into(), - slot: d.slot, + slot: d.pcie_slot, variant: d.variant.into(), disk_id: PhysicalDiskManufacturerIdentity { vendor: d.vendor, diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index ed3acafef42..b730fe71fff 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -613,8 +613,15 @@ pub struct PhysicalDisk { /// This is the `physical-slot#` of the parent `pcieb` device. It is /// internal to the board's PCIe topology and board-specific: the same /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the - /// location label printed on the chassis. + /// location label printed on the chassis; see `location` for that. pub pcie_slot: i64, + /// Where this disk sits in the chassis, as labelled by the platform's + /// 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. + pub location: Option, pub firmware: PhysicalDiskFirmware, } @@ -623,7 +630,8 @@ impl From for PhysicalDisk { PhysicalDisk { identity: disk.identity, variant: disk.variant.into(), - pcie_slot: disk.slot, + pcie_slot: disk.pcie_slot, + location: disk.location, firmware: PhysicalDiskFirmware::Nvme(NvmeFirmware { active_slot: disk.active_firmware_slot, next_active_slot: disk.next_active_firmware_slot, diff --git a/nexus/types/src/inventory/display.rs b/nexus/types/src/inventory/display.rs index ff13e25141d..0ded57c1de5 100644 --- a/nexus/types/src/inventory/display.rs +++ b/nexus/types/src/inventory/display.rs @@ -666,9 +666,14 @@ fn display_sleds( writeln!(indented, "physical disks:")?; } for disk in disks { - let PhysicalDisk { identity, variant, pcie_slot, .. } = disk; + let PhysicalDisk { identity, variant, pcie_slot, location, .. } = + disk; let mut indent2 = IndentWriter::new(" ", &mut indented); - writeln!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?; + write!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?; + if let Some(location) = location { + write!(indent2, " ({location})")?; + } + writeln!(indent2)?; } if !zpools.is_empty() { diff --git a/openapi/sled-agent/sled-agent-53.0.0-84b2d6.json.gitstub b/openapi/sled-agent/sled-agent-53.0.0-84b2d6.json.gitstub new file mode 100644 index 00000000000..d811a99ab0f --- /dev/null +++ b/openapi/sled-agent/sled-agent-53.0.0-84b2d6.json.gitstub @@ -0,0 +1 @@ +7da3cfe979152fe9680ab36e5f0d4b424a5dfa86:openapi/sled-agent/sled-agent-53.0.0-84b2d6.json diff --git a/openapi/sled-agent/sled-agent-53.0.0-84b2d6.json b/openapi/sled-agent/sled-agent-54.0.0-c5ef76.json similarity index 99% rename from openapi/sled-agent/sled-agent-53.0.0-84b2d6.json rename to openapi/sled-agent/sled-agent-54.0.0-c5ef76.json index e3ed57115f5..6305793621b 100644 --- a/openapi/sled-agent/sled-agent-53.0.0-84b2d6.json +++ b/openapi/sled-agent/sled-agent-54.0.0-c5ef76.json @@ -7,7 +7,7 @@ "url": "https://oxide.computer", "email": "api@oxide.computer" }, - "version": "53.0.0" + "version": "54.0.0" }, "paths": { "/artifacts": { @@ -6441,6 +6441,11 @@ "identity": { "$ref": "#/components/schemas/DiskIdentity" }, + "location": { + "nullable": true, + "description": "Where this disk sits in the chassis, as labelled by the platform's hardware topology: \"N5\" for a U.2 bay, \"M.2 East\" for a boot device.\n\nThis is the operator-facing position, matching what is printed on the sled. It is best-effort: `None` means the topology had no label for the disk or could not be read.", + "type": "string" + }, "next_active_firmware_slot": { "nullable": true, "type": "integer", @@ -6452,7 +6457,8 @@ "format": "uint8", "minimum": 0 }, - "slot": { + "pcie_slot": { + "description": "PCIe physical slot number of the bridge above this disk.\n\nThis is the `physical-slot#` of the parent `pcieb` device. It is internal to the board's PCIe topology and board-specific: the same U.2 bay is numbered differently on Gimlet and Cosmo. It is not the location label printed on the chassis; see `location` for that.", "type": "integer", "format": "int64" }, @@ -6474,7 +6480,7 @@ "active_firmware_slot", "identity", "number_of_firmware_slots", - "slot", + "pcie_slot", "slot1_is_read_only", "slot_firmware_versions", "variant" diff --git a/openapi/sled-agent/sled-agent-latest.json b/openapi/sled-agent/sled-agent-latest.json index da5598896a7..34505f7c7d6 120000 --- a/openapi/sled-agent/sled-agent-latest.json +++ b/openapi/sled-agent/sled-agent-latest.json @@ -1 +1 @@ -sled-agent-53.0.0-84b2d6.json \ No newline at end of file +sled-agent-54.0.0-c5ef76.json \ No newline at end of file diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index e5b6662d4fd..ecd4da383c8 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4753,7 +4753,12 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_physical_disk ( -- unique id for this sled (should be foreign keys into `sled` table, though -- it's conceivable a sled will report an id that we don't know about) sled_id UUID NOT NULL, - -- The slot where this disk was last observed + -- The PCIe physical slot number of the bridge above this disk, as + -- reported by the sled. This is internal to the board's PCIe topology and + -- board-specific (the same U.2 bay is numbered differently on Gimlet and + -- Cosmo); it is not the location printed on the chassis. See `location` + -- for that. The column is named "slot" for historical reasons; the + -- corresponding Rust field is `pcie_slot`. slot INT8 CHECK (slot >= 0) NOT NULL, vendor STRING(63) NOT NULL, @@ -4762,10 +4767,15 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_physical_disk ( variant omicron.public.physical_disk_kind NOT NULL, + -- Where the disk sits in the chassis, as labelled by the sled's hardware + -- topology (e.g. "N5" for a U.2 bay, "M.2 East" for a boot device). NULL + -- if the sled did not report one. + location STRING(63), + -- PK consisting of: -- - Which collection this was -- - The sled reporting the disk - -- - The slot in which this disk was found + -- - The PCIe slot in which this disk was found PRIMARY KEY (inv_collection_id, sled_id, slot) ); @@ -4777,7 +4787,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_nvme_disk_firmware ( -- unique id for this sled (should be foreign keys into `sled` table, though -- it's conceivable a sled will report an id that we don't know about) sled_id UUID NOT NULL, - -- The slot where this disk was last observed + -- The PCIe physical slot number of the disk this firmware belongs to; + -- matches `inv_physical_disk.slot`. slot INT8 CHECK (slot >= 0) NOT NULL, -- total number of firmware slots the device has @@ -4794,7 +4805,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.inv_nvme_disk_firmware ( -- PK consisting of: -- - Which collection this was -- - The sled reporting the disk - -- - The slot in which the disk was found + -- - The PCIe slot in which the disk was found PRIMARY KEY (inv_collection_id, sled_id, slot) ); @@ -9506,7 +9517,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '299.0.0', NULL) + (TRUE, NOW(), NOW(), '300.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/inv-physical-disk-location/up.sql b/schema/crdb/inv-physical-disk-location/up.sql new file mode 100644 index 00000000000..9aa99642434 --- /dev/null +++ b/schema/crdb/inv-physical-disk-location/up.sql @@ -0,0 +1 @@ +ALTER TABLE omicron.public.inv_physical_disk ADD COLUMN IF NOT EXISTS location STRING(63); diff --git a/sled-agent/api/src/lib.rs b/sled-agent/api/src/lib.rs index 34a2991feca..4aadb803929 100644 --- a/sled-agent/api/src/lib.rs +++ b/sled-agent/api/src/lib.rs @@ -22,7 +22,7 @@ use omicron_common::api::internal::{ use sled_agent_types_versions::{ latest, v1, v4, v6, v7, v9, v10, v11, v12, v14, v16, v17, v18, v20, v22, v24, v25, v26, v28, v29, v30, v31, v32, v33, v34, v37, v39, v40, v41, v42, - v43, v46, v47, v48, v49, v50, v51, + v43, v46, v47, v48, v49, v50, v51, v53, }; use sled_diagnostics::SledDiagnosticsQueryOutput; use slog_error_chain::InlineErrorChain; @@ -39,6 +39,7 @@ api_versions!([ // | example for the next person. // v // (next_int, IDENT), + (54, ADD_DISK_LOCATION_TO_INVENTORY), (53, ADD_INSTANCE_MANAGER_STATUS_TO_INVENTORY), (52, TYPED_ARTIFACT_CONFIG_GENERATION), (51, MULTIPLE_ZONE_EXTERNAL_IPS), @@ -1296,12 +1297,27 @@ pub trait SledAgentApi { #[endpoint { method = GET, path = "/inventory", - versions = VERSION_ADD_INSTANCE_MANAGER_STATUS_TO_INVENTORY.., + versions = VERSION_ADD_DISK_LOCATION_TO_INVENTORY.., }] async fn inventory( rqctx: RequestContext, ) -> Result, HttpError>; + /// Fetch basic information about this sled + #[endpoint { + operation_id = "inventory", + method = GET, + path = "/inventory", + versions = VERSION_ADD_INSTANCE_MANAGER_STATUS_TO_INVENTORY..VERSION_ADD_DISK_LOCATION_TO_INVENTORY, + }] + async fn inventory_v53( + rqctx: RequestContext, + ) -> Result, HttpError> { + Self::inventory(rqctx).await.map(|HttpResponseOk(inv)| { + HttpResponseOk(v53::inventory::Inventory::from(inv)) + }) + } + /// Fetch basic information about this sled #[endpoint { operation_id = "inventory", @@ -1312,7 +1328,7 @@ pub trait SledAgentApi { async fn inventory_v51( rqctx: RequestContext, ) -> Result, HttpError> { - Self::inventory(rqctx).await.map(|HttpResponseOk(inv)| { + Self::inventory_v53(rqctx).await.map(|HttpResponseOk(inv)| { HttpResponseOk(v51::inventory::Inventory::from(inv)) }) } diff --git a/sled-agent/config-reconciler/src/raw_disks.rs b/sled-agent/config-reconciler/src/raw_disks.rs index 695e17c3a02..a0f1a4aa827 100644 --- a/sled-agent/config-reconciler/src/raw_disks.rs +++ b/sled-agent/config-reconciler/src/raw_disks.rs @@ -111,7 +111,8 @@ impl RawDisksSender { InventoryDisk { identity: disk.identity().clone(), variant: disk.variant(), - slot: disk.pcie_slot(), + pcie_slot: disk.pcie_slot(), + location: disk.location().map(str::to_string), active_firmware_slot: firmware.active_slot(), next_active_firmware_slot: firmware.next_active_slot(), number_of_firmware_slots: firmware.number_of_slots(), diff --git a/sled-agent/rack-setup/src/plan/service.rs b/sled-agent/rack-setup/src/plan/service.rs index 7c13de1b3cd..0eb7fe36e0d 100644 --- a/sled-agent/rack-setup/src/plan/service.rs +++ b/sled-agent/rack-setup/src/plan/service.rs @@ -1585,7 +1585,8 @@ mod tests { serial: format!("test-{sled_index}-{i}"), }, variant: DiskVariant::U2, - slot: i as i64, + pcie_slot: i as i64, + location: None, active_firmware_slot: 0, next_active_firmware_slot: None, number_of_firmware_slots: 8, diff --git a/sled-agent/rack-setup/src/service.rs b/sled-agent/rack-setup/src/service.rs index 15bf5ce457a..c9097a8a2ce 100644 --- a/sled-agent/rack-setup/src/service.rs +++ b/sled-agent/rack-setup/src/service.rs @@ -1696,7 +1696,8 @@ mod test { model: "v1".to_string(), }, variant: DiskVariant::U2, - slot: i.try_into().unwrap(), + pcie_slot: i.try_into().unwrap(), + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, diff --git a/sled-agent/src/sim/sled_agent.rs b/sled-agent/src/sim/sled_agent.rs index de7e4591e57..7ce9d49822e 100644 --- a/sled-agent/src/sim/sled_agent.rs +++ b/sled-agent/src/sim/sled_agent.rs @@ -970,7 +970,8 @@ impl SledAgent { .map(|info| InventoryDisk { identity: info.identity.clone(), variant: info.variant, - slot: info.slot, + pcie_slot: info.slot, + location: None, active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, diff --git a/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs b/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs new file mode 100644 index 00000000000..aef6cd2521a --- /dev/null +++ b/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs @@ -0,0 +1,158 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use std::net::SocketAddrV6; + +use iddqd::IdOrdMap; +use omicron_common::api::external::ByteCount; +use omicron_common::snake_case_result::{self, SnakeCaseResult}; +use omicron_uuid_kinds::SledUuid; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; +use sled_hardware_types::{BaseboardId, SledCpuFamily}; + +use crate::v1; +use crate::v1::disk::{DiskIdentity, DiskVariant}; +use crate::v1::inventory::{InventoryDataset, SledRole}; +use crate::v14::inventory::OmicronFileSourceResolverInventory; +use crate::v16::inventory::SingleMeasurementInventory; +use crate::v24::inventory::InventoryZpool; +use crate::v40::inventory::{FmdInventory, FmdInventoryError}; +use crate::v46::inventory::SvcsEnabledNotOnlineResult; +use crate::v51::inventory::{ + ConfigReconcilerInventory, ConfigReconcilerInventoryStatus, + OmicronSledConfig, +}; +use crate::v53; +use crate::v53::inventory::InstanceManagerStatus; + +/// Identifies information about disks which may be attached to Sleds. +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +pub struct InventoryDisk { + pub identity: DiskIdentity, + pub variant: DiskVariant, + /// PCIe physical slot number of the bridge above this disk. + /// + /// This is the `physical-slot#` of the parent `pcieb` device. It is + /// internal to the board's PCIe topology and board-specific: the same + /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the + /// location label printed on the chassis; see `location` for that. + pub pcie_slot: i64, + /// Where this disk sits in the chassis, as labelled by the platform's + /// 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 topology had no label for + /// the disk or could not be read. + pub location: Option, + // Today we only have NVMe disks so we embedded the firmware metadata here. + // In the future we can track firmware metadata in a unique type if we + // support more than one disk format. + pub active_firmware_slot: u8, + pub next_active_firmware_slot: Option, + pub number_of_firmware_slots: u8, + pub slot1_is_read_only: bool, + pub slot_firmware_versions: Vec>, +} + +impl From for v1::inventory::InventoryDisk { + fn from(new: InventoryDisk) -> Self { + let InventoryDisk { + identity, + variant, + pcie_slot, + location: _, + active_firmware_slot, + next_active_firmware_slot, + number_of_firmware_slots, + slot1_is_read_only, + slot_firmware_versions, + } = new; + Self { + identity, + variant, + slot: pcie_slot, + active_firmware_slot, + next_active_firmware_slot, + number_of_firmware_slots, + slot1_is_read_only, + slot_firmware_versions, + } + } +} + +/// Identity and basic status information about this sled agent +#[derive(Clone, Debug, Deserialize, JsonSchema, Serialize)] +pub struct Inventory { + pub sled_id: SledUuid, + pub sled_agent_address: SocketAddrV6, + pub sled_role: SledRole, + pub baseboard_id: BaseboardId, + pub usable_hardware_threads: u32, + pub usable_physical_ram: ByteCount, + pub cpu_family: SledCpuFamily, + pub reservoir_size: ByteCount, + pub disks: Vec, + pub zpools: Vec, + pub datasets: Vec, + pub ledgered_sled_config: Option, + pub reconciler_status: ConfigReconcilerInventoryStatus, + pub last_reconciliation: Option, + pub instance_manager_status: InstanceManagerStatus, + pub file_source_resolver: OmicronFileSourceResolverInventory, + pub smf_services_enabled_not_online: SvcsEnabledNotOnlineResult, + pub reference_measurements: IdOrdMap, + #[serde(with = "snake_case_result")] + #[schemars( + schema_with = "SnakeCaseResult::::json_schema" + )] + pub fmd: Result, +} + +impl From for v53::inventory::Inventory { + fn from(new: Inventory) -> Self { + let Inventory { + sled_id, + sled_agent_address, + sled_role, + baseboard_id, + usable_hardware_threads, + usable_physical_ram, + cpu_family, + reservoir_size, + disks, + zpools, + datasets, + ledgered_sled_config, + reconciler_status, + last_reconciliation, + instance_manager_status, + file_source_resolver, + smf_services_enabled_not_online, + reference_measurements, + fmd, + } = new; + Self { + sled_id, + sled_agent_address, + sled_role, + baseboard_id, + usable_hardware_threads, + usable_physical_ram, + cpu_family, + reservoir_size, + disks: disks.into_iter().map(Into::into).collect(), + zpools, + datasets, + ledgered_sled_config, + reconciler_status, + last_reconciliation, + instance_manager_status, + file_source_resolver, + smf_services_enabled_not_online, + reference_measurements, + fmd, + } + } +} diff --git a/sled-agent/types/versions/src/add_disk_location_to_inventory/mod.rs b/sled-agent/types/versions/src/add_disk_location_to_inventory/mod.rs new file mode 100644 index 00000000000..4f138d6b893 --- /dev/null +++ b/sled-agent/types/versions/src/add_disk_location_to_inventory/mod.rs @@ -0,0 +1,12 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Version `ADD_DISK_LOCATION_TO_INVENTORY` of the Sled Agent API. +//! +//! This version adds a `location` field to `InventoryDisk`, carrying the +//! chassis label the platform's hardware topology gives the disk's bay or +//! socket, and renames its `slot` field to `pcie_slot` to say what that +//! number is. + +pub mod inventory; diff --git a/sled-agent/types/versions/src/latest.rs b/sled-agent/types/versions/src/latest.rs index 6e9fd041d92..14c69178c7e 100644 --- a/sled-agent/types/versions/src/latest.rs +++ b/sled-agent/types/versions/src/latest.rs @@ -146,7 +146,6 @@ pub mod inventory { pub use crate::v1::inventory::HostPhase2DesiredContents; pub use crate::v1::inventory::HostPhase2DesiredSlots; pub use crate::v1::inventory::InventoryDataset; - pub use crate::v1::inventory::InventoryDisk; pub use crate::v1::inventory::ManifestBootInventory; pub use crate::v1::inventory::ManifestInventory; pub use crate::v1::inventory::ManifestNonBootInventory; @@ -213,7 +212,9 @@ pub mod inventory { pub use crate::v53::inventory::CurrentUpdateDisposition; pub use crate::v53::inventory::InstanceManagerStatus; - pub use crate::v53::inventory::Inventory; + + pub use crate::v54::inventory::Inventory; + pub use crate::v54::inventory::InventoryDisk; pub use crate::impls::inventory::FmdHostCaseDisplay; pub use crate::impls::inventory::FmdInventoryDisplay; diff --git a/sled-agent/types/versions/src/lib.rs b/sled-agent/types/versions/src/lib.rs index 3d6c45e034e..2762dab3bac 100644 --- a/sled-agent/types/versions/src/lib.rs +++ b/sled-agent/types/versions/src/lib.rs @@ -111,6 +111,8 @@ pub mod v51; pub mod v52; #[path = "add_instance_manager_status_to_inventory/mod.rs"] pub mod v53; +#[path = "add_disk_location_to_inventory/mod.rs"] +pub mod v54; #[path = "add_probe_put_endpoint/mod.rs"] pub mod v6; #[path = "multicast_support/mod.rs"] From 603073f3226c6b69f3e669f6882d33cc83ba1127 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 10:05:23 -0700 Subject: [PATCH 05/19] Add /usr/lib/fm/amd64 to the illumos runpath for libtopo sled-hardware now links libtopo, which Helios installs in /usr/lib/fm/amd64 rather than a default loader search path. Without a runpath entry, sled-agent and every test binary that links sled-hardware fail to start with "libtopo.so.1: open failed", which is how the helios build-and-test job died before listing installinator's tests. A -sys crate's build script cannot set the runpath on dependents' binaries, so this follows the existing arrangement for libipcc: the illumos target rustflags in .cargo/config.toml carry the directory, and build-and-test.sh, which overrides RUSTFLAGS, repeats it. The host image library check is told to expect libtopo.so.1 on the same set of binaries that may link libnvme, since both come from sled-hardware and both are global zone only. Test coverage: on a Helios workstation, cargo test of sled-hardware and installinator passes (the installinator test binary is the one that failed to load in CI), and elfdump shows the runpath on sled-agent and on both test binaries with ldd resolving libtopo.so.1 from /usr/lib/fm/amd64. --- .cargo/config.toml | 6 +++++- .cargo/xtask.toml | 13 +++++++++++++ .github/buildomat/build-and-test.sh | 6 +++--- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 6dc94b48356..4c8591bcf2b 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -64,7 +64,11 @@ rustflags = "--cfg tokio_unstable" # variety of things such as host OS. [target.x86_64-unknown-illumos] rustflags = [ - "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64", + # Runpath for libraries outside the default search path: + # /usr/platform/oxide/lib/amd64 holds oxide-specific libraries such as + # libipcc, and /usr/lib/fm/amd64 holds the fault management libraries + # such as libtopo. + "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64", # enable the `tokio-unstable` config for `tokio-dtrace` probes "--cfg", "tokio_unstable" ] diff --git a/.cargo/xtask.toml b/.cargo/xtask.toml index 76d7b342d94..c8bfb5d0a8b 100644 --- a/.cargo/xtask.toml +++ b/.cargo/xtask.toml @@ -59,3 +59,16 @@ binary_allow_list = [ "sled-agent", "sled-agent-sim", ] + +# libtopo is the illumos hardware topology library, used by sled-hardware to +# find each disk's chassis location. Like libnvme, it is only needed by +# programs that inspect a sled's hardware from the global zone. +[libraries."libtopo.so.1"] +binary_allow_list = [ + "installinator", + "omicron-dev", + "omicron-package", + "services-ledger-check-migrate", + "sled-agent", + "sled-agent-sim", +] diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index 030d3abe791..c443c236999 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -101,10 +101,10 @@ export RUSTFLAGS="--cfg tokio_unstable -D warnings" export RUSTDOCFLAGS="--document-private-items -D warnings" # When running on illumos we need to pass an additional runpath that is # usually configured via ".cargo/config" but the `RUSTFLAGS` env variable -# takes precedence. This path contains oxide specific libraries such as -# libipcc. +# takes precedence. These paths contain oxide specific libraries such as +# libipcc, and the fault management libraries such as libtopo. if [[ $target_os == "illumos" ]]; then - RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64" + RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64" fi export TMPDIR="$TEST_TMPDIR" export RUST_BACKTRACE=1 From c51a8fa62be99ec8ceafe72a1ecd25976a8ca644 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 11:36:08 -0700 Subject: [PATCH 06/19] Give the example inventory disks chassis locations Review feedback on #11279: real disks almost always report a location, so the representative collection should too. The four example disks now sit in N0, N1, N2, and M.2 West, with PCIe slot numbers matching what a Gimlet reports for those positions (0, 1, 2, and 18). The datastore reads disks back in slot order, so the list is kept sorted by slot, which moves the M.2 to the end; the builder unit test that pinned the M.2 as the first disk now checks the first and last entries, including their locations and slots. Test coverage: nexus-inventory unit tests and the nexus-db-queries inventory round-trip tests, which compare the collection with what the datastore reads back. --- nexus/inventory/src/builder.rs | 11 ++++++--- nexus/inventory/src/examples.rs | 41 +++++++++++++++++---------------- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 513885051d9..b300ea70d9c 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -1174,9 +1174,14 @@ mod test { assert_eq!(sled1_bb.part_number, "model1"); assert_eq!(sled1_bb.serial_number, "s1"); assert_eq!(sled1_agent.disks.len(), 4); - assert_eq!(sled1_agent.disks[0].identity.vendor, "macrohard"); - assert_eq!(sled1_agent.disks[0].identity.model, "box"); - assert_eq!(sled1_agent.disks[0].identity.serial, "XXIV"); + 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[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")); let sled4_agent = collection.sled_agents.get(&sled_agent_id_extra).unwrap(); diff --git a/nexus/inventory/src/examples.rs b/nexus/inventory/src/examples.rs index 911d176024a..1b362e48f92 100644 --- a/nexus/inventory/src/examples.rs +++ b/nexus/inventory/src/examples.rs @@ -477,34 +477,34 @@ pub fn representative() -> Representative { let sled_agent_id_basic = "c5aec1df-b897-49e4-8085-ccd975f9b529".parse().unwrap(); - // Add some disks to this first sled. + // Add some disks to this first sled. The datastore reads disks back in + // PCIe slot order, so keep this list sorted by `pcie_slot`. let disks = vec![ - // Let's say we have one manufacturer for our M.2... + // Let's say we have a couple different vendors for our U.2s... InventoryDisk { identity: DiskIdentity { - vendor: "macrohard".to_string(), - model: "box".to_string(), - serial: "XXIV".to_string(), + vendor: "memetendo".to_string(), + model: "swatch".to_string(), + serial: "0001".to_string(), }, - variant: DiskVariant::M2, + variant: DiskVariant::U2, pcie_slot: 0, - location: None, + location: Some("N0".to_string()), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, slot1_is_read_only: true, slot_firmware_versions: vec![Some("EXAMP1".to_string())], }, - // ... and a couple different vendors for our U.2s InventoryDisk { identity: DiskIdentity { vendor: "memetendo".to_string(), model: "swatch".to_string(), - serial: "0001".to_string(), + serial: "0002".to_string(), }, variant: DiskVariant::U2, pcie_slot: 1, - location: None, + location: Some("N1".to_string()), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, @@ -513,28 +513,29 @@ pub fn representative() -> Representative { }, InventoryDisk { identity: DiskIdentity { - vendor: "memetendo".to_string(), - model: "swatch".to_string(), - serial: "0002".to_string(), + vendor: "tony".to_string(), + model: "craystation".to_string(), + serial: "5".to_string(), }, variant: DiskVariant::U2, pcie_slot: 2, - location: None, + location: Some("N2".to_string()), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, slot1_is_read_only: true, slot_firmware_versions: vec![Some("EXAMP1".to_string())], }, + // ... and one manufacturer for our M.2. InventoryDisk { identity: DiskIdentity { - vendor: "tony".to_string(), - model: "craystation".to_string(), - serial: "5".to_string(), + vendor: "macrohard".to_string(), + model: "box".to_string(), + serial: "XXIV".to_string(), }, - variant: DiskVariant::U2, - pcie_slot: 3, - location: None, + variant: DiskVariant::M2, + pcie_slot: 18, + location: Some("M.2 West".to_string()), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, From 62b18e048553bfb2182dbb04838cf4ba264d43d9 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 11:43:38 -0700 Subject: [PATCH 07/19] Show only the chassis location on the inventory disk line Review feedback on #11279: printing "in " reads as a chassis position, which is the misreading this change set exists to remove, and omitting the location when unknown made that worse. The line now says where the disk is, or "unknown location" when the sled reported none, and leaves the PCIe slot number out; it remains in the database and in omdb's physical-disks table for anyone correlating with a sled. Test coverage: no expectorate output renders this line; verified with cargo check of nexus-types and the omdb test suite. --- nexus/types/src/inventory/display.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/nexus/types/src/inventory/display.rs b/nexus/types/src/inventory/display.rs index 0ded57c1de5..06da7cec84f 100644 --- a/nexus/types/src/inventory/display.rs +++ b/nexus/types/src/inventory/display.rs @@ -666,14 +666,10 @@ fn display_sleds( writeln!(indented, "physical disks:")?; } for disk in disks { - let PhysicalDisk { identity, variant, pcie_slot, location, .. } = - disk; + let PhysicalDisk { identity, variant, location, .. } = disk; + let loc = location.as_deref().unwrap_or("unknown location"); let mut indent2 = IndentWriter::new(" ", &mut indented); - write!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?; - if let Some(location) = location { - write!(indent2, " ({location})")?; - } - writeln!(indent2)?; + writeln!(indent2, "{variant:?}: {identity:?} in {loc}")?; } if !zpools.is_empty() { From e35b60af15d23e27a1d56e68c05802bee822d3f9 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 12:24:20 -0700 Subject: [PATCH 08/19] Tidy the topo lookup after review Review feedback on #11279, all in sled-hardware: - The topo module doc now describes the module (queries against the illumos hardware topology) rather than its one current function. - The note on opening a fresh handle per call says why in one sentence and points at oxidecomputer/libtopo#14, which tracks removing the one-snapshot-per-handle rule now that illumos 18110 is fixed. - The io/instance lookup is two sequential matches instead of a match nested in a match arm. - A comment explains the fallback to the parent node's label and why only bay and slot parents count. - Two nvme nodes reporting the same instance used to silently overwrite each other in the label map; the first label is now kept and the collision logged at error level with both labels. - The private location field on UnparsedDisk no longer carries a "see" pointer at its own accessor. Test coverage: no behavior change on a well-formed topology. cargo check --all-targets of sled-hardware and sled-agent and the sled-hardware test suite on a Helios workstation, plus cargo xtask clippy and cargo fmt on Linux. --- sled-hardware/src/disk.rs | 1 - sled-hardware/src/illumos/topo.rs | 63 ++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index c73f6262508..00b975ef773 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -217,7 +217,6 @@ pub struct UnparsedDisk { identity: DiskIdentity, is_boot_disk: bool, firmware: DiskFirmware, - /// See [`Self::location`]. #[serde(default)] location: Option, } diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs index 131b083e9c8..d13eff90766 100644 --- a/sled-hardware/src/illumos/topo.rs +++ b/sled-hardware/src/illumos/topo.rs @@ -2,12 +2,17 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Reading disk chassis locations from the illumos hardware topology. +//! Queries against the illumos hardware topology (libtopo). +//! +//! libtopo describes the platform as a tree of nodes with labels and +//! properties, built from platform topology maps, devinfo, and other +//! sources. This module holds sled-hardware's lookups against that tree. use crate::disk_location::NvmeInstance; use libtopo::{Node, PropValue, Scheme, TopoHdl, WalkAction}; -use slog::{Logger, debug, warn}; +use slog::{Logger, debug, error, warn}; use std::collections::HashMap; +use std::collections::hash_map::Entry; use std::time::Instant; // Node and property names from . @@ -26,9 +31,9 @@ const TOPO_IO_INSTANCE: &str = "instance"; /// a `bay` (a U.2 bay) or a `slot` (an M.2 socket). Controllers with neither /// are omitted from the result. /// -/// A new handle is opened on every call. Taking a second snapshot on the same -/// handle is unsafe in libtopo (illumos issue 18110), and taking the snapshot -/// is what enumerates the hardware. +/// A new handle is opened on every call: the wrapper allows one snapshot per +/// handle (see ), and a +/// snapshot costs far more than the handle anyway. pub(super) fn read_disk_locations( log: &Logger, ) -> Result, libtopo::Error> { @@ -42,19 +47,8 @@ pub(super) fn read_disk_locations( return Ok(WalkAction::Continue); } - let instance = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { - Ok(PropValue::UInt32(value)) => match NvmeInstance::try_from(value) - { - Ok(instance) => instance, - Err(err) => { - warn!( - log, - "ignoring nvme topology node with unusable instance"; - "err" => %err, - ); - return Ok(WalkAction::Continue); - } - }, + let value = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { + Ok(PropValue::UInt32(value)) => value, Ok(other) => { warn!( log, @@ -73,7 +67,24 @@ pub(super) fn read_disk_locations( return Ok(WalkAction::Continue); } }; + let instance = match NvmeInstance::try_from(value) { + Ok(instance) => instance, + Err(err) => { + warn!( + log, + "ignoring nvme topology node with unusable instance"; + "err" => %err, + ); + return Ok(WalkAction::Continue); + } + }; + // On Oxide platforms the label is carried by the enclosing `bay` (a + // U.2 bay) or `slot` (an M.2 socket) node rather than by the nvme + // node itself, so fall back to the parent. Only those two node types + // count: a label further up the tree would describe something + // unrelated to this disk. This is the same rule nvmeadm(8) applies + // for `-L`. let label = label_of(&node).or_else(|| { node.parent() .filter(|parent| { @@ -83,9 +94,19 @@ pub(super) fn read_disk_locations( .and_then(|parent| label_of(&parent)) }); match label { - Some(label) => { - labels.insert(instance, label); - } + Some(label) => match labels.entry(instance) { + Entry::Vacant(entry) => { + entry.insert(label); + } + Entry::Occupied(entry) => error!( + log, + "hardware topology has two nvme nodes with the same \ + instance; keeping the first label"; + "nvme_instance" => %instance, + "kept" => entry.get(), + "ignored" => &label, + ), + }, None => debug!( log, "nvme topology node has no location label"; From 7ba9b639f6c8762b580235cd787b269c3c3b7838 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 11 Sep 2026 13:51:52 -0700 Subject: [PATCH 09/19] Give the planning system builder's disks chassis locations The reconfigurator-cli script tests render inventory through the collection display, and after the disk line switched to showing the chassis location they printed "in unknown location" for every disk of the synthetic sleds built by SystemDescription. Those disks now carry a label following the real U.2 scheme, N0 upward matching their slot numbers, so the outputs read the way a real rack does. Test coverage: the five reconfigurator-cli script tests whose expected output changed (cmds-example, cmds-nexus-generation-autobump, cmds-mupdate-update-flow, cmds-unsafe-zone-mgs, cmds-target-release), regenerated with EXPECTORATE=overwrite and reviewed: every changed line is a disk line gaining a location. The reconfigurator planning and omdb suites also pass. --- .../tests/output/cmds-example-stdout | 18 +++++++++--------- .../output/cmds-mupdate-update-flow-stdout | 6 +++--- .../cmds-nexus-generation-autobump-stdout | 18 +++++++++--------- .../tests/output/cmds-target-release-stdout | 18 +++++++++--------- .../tests/output/cmds-unsafe-zone-mgs-stdout | 18 +++++++++--------- nexus/reconfigurator/planning/src/system.rs | 2 +- 6 files changed, 40 insertions(+), 40 deletions(-) diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout index 0164f5b66d5..8be09295749 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout @@ -1612,9 +1612,9 @@ sled 2eb69596-f081-4e2d-9425-9994926e0832 (role = Gimlet, serial serial1) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-64ea76fb-6673-4810-8e53-c2458b75eb01" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-6d012675-6f54-4b6a-8658-ab0076237569" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-9a065406-12a0-4b29-926e-d66bb785d17b" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-64ea76fb-6673-4810-8e53-c2458b75eb01" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-6d012675-6f54-4b6a-8658-ab0076237569" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-9a065406-12a0-4b29-926e-d66bb785d17b" } in N2 zpools 64ea76fb-6673-4810-8e53-c2458b75eb01: total size: 100 GiB health: online 6d012675-6f54-4b6a-8658-ab0076237569: total size: 100 GiB health: online @@ -1766,9 +1766,9 @@ sled 32d8d836-4d8a-4e54-8fa9-f31d79c42646 (role = Gimlet, serial serial2) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-2db3c119-38fa-41c0-8dbe-89689b84d655" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-687e054b-a9b5-4404-8b3f-0ea119c44a6a" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-a4e8a5cc-cd1c-4a08-bcf1-31785f7cda28" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-2db3c119-38fa-41c0-8dbe-89689b84d655" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-687e054b-a9b5-4404-8b3f-0ea119c44a6a" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-a4e8a5cc-cd1c-4a08-bcf1-31785f7cda28" } in N2 zpools 2db3c119-38fa-41c0-8dbe-89689b84d655: total size: 100 GiB health: online 687e054b-a9b5-4404-8b3f-0ea119c44a6a: total size: 100 GiB health: online @@ -1920,9 +1920,9 @@ sled 89d02b1b-478c-401a-8e28-7a26f74fa41b (role = Gimlet, serial serial0) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-0477165a-a72e-4814-b8d6-74aa02cb2040" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-ab94a812-86ce-428c-bbbb-6ce1ab0b071b" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-f96f5901-2907-4f21-bfeb-772f8a3c4e44" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-0477165a-a72e-4814-b8d6-74aa02cb2040" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-ab94a812-86ce-428c-bbbb-6ce1ab0b071b" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-f96f5901-2907-4f21-bfeb-772f8a3c4e44" } in N2 zpools 0477165a-a72e-4814-b8d6-74aa02cb2040: total size: 100 GiB health: online ab94a812-86ce-428c-bbbb-6ce1ab0b071b: total size: 100 GiB health: online diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-mupdate-update-flow-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-mupdate-update-flow-stdout index 93afbb5cd03..d683c6728ff 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-mupdate-update-flow-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-mupdate-update-flow-stdout @@ -182,7 +182,7 @@ sled 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c (role = Gimlet, serial serial1) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in 0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in N0 zpools 72c59873-31ff-4e36-8d76-ff834009349a: total size: 100 GiB health: online datasets: @@ -317,7 +317,7 @@ sled 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6 (role = Gimlet, serial serial0) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in 0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in N0 zpools c6d33b64-fb96-4129-bab1-7878a06a5f9b: total size: 100 GiB health: online datasets: @@ -456,7 +456,7 @@ sled d81c6a84-79b8-4958-ae41-ea46c9b19763 (role = Gimlet, serial serial2) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in 0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in N0 zpools 4930954e-9ac7-4453-b63f-5ab97c389a99: total size: 100 GiB health: online datasets: diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-nexus-generation-autobump-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-nexus-generation-autobump-stdout index cd2c1b98dae..abf739164d2 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-nexus-generation-autobump-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-nexus-generation-autobump-stdout @@ -543,9 +543,9 @@ sled 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c (role = Gimlet, serial serial1) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in N2 zpools 727522a7-934f-494d-b5b3-160968e74463: total size: 100 GiB health: online 72c59873-31ff-4e36-8d76-ff834009349a: total size: 100 GiB health: online @@ -750,9 +750,9 @@ sled 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6 (role = Gimlet, serial serial0) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in N2 zpools 073979dd-3248-44a5-9fa1-cc72a140d682: total size: 100 GiB health: online c6d33b64-fb96-4129-bab1-7878a06a5f9b: total size: 100 GiB health: online @@ -948,9 +948,9 @@ sled d81c6a84-79b8-4958-ae41-ea46c9b19763 (role = Gimlet, serial serial2) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in N2 zpools 18b20749-0748-4105-bb10-7b13cfc776e2: total size: 100 GiB health: online 30c16fe4-4229-49d0-ab01-3138f2c7dff2: total size: 100 GiB health: online diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout index d29a824c141..feb5ca9860d 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout @@ -523,9 +523,9 @@ sled 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c (role = Gimlet, serial serial1) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in N2 zpools 727522a7-934f-494d-b5b3-160968e74463: total size: 100 GiB health: online 72c59873-31ff-4e36-8d76-ff834009349a: total size: 100 GiB health: online @@ -730,9 +730,9 @@ sled 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6 (role = Gimlet, serial serial0) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in N2 zpools 073979dd-3248-44a5-9fa1-cc72a140d682: total size: 100 GiB health: online c6d33b64-fb96-4129-bab1-7878a06a5f9b: total size: 100 GiB health: online @@ -928,9 +928,9 @@ sled d81c6a84-79b8-4958-ae41-ea46c9b19763 (role = Gimlet, serial serial2) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in N2 zpools 18b20749-0748-4105-bb10-7b13cfc776e2: total size: 100 GiB health: online 30c16fe4-4229-49d0-ab01-3138f2c7dff2: total size: 100 GiB health: online diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-unsafe-zone-mgs-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-unsafe-zone-mgs-stdout index ef026ce18aa..c78f363e5f5 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-unsafe-zone-mgs-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-unsafe-zone-mgs-stdout @@ -577,9 +577,9 @@ sled 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c (role = Gimlet, serial serial1) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-727522a7-934f-494d-b5b3-160968e74463" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-72c59873-31ff-4e36-8d76-ff834009349a" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-b5fd5bc1-099e-4e77-8028-a9793c11f43b" } in N2 zpools 727522a7-934f-494d-b5b3-160968e74463: total size: 100 GiB health: online 72c59873-31ff-4e36-8d76-ff834009349a: total size: 100 GiB health: online @@ -784,9 +784,9 @@ sled 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6 (role = Gimlet, serial serial0) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-073979dd-3248-44a5-9fa1-cc72a140d682" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-c6d33b64-fb96-4129-bab1-7878a06a5f9b" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-e4d937e1-6ddc-4eca-bb08-c1f73791e608" } in N2 zpools 073979dd-3248-44a5-9fa1-cc72a140d682: total size: 100 GiB health: online c6d33b64-fb96-4129-bab1-7878a06a5f9b: total size: 100 GiB health: online @@ -982,9 +982,9 @@ sled d81c6a84-79b8-4958-ae41-ea46c9b19763 (role = Gimlet, serial serial2) usable memory (GiB): 0 reservoir (GiB): 0 physical disks: - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in 0 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in 1 - U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in 2 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-18b20749-0748-4105-bb10-7b13cfc776e2" } in N0 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-30c16fe4-4229-49d0-ab01-3138f2c7dff2" } in N1 + U2: DiskIdentity { vendor: "fake-vendor", model: "fake-model", serial: "serial-4930954e-9ac7-4453-b63f-5ab97c389a99" } in N2 zpools 18b20749-0748-4105-bb10-7b13cfc776e2: total size: 100 GiB health: online 30c16fe4-4229-49d0-ab01-3138f2c7dff2: total size: 100 GiB health: online diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 22a09ed064b..bd82c054456 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -1536,7 +1536,7 @@ impl Sled { identity: disk.disk_identity.clone(), variant: DiskVariant::U2, pcie_slot: i64::try_from(i).unwrap(), - location: None, + location: Some(format!("N{i}")), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, From a226091973002dbc2f68ab270da6d98f43841459 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 14 Sep 2026 16:26:15 -0700 Subject: [PATCH 10/19] Disk location follow-ups: reconcile late labels, merge topo reads, load old state Review follow-ups for the disk chassis location work. Correctness: - A managed disk adopted before its chassis location was known never picked the label up: the reconciler's property-update path only copied firmware, so RawDisk equality never held again and every pass logged "other properties are different". Disk::update_mutable_properties now copies location too, with a test. - DiskLocationCache replaced its label map wholesale on every successful topo read, so a snapshot that omitted an already-labelled controller (which topo can do without reporting an error) flipped that disk's location to None until the next retry. Reads are now merged: new or changed labels are adopted, a labelled controller missing from a read keeps its label, and both events are logged. Tests cover both. - nexus_types::inventory::PhysicalDisk is what reconfigurator state files (support bundles, omdb exports) carry, and the slot -> pcie_slot rename plus the new location field made pre-existing files fail to load. Add #[serde(alias = "slot")] and #[serde(default)] so they still do, with a test in the style of the existing intended_target_blueprint note. Cleanup: - poll_blkdev_node returns the disk and its nvme instance instead of writing into two maps keyed by DiskIdentity that were rejoined afterwards; the cfg(illumos) UnparsedDisk::set_location is gone. - The topo walk closure is split into nvme_instance_of and location_label_of, which hold exactly the code that libtopo#15 and libtopo#16 will let us delete. - Drop a debug line the cache logged in addition to the reader's. - InvNvmeDiskFirmware::new names its parameter pcie_slot like the field. - Rewrite the runpath comment in .cargo/config.toml: it forbade adding paths while the entry below it added one. It now states when a path belongs there versus the rpaths crate, and covers /usr/lib/fm/amd64. sled-hardware was checked and tested on a Helios 3 host for the illumos-only code; everything else was checked on Linux. --- .cargo/config.toml | 59 +++++---- nexus/db-model/src/inventory.rs | 4 +- nexus/types/src/inventory.rs | 51 ++++++++ .../config-reconciler/src/disks_common.rs | 89 ++++++++++++- sled-hardware/src/disk.rs | 5 - sled-hardware/src/disk_location.rs | 117 ++++++++++++++++-- sled-hardware/src/illumos/mod.rs | 48 ++++--- sled-hardware/src/illumos/topo.rs | 104 +++++++++------- sled-storage/src/disk.rs | 5 +- 9 files changed, 358 insertions(+), 124 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 4c8591bcf2b..e83cf8e6132 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -21,28 +21,34 @@ rustdocflags = "--document-private-items" rustflags = "--cfg tokio_unstable" -# On illumos, use `-znocompstrtab` to reduce link time. We also add the Oxide -# specific platform directory to the RPATH where additional libraries can be -# found such as libipcc. +# On illumos, use `-znocompstrtab` to reduce link time. We also add two +# directories to the RPATH where libraries outside the default search path +# live: # -# Our reasoning for including `-R/usr/platform/oxide/lib/amd64` here: -# - Oxide specific features - This path contains Oxide specific libraries such -# as libipcc and will likely grow over time to include more functionality. -# - Avoid the rpaths crate - The rpaths crate was built to deal with runtime -# paths that are dynamic such as with libraries like libpq which can live in -# different locations based on OS. This path will only ever be found on -# illumos and will be tied directly to the Oxide platform. -# - Less developer burden - Having something like the ipcc crate emit -# `DEP_IPCC_LIBDIRS` means that we end up littering the repo with Cargo.toml -# and build.rs changes whenever ipcc shows up somewhere in the dependency -# tree. While initially exploring that path we ran into a significant number -# of tests failing due to not being able to find libipcc in the runtime path +# - /usr/platform/oxide/lib/amd64 holds Oxide-specific libraries such as +# libipcc, and will likely grow over time to include more functionality. +# - /usr/lib/fm/amd64 holds the illumos fault management libraries such as +# libtopo, which sled-hardware uses to find each disk's chassis location. +# +# Our reasoning for putting these here rather than going through the rpaths +# crate: +# - Static, illumos-only paths - The rpaths crate was built to deal with +# runtime paths that are dynamic such as with libraries like libpq which can +# live in different locations based on OS. These paths will only ever be +# found on illumos and never move. +# - Less developer burden - Having a -sys crate emit `DEP_*_LIBDIRS` means that +# we end up littering the repo with Cargo.toml and build.rs changes in every +# crate whose binaries or tests transitively link the library. For libipcc +# that was already painful; libtopo enters through sled-hardware, which well +# over a hundred workspace crates reach transitively. While initially +# exploring that path for libipcc we ran into a significant number of tests +# failing due to not being able to find the library in the runtime path, # which can be confusing or surprising to someone doing work on omicron. # -# We could also update Helios so that a symlink is created from -# /usr/platform/oxide/lib/amd64 into /usr/lib/64 but we opted to not take -# this route forward as it meant waiting for another round of updates on -# shared build machines and to buildomat itself. +# We could also update Helios so that symlinks are created from these +# directories into /usr/lib/64 but we opted to not take this route forward as +# it meant waiting for another round of updates on shared build machines and +# to buildomat itself. # # As documented at: # https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags @@ -58,16 +64,15 @@ rustflags = "--cfg tokio_unstable" # When overriding the defaults in this config by environment variable the user # may need to manually pass the additional options found below. # -# Note that other runtime paths should not be added here, but should instead -# reuse the infrastructure found in the `rpaths` crate which can be found in -# this repo. Those paths are usually platform specific and will vary based on a -# variety of things such as host OS. +# A runtime path belongs here only if it is fixed and illumos-only, and the +# library behind it is reached transitively by a large share of the workspace. +# Paths that vary with the host OS, or that only a handful of crates need, +# should instead reuse the infrastructure found in the `rpaths` crate in this +# repo. Whatever is added here must also be added to the `RUSTFLAGS` override +# in .github/buildomat/build-and-test.sh, which takes precedence over this +# file. [target.x86_64-unknown-illumos] rustflags = [ - # Runpath for libraries outside the default search path: - # /usr/platform/oxide/lib/amd64 holds oxide-specific libraries such as - # libipcc, and /usr/lib/fm/amd64 holds the fault management libraries - # such as libtopo. "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64", # enable the `tokio-unstable` config for `tokio-dtrace` probes "--cfg", "tokio_unstable" diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index 89920e801dc..351b7c01e90 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -2431,7 +2431,7 @@ impl InvNvmeDiskFirmware { pub fn new( inv_collection_id: CollectionUuid, sled_id: SledUuid, - sled_slot: i64, + pcie_slot: i64, firmware: &NvmeFirmware, ) -> Result { // NB: We first validate that the data given to us from an NVMe disk @@ -2505,7 +2505,7 @@ impl InvNvmeDiskFirmware { Ok(Self { inv_collection_id: inv_collection_id.into(), sled_id: sled_id.into(), - pcie_slot: sled_slot, + pcie_slot, active_slot: firmware.active_slot.into(), next_active_slot: firmware.next_active_slot.map(|nas| nas.into()), number_of_slots: firmware.number_of_slots.into(), diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index b730fe71fff..1fa6ada8396 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -614,6 +614,12 @@ pub struct PhysicalDisk { /// internal to the board's PCIe topology and board-specific: the same /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the /// location label printed on the chassis; see `location` for that. + // + // Older serialized state (support bundles, `omdb` exports) called this + // field `slot`. Accept that name too so those files still load. The + // format is documented as unstable, so this isn't strictly necessary, but + // it is cheap and fail-safe. + #[serde(alias = "slot")] pub pcie_slot: i64, /// Where this disk sits in the chassis, as labelled by the platform's /// hardware topology: "N5" for a U.2 bay, "M.2 East" for a boot device. @@ -621,6 +627,10 @@ pub struct PhysicalDisk { /// 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` is exactly right, since nothing recorded a location then. + #[serde(default)] pub location: Option, pub firmware: PhysicalDiskFirmware, } @@ -788,3 +798,44 @@ impl IdOrdItem for InternalDnsGenerationStatus { } id_upcast!(); } + +#[cfg(test)] +mod tests { + use super::*; + + // Reconfigurator state files written before `PhysicalDisk` gained + // `location` (and renamed `slot` to `pcie_slot`) must still deserialize. + #[test] + fn physical_disk_loads_older_serialized_form() { + let disk = PhysicalDisk { + identity: DiskIdentity { + vendor: "vendor".to_string(), + model: "model".to_string(), + serial: "serial".to_string(), + }, + variant: PhysicalDiskKind::U2, + pcie_slot: 5, + location: Some("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"); + assert_eq!( + serde_json::from_value::(current.clone()).unwrap(), + 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(); + + let loaded: PhysicalDisk = serde_json::from_value(current).unwrap(); + assert_eq!(loaded, PhysicalDisk { location: None, ..disk }); + } +} diff --git a/sled-agent/config-reconciler/src/disks_common.rs b/sled-agent/config-reconciler/src/disks_common.rs index fe13b28e248..28e2003e2d6 100644 --- a/sled-agent/config-reconciler/src/disks_common.rs +++ b/sled-agent/config-reconciler/src/disks_common.rs @@ -29,21 +29,23 @@ pub(crate) fn update_properties_from_raw_disk( return MaybeUpdatedDisk::Unchanged; } - // The only property we expect to change is the firmware metadata. Update - // that and check again; if they're still not equal, something weird is - // going on. At least log a warning. + // 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. let mut disk = disk.clone(); - disk.update_firmware_metadata(raw_disk); + disk.update_mutable_properties(raw_disk); if *raw_disk == RawDisk::from(disk.clone()) { info!( - log, "Updated disk firmware metadata"; + log, "Updated disk properties"; "firmware" => ?disk.firmware(), + "location" => ?disk.location(), "identity" => ?disk.identity(), ); } else { warn!( log, - "Updated disk firmware metadata from raw disk properties, \ + "Updated disk properties from raw disk, \ but other properties are different!"; "disk" => ?disk, "raw_disk" => ?*raw_disk, @@ -51,3 +53,78 @@ pub(crate) fn update_properties_from_raw_disk( } MaybeUpdatedDisk::Updated(disk) } + +#[cfg(test)] +mod tests { + use super::*; + use assert_matches::assert_matches; + use omicron_common::zpool_name::ZpoolName; + use omicron_test_utils::dev; + use omicron_uuid_kinds::ExternalZpoolUuid; + use sled_agent_types::disk::DiskIdentity; + use sled_agent_types::disk::DiskVariant; + use sled_hardware::DiskFirmware; + use sled_hardware::DiskPaths; + 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. + #[test] + fn location_learned_after_adoption_is_propagated() { + let logctx = dev::test_setup_log( + "location_learned_after_adoption_is_propagated", + ); + + let identity = DiskIdentity { + vendor: "test".into(), + model: "test".into(), + serial: "test-u2".into(), + }; + let firmware = DiskFirmware::new(0, None, false, 1, vec![]); + let adopted = Disk::Real(PooledDisk { + paths: DiskPaths { + devfs_path: "/test-devfs".into(), + dev_path: None, + }, + pcie_slot: 0, + identity: identity.clone(), + is_boot_disk: false, + location: None, + 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())), + ); + + 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!(RawDisk::from(updated.clone()), raw_disk); + + // A second pass sees nothing left to update. + assert_matches!( + update_properties_from_raw_disk(&updated, &raw_disk, &logctx.log), + MaybeUpdatedDisk::Unchanged + ); + + logctx.cleanup_successful(); + } +} diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index 00b975ef773..a5328435de8 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -290,11 +290,6 @@ impl UnparsedDisk { self.location.as_deref() } - #[cfg(target_os = "illumos")] - pub(crate) fn set_location(&mut self, location: Option) { - self.location = location; - } - pub fn firmware(&self) -> &DiskFirmware { &self.firmware } diff --git a/sled-hardware/src/disk_location.rs b/sled-hardware/src/disk_location.rs index 6cc44eb021b..fc036d5fdf5 100644 --- a/sled-hardware/src/disk_location.rs +++ b/sled-hardware/src/disk_location.rs @@ -11,8 +11,9 @@ //! joins devinfo's view of a controller to topo's, and a cache that decides //! when topo needs to be consulted at all. -use slog::{Logger, debug, warn}; +use slog::{Logger, warn}; use std::collections::HashMap; +use std::collections::hash_map::Entry; use std::fmt; use std::time::{Duration, Instant}; @@ -104,7 +105,9 @@ impl DiskLocationCache { /// never touches topo. /// /// `read_labels` returns the label of every controller topo has one for. - /// On success it replaces the cache wholesale. On failure the labels + /// On success its labels are merged into the cache: a new or changed + /// label is adopted, but a controller the cache has already labelled is + /// never forgotten because a later read omitted it. On failure the labels /// already known are kept and the failure is logged. pub fn refresh_if_needed( &mut self, @@ -122,14 +125,47 @@ impl DiskLocationCache { self.last_attempt = Some(now); let read_ok = match read_labels() { - Ok(labels) => { - debug!( - log, - "read disk locations from hardware topology"; - "count" => labels.len(), - ); - self.labels = - labels.into_iter().map(|(i, l)| (i, Some(l))).collect(); + Ok(read) => { + // A present controller we already labelled should be in every + // successful read. If it is not, keep its label: the omission + // could be a transient enumeration failure or a topo bug, and + // a controller's location cannot change while the system is + // up. + for instance in present { + if let Some(Some(kept)) = self.labels.get(instance) + && !read.contains_key(instance) + { + warn!( + log, + "hardware topology omitted a disk controller it \ + previously labelled; keeping the label"; + "nvme_instance" => %instance, + "label" => kept, + ); + } + } + + for (instance, label) in read { + match self.labels.entry(instance) { + Entry::Occupied(mut entry) => { + if let Some(old) = entry.get() + && *old != label + { + warn!( + log, + "disk controller location label changed"; + "nvme_instance" => %instance, + "old" => old, + "new" => &label, + ); + } + entry.insert(Some(label)); + } + Entry::Vacant(entry) => { + entry.insert(Some(label)); + } + } + } true } Err(err) => { @@ -335,4 +371,65 @@ mod tests { assert_eq!(cache.location(inst(1)), Some("N1")); logctx.cleanup_successful(); } + + #[test] + fn omitted_controller_keeps_its_label() { + let logctx = test_setup_log("omitted_controller_keeps_its_label"); + let mut cache = DiskLocationCache::new(); + let calls = Cell::new(0); + let t0 = Instant::now(); + + cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(0, "N0")])) + }); + assert_eq!(calls.get(), 1); + assert_eq!(cache.location(inst(0)), Some("N0")); + + // A new controller forces a refresh, and this read leaves out the + // controller we already know about. + let t1 = t0 + Duration::from_secs(1); + let present = [inst(0), inst(1)]; + cache.refresh_if_needed(&logctx.log, &present, t1, || { + calls.set(calls.get() + 1); + Ok::<_, String>(labels(&[(1, "N1")])) + }); + assert_eq!(calls.get(), 2); + assert_eq!(cache.location(inst(0)), Some("N0")); + assert_eq!(cache.location(inst(1)), Some("N1")); + + // Both controllers are resolved, so nothing prompts another read. + let later = t1 + Duration::from_secs(3600); + cache.refresh_if_needed(&logctx.log, &present, later, || { + calls.set(calls.get() + 1); + Ok::<_, String>(HashMap::new()) + }); + assert_eq!(calls.get(), 2); + logctx.cleanup_successful(); + } + + #[test] + fn changed_label_is_adopted() { + let logctx = test_setup_log("changed_label_is_adopted"); + let mut cache = DiskLocationCache::new(); + let t0 = Instant::now(); + + cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { + Ok::<_, String>(labels(&[(0, "N0")])) + }); + assert_eq!(cache.location(inst(0)), Some("N0")); + + // A new controller forces a refresh, and topo now reports a different + // label for the controller we already know about. + let present = [inst(0), inst(1)]; + cache.refresh_if_needed( + &logctx.log, + &present, + t0 + Duration::from_secs(1), + || Ok::<_, String>(labels(&[(0, "N9"), (1, "N1")])), + ); + assert_eq!(cache.location(inst(0)), Some("N9")); + assert_eq!(cache.location(inst(1)), Some("N1")); + logctx.cleanup_successful(); + } } diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index cc316eb9924..a038f2059d4 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -172,40 +172,39 @@ impl HardwareSnapshot { let mut disks = HashMap::new(); match external_disks { ExternalDisks::DetectPhysical => { - let mut nvme_instances = HashMap::new(); + let mut found = Vec::new(); let mut node_walker = device_info.walk_driver("blkdev"); while let Some(node) = node_walker.next().transpose().map_err(Error::DevInfo)? { - poll_blkdev_node( + if let Some(found_disk) = poll_blkdev_node( &log, sled_type, - &mut disks, - &mut nvme_instances, node, boot_storage_unit, - )?; + )? { + found.push(found_disk); + } } // Now that we know which controllers are present, ask the // hardware topology where they are. The cache decides whether // topo actually needs to be consulted this time around. let present: Vec = - nvme_instances.values().copied().collect(); + found.iter().map(|(_, instance)| *instance).collect(); location_cache.refresh_if_needed( log, &present, Instant::now(), || topo::read_disk_locations(log), ); - for (identity, instance) in &nvme_instances { - if let Some(disk) = disks.get_mut(identity) { - disk.set_location( - location_cache - .location(*instance) - .map(str::to_string), - ); - } + for (disk, instance) in found { + let location = + location_cache.location(instance).map(str::to_string); + disks.insert( + disk.identity().clone(), + disk.with_location(location), + ); } } @@ -420,20 +419,21 @@ fn find_properties<'a, const N: usize>( Ok(output.try_into().map_err(|_| "Unexpected output size").unwrap()) } +// Describes the disk behind a "blkdev" devinfo node, along with the instance +// of the "nvme" controller above it. The disk's chassis location is not yet +// known here; the caller fills it in once every controller has been found. fn poll_blkdev_node( log: &Logger, sled: OxideSled, - disks: &mut HashMap, - nvme_instances: &mut HashMap, node: Node<'_>, boot_storage_unit: BootStorageUnit, -) -> Result<(), Error> { +) -> Result, Error> { let Some(driver_name) = node.driver_name() else { - return Ok(()); + return Ok(None); }; if driver_name != "blkdev" { - return Ok(()); + return Ok(None); } let devfs_path = node.devfs_path().map_err(Error::DevInfo)?; @@ -542,15 +542,11 @@ fn poll_blkdev_node( dev_path, pcie_slot, variant, - device_id.clone(), + device_id, pcie_slot_is_boot_disk(sled, pcie_slot, boot_storage_unit), - firmware.clone(), + firmware, ); - // The location is filled in once every controller has been found; see - // `HardwareSnapshot::new`. - nvme_instances.insert(device_id.clone(), nvme_instance); - disks.insert(device_id, disk); - Ok(()) + Ok(Some((disk, nvme_instance))) } // Poll just enough of the device info tree to get the Baseboard. We really diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs index d13eff90766..056498abfc1 100644 --- a/sled-hardware/src/illumos/topo.rs +++ b/sled-hardware/src/illumos/topo.rs @@ -46,54 +46,10 @@ pub(super) fn read_disk_locations( if node.name() != NVME { return Ok(WalkAction::Continue); } - - let value = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { - Ok(PropValue::UInt32(value)) => value, - Ok(other) => { - warn!( - log, - "ignoring nvme topology node whose io/instance is not a \ - uint32"; - "value" => ?other, - ); - return Ok(WalkAction::Continue); - } - Err(_) => { - debug!( - log, - "nvme topology node has no io/instance property"; - "node_instance" => node.instance(), - ); - return Ok(WalkAction::Continue); - } - }; - let instance = match NvmeInstance::try_from(value) { - Ok(instance) => instance, - Err(err) => { - warn!( - log, - "ignoring nvme topology node with unusable instance"; - "err" => %err, - ); - return Ok(WalkAction::Continue); - } + let Some(instance) = nvme_instance_of(log, &node) else { + return Ok(WalkAction::Continue); }; - - // On Oxide platforms the label is carried by the enclosing `bay` (a - // U.2 bay) or `slot` (an M.2 socket) node rather than by the nvme - // node itself, so fall back to the parent. Only those two node types - // count: a label further up the tree would describe something - // unrelated to this disk. This is the same rule nvmeadm(8) applies - // for `-L`. - let label = label_of(&node).or_else(|| { - node.parent() - .filter(|parent| { - let name = parent.name(); - name == BAY || name == SLOT - }) - .and_then(|parent| label_of(&parent)) - }); - match label { + match location_label_of(&node) { Some(label) => match labels.entry(instance) { Entry::Vacant(entry) => { entry.insert(label); @@ -125,6 +81,60 @@ pub(super) fn read_disk_locations( Ok(labels) } +/// The driver instance of an `nvme` node, from its `io/instance` property. +/// `None`, with a log line, if the node has no such property or it is not the +/// uint32 topo documents it as. +fn nvme_instance_of(log: &Logger, node: &Node<'_>) -> Option { + let value = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { + Ok(PropValue::UInt32(value)) => value, + Ok(other) => { + warn!( + log, + "ignoring nvme topology node whose io/instance is not a uint32"; + "value" => ?other, + ); + return None; + } + Err(_) => { + debug!( + log, + "nvme topology node has no io/instance property"; + "node_instance" => node.instance(), + ); + return None; + } + }; + match NvmeInstance::try_from(value) { + Ok(instance) => Some(instance), + Err(err) => { + warn!( + log, + "ignoring nvme topology node with unusable instance"; + "err" => %err, + ); + None + } + } +} + +/// The chassis location label for an `nvme` node. +/// +/// On Oxide platforms the label is carried by the enclosing `bay` (a U.2 bay) +/// or `slot` (an M.2 socket) node rather than by the nvme node itself, so fall +/// back to the parent. Only those two node types count: a label further up the +/// tree would describe something unrelated to this disk. This is the same rule +/// nvmeadm(8) applies for `-L`. +fn location_label_of(node: &Node<'_>) -> Option { + label_of(node).or_else(|| { + node.parent() + .filter(|parent| { + let name = parent.name(); + name == BAY || name == SLOT + }) + .and_then(|parent| label_of(&parent)) + }) +} + /// A node's label, treating "no label" and an empty label alike. fn label_of(node: &Node<'_>) -> Option { node.label().ok().filter(|label| !label.is_empty()) diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index 07662c2d4c0..b832bb24870 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -399,10 +399,13 @@ impl Disk { } } - pub fn update_firmware_metadata(&mut self, raw_disk: &RawDisk) { + /// Copies the properties that may legitimately change over a disk's + /// lifetime (firmware metadata and chassis location) from `raw_disk`. + pub fn update_mutable_properties(&mut self, raw_disk: &RawDisk) { match self { Disk::Real(pooled_disk) => { pooled_disk.firmware = raw_disk.firmware().clone(); + pooled_disk.location = raw_disk.location().map(str::to_string); } Disk::Synthetic(synthetic_disk) => { synthetic_disk.raw.firmware = raw_disk.firmware().clone(); From 7807fec93ae8a26e0a05aa4f0fa5517a48baac2c Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 16 Sep 2026 08:46:57 -0700 Subject: [PATCH 11/19] Use libtopo 0.2.0's hc constants and typed property getter libtopo 0.2.0 re-exports the node and property names as &str constants in its hc module and adds typed getters mirroring topo_prop_get_*. Drop sled-hardware's hand-copied name constants in favor of hc::NVME, hc::BAY, hc::SLOT, hc::TOPO_PGROUP_IO, and hc::TOPO_IO_INSTANCE, and read io/instance with property_u32 so a type mismatch surfaces as a libtopo error rather than a PropValue variant to check by hand. Test coverage: sled-hardware compiles and passes clippy on illumos (thelio) with the new crate; the Linux build does not compile this module. --- Cargo.lock | 8 ++--- Cargo.toml | 2 +- sled-hardware/src/illumos/topo.rs | 52 ++++++++++++++----------------- 3 files changed, 28 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ed254c70f35..6112af103fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6580,9 +6580,9 @@ dependencies = [ [[package]] name = "libtopo" -version = "0.1.0" +version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "08099e1e02473bb103f1ad4bfa9c81c04ebf7a638573c72959817a0347040022" +checksum = "92b90fcffd6cba36f39a92ac8bad23d80ad4de13d8c15f217585364875e6dbdf" dependencies = [ "illumos-nvpair", "illumos-nvpair-sys", @@ -6592,9 +6592,9 @@ dependencies = [ [[package]] name = "libtopo-sys" -version = "0.1.0" +version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e9c1776be01e57714dfe8247395585d69479444508dad3f4e35270801152001" +checksum = "e556c0588d42db0408f33928d4d236566a77d430a9c43615ee67b0343d37c4a4" [[package]] name = "libxml" diff --git a/Cargo.toml b/Cargo.toml index 2950eae4272..89ebd379907 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -620,7 +620,7 @@ libc = "0.2.174" linkme = "0.3" libipcc = { git = "https://github.com/oxidecomputer/ipcc-rs", rev = "524eb8f125003dff50b9703900c6b323f00f9e1b" } libnvme = { git = "https://github.com/oxidecomputer/libnvme", rev = "dd5bb221d327a1bc9287961718c3c10d6bd37da0" } -libtopo = "0.1.0" +libtopo = "0.2.0" linear-map = "1.2.0" live-tests-macros = { path = "live-tests/macros" } lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" } diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs index 056498abfc1..e274cf666f8 100644 --- a/sled-hardware/src/illumos/topo.rs +++ b/sled-hardware/src/illumos/topo.rs @@ -9,19 +9,12 @@ //! sources. This module holds sled-hardware's lookups against that tree. use crate::disk_location::NvmeInstance; -use libtopo::{Node, PropValue, Scheme, TopoHdl, WalkAction}; +use libtopo::{Error, Node, Scheme, TopoHdl, WalkAction, hc}; use slog::{Logger, debug, error, warn}; use std::collections::HashMap; use std::collections::hash_map::Entry; use std::time::Instant; -// Node and property names from . -const NVME: &str = "nvme"; -const BAY: &str = "bay"; -const SLOT: &str = "slot"; -const TOPO_PGROUP_IO: &str = "io"; -const TOPO_IO_INSTANCE: &str = "instance"; - /// Takes a fresh topology snapshot and returns the chassis label of every /// NVMe controller that has one, keyed by driver instance. /// @@ -43,7 +36,7 @@ pub(super) fn read_disk_locations( let mut labels = HashMap::new(); snap.walk(Scheme::Hc, |node| { - if node.name() != NVME { + if node.name() != hc::NVME { return Ok(WalkAction::Continue); } let Some(instance) = nvme_instance_of(log, &node) else { @@ -85,25 +78,26 @@ pub(super) fn read_disk_locations( /// `None`, with a log line, if the node has no such property or it is not the /// uint32 topo documents it as. fn nvme_instance_of(log: &Logger, node: &Node<'_>) -> Option { - let value = match node.property(TOPO_PGROUP_IO, TOPO_IO_INSTANCE) { - Ok(PropValue::UInt32(value)) => value, - Ok(other) => { - warn!( - log, - "ignoring nvme topology node whose io/instance is not a uint32"; - "value" => ?other, - ); - return None; - } - Err(_) => { - debug!( - log, - "nvme topology node has no io/instance property"; - "node_instance" => node.instance(), - ); - return None; - } - }; + let value = + match node.property_u32(hc::TOPO_PGROUP_IO, hc::TOPO_IO_INSTANCE) { + Ok(value) => value, + Err(Error::PropertyNotFound { .. }) => { + debug!( + log, + "nvme topology node has no io/instance property"; + "node_instance" => node.instance(), + ); + return None; + } + Err(err) => { + warn!( + log, + "ignoring nvme topology node with unreadable io/instance"; + "err" => %err, + ); + return None; + } + }; match NvmeInstance::try_from(value) { Ok(instance) => Some(instance), Err(err) => { @@ -129,7 +123,7 @@ fn location_label_of(node: &Node<'_>) -> Option { node.parent() .filter(|parent| { let name = parent.name(); - name == BAY || name == SLOT + name == hc::BAY || name == hc::SLOT }) .and_then(|parent| label_of(&parent)) }) From 901fb2acd72dad0ef32855210d0b1c3a72d15768 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 16 Sep 2026 11:34:13 -0700 Subject: [PATCH 12/19] Set libtopo's runpath through omicron-rpaths, not .cargo/config.toml The workspace's documented rule is that .cargo/config.toml carries only the Oxide platform directory, and every other runtime path goes through the omicron-rpaths crate with a direct dependency on the -sys crate that emits DEP_*_LIBDIRS. libfmd_adm, which lives in the same /usr/lib/fm/amd64 directory, already follows that rule. Follow it for libtopo too: - omicron-rpaths scans DEP_TOPO_LIBDIRS, which libtopo-sys emits. - Every crate whose binaries or test binaries link libtopo.so.1 declares libtopo-sys directly and calls omicron-rpaths from build.rs. On illumos rustc links with -z ignore, so only binaries that actually reach the topo code record the dependency. The set was found by building test binaries for every crate that transitively depends on sled-hardware on a Helios host and checking NEEDED with elfdump. - Revert the /usr/lib/fm/amd64 additions to .cargo/config.toml and .github/buildomat/build-and-test.sh. Test coverage: on a Helios host, cargo test --no-run for all 40 workspace crates that reach sled-hardware, then elfdump on every produced executable. 21 need libtopo.so.1 and all carry /usr/lib/fm/amd64 in RUNPATH; ldd resolves libtopo and the test binaries run. cargo hakari generate --diff, cargo xtask check-workspace-deps, and cargo xtask clippy are clean on Linux. --- .cargo/config.toml | 57 ++++++++----------- .github/buildomat/build-and-test.sh | 6 +- Cargo.lock | 25 ++++++++ Cargo.toml | 1 + dev-tools/omdb/Cargo.toml | 2 + dev-tools/omicron-dev/Cargo.toml | 2 + installinator/Cargo.toml | 7 +++ installinator/build.rs | 10 ++++ nexus/Cargo.toml | 2 + nexus/db-queries/Cargo.toml | 2 + nexus/inventory/Cargo.toml | 2 + .../cli-integration-tests/Cargo.toml | 2 + nexus/reconfigurator/execution/Cargo.toml | 2 + nexus/test-utils/Cargo.toml | 2 + package/Cargo.toml | 7 +++ package/build.rs | 10 ++++ rpaths/src/lib.rs | 16 +++--- sled-agent/Cargo.toml | 2 + sled-agent/bootstrap-common/Cargo.toml | 7 +++ sled-agent/bootstrap-common/build.rs | 10 ++++ sled-agent/config-reconciler/Cargo.toml | 7 +++ sled-agent/config-reconciler/build.rs | 10 ++++ sled-agent/rack-setup/Cargo.toml | 7 +++ sled-agent/rack-setup/build.rs | 10 ++++ sled-agent/resolvable-files/Cargo.toml | 7 +++ sled-agent/resolvable-files/build.rs | 10 ++++ sled-diagnostics/Cargo.toml | 7 +++ sled-diagnostics/build.rs | 10 ++++ trust-quorum/Cargo.toml | 7 +++ trust-quorum/build.rs | 10 ++++ 30 files changed, 216 insertions(+), 43 deletions(-) create mode 100644 installinator/build.rs create mode 100644 package/build.rs create mode 100644 sled-agent/bootstrap-common/build.rs create mode 100644 sled-agent/config-reconciler/build.rs create mode 100644 sled-agent/rack-setup/build.rs create mode 100644 sled-agent/resolvable-files/build.rs create mode 100644 sled-diagnostics/build.rs create mode 100644 trust-quorum/build.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index e83cf8e6132..6dc94b48356 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -21,34 +21,28 @@ rustdocflags = "--document-private-items" rustflags = "--cfg tokio_unstable" -# On illumos, use `-znocompstrtab` to reduce link time. We also add two -# directories to the RPATH where libraries outside the default search path -# live: +# On illumos, use `-znocompstrtab` to reduce link time. We also add the Oxide +# specific platform directory to the RPATH where additional libraries can be +# found such as libipcc. # -# - /usr/platform/oxide/lib/amd64 holds Oxide-specific libraries such as -# libipcc, and will likely grow over time to include more functionality. -# - /usr/lib/fm/amd64 holds the illumos fault management libraries such as -# libtopo, which sled-hardware uses to find each disk's chassis location. -# -# Our reasoning for putting these here rather than going through the rpaths -# crate: -# - Static, illumos-only paths - The rpaths crate was built to deal with -# runtime paths that are dynamic such as with libraries like libpq which can -# live in different locations based on OS. These paths will only ever be -# found on illumos and never move. -# - Less developer burden - Having a -sys crate emit `DEP_*_LIBDIRS` means that -# we end up littering the repo with Cargo.toml and build.rs changes in every -# crate whose binaries or tests transitively link the library. For libipcc -# that was already painful; libtopo enters through sled-hardware, which well -# over a hundred workspace crates reach transitively. While initially -# exploring that path for libipcc we ran into a significant number of tests -# failing due to not being able to find the library in the runtime path, +# Our reasoning for including `-R/usr/platform/oxide/lib/amd64` here: +# - Oxide specific features - This path contains Oxide specific libraries such +# as libipcc and will likely grow over time to include more functionality. +# - Avoid the rpaths crate - The rpaths crate was built to deal with runtime +# paths that are dynamic such as with libraries like libpq which can live in +# different locations based on OS. This path will only ever be found on +# illumos and will be tied directly to the Oxide platform. +# - Less developer burden - Having something like the ipcc crate emit +# `DEP_IPCC_LIBDIRS` means that we end up littering the repo with Cargo.toml +# and build.rs changes whenever ipcc shows up somewhere in the dependency +# tree. While initially exploring that path we ran into a significant number +# of tests failing due to not being able to find libipcc in the runtime path # which can be confusing or surprising to someone doing work on omicron. # -# We could also update Helios so that symlinks are created from these -# directories into /usr/lib/64 but we opted to not take this route forward as -# it meant waiting for another round of updates on shared build machines and -# to buildomat itself. +# We could also update Helios so that a symlink is created from +# /usr/platform/oxide/lib/amd64 into /usr/lib/64 but we opted to not take +# this route forward as it meant waiting for another round of updates on +# shared build machines and to buildomat itself. # # As documented at: # https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags @@ -64,16 +58,13 @@ rustflags = "--cfg tokio_unstable" # When overriding the defaults in this config by environment variable the user # may need to manually pass the additional options found below. # -# A runtime path belongs here only if it is fixed and illumos-only, and the -# library behind it is reached transitively by a large share of the workspace. -# Paths that vary with the host OS, or that only a handful of crates need, -# should instead reuse the infrastructure found in the `rpaths` crate in this -# repo. Whatever is added here must also be added to the `RUSTFLAGS` override -# in .github/buildomat/build-and-test.sh, which takes precedence over this -# file. +# Note that other runtime paths should not be added here, but should instead +# reuse the infrastructure found in the `rpaths` crate which can be found in +# this repo. Those paths are usually platform specific and will vary based on a +# variety of things such as host OS. [target.x86_64-unknown-illumos] rustflags = [ - "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64", + "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64", # enable the `tokio-unstable` config for `tokio-dtrace` probes "--cfg", "tokio_unstable" ] diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index c443c236999..030d3abe791 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -101,10 +101,10 @@ export RUSTFLAGS="--cfg tokio_unstable -D warnings" export RUSTDOCFLAGS="--document-private-items -D warnings" # When running on illumos we need to pass an additional runpath that is # usually configured via ".cargo/config" but the `RUSTFLAGS` env variable -# takes precedence. These paths contain oxide specific libraries such as -# libipcc, and the fault management libraries such as libtopo. +# takes precedence. This path contains oxide specific libraries such as +# libipcc. if [[ $target_os == "illumos" ]]; then - RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64" + RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64" fi export TMPDIR="$TEST_TMPDIR" export RUST_BACKTRACE=1 diff --git a/Cargo.lock b/Cargo.lock index 6112af103fe..f92b4124f2b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5800,8 +5800,10 @@ dependencies = [ "ipcc", "itertools 0.14.0", "libc", + "libtopo-sys", "omicron-common", "omicron-ddm-admin-client", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -7502,6 +7504,7 @@ dependencies = [ "internal-dns-types", "ipnetwork", "itertools 0.14.0", + "libtopo-sys", "macaddr", "nexus-auth", "nexus-config", @@ -7722,6 +7725,7 @@ dependencies = [ "iddqd", "illumos-utils", "itertools 0.14.0", + "libtopo-sys", "nexus-client", "nexus-types", "ntp-admin-client", @@ -7939,6 +7943,7 @@ dependencies = [ "camino-tempfile", "clap", "fmd-adm-sys", + "libtopo-sys", "nexus-db-queries", "nexus-lockstep-client", "nexus-reconfigurator-preparation", @@ -7983,6 +7988,7 @@ dependencies = [ "internal-dns-types", "ipnet", "ipnetwork", + "libtopo-sys", "newtype-uuid", "nexus-config", "nexus-db-lookup", @@ -8276,6 +8282,7 @@ dependencies = [ "illumos-utils", "internal-dns-resolver", "internal-dns-types", + "libtopo-sys", "nexus-client", "nexus-config", "nexus-db-queries", @@ -9047,6 +9054,7 @@ dependencies = [ "gateway-client", "gateway-test-utils", "libc", + "libtopo-sys", "nexus-config", "nexus-test-interface", "nexus-test-utils", @@ -9322,6 +9330,7 @@ dependencies = [ "ipnetwork", "itertools 0.14.0", "jiff", + "libtopo-sys", "lldpd-client", "macaddr", "maplit", @@ -9534,6 +9543,7 @@ dependencies = [ "internal-dns-types", "ipnetwork", "itertools 0.14.0", + "libtopo-sys", "multimap", "nexus-config", "nexus-db-errors", @@ -9611,6 +9621,8 @@ dependencies = [ "illumos-utils", "indent_write", "indicatif", + "libtopo-sys", + "omicron-rpaths", "omicron-workspace-hack", "omicron-zone-package", "oxide-tokio-rt", @@ -9831,6 +9843,7 @@ dependencies = [ "itertools 0.14.0", "key-manager", "libc", + "libtopo-sys", "macaddr", "nexus-client", "nexus-config", @@ -14285,8 +14298,10 @@ version = "0.1.0" dependencies = [ "bootstore", "camino", + "libtopo-sys", "omicron-common", "omicron-ledger", + "omicron-rpaths", "omicron-uuid-kinds", "omicron-workspace-hack", "serde", @@ -14358,11 +14373,13 @@ dependencies = [ "key-manager", "key-manager-types", "libc", + "libtopo-sys", "ntp-admin-client", "omicron-common", "omicron-debug-dropbox", "omicron-generation-kinds", "omicron-ledger", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -14493,6 +14510,7 @@ dependencies = [ "internal-dns-resolver", "internal-dns-types", "itertools 0.14.0", + "libtopo-sys", "nexus-lockstep-client", "nexus-reconfigurator-blippy", "nexus-types", @@ -14501,6 +14519,7 @@ dependencies = [ "omicron-ddm-admin-client", "omicron-generation-kinds", "omicron-ledger", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -14538,7 +14557,9 @@ dependencies = [ "expectorate", "iddqd", "illumos-utils", + "libtopo-sys", "omicron-common", + "omicron-rpaths", "omicron-uuid-kinds", "omicron-workspace-hack", "pretty_assertions", @@ -14703,7 +14724,9 @@ dependencies = [ "illumos-utils", "jiff", "libc", + "libtopo-sys", "omicron-common", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -16713,7 +16736,9 @@ dependencies = [ "hex", "hkdf 0.12.4", "iddqd", + "libtopo-sys", "omicron-ledger", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", diff --git a/Cargo.toml b/Cargo.toml index 89ebd379907..3dc5e55970c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -621,6 +621,7 @@ linkme = "0.3" libipcc = { git = "https://github.com/oxidecomputer/ipcc-rs", rev = "524eb8f125003dff50b9703900c6b323f00f9e1b" } libnvme = { git = "https://github.com/oxidecomputer/libnvme", rev = "dd5bb221d327a1bc9287961718c3c10d6bd37da0" } libtopo = "0.2.0" +libtopo-sys = "0.1.1" linear-map = "1.2.0" live-tests-macros = { path = "live-tests/macros" } lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" } diff --git a/dev-tools/omdb/Cargo.toml b/dev-tools/omdb/Cargo.toml index 85e1e1a4986..4391b51a4ae 100644 --- a/dev-tools/omdb/Cargo.toml +++ b/dev-tools/omdb/Cargo.toml @@ -109,6 +109,8 @@ uuid.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] camino-tempfile.workspace = true diff --git a/dev-tools/omicron-dev/Cargo.toml b/dev-tools/omicron-dev/Cargo.toml index a21ed7e90a6..9895bd5b209 100644 --- a/dev-tools/omicron-dev/Cargo.toml +++ b/dev-tools/omicron-dev/Cargo.toml @@ -34,6 +34,8 @@ toml.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] expectorate.workspace = true diff --git a/installinator/Cargo.toml b/installinator/Cargo.toml index 119605aa53c..c08a136c47b 100644 --- a/installinator/Cargo.toml +++ b/installinator/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true async-trait.workspace = true @@ -54,6 +57,10 @@ oxide-update-engine.workspace = true oxide-update-engine-types.workspace = true omicron-workspace-hack.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] omicron-test-utils.workspace = true hex-literal.workspace = true diff --git a/installinator/build.rs b/installinator/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/installinator/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index fd925632d9f..fc6aaa967ac 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -173,6 +173,8 @@ zip = { workspace = true, features = ["jiff-02"] } [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] async-bb8-diesel.workspace = true diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index e12449f630c..28aaa89d3c5 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -90,6 +90,8 @@ omicron-test-utils = { workspace = true, optional = true } [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [features] # Enable to export `TestDatabase` diff --git a/nexus/inventory/Cargo.toml b/nexus/inventory/Cargo.toml index 053c6a5a4d3..edf5d2cafb4 100644 --- a/nexus/inventory/Cargo.toml +++ b/nexus/inventory/Cargo.toml @@ -54,6 +54,8 @@ omicron-workspace-hack.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] expectorate.workspace = true diff --git a/nexus/reconfigurator/cli-integration-tests/Cargo.toml b/nexus/reconfigurator/cli-integration-tests/Cargo.toml index 6ce589023c3..03f1c72755b 100644 --- a/nexus/reconfigurator/cli-integration-tests/Cargo.toml +++ b/nexus/reconfigurator/cli-integration-tests/Cargo.toml @@ -22,6 +22,8 @@ reconfigurator-cli.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] camino.workspace = true diff --git a/nexus/reconfigurator/execution/Cargo.toml b/nexus/reconfigurator/execution/Cargo.toml index bffb9487e87..5b7ffb9c909 100644 --- a/nexus/reconfigurator/execution/Cargo.toml +++ b/nexus/reconfigurator/execution/Cargo.toml @@ -53,6 +53,8 @@ omicron-workspace-hack.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [dev-dependencies] async-bb8-diesel.workspace = true diff --git a/nexus/test-utils/Cargo.toml b/nexus/test-utils/Cargo.toml index 1f5f8f66aa8..01692c70e8b 100644 --- a/nexus/test-utils/Cargo.toml +++ b/nexus/test-utils/Cargo.toml @@ -73,6 +73,8 @@ uuid.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true [features] omicron-dev = ["omicron-test-utils/seed-gen"] diff --git a/package/Cargo.toml b/package/Cargo.toml index bdae0a5cf38..10cb2c3f35e 100644 --- a/package/Cargo.toml +++ b/package/Cargo.toml @@ -8,6 +8,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true camino.workspace = true @@ -42,6 +45,10 @@ tokio = { workspace = true, features = [ "full" ] } toml.workspace = true walkdir.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] expectorate.workspace = true diff --git a/package/build.rs b/package/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/package/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/rpaths/src/lib.rs b/rpaths/src/lib.rs index 3f95f67e368..080b5b43e4d 100644 --- a/rpaths/src/lib.rs +++ b/rpaths/src/lib.rs @@ -8,10 +8,11 @@ //! //! This build-time crate is used by several top-level Omicron crates to set //! RPATH so that native libraries linked via *-sys crates can be found at -//! runtime. Currently we do this for two libraries: +//! runtime. Currently we do this for three libraries: //! //! - **libpq** (via pq-sys, pulled in by diesel) //! - **libfmd_adm** (via fmd-adm-sys, pulled in by fmd-adm in sled-agent) +//! - **libtopo** (via libtopo-sys, pulled in by libtopo in sled-hardware) //! //! Cargo/Rust have no built-in way to set the RPATH for a transitively-linked //! native library. (See below.) So we've developed the pattern here instead. @@ -19,7 +20,8 @@ //! //! 1. Any crate that depends (directly or transitively) on a -sys crate from //! the list above needs to follow these instructions. Often the dep is -//! indirect — pq-sys arrives via diesel, fmd-adm-sys via omicron-sled-agent. +//! indirect — pq-sys arrives via diesel, fmd-adm-sys via omicron-sled-agent, +//! libtopo-sys via sled-hardware. //! 2. Affected crates (e.g., omicron-nexus) have a build.rs that just calls //! `omicron_rpath::configure_default_omicron_rpaths()`. //! 3. These crates must also add a *direct* dependency on the corresponding @@ -33,8 +35,8 @@ //! 4. The metadata that drives this comes from the -sys crate's build.rs: //! - For pq-sys, we maintain a fork (see `[patch.crates-io.pq-sys]` //! in the workspace Cargo.toml) that emits `cargo:LIBDIRS=...`. -//! - For fmd-adm-sys, the upstream crate emits the metadata directly, -//! so no patch is needed. +//! - For fmd-adm-sys and libtopo-sys, the upstream crates emit the +//! metadata directly, so no patch is needed. //! //! `configure_default_omicron_rpaths()` scans for every `DEP_*_LIBDIRS` env //! var in `RPATH_ENV_VARS`. Each crate's build.rs makes the same call — @@ -69,8 +71,8 @@ //! As of 1.56, Cargo supports the "cargo:rustc-link-arg" instruction for use by //! [Build Scripts][3] to pass arbitrary options to the linker. We use that //! here to tell the linker to include the correct RPATH entries for the -//! native dependencies affected by this (currently libpq via pq-sys and -//! libfmd_adm via fmd-adm-sys). +//! native dependencies affected by this (currently libpq via pq-sys, +//! libfmd_adm via fmd-adm-sys, and libtopo via libtopo-sys). //! //! A subtle but critical point here is that the RPATH is knowable only by the //! system that's building the top-level executable binary. This mechanism can't @@ -177,7 +179,7 @@ mod internal { /// (say) fmd-adm-sys simply won't have `DEP_FMD_ADM_LIBDIRS` set, and we /// skip it silently. pub static RPATH_ENV_VARS: &'static [&'static str] = - &["DEP_PQ_LIBDIRS", "DEP_FMD_ADM_LIBDIRS"]; + &["DEP_PQ_LIBDIRS", "DEP_FMD_ADM_LIBDIRS", "DEP_TOPO_LIBDIRS"]; /// Tells Cargo to pass linker arguments that specify RPATHs from the /// environment variable `env_var_name`, if it is set. diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index 47577484c32..4ffa5c305be 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -138,6 +138,8 @@ fmd-adm.workspace = true # Direct dep on fmd-adm-sys so this crate sees DEP_FMD_ADM_LIBDIRS in build.rs # and can configure RPATH via omicron-rpaths. fmd-adm-sys.workspace = true +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true opte-ioctl.workspace = true [dev-dependencies] diff --git a/sled-agent/bootstrap-common/Cargo.toml b/sled-agent/bootstrap-common/Cargo.toml index b886631c98b..84eaeb4cc48 100644 --- a/sled-agent/bootstrap-common/Cargo.toml +++ b/sled-agent/bootstrap-common/Cargo.toml @@ -8,6 +8,9 @@ description = "shared code for bootstrap agent related crates" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] bootstore.workspace = true camino.workspace = true @@ -28,3 +31,7 @@ trust-quorum.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true + +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true diff --git a/sled-agent/bootstrap-common/build.rs b/sled-agent/bootstrap-common/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/sled-agent/bootstrap-common/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/sled-agent/config-reconciler/Cargo.toml b/sled-agent/config-reconciler/Cargo.toml index 1f407d6d5a2..61a920dd727 100644 --- a/sled-agent/config-reconciler/Cargo.toml +++ b/sled-agent/config-reconciler/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true async-recursion.workspace = true @@ -56,6 +59,10 @@ zone.workspace = true omicron-workspace-hack.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] assert_matches.workspace = true camino-tempfile-ext.workspace = true diff --git a/sled-agent/config-reconciler/build.rs b/sled-agent/config-reconciler/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/sled-agent/config-reconciler/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/sled-agent/rack-setup/Cargo.toml b/sled-agent/rack-setup/Cargo.toml index bb4dc43b439..9ba90457a71 100644 --- a/sled-agent/rack-setup/Cargo.toml +++ b/sled-agent/rack-setup/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true bootstore.workspace = true @@ -54,6 +57,10 @@ trust-quorum-protocol.workspace = true trust-quorum-types.workspace = true uuid.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] camino-tempfile.workspace = true nexus-reconfigurator-blippy.workspace = true diff --git a/sled-agent/rack-setup/build.rs b/sled-agent/rack-setup/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/sled-agent/rack-setup/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/sled-agent/resolvable-files/Cargo.toml b/sled-agent/resolvable-files/Cargo.toml index da3c15bebb6..b213b3a9c85 100644 --- a/sled-agent/resolvable-files/Cargo.toml +++ b/sled-agent/resolvable-files/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] camino.workspace = true iddqd.workspace = true @@ -26,6 +29,10 @@ slog-error-chain.workspace = true thiserror.workspace = true tufaceous-artifact.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] camino-tempfile-ext.workspace = true dropshot.workspace = true diff --git a/sled-agent/resolvable-files/build.rs b/sled-agent/resolvable-files/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/sled-agent/resolvable-files/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/sled-diagnostics/Cargo.toml b/sled-diagnostics/Cargo.toml index 1d4725175bc..4b9a6e4e337 100644 --- a/sled-diagnostics/Cargo.toml +++ b/sled-diagnostics/Cargo.toml @@ -6,6 +6,9 @@ edition.workspace = true [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true camino.workspace = true @@ -31,6 +34,10 @@ tokio = { workspace = true, features = ["full"] } zfs-test-harness.workspace = true zip = { workspace = true, features = ["jiff-02","zstd"] } +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] omicron-common.workspace = true omicron-test-utils.workspace = true diff --git a/sled-diagnostics/build.rs b/sled-diagnostics/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/sled-diagnostics/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/trust-quorum/Cargo.toml b/trust-quorum/Cargo.toml index 7b2b21a5835..e3be7a115bf 100644 --- a/trust-quorum/Cargo.toml +++ b/trust-quorum/Cargo.toml @@ -8,6 +8,9 @@ description = "trust quorum library for use by bootstrap agent" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true bootstore.workspace = true @@ -46,6 +49,10 @@ uuid.workspace = true zeroize.workspace = true omicron-workspace-hack.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] assert_matches.workspace = true attest-mock.workspace = true diff --git a/trust-quorum/build.rs b/trust-quorum/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/trust-quorum/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} From 37dec5e54cec9b112a6130dde0a96d3835f90b18 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 16 Sep 2026 11:59:28 -0700 Subject: [PATCH 13/19] Read disk locations from topo on every hardware poll DiskLocationCache decided whether to take a topo snapshot based on whether every present NVMe controller already had a label. That is the wrong signal once anything else is read from the snapshot, and a wrong cache decision silently withholds data. Drop the cache: each hardware poll takes one topo snapshot and joins its labels to the disks found through devinfo. A topo failure still costs only the locations, and an unlabelled controller is logged once per poll. The snapshot costs a few hundred milliseconds on a sled against a five second poll interval. If that matters, the fix is a slower poll woken eagerly by sysevents, not a cache in front of topo. Test coverage: sled-hardware unit tests and clippy for sled-hardware and omicron-sled-agent on a Helios host; cargo check and the remaining disk_location test on Linux. --- sled-hardware/src/disk_location.rs | 360 +---------------------------- sled-hardware/src/illumos/mod.rs | 72 +++--- 2 files changed, 39 insertions(+), 393 deletions(-) diff --git a/sled-hardware/src/disk_location.rs b/sled-hardware/src/disk_location.rs index fc036d5fdf5..2bd45c06e7e 100644 --- a/sled-hardware/src/disk_location.rs +++ b/sled-hardware/src/disk_location.rs @@ -4,18 +4,12 @@ //! Chassis locations of disks, learned from the platform hardware topology. //! -//! The illumos fault management topology (libtopo) knows which bay or M.2 -//! socket each NVMe controller sits in and labels it the way the chassis -//! does: "N5" for a U.2 bay, "M.2 East" for a boot device. This module holds -//! the platform-independent parts of asking for that label: the key that -//! joins devinfo's view of a controller to topo's, and a cache that decides -//! when topo needs to be consulted at all. +//! libtopo knows which bay or M.2 socket each NVMe controller sits in and +//! labels it the way the chassis does: "N5" for a U.2 bay, "M.2 East" for a +//! boot device. This module holds the key that joins devinfo's view of a +//! controller to topo's, so the label can be attached to the right disk. -use slog::{Logger, warn}; -use std::collections::HashMap; -use std::collections::hash_map::Entry; use std::fmt; -use std::time::{Duration, Instant}; /// Instance number of an `nvme` driver node, the `N` in `nvme`. /// @@ -69,163 +63,14 @@ impl fmt::Display for NvmeInstance { } } -/// Remembers what the platform topology said about each NVMe controller, so -/// that topo, whose snapshot takes hundreds of milliseconds on a sled, is -/// consulted only when a controller appears that it has not been asked about. -/// -/// A controller's location cannot change while the system is up. Its instance -/// number is fixed by its devinfo path, and its label comes from the -/// platform's static topology map. A label, once learned, is therefore kept -/// for the life of the process. -#[derive(Debug, Default)] -pub struct DiskLocationCache { - /// What topo said about each controller the last time it was asked. - /// `None` means it was asked and had no label, or topo could not be read. - /// A controller absent from the map has never been asked about. - labels: HashMap>, - /// When topo was last consulted. Throttles re-asking about `None` - /// entries; never delays the first question about a new controller. - last_attempt: Option, -} - -impl DiskLocationCache { - /// Minimum time between repeated attempts to resolve a controller that - /// topo previously had no label for. - pub const RETRY_INTERVAL: Duration = Duration::from_secs(60); - - pub fn new() -> Self { - Self::default() - } - - /// Consults topo, through `read_labels`, if any controller in `present` - /// calls for it: immediately for a controller never asked about, and at - /// most once per [`Self::RETRY_INTERVAL`] for controllers topo previously - /// had no label for. Afterwards every controller in `present` has an - /// entry, so the steady state of repeated polls with the same controllers - /// never touches topo. - /// - /// `read_labels` returns the label of every controller topo has one for. - /// On success its labels are merged into the cache: a new or changed - /// label is adopted, but a controller the cache has already labelled is - /// never forgotten because a later read omitted it. On failure the labels - /// already known are kept and the failure is logged. - pub fn refresh_if_needed( - &mut self, - log: &Logger, - present: &[NvmeInstance], - now: Instant, - read_labels: F, - ) where - F: FnOnce() -> Result, E>, - E: fmt::Display, - { - if !self.should_refresh(present, now) { - return; - } - self.last_attempt = Some(now); - - let read_ok = match read_labels() { - Ok(read) => { - // A present controller we already labelled should be in every - // successful read. If it is not, keep its label: the omission - // could be a transient enumeration failure or a topo bug, and - // a controller's location cannot change while the system is - // up. - for instance in present { - if let Some(Some(kept)) = self.labels.get(instance) - && !read.contains_key(instance) - { - warn!( - log, - "hardware topology omitted a disk controller it \ - previously labelled; keeping the label"; - "nvme_instance" => %instance, - "label" => kept, - ); - } - } - - for (instance, label) in read { - match self.labels.entry(instance) { - Entry::Occupied(mut entry) => { - if let Some(old) = entry.get() - && *old != label - { - warn!( - log, - "disk controller location label changed"; - "nvme_instance" => %instance, - "old" => old, - "new" => &label, - ); - } - entry.insert(Some(label)); - } - Entry::Vacant(entry) => { - entry.insert(Some(label)); - } - } - } - true - } - Err(err) => { - warn!( - log, - "failed to read disk locations from hardware topology"; - "err" => %err, - ); - false - } - }; - - for instance in present { - let entry = self.labels.entry(*instance).or_insert(None); - if entry.is_none() && read_ok { - warn!( - log, - "hardware topology has no location label for disk \ - controller"; - "nvme_instance" => %instance, - ); - } - } - } - - fn should_refresh(&self, present: &[NvmeInstance], now: Instant) -> bool { - let mut retry_unlabelled = false; - for instance in present { - match self.labels.get(instance) { - None => return true, - Some(None) => retry_unlabelled = true, - Some(Some(_)) => {} - } - } - retry_unlabelled - && self.last_attempt.is_none_or(|last| { - now.duration_since(last) >= Self::RETRY_INTERVAL - }) - } - - /// The chassis location label of a controller, if topo had one. - pub fn location(&self, instance: NvmeInstance) -> Option<&str> { - self.labels.get(&instance).and_then(|label| label.as_deref()) - } -} - #[cfg(test)] mod tests { use super::*; - use omicron_test_utils::dev::test_setup_log; - use std::cell::Cell; fn inst(n: i32) -> NvmeInstance { NvmeInstance::try_from(n).unwrap() } - fn labels(pairs: &[(i32, &str)]) -> HashMap { - pairs.iter().map(|(n, l)| (inst(*n), l.to_string())).collect() - } - #[test] fn nvme_instance_conversions() { assert_eq!(inst(0).as_i32(), 0); @@ -235,201 +80,4 @@ mod tests { assert!(NvmeInstance::try_from(u32::MAX).is_err()); assert_eq!(inst(5).to_string(), "nvme5"); } - - #[test] - fn first_sight_consults_topo_then_steady_state_does_not() { - let logctx = test_setup_log( - "first_sight_consults_topo_then_steady_state_does_not", - ); - let mut cache = DiskLocationCache::new(); - let calls = Cell::new(0); - let t0 = Instant::now(); - let present = [inst(0), inst(1)]; - - cache.refresh_if_needed(&logctx.log, &present, t0, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) - }); - assert_eq!(calls.get(), 1); - assert_eq!(cache.location(inst(0)), Some("N0")); - assert_eq!(cache.location(inst(1)), Some("N1")); - - // Same controllers, an hour later: nothing to ask. - let later = t0 + Duration::from_secs(3600); - cache.refresh_if_needed(&logctx.log, &present, later, || { - calls.set(calls.get() + 1); - Ok::<_, String>(HashMap::new()) - }); - assert_eq!(calls.get(), 1); - assert_eq!(cache.location(inst(0)), Some("N0")); - logctx.cleanup_successful(); - } - - #[test] - fn unlabelled_controller_is_retried_with_throttle() { - let logctx = - test_setup_log("unlabelled_controller_is_retried_with_throttle"); - let mut cache = DiskLocationCache::new(); - let calls = Cell::new(0); - let t0 = Instant::now(); - let present = [inst(0)]; - - // Topo knows nothing about this controller. - cache.refresh_if_needed(&logctx.log, &present, t0, || { - calls.set(calls.get() + 1); - Ok::<_, String>(HashMap::new()) - }); - assert_eq!(calls.get(), 1); - assert_eq!(cache.location(inst(0)), None); - - // Too soon to ask again. - let soon = t0 + Duration::from_secs(30); - cache.refresh_if_needed(&logctx.log, &present, soon, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0")])) - }); - assert_eq!(calls.get(), 1); - assert_eq!(cache.location(inst(0)), None); - - // After the retry interval the question is asked again. - let retry = t0 + DiskLocationCache::RETRY_INTERVAL; - cache.refresh_if_needed(&logctx.log, &present, retry, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0")])) - }); - assert_eq!(calls.get(), 2); - assert_eq!(cache.location(inst(0)), Some("N0")); - logctx.cleanup_successful(); - } - - #[test] - fn new_controller_bypasses_throttle() { - let logctx = test_setup_log("new_controller_bypasses_throttle"); - let mut cache = DiskLocationCache::new(); - let calls = Cell::new(0); - let t0 = Instant::now(); - - cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { - calls.set(calls.get() + 1); - Ok::<_, String>(HashMap::new()) - }); - assert_eq!(calls.get(), 1); - - // A controller never seen before shows up well inside the retry - // interval; it is asked about right away. - let soon = t0 + Duration::from_secs(5); - cache.refresh_if_needed(&logctx.log, &[inst(0), inst(1)], soon, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(1, "N1")])) - }); - assert_eq!(calls.get(), 2); - assert_eq!(cache.location(inst(0)), None); - assert_eq!(cache.location(inst(1)), Some("N1")); - logctx.cleanup_successful(); - } - - #[test] - fn topo_failure_keeps_known_labels_and_is_throttled() { - let logctx = - test_setup_log("topo_failure_keeps_known_labels_and_is_throttled"); - let mut cache = DiskLocationCache::new(); - let calls = Cell::new(0); - let t0 = Instant::now(); - - cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0")])) - }); - assert_eq!(calls.get(), 1); - - // A new controller appears but topo is broken. - let t1 = t0 + Duration::from_secs(1); - let present = [inst(0), inst(1)]; - cache.refresh_if_needed(&logctx.log, &present, t1, || { - calls.set(calls.get() + 1); - Err::, _>("topo exploded") - }); - assert_eq!(calls.get(), 2); - assert_eq!(cache.location(inst(0)), Some("N0")); - assert_eq!(cache.location(inst(1)), None); - - // The failure is throttled like any other unresolved controller. - let t2 = t1 + Duration::from_secs(2); - cache.refresh_if_needed(&logctx.log, &present, t2, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) - }); - assert_eq!(calls.get(), 2); - assert_eq!(cache.location(inst(1)), None); - - let t3 = t1 + DiskLocationCache::RETRY_INTERVAL; - cache.refresh_if_needed(&logctx.log, &present, t3, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0"), (1, "N1")])) - }); - assert_eq!(calls.get(), 3); - assert_eq!(cache.location(inst(1)), Some("N1")); - logctx.cleanup_successful(); - } - - #[test] - fn omitted_controller_keeps_its_label() { - let logctx = test_setup_log("omitted_controller_keeps_its_label"); - let mut cache = DiskLocationCache::new(); - let calls = Cell::new(0); - let t0 = Instant::now(); - - cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(0, "N0")])) - }); - assert_eq!(calls.get(), 1); - assert_eq!(cache.location(inst(0)), Some("N0")); - - // A new controller forces a refresh, and this read leaves out the - // controller we already know about. - let t1 = t0 + Duration::from_secs(1); - let present = [inst(0), inst(1)]; - cache.refresh_if_needed(&logctx.log, &present, t1, || { - calls.set(calls.get() + 1); - Ok::<_, String>(labels(&[(1, "N1")])) - }); - assert_eq!(calls.get(), 2); - assert_eq!(cache.location(inst(0)), Some("N0")); - assert_eq!(cache.location(inst(1)), Some("N1")); - - // Both controllers are resolved, so nothing prompts another read. - let later = t1 + Duration::from_secs(3600); - cache.refresh_if_needed(&logctx.log, &present, later, || { - calls.set(calls.get() + 1); - Ok::<_, String>(HashMap::new()) - }); - assert_eq!(calls.get(), 2); - logctx.cleanup_successful(); - } - - #[test] - fn changed_label_is_adopted() { - let logctx = test_setup_log("changed_label_is_adopted"); - let mut cache = DiskLocationCache::new(); - let t0 = Instant::now(); - - cache.refresh_if_needed(&logctx.log, &[inst(0)], t0, || { - Ok::<_, String>(labels(&[(0, "N0")])) - }); - assert_eq!(cache.location(inst(0)), Some("N0")); - - // A new controller forces a refresh, and topo now reports a different - // label for the controller we already know about. - let present = [inst(0), inst(1)]; - cache.refresh_if_needed( - &logctx.log, - &present, - t0 + Duration::from_secs(1), - || Ok::<_, String>(labels(&[(0, "N9"), (1, "N1")])), - ); - assert_eq!(cache.location(inst(0)), Some("N9")); - assert_eq!(cache.location(inst(1)), Some("N1")); - logctx.cleanup_successful(); - } } diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index a038f2059d4..56d05883128 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -7,7 +7,7 @@ use crate::ExternalDisks; use crate::HardwareView; use crate::TofinoSnapshot; use crate::TofinoView; -use crate::disk_location::{DiskLocationCache, NvmeInstance}; +use crate::disk_location::NvmeInstance; use crate::{DendriteAsic, SledMode, UnparsedDisk}; use camino::Utf8PathBuf; use gethostname::gethostname; @@ -23,7 +23,6 @@ use slog::info; use slog::o; use slog::warn; use std::collections::{HashMap, HashSet}; -use std::time::Instant; use tokio::sync::watch; use uuid::Uuid; @@ -139,7 +138,6 @@ impl HardwareSnapshot { fn new( log: &Logger, external_disks: &ExternalDisks, - location_cache: &mut DiskLocationCache, ) -> Result { let mut device_info = DevInfo::new_force_load().map_err(Error::DevInfo)?; @@ -188,24 +186,42 @@ impl HardwareSnapshot { } // Now that we know which controllers are present, ask the - // hardware topology where they are. The cache decides whether - // topo actually needs to be consulted this time around. - let present: Vec = - found.iter().map(|(_, instance)| *instance).collect(); - location_cache.refresh_if_needed( - log, - &present, - Instant::now(), - || topo::read_disk_locations(log), - ); + // hardware topology where they are. A failure to read the + // topology costs only the locations, never the disks. + let locations = match topo::read_disk_locations(log) { + Ok(locations) => Some(locations), + Err(err) => { + warn!( + log, + "failed to read disk locations from hardware \ + topology"; + "err" => %err, + ); + None + } + }; + let mut unlabelled = Vec::new(); for (disk, instance) in found { - let location = - location_cache.location(instance).map(str::to_string); + let location = locations + .as_ref() + .and_then(|locations| locations.get(&instance)) + .cloned(); + if location.is_none() && locations.is_some() { + unlabelled.push(instance.to_string()); + } disks.insert( disk.identity().clone(), disk.with_location(location), ); } + if !unlabelled.is_empty() { + warn!( + log, + "hardware topology has no location label for some \ + disk controllers"; + "nvme_instances" => ?unlabelled, + ); + } } ExternalDisks::Hardcoded { vdevs: _, disks: hardcoded_disks } => { @@ -590,10 +606,9 @@ fn poll_device_tree( log: &Logger, hardware_view_tx: &watch::Sender, external_disks: &ExternalDisks, - location_cache: &mut DiskLocationCache, ) -> Result<(), Error> { // Construct a view of hardware by walking the device tree. - let polled_hw = HardwareSnapshot::new(log, external_disks, location_cache); + let polled_hw = HardwareSnapshot::new(log, external_disks); let polled_hw = match polled_hw { Ok(polled_hw) => polled_hw, @@ -810,15 +825,9 @@ fn hardware_tracking_task( log: Logger, hardware_view_tx: watch::Sender, external_disks: ExternalDisks, - mut location_cache: DiskLocationCache, ) { loop { - match poll_device_tree( - &log, - &hardware_view_tx, - &external_disks, - &mut location_cache, - ) { + match poll_device_tree(&log, &hardware_view_tx, &external_disks) { // We've already warned about `NotAnOxideSled` by this point, // so let's not spam the logs. Ok(_) | Err(Error::NotAnOxideSled(_)) => (), @@ -912,13 +921,7 @@ impl HardwareManager { // This mitigates issues where the Sled Agent could try to propagate // an "empty" view of hardware to other consumers before the first // query. - let mut location_cache = DiskLocationCache::new(); - match poll_device_tree( - &log, - &hardware_view_tx, - &external_disks, - &mut location_cache, - ) { + match poll_device_tree(&log, &hardware_view_tx, &external_disks) { Ok(_) => (), // Allow non-sled devices to proceed with a "null" view of // hardware, otherwise they won't be able to start. @@ -945,12 +948,7 @@ impl HardwareManager { }); std::thread::spawn(move || { - hardware_tracking_task( - log, - hardware_view_tx, - external_disks, - location_cache, - ); + hardware_tracking_task(log, hardware_view_tx, external_disks); }); Ok(Self { hardware_view_rx }) From 8eb6538d24e92b07e9c5354dbe3f8ece66e6b2af Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 16 Sep 2026 12:40:44 -0700 Subject: [PATCH 14/19] Rename sled-hardware disk_location.rs to nvme_instance.rs With the location cache gone the module holds only NvmeInstance, the driver instance number that joins devinfo's view of a controller to libtopo's. Name the file for what it contains; the location logic lives in illumos/topo.rs and HardwareSnapshot::new. Test coverage: cargo check on Linux, clippy for sled-hardware and omicron-sled-agent on a Helios host. --- sled-hardware/src/illumos/mod.rs | 4 ++-- sled-hardware/src/illumos/topo.rs | 2 +- sled-hardware/src/lib.rs | 2 +- .../src/{disk_location.rs => nvme_instance.rs} | 15 ++++++--------- 4 files changed, 10 insertions(+), 13 deletions(-) rename sled-hardware/src/{disk_location.rs => nvme_instance.rs} (76%) diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index 56d05883128..5470246adfc 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -7,7 +7,7 @@ use crate::ExternalDisks; use crate::HardwareView; use crate::TofinoSnapshot; use crate::TofinoView; -use crate::disk_location::NvmeInstance; +use crate::nvme_instance::NvmeInstance; use crate::{DendriteAsic, SledMode, UnparsedDisk}; use camino::Utf8PathBuf; use gethostname::gethostname; @@ -54,7 +54,7 @@ enum Error { InvalidNvmeInstance { node: String, #[source] - err: crate::disk_location::InvalidNvmeInstance, + err: crate::nvme_instance::InvalidNvmeInstance, }, #[error("Invalid value for boot-storage-unit property: {0}")] diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs index e274cf666f8..e9b8f31b26b 100644 --- a/sled-hardware/src/illumos/topo.rs +++ b/sled-hardware/src/illumos/topo.rs @@ -8,7 +8,7 @@ //! properties, built from platform topology maps, devinfo, and other //! sources. This module holds sled-hardware's lookups against that tree. -use crate::disk_location::NvmeInstance; +use crate::nvme_instance::NvmeInstance; use libtopo::{Error, Node, Scheme, TopoHdl, WalkAction, hc}; use slog::{Logger, debug, error, warn}; use std::collections::HashMap; diff --git a/sled-hardware/src/lib.rs b/sled-hardware/src/lib.rs index f407b0fa4a8..67e3b76e5f2 100644 --- a/sled-hardware/src/lib.rs +++ b/sled-hardware/src/lib.rs @@ -23,7 +23,7 @@ cfg_if::cfg_if! { pub mod cleanup; pub mod disk; pub use disk::*; -pub mod disk_location; +pub mod nvme_instance; pub mod underlay; // The type of networking 'ASIC' the Dendrite service is expected to manage diff --git a/sled-hardware/src/disk_location.rs b/sled-hardware/src/nvme_instance.rs similarity index 76% rename from sled-hardware/src/disk_location.rs rename to sled-hardware/src/nvme_instance.rs index 2bd45c06e7e..a01d474bd7b 100644 --- a/sled-hardware/src/disk_location.rs +++ b/sled-hardware/src/nvme_instance.rs @@ -2,21 +2,18 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Chassis locations of disks, learned from the platform hardware topology. +//! The instance number of an `nvme` driver node, the `N` in `nvme`. //! -//! libtopo knows which bay or M.2 socket each NVMe controller sits in and -//! labels it the way the chassis does: "N5" for a U.2 bay, "M.2 East" for a -//! boot device. This module holds the key that joins devinfo's view of a -//! controller to topo's, so the label can be attached to the right disk. +//! devinfo reports it on the controller's node and libtopo reports it as +//! the `io/instance` property of its `nvme` node, so it is the key that +//! joins the two views of a controller. sled-hardware uses that join to +//! attach the chassis location topo knows about to the disk devinfo found. use std::fmt; /// Instance number of an `nvme` driver node, the `N` in `nvme`. /// -/// devinfo reports it on the `nvme` node and topo reports it as the -/// `io/instance` property of its `nvme` node, so it is the key that joins the -/// two views of a controller. It is unrelated to the PCIe physical slot -/// number and to NVMe firmware slots. +/// Unrelated to the PCIe physical slot number and to NVMe firmware slots. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] pub struct NvmeInstance(i32); From 831a0efc5e949497fc3b4e52488f5346a4b933b1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Wed, 16 Sep 2026 13:59:22 -0700 Subject: [PATCH 15/19] Set libtopo's runpath for crates that reach it only via dev-dependencies The helios build-and-test job failed while nextest listed tests: ld.so.1: rack_init_config-...: fatal: libtopo.so.1: open failed rack-init-config reaches sled-hardware, and so libtopo, only through its dev-dependency on sled-agent-rack-setup. Its test binary records libtopo.so.1 as NEEDED but has no build.rs, so nothing emitted the /usr/lib/fm/amd64 runpath. wicketd's integration test binary has the same problem through its dev-dependencies on installinator, sled-storage, sled-agent-config-reconciler and sled-agent-resolvable-files. Both crates were outside the earlier enumeration because it used cargo tree without dev-dependency edges: 40 crates reach sled-hardware through normal edges, 51 once dev edges are included. Wire both crates the same way as the other 20 libtopo users: a build.rs that calls omicron-rpaths and a direct illumos-gated dependency on libtopo-sys. The omicron-rpaths docs now say that dev-dependency edges count. Test coverage: on a Helios host, built the test binaries for all 51 crates that reach libtopo-sys including dev edges (89 executables, no build errors) and checked NEEDED and RUNPATH with elfdump. 22 need libtopo.so.1; before this change 20 had the runpath and the two above did not. After this change cargo nextest run for rack-init-config and wicketd passes all 60 tests there and both binaries carry /usr/lib/fm/amd64 in RUNPATH. On Linux, cargo hakari generate --diff, cargo xtask check-workspace-deps and cargo xtask clippy are clean. --- Cargo.lock | 4 ++++ dev-tools/rack-init-config/Cargo.toml | 7 +++++++ dev-tools/rack-init-config/build.rs | 10 ++++++++++ rpaths/src/lib.rs | 6 +++++- wicketd/Cargo.toml | 7 +++++++ wicketd/build.rs | 10 ++++++++++ 6 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 dev-tools/rack-init-config/build.rs create mode 100644 wicketd/build.rs diff --git a/Cargo.lock b/Cargo.lock index f92b4124f2b..140164fcfef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -12353,8 +12353,10 @@ dependencies = [ "bootstrap-agent-lockstep-types", "camino-tempfile", "iddqd", + "libtopo-sys", "omicron-common", "omicron-passwords", + "omicron-rpaths", "omicron-workspace-hack", "oxnet", "sled-agent-rack-setup", @@ -17945,12 +17947,14 @@ dependencies = [ "internal-dns-resolver", "internal-dns-types", "itertools 0.14.0", + "libtopo-sys", "lpc55_areas 0.2.5 (git+https://github.com/oxidecomputer/lpc55_support)", "maplit", "omicron-certificates", "omicron-common", "omicron-ddm-admin-client", "omicron-passwords", + "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", diff --git a/dev-tools/rack-init-config/Cargo.toml b/dev-tools/rack-init-config/Cargo.toml index c2b36cd99d3..3091c2668a4 100644 --- a/dev-tools/rack-init-config/Cargo.toml +++ b/dev-tools/rack-init-config/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] bootstrap-agent-lockstep-types.workspace = true iddqd.workspace = true @@ -18,6 +21,10 @@ sled-agent-types.workspace = true sled-hardware-types.workspace = true toml.workspace = true +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] camino-tempfile.workspace = true sled-agent-rack-setup.workspace = true diff --git a/dev-tools/rack-init-config/build.rs b/dev-tools/rack-init-config/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/dev-tools/rack-init-config/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} diff --git a/rpaths/src/lib.rs b/rpaths/src/lib.rs index 080b5b43e4d..54d4ef55216 100644 --- a/rpaths/src/lib.rs +++ b/rpaths/src/lib.rs @@ -21,7 +21,11 @@ //! 1. Any crate that depends (directly or transitively) on a -sys crate from //! the list above needs to follow these instructions. Often the dep is //! indirect — pq-sys arrives via diesel, fmd-adm-sys via omicron-sled-agent, -//! libtopo-sys via sled-hardware. +//! libtopo-sys via sled-hardware. Dev-dependencies count too: a crate +//! whose only path to the -sys crate runs through a dev-dependency still +//! links the native library into its test binaries, and those binaries +//! need the RPATH just as much. When enumerating affected crates with +//! `cargo tree -i`, include dev edges (`-e normal,build,dev`). //! 2. Affected crates (e.g., omicron-nexus) have a build.rs that just calls //! `omicron_rpath::configure_default_omicron_rpaths()`. //! 3. These crates must also add a *direct* dependency on the corresponding diff --git a/wicketd/Cargo.toml b/wicketd/Cargo.toml index 08a206b2ffc..27c2de32164 100644 --- a/wicketd/Cargo.toml +++ b/wicketd/Cargo.toml @@ -7,6 +7,9 @@ license = "MPL-2.0" [lints] workspace = true +[build-dependencies] +omicron-rpaths.workspace = true + [dependencies] anyhow.workspace = true async-trait.workspace = true @@ -85,6 +88,10 @@ lpc55_areas.workspace = true name = "wicketd" doc = false +[target.'cfg(target_os = "illumos")'.dependencies] +# See omicron-rpaths for more about the "libtopo-sys" dependency. +libtopo-sys.workspace = true + [dev-dependencies] chrono.workspace = true expectorate.workspace = true diff --git a/wicketd/build.rs b/wicketd/build.rs new file mode 100644 index 00000000000..1ba9acd41c9 --- /dev/null +++ b/wicketd/build.rs @@ -0,0 +1,10 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +// See omicron-rpaths for documentation. +// NOTE: This file MUST be kept in sync with the other build.rs files in this +// repository. +fn main() { + omicron_rpaths::configure_default_omicron_rpaths(); +} From 7564f16b376ca91f3a11024cfb1af0b438fb528f Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 17 Sep 2026 11:22:26 -0700 Subject: [PATCH 16/19] Set libtopo's runpath in .cargo/config.toml, not omicron-rpaths This reverts 901fb2acd7 and 831a0efc5e, which routed libtopo's runpath through omicron-rpaths, and returns to the arrangement 603073f322 set up: the illumos target rustflags carry -R/usr/lib/fm/amd64 next to the Oxide platform directory, and build-and-test.sh repeats it in its RUSTFLAGS override. omicron-rpaths exists for cross-platform libraries whose install location varies with the host OS, so only the build machine knows the path. libtopo is not one of those. It lives at /usr/lib/fm/amd64 on every illumos host and nowhere else, exactly like libipcc, which is already handled in .cargo/config.toml. Going through omicron-rpaths for a fixed-path library also costs more than it looks. Every crate whose test binary might retain a reference to libtopo needs a fake direct dependency on libtopo-sys and a build.rs, and the set of such crates is the transitive dependency closure of sled-hardware: 21 crates in the reverted commits, 51 counting dev-dependency edges. Whether a given binary actually records libtopo as needed depends on what the linker's -z ignore keeps, so the set shifts as tests change, and a missed crate only shows up as an ld.so.1 failure on illumos. This branch hit that twice. The comments in .cargo/config.toml and omicron-rpaths now state the rule as: fixed-path illumos-only libraries go in the target rustflags, cross-platform libraries go through omicron-rpaths. Test coverage: on a Helios host, built the test binaries for sled-storage, sled-hardware, zfs-test-harness, omicron-cockroach-admin, installinator and omicron-sled-agent. Six of the seventeen executables record libtopo.so.1 as needed and all six carry /usr/lib/fm/amd64 in RUNPATH; without the runpath the sled-storage test binary failed to load. cargo nextest run for sled-storage, sled-hardware and zfs-test-harness passes there with no LD_LIBRARY_PATH set. On Linux, cargo hakari generate --diff, cargo xtask check-workspace-deps, and a locked cargo check of the affected crates are clean. --- .cargo/config.toml | 57 +++++++++++-------- .github/buildomat/build-and-test.sh | 6 +- Cargo.lock | 29 ---------- Cargo.toml | 1 - dev-tools/omdb/Cargo.toml | 2 - dev-tools/omicron-dev/Cargo.toml | 2 - dev-tools/rack-init-config/Cargo.toml | 7 --- dev-tools/rack-init-config/build.rs | 10 ---- installinator/Cargo.toml | 7 --- installinator/build.rs | 10 ---- nexus/Cargo.toml | 2 - nexus/db-queries/Cargo.toml | 2 - nexus/inventory/Cargo.toml | 2 - .../cli-integration-tests/Cargo.toml | 2 - nexus/reconfigurator/execution/Cargo.toml | 2 - nexus/test-utils/Cargo.toml | 2 - package/Cargo.toml | 7 --- package/build.rs | 10 ---- rpaths/src/lib.rs | 25 ++++---- sled-agent/Cargo.toml | 2 - sled-agent/bootstrap-common/Cargo.toml | 7 --- sled-agent/bootstrap-common/build.rs | 10 ---- sled-agent/config-reconciler/Cargo.toml | 7 --- sled-agent/config-reconciler/build.rs | 10 ---- sled-agent/rack-setup/Cargo.toml | 7 --- sled-agent/rack-setup/build.rs | 10 ---- sled-agent/resolvable-files/Cargo.toml | 7 --- sled-agent/resolvable-files/build.rs | 10 ---- sled-diagnostics/Cargo.toml | 7 --- sled-diagnostics/build.rs | 10 ---- trust-quorum/Cargo.toml | 7 --- trust-quorum/build.rs | 10 ---- wicketd/Cargo.toml | 7 --- wicketd/build.rs | 10 ---- 34 files changed, 48 insertions(+), 258 deletions(-) delete mode 100644 dev-tools/rack-init-config/build.rs delete mode 100644 installinator/build.rs delete mode 100644 package/build.rs delete mode 100644 sled-agent/bootstrap-common/build.rs delete mode 100644 sled-agent/config-reconciler/build.rs delete mode 100644 sled-agent/rack-setup/build.rs delete mode 100644 sled-agent/resolvable-files/build.rs delete mode 100644 sled-diagnostics/build.rs delete mode 100644 trust-quorum/build.rs delete mode 100644 wicketd/build.rs diff --git a/.cargo/config.toml b/.cargo/config.toml index 6dc94b48356..a9bb8cd6bda 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -21,28 +21,34 @@ rustdocflags = "--document-private-items" rustflags = "--cfg tokio_unstable" -# On illumos, use `-znocompstrtab` to reduce link time. We also add the Oxide -# specific platform directory to the RPATH where additional libraries can be -# found such as libipcc. +# On illumos, use `-znocompstrtab` to reduce link time. We also add two +# directories to the RPATH where libraries outside the default search path +# live: # -# Our reasoning for including `-R/usr/platform/oxide/lib/amd64` here: -# - Oxide specific features - This path contains Oxide specific libraries such -# as libipcc and will likely grow over time to include more functionality. -# - Avoid the rpaths crate - The rpaths crate was built to deal with runtime -# paths that are dynamic such as with libraries like libpq which can live in -# different locations based on OS. This path will only ever be found on -# illumos and will be tied directly to the Oxide platform. -# - Less developer burden - Having something like the ipcc crate emit -# `DEP_IPCC_LIBDIRS` means that we end up littering the repo with Cargo.toml -# and build.rs changes whenever ipcc shows up somewhere in the dependency -# tree. While initially exploring that path we ran into a significant number -# of tests failing due to not being able to find libipcc in the runtime path +# - /usr/platform/oxide/lib/amd64 holds Oxide-specific libraries such as +# libipcc, and will likely grow over time to include more functionality. +# - /usr/lib/fm/amd64 holds the illumos fault management libraries such as +# libtopo, which sled-hardware uses to find each disk's chassis location. +# +# Our reasoning for putting these here rather than going through the rpaths +# crate: +# - Static, illumos-only paths - The rpaths crate was built to deal with +# runtime paths that are dynamic such as with libraries like libpq which can +# live in different locations based on OS. These paths will only ever be +# found on illumos and never move. +# - Less developer burden - Having a -sys crate emit `DEP_*_LIBDIRS` means that +# we end up littering the repo with Cargo.toml and build.rs changes in every +# crate whose binaries or tests transitively link the library. For libipcc +# that was already painful; libtopo enters through sled-hardware, which well +# over a hundred workspace crates reach transitively. While initially +# exploring that path for libipcc we ran into a significant number of tests +# failing due to not being able to find the library in the runtime path, # which can be confusing or surprising to someone doing work on omicron. # -# We could also update Helios so that a symlink is created from -# /usr/platform/oxide/lib/amd64 into /usr/lib/64 but we opted to not take -# this route forward as it meant waiting for another round of updates on -# shared build machines and to buildomat itself. +# We could also update Helios so that symlinks are created from these +# directories into /usr/lib/64 but we opted to not take this route forward as +# it meant waiting for another round of updates on shared build machines and +# to buildomat itself. # # As documented at: # https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags @@ -58,13 +64,16 @@ rustflags = "--cfg tokio_unstable" # When overriding the defaults in this config by environment variable the user # may need to manually pass the additional options found below. # -# Note that other runtime paths should not be added here, but should instead -# reuse the infrastructure found in the `rpaths` crate which can be found in -# this repo. Those paths are usually platform specific and will vary based on a -# variety of things such as host OS. +# Runtime paths for illumos-only libraries with a fixed install location, like +# the two above, belong here. Runtime paths for cross-platform libraries should +# not be added here, but should instead reuse the infrastructure found in the +# `rpaths` crate in this repo. Those paths are usually platform specific and +# will vary based on a variety of things such as host OS. Whatever is added +# here must also be added to the `RUSTFLAGS` override in +# .github/buildomat/build-and-test.sh, which takes precedence over this file. [target.x86_64-unknown-illumos] rustflags = [ - "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64", + "-C", "link-arg=-Wl,-znocompstrtab,-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64", # enable the `tokio-unstable` config for `tokio-dtrace` probes "--cfg", "tokio_unstable" ] diff --git a/.github/buildomat/build-and-test.sh b/.github/buildomat/build-and-test.sh index 030d3abe791..c443c236999 100755 --- a/.github/buildomat/build-and-test.sh +++ b/.github/buildomat/build-and-test.sh @@ -101,10 +101,10 @@ export RUSTFLAGS="--cfg tokio_unstable -D warnings" export RUSTDOCFLAGS="--document-private-items -D warnings" # When running on illumos we need to pass an additional runpath that is # usually configured via ".cargo/config" but the `RUSTFLAGS` env variable -# takes precedence. This path contains oxide specific libraries such as -# libipcc. +# takes precedence. These paths contain oxide specific libraries such as +# libipcc, and the fault management libraries such as libtopo. if [[ $target_os == "illumos" ]]; then - RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64" + RUSTFLAGS="$RUSTFLAGS -C link-arg=-R/usr/platform/oxide/lib/amd64:/usr/lib/fm/amd64" fi export TMPDIR="$TEST_TMPDIR" export RUST_BACKTRACE=1 diff --git a/Cargo.lock b/Cargo.lock index 140164fcfef..6112af103fe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5800,10 +5800,8 @@ dependencies = [ "ipcc", "itertools 0.14.0", "libc", - "libtopo-sys", "omicron-common", "omicron-ddm-admin-client", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -7504,7 +7502,6 @@ dependencies = [ "internal-dns-types", "ipnetwork", "itertools 0.14.0", - "libtopo-sys", "macaddr", "nexus-auth", "nexus-config", @@ -7725,7 +7722,6 @@ dependencies = [ "iddqd", "illumos-utils", "itertools 0.14.0", - "libtopo-sys", "nexus-client", "nexus-types", "ntp-admin-client", @@ -7943,7 +7939,6 @@ dependencies = [ "camino-tempfile", "clap", "fmd-adm-sys", - "libtopo-sys", "nexus-db-queries", "nexus-lockstep-client", "nexus-reconfigurator-preparation", @@ -7988,7 +7983,6 @@ dependencies = [ "internal-dns-types", "ipnet", "ipnetwork", - "libtopo-sys", "newtype-uuid", "nexus-config", "nexus-db-lookup", @@ -8282,7 +8276,6 @@ dependencies = [ "illumos-utils", "internal-dns-resolver", "internal-dns-types", - "libtopo-sys", "nexus-client", "nexus-config", "nexus-db-queries", @@ -9054,7 +9047,6 @@ dependencies = [ "gateway-client", "gateway-test-utils", "libc", - "libtopo-sys", "nexus-config", "nexus-test-interface", "nexus-test-utils", @@ -9330,7 +9322,6 @@ dependencies = [ "ipnetwork", "itertools 0.14.0", "jiff", - "libtopo-sys", "lldpd-client", "macaddr", "maplit", @@ -9543,7 +9534,6 @@ dependencies = [ "internal-dns-types", "ipnetwork", "itertools 0.14.0", - "libtopo-sys", "multimap", "nexus-config", "nexus-db-errors", @@ -9621,8 +9611,6 @@ dependencies = [ "illumos-utils", "indent_write", "indicatif", - "libtopo-sys", - "omicron-rpaths", "omicron-workspace-hack", "omicron-zone-package", "oxide-tokio-rt", @@ -9843,7 +9831,6 @@ dependencies = [ "itertools 0.14.0", "key-manager", "libc", - "libtopo-sys", "macaddr", "nexus-client", "nexus-config", @@ -12353,10 +12340,8 @@ dependencies = [ "bootstrap-agent-lockstep-types", "camino-tempfile", "iddqd", - "libtopo-sys", "omicron-common", "omicron-passwords", - "omicron-rpaths", "omicron-workspace-hack", "oxnet", "sled-agent-rack-setup", @@ -14300,10 +14285,8 @@ version = "0.1.0" dependencies = [ "bootstore", "camino", - "libtopo-sys", "omicron-common", "omicron-ledger", - "omicron-rpaths", "omicron-uuid-kinds", "omicron-workspace-hack", "serde", @@ -14375,13 +14358,11 @@ dependencies = [ "key-manager", "key-manager-types", "libc", - "libtopo-sys", "ntp-admin-client", "omicron-common", "omicron-debug-dropbox", "omicron-generation-kinds", "omicron-ledger", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -14512,7 +14493,6 @@ dependencies = [ "internal-dns-resolver", "internal-dns-types", "itertools 0.14.0", - "libtopo-sys", "nexus-lockstep-client", "nexus-reconfigurator-blippy", "nexus-types", @@ -14521,7 +14501,6 @@ dependencies = [ "omicron-ddm-admin-client", "omicron-generation-kinds", "omicron-ledger", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -14559,9 +14538,7 @@ dependencies = [ "expectorate", "iddqd", "illumos-utils", - "libtopo-sys", "omicron-common", - "omicron-rpaths", "omicron-uuid-kinds", "omicron-workspace-hack", "pretty_assertions", @@ -14726,9 +14703,7 @@ dependencies = [ "illumos-utils", "jiff", "libc", - "libtopo-sys", "omicron-common", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -16738,9 +16713,7 @@ dependencies = [ "hex", "hkdf 0.12.4", "iddqd", - "libtopo-sys", "omicron-ledger", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", @@ -17947,14 +17920,12 @@ dependencies = [ "internal-dns-resolver", "internal-dns-types", "itertools 0.14.0", - "libtopo-sys", "lpc55_areas 0.2.5 (git+https://github.com/oxidecomputer/lpc55_support)", "maplit", "omicron-certificates", "omicron-common", "omicron-ddm-admin-client", "omicron-passwords", - "omicron-rpaths", "omicron-test-utils", "omicron-uuid-kinds", "omicron-workspace-hack", diff --git a/Cargo.toml b/Cargo.toml index 3dc5e55970c..89ebd379907 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -621,7 +621,6 @@ linkme = "0.3" libipcc = { git = "https://github.com/oxidecomputer/ipcc-rs", rev = "524eb8f125003dff50b9703900c6b323f00f9e1b" } libnvme = { git = "https://github.com/oxidecomputer/libnvme", rev = "dd5bb221d327a1bc9287961718c3c10d6bd37da0" } libtopo = "0.2.0" -libtopo-sys = "0.1.1" linear-map = "1.2.0" live-tests-macros = { path = "live-tests/macros" } lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" } diff --git a/dev-tools/omdb/Cargo.toml b/dev-tools/omdb/Cargo.toml index 4391b51a4ae..85e1e1a4986 100644 --- a/dev-tools/omdb/Cargo.toml +++ b/dev-tools/omdb/Cargo.toml @@ -109,8 +109,6 @@ uuid.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] camino-tempfile.workspace = true diff --git a/dev-tools/omicron-dev/Cargo.toml b/dev-tools/omicron-dev/Cargo.toml index 9895bd5b209..a21ed7e90a6 100644 --- a/dev-tools/omicron-dev/Cargo.toml +++ b/dev-tools/omicron-dev/Cargo.toml @@ -34,8 +34,6 @@ toml.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] expectorate.workspace = true diff --git a/dev-tools/rack-init-config/Cargo.toml b/dev-tools/rack-init-config/Cargo.toml index 3091c2668a4..c2b36cd99d3 100644 --- a/dev-tools/rack-init-config/Cargo.toml +++ b/dev-tools/rack-init-config/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] bootstrap-agent-lockstep-types.workspace = true iddqd.workspace = true @@ -21,10 +18,6 @@ sled-agent-types.workspace = true sled-hardware-types.workspace = true toml.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] camino-tempfile.workspace = true sled-agent-rack-setup.workspace = true diff --git a/dev-tools/rack-init-config/build.rs b/dev-tools/rack-init-config/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/dev-tools/rack-init-config/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/installinator/Cargo.toml b/installinator/Cargo.toml index c08a136c47b..119605aa53c 100644 --- a/installinator/Cargo.toml +++ b/installinator/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true async-trait.workspace = true @@ -57,10 +54,6 @@ oxide-update-engine.workspace = true oxide-update-engine-types.workspace = true omicron-workspace-hack.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] omicron-test-utils.workspace = true hex-literal.workspace = true diff --git a/installinator/build.rs b/installinator/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/installinator/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/nexus/Cargo.toml b/nexus/Cargo.toml index fc6aaa967ac..fd925632d9f 100644 --- a/nexus/Cargo.toml +++ b/nexus/Cargo.toml @@ -173,8 +173,6 @@ zip = { workspace = true, features = ["jiff-02"] } [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] async-bb8-diesel.workspace = true diff --git a/nexus/db-queries/Cargo.toml b/nexus/db-queries/Cargo.toml index 28aaa89d3c5..e12449f630c 100644 --- a/nexus/db-queries/Cargo.toml +++ b/nexus/db-queries/Cargo.toml @@ -90,8 +90,6 @@ omicron-test-utils = { workspace = true, optional = true } [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [features] # Enable to export `TestDatabase` diff --git a/nexus/inventory/Cargo.toml b/nexus/inventory/Cargo.toml index edf5d2cafb4..053c6a5a4d3 100644 --- a/nexus/inventory/Cargo.toml +++ b/nexus/inventory/Cargo.toml @@ -54,8 +54,6 @@ omicron-workspace-hack.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] expectorate.workspace = true diff --git a/nexus/reconfigurator/cli-integration-tests/Cargo.toml b/nexus/reconfigurator/cli-integration-tests/Cargo.toml index 03f1c72755b..6ce589023c3 100644 --- a/nexus/reconfigurator/cli-integration-tests/Cargo.toml +++ b/nexus/reconfigurator/cli-integration-tests/Cargo.toml @@ -22,8 +22,6 @@ reconfigurator-cli.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] camino.workspace = true diff --git a/nexus/reconfigurator/execution/Cargo.toml b/nexus/reconfigurator/execution/Cargo.toml index 5b7ffb9c909..bffb9487e87 100644 --- a/nexus/reconfigurator/execution/Cargo.toml +++ b/nexus/reconfigurator/execution/Cargo.toml @@ -53,8 +53,6 @@ omicron-workspace-hack.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [dev-dependencies] async-bb8-diesel.workspace = true diff --git a/nexus/test-utils/Cargo.toml b/nexus/test-utils/Cargo.toml index 01692c70e8b..1f5f8f66aa8 100644 --- a/nexus/test-utils/Cargo.toml +++ b/nexus/test-utils/Cargo.toml @@ -73,8 +73,6 @@ uuid.workspace = true [target.'cfg(target_os = "illumos")'.dependencies] # See omicron-rpaths for more about the "fmd-adm-sys" dependency. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true [features] omicron-dev = ["omicron-test-utils/seed-gen"] diff --git a/package/Cargo.toml b/package/Cargo.toml index 10cb2c3f35e..bdae0a5cf38 100644 --- a/package/Cargo.toml +++ b/package/Cargo.toml @@ -8,9 +8,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true camino.workspace = true @@ -45,10 +42,6 @@ tokio = { workspace = true, features = [ "full" ] } toml.workspace = true walkdir.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] expectorate.workspace = true diff --git a/package/build.rs b/package/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/package/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/rpaths/src/lib.rs b/rpaths/src/lib.rs index 54d4ef55216..cdcb514db25 100644 --- a/rpaths/src/lib.rs +++ b/rpaths/src/lib.rs @@ -8,11 +8,15 @@ //! //! This build-time crate is used by several top-level Omicron crates to set //! RPATH so that native libraries linked via *-sys crates can be found at -//! runtime. Currently we do this for three libraries: +//! runtime. Currently we do this for two libraries: //! //! - **libpq** (via pq-sys, pulled in by diesel) //! - **libfmd_adm** (via fmd-adm-sys, pulled in by fmd-adm in sled-agent) -//! - **libtopo** (via libtopo-sys, pulled in by libtopo in sled-hardware) +//! +//! This crate is for cross-platform libraries whose install location varies +//! with the host OS. An illumos-only library with a fixed install location, +//! such as libipcc or libtopo, is handled once for the whole workspace by the +//! illumos target rustflags in `.cargo/config.toml` instead. //! //! Cargo/Rust have no built-in way to set the RPATH for a transitively-linked //! native library. (See below.) So we've developed the pattern here instead. @@ -20,12 +24,7 @@ //! //! 1. Any crate that depends (directly or transitively) on a -sys crate from //! the list above needs to follow these instructions. Often the dep is -//! indirect — pq-sys arrives via diesel, fmd-adm-sys via omicron-sled-agent, -//! libtopo-sys via sled-hardware. Dev-dependencies count too: a crate -//! whose only path to the -sys crate runs through a dev-dependency still -//! links the native library into its test binaries, and those binaries -//! need the RPATH just as much. When enumerating affected crates with -//! `cargo tree -i`, include dev edges (`-e normal,build,dev`). +//! indirect — pq-sys arrives via diesel, fmd-adm-sys via omicron-sled-agent. //! 2. Affected crates (e.g., omicron-nexus) have a build.rs that just calls //! `omicron_rpath::configure_default_omicron_rpaths()`. //! 3. These crates must also add a *direct* dependency on the corresponding @@ -39,8 +38,8 @@ //! 4. The metadata that drives this comes from the -sys crate's build.rs: //! - For pq-sys, we maintain a fork (see `[patch.crates-io.pq-sys]` //! in the workspace Cargo.toml) that emits `cargo:LIBDIRS=...`. -//! - For fmd-adm-sys and libtopo-sys, the upstream crates emit the -//! metadata directly, so no patch is needed. +//! - For fmd-adm-sys, the upstream crate emits the metadata directly, +//! so no patch is needed. //! //! `configure_default_omicron_rpaths()` scans for every `DEP_*_LIBDIRS` env //! var in `RPATH_ENV_VARS`. Each crate's build.rs makes the same call — @@ -75,8 +74,8 @@ //! As of 1.56, Cargo supports the "cargo:rustc-link-arg" instruction for use by //! [Build Scripts][3] to pass arbitrary options to the linker. We use that //! here to tell the linker to include the correct RPATH entries for the -//! native dependencies affected by this (currently libpq via pq-sys, -//! libfmd_adm via fmd-adm-sys, and libtopo via libtopo-sys). +//! native dependencies affected by this (currently libpq via pq-sys and +//! libfmd_adm via fmd-adm-sys). //! //! A subtle but critical point here is that the RPATH is knowable only by the //! system that's building the top-level executable binary. This mechanism can't @@ -183,7 +182,7 @@ mod internal { /// (say) fmd-adm-sys simply won't have `DEP_FMD_ADM_LIBDIRS` set, and we /// skip it silently. pub static RPATH_ENV_VARS: &'static [&'static str] = - &["DEP_PQ_LIBDIRS", "DEP_FMD_ADM_LIBDIRS", "DEP_TOPO_LIBDIRS"]; + &["DEP_PQ_LIBDIRS", "DEP_FMD_ADM_LIBDIRS"]; /// Tells Cargo to pass linker arguments that specify RPATHs from the /// environment variable `env_var_name`, if it is set. diff --git a/sled-agent/Cargo.toml b/sled-agent/Cargo.toml index 4ffa5c305be..47577484c32 100644 --- a/sled-agent/Cargo.toml +++ b/sled-agent/Cargo.toml @@ -138,8 +138,6 @@ fmd-adm.workspace = true # Direct dep on fmd-adm-sys so this crate sees DEP_FMD_ADM_LIBDIRS in build.rs # and can configure RPATH via omicron-rpaths. fmd-adm-sys.workspace = true -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true opte-ioctl.workspace = true [dev-dependencies] diff --git a/sled-agent/bootstrap-common/Cargo.toml b/sled-agent/bootstrap-common/Cargo.toml index 84eaeb4cc48..b886631c98b 100644 --- a/sled-agent/bootstrap-common/Cargo.toml +++ b/sled-agent/bootstrap-common/Cargo.toml @@ -8,9 +8,6 @@ description = "shared code for bootstrap agent related crates" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] bootstore.workspace = true camino.workspace = true @@ -31,7 +28,3 @@ trust-quorum.workspace = true uuid.workspace = true omicron-workspace-hack.workspace = true - -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true diff --git a/sled-agent/bootstrap-common/build.rs b/sled-agent/bootstrap-common/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/sled-agent/bootstrap-common/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/sled-agent/config-reconciler/Cargo.toml b/sled-agent/config-reconciler/Cargo.toml index 61a920dd727..1f407d6d5a2 100644 --- a/sled-agent/config-reconciler/Cargo.toml +++ b/sled-agent/config-reconciler/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true async-recursion.workspace = true @@ -59,10 +56,6 @@ zone.workspace = true omicron-workspace-hack.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] assert_matches.workspace = true camino-tempfile-ext.workspace = true diff --git a/sled-agent/config-reconciler/build.rs b/sled-agent/config-reconciler/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/sled-agent/config-reconciler/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/sled-agent/rack-setup/Cargo.toml b/sled-agent/rack-setup/Cargo.toml index 9ba90457a71..bb4dc43b439 100644 --- a/sled-agent/rack-setup/Cargo.toml +++ b/sled-agent/rack-setup/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true bootstore.workspace = true @@ -57,10 +54,6 @@ trust-quorum-protocol.workspace = true trust-quorum-types.workspace = true uuid.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] camino-tempfile.workspace = true nexus-reconfigurator-blippy.workspace = true diff --git a/sled-agent/rack-setup/build.rs b/sled-agent/rack-setup/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/sled-agent/rack-setup/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/sled-agent/resolvable-files/Cargo.toml b/sled-agent/resolvable-files/Cargo.toml index b213b3a9c85..da3c15bebb6 100644 --- a/sled-agent/resolvable-files/Cargo.toml +++ b/sled-agent/resolvable-files/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] camino.workspace = true iddqd.workspace = true @@ -29,10 +26,6 @@ slog-error-chain.workspace = true thiserror.workspace = true tufaceous-artifact.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] camino-tempfile-ext.workspace = true dropshot.workspace = true diff --git a/sled-agent/resolvable-files/build.rs b/sled-agent/resolvable-files/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/sled-agent/resolvable-files/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/sled-diagnostics/Cargo.toml b/sled-diagnostics/Cargo.toml index 4b9a6e4e337..1d4725175bc 100644 --- a/sled-diagnostics/Cargo.toml +++ b/sled-diagnostics/Cargo.toml @@ -6,9 +6,6 @@ edition.workspace = true [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true camino.workspace = true @@ -34,10 +31,6 @@ tokio = { workspace = true, features = ["full"] } zfs-test-harness.workspace = true zip = { workspace = true, features = ["jiff-02","zstd"] } -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] omicron-common.workspace = true omicron-test-utils.workspace = true diff --git a/sled-diagnostics/build.rs b/sled-diagnostics/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/sled-diagnostics/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/trust-quorum/Cargo.toml b/trust-quorum/Cargo.toml index e3be7a115bf..7b2b21a5835 100644 --- a/trust-quorum/Cargo.toml +++ b/trust-quorum/Cargo.toml @@ -8,9 +8,6 @@ description = "trust quorum library for use by bootstrap agent" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true bootstore.workspace = true @@ -49,10 +46,6 @@ uuid.workspace = true zeroize.workspace = true omicron-workspace-hack.workspace = true -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] assert_matches.workspace = true attest-mock.workspace = true diff --git a/trust-quorum/build.rs b/trust-quorum/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/trust-quorum/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} diff --git a/wicketd/Cargo.toml b/wicketd/Cargo.toml index 27c2de32164..08a206b2ffc 100644 --- a/wicketd/Cargo.toml +++ b/wicketd/Cargo.toml @@ -7,9 +7,6 @@ license = "MPL-2.0" [lints] workspace = true -[build-dependencies] -omicron-rpaths.workspace = true - [dependencies] anyhow.workspace = true async-trait.workspace = true @@ -88,10 +85,6 @@ lpc55_areas.workspace = true name = "wicketd" doc = false -[target.'cfg(target_os = "illumos")'.dependencies] -# See omicron-rpaths for more about the "libtopo-sys" dependency. -libtopo-sys.workspace = true - [dev-dependencies] chrono.workspace = true expectorate.workspace = true diff --git a/wicketd/build.rs b/wicketd/build.rs deleted file mode 100644 index 1ba9acd41c9..00000000000 --- a/wicketd/build.rs +++ /dev/null @@ -1,10 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -// See omicron-rpaths for documentation. -// NOTE: This file MUST be kept in sync with the other build.rs files in this -// repository. -fn main() { - omicron_rpaths::configure_default_omicron_rpaths(); -} From ce390cc999c5d4eb91020904ab14fce78fedbbc7 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 17 Sep 2026 08:51:13 -0700 Subject: [PATCH 17/19] Give synthetic disks chassis locations that match real hardware Synthetic disks reported no location, so a dev host running sled-agent with hardcoded vdevs showed every disk with location None. That is the same value a real sled reports when the topology read fails, and it leaves any code keyed on the labels, such as the planned derivation of M2Slot from "M.2 East" and "M.2 West", with nothing to work with on a dev host. RawSyntheticDisk now carries a location derived from the vdev filename, spelled exactly as libtopo spells it on Gimlet and Cosmo: u2_3.vdev is N3, m2_0.vdev is M.2 East and m2_1.vdev is M.2 West. A serial that is not an integer, such as the zpool UUIDs the dataset serialization test uses, falls back to the caller's slot index. The synthetic arms of RawDisk::location and Disk::location return it. The synthetic identity fields keep their synthetic- prefixes, so the disk is still obviously fake. Test coverage: unit tests in sled-storage cover the filename mapping, the fallback, and the accessor. The sled-agent-config-reconciler suite passes on Linux, and the sled-storage and zfs-test-harness suites pass on a Helios host. --- sled-agent/config-reconciler/src/raw_disks.rs | 1 + sled-storage/src/disk.rs | 62 ++++++++++++++++++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/sled-agent/config-reconciler/src/raw_disks.rs b/sled-agent/config-reconciler/src/raw_disks.rs index a0f1a4aa827..496c94f852e 100644 --- a/sled-agent/config-reconciler/src/raw_disks.rs +++ b/sled-agent/config-reconciler/src/raw_disks.rs @@ -211,6 +211,7 @@ mod tests { variant, pcie_slot: slot, firmware, + location: "N0".to_string(), }) } } diff --git a/sled-storage/src/disk.rs b/sled-storage/src/disk.rs index b832bb24870..495e1ae7f53 100644 --- a/sled-storage/src/disk.rs +++ b/sled-storage/src/disk.rs @@ -104,6 +104,10 @@ pub struct RawSyntheticDisk { /// real disk. See [`UnparsedDisk::pcie_slot`]. pub pcie_slot: i64, pub firmware: DiskFirmware, + /// A chassis location spelled the way real hardware spells it, such as + /// "N3" or "M.2 East", so that anything keyed on those labels treats a + /// synthetic disk like a real one. See [`UnparsedDisk::location`]. + pub location: String, } impl RawSyntheticDisk { @@ -121,6 +125,11 @@ impl RawSyntheticDisk { /// Treats a file at path `vdev` as a synthetic disk. The file /// should already exist, and have the desired length. + /// + /// The file must be named `m2_.vdev` or `u2_.vdev`. When + /// `` is an integer it also picks the disk's chassis location: + /// `u2_3.vdev` sits in bay N3, and `m2_0.vdev` and `m2_1.vdev` are M.2 + /// East and M.2 West. Otherwise `pcie_slot` picks the location. pub fn load>( vdev: P, pcie_slot: i64, @@ -148,6 +157,18 @@ impl RawSyntheticDisk { model: format!("synthetic-model-{variant:?}"), }; + let ordinal = serial.parse::().unwrap_or(pcie_slot); + let location = match variant { + DiskVariant::U2 => format!("N{ordinal}"), + DiskVariant::M2 => match ordinal { + 0 => "M.2 East".to_string(), + 1 => "M.2 West".to_string(), + // Real sleds have two M.2 sockets, but a test may number its + // vdevs with one counter across both variants. + n => format!("M.2 {n}"), + }, + }; + let firmware = DiskFirmware::new( 1, None, @@ -162,6 +183,7 @@ impl RawSyntheticDisk { variant, pcie_slot: pcie_slot + SYNTHETIC_PCIE_SLOT_OFFSET, firmware, + location, }) } } @@ -241,7 +263,7 @@ impl RawDisk { pub fn location(&self) -> Option<&str> { match self { Self::Real(disk) => disk.location(), - Self::Synthetic(_) => None, + Self::Synthetic(disk) => Some(&disk.location), } } @@ -395,7 +417,7 @@ impl Disk { pub fn location(&self) -> Option<&str> { match self { Self::Real(disk) => disk.location.as_deref(), - Self::Synthetic(_) => None, + Self::Synthetic(disk) => Some(&disk.raw.location), } } @@ -442,3 +464,39 @@ impl From for RawDisk { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn location_of(vdev: &str, slot: i64) -> String { + RawSyntheticDisk::load(Utf8Path::new(vdev), slot) + .expect("vdev name parses") + .location + } + + #[test] + fn synthetic_location_matches_real_labels() { + assert_eq!(location_of("u2_0.vdev", 7), "N0"); + assert_eq!(location_of("/some/dir/u2_9.vdev", 7), "N9"); + assert_eq!(location_of("m2_0.vdev", 7), "M.2 East"); + assert_eq!(location_of("m2_1.vdev", 7), "M.2 West"); + } + + #[test] + fn synthetic_location_falls_back_to_slot() { + // A non-numeric serial, such as a zpool UUID, cannot pick a bay, so + // the caller's slot index does instead. + assert_eq!(location_of("u2_deadbeef.vdev", 4), "N4"); + assert_eq!(location_of("m2_deadbeef.vdev", 0), "M.2 East"); + assert_eq!(location_of("m2_deadbeef.vdev", 5), "M.2 5"); + } + + #[test] + fn synthetic_location_is_reported() { + let disk = RawDisk::Synthetic( + RawSyntheticDisk::load(Utf8Path::new("u2_3.vdev"), 0).unwrap(), + ); + assert_eq!(disk.location(), Some("N3")); + } +} From 5219defbca2eb5df473ce759362af708973a3f5b Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 17 Sep 2026 13:02:05 -0700 Subject: [PATCH 18/19] Disk location review follow-ups Second round of review changes for the chassis location work. - NvmeInstance is documented as the number the illumos kernel assigns when it binds the nvme driver, rather than by where the number shows up, and the module doc is one line with the detail on the type. Its TryFrom impls are replaced by from_devinfo and from_topo, which name the source each conversion validates. - Building the disk map in HardwareSnapshot::new no longer branches on whether the topology read succeeded. The check for controllers topo has no label for moves into the Ok arm, where the map is known to exist, and the Err arm yields an empty map. The failure is logged with InlineErrorChain. - Two nvme topo nodes with one instance is logged at warn, like the other malformed-topology cases in the same walk. - pcie_slot documents that the number is not guaranteed stable across host OS versions and that location should be preferred. - InvNvmeDiskFirmware's fields each say which kind of slot they mean, since the struct mixes the PCIe slot with NVMe firmware slots. InvPhysicalDisk::pcie_slot says the column was originally named slot. - Comment wording nits in nexus-types inventory and the installinator log line for an unexpected PCIe slot. Test coverage: sled-hardware clippy with warnings denied and its unit tests pass on a Helios host, which is where the illumos-only code compiles. cargo xtask clippy and cargo fmt are clean on Linux. --- installinator/src/write.rs | 2 +- nexus/db-model/src/inventory.rs | 22 ++++++++++--- nexus/types/src/inventory.rs | 8 ++--- sled-hardware/src/disk.rs | 4 +++ sled-hardware/src/illumos/mod.rs | 47 +++++++++++++------------- sled-hardware/src/illumos/topo.rs | 6 ++-- sled-hardware/src/nvme_instance.rs | 53 ++++++++++++++---------------- 7 files changed, 80 insertions(+), 62 deletions(-) diff --git a/installinator/src/write.rs b/installinator/src/write.rs index 17f843b822a..4f2ba431b5f 100644 --- a/installinator/src/write.rs +++ b/installinator/src/write.rs @@ -124,7 +124,7 @@ impl WriteDestination { for disk in hardware.m2_disks() { let Ok(slot) = M2Slot::try_from(disk.pcie_slot()) else { warn!( - log, "skipping M.2 drive with unexpected slot number"; + log, "skipping M.2 drive with unexpected PCIe slot number"; "pcie_slot" => disk.pcie_slot(), ); continue; diff --git a/nexus/db-model/src/inventory.rs b/nexus/db-model/src/inventory.rs index a4eb2b7d3c8..8db608cba31 100644 --- a/nexus/db-model/src/inventory.rs +++ b/nexus/db-model/src/inventory.rs @@ -2444,15 +2444,16 @@ pub struct InvPhysicalDisk { pub inv_collection_id: DbTypedUuid, pub sled_id: DbTypedUuid, /// See [`nexus_types::inventory::PhysicalDisk::pcie_slot`]. The column - /// keeps its original name because columns cannot be renamed - /// idempotently. + /// was originally named `slot`, but we try to reference it as `pcie_slot` + /// to disambiguate it from `location`. It cannot be actually renamed, + /// however, because columns cannot be renamed idempotently. #[diesel(column_name = slot)] pub pcie_slot: i64, pub vendor: String, pub model: String, pub serial: String, pub variant: PhysicalDiskKind, - /// See [`nexus_types::inventory::PhysicalDisk::location`]. + /// The location of the disk in the chassis, as identified by libtopo. pub location: Option, } @@ -2501,13 +2502,26 @@ pub enum InvNvmeDiskFirmwareError { pub struct InvNvmeDiskFirmware { inv_collection_id: DbTypedUuid, sled_id: DbTypedUuid, - /// See [`InvPhysicalDisk::pcie_slot`]. + /// The PCIe slot number for the disk. + /// + /// Note that PCIe slots are specific to the board topology, + /// and are unrelated to NVMe firmware slots. #[diesel(column_name = slot)] pcie_slot: i64, + /// The NVMe firmware slot being used. active_slot: SqlU8, + /// The next NVMe firmware slot to be used after a reset. next_active_slot: Option, + /// The number of NVMe firmware slots that are accessible. number_of_slots: SqlU8, + /// Returns true if the first firmware slot is locked-down + /// as read-only. (This is sometimes the case for locked-down + /// factory firmware slots). slot1_is_read_only: bool, + /// The per-slot version string list. + /// + /// NVMe slots are generally one-indexed, but this Vec is zero-indexed. + /// Be aware that NVMe slot N is stored in slot_firmware_versions[N - 1]. slot_firmware_versions: Vec>, } diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index a2b343c307b..e5f08ee2858 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -617,9 +617,9 @@ pub struct PhysicalDisk { /// location label printed on the chassis; see `location` for that. // // Older serialized state (support bundles, `omdb` exports) called this - // field `slot`. Accept that name too so those files still load. The - // format is documented as unstable, so this isn't strictly necessary, but - // it is cheap and fail-safe. + // field `slot`. Accepting that name lets us still deserialize those files. + // The format is documented as unstable, so this isn't strictly necessary, + // but it is cheap and fail-safe. #[serde(alias = "slot")] pub pcie_slot: i64, /// Where this disk sits in the chassis, as labelled by the platform's @@ -630,7 +630,7 @@ pub struct PhysicalDisk { /// for the disk or could not be read. // // Older serialized state had no such field; treating its absence as - // `None` is exactly right, since nothing recorded a location then. + // `None` accurately represents that nothing recorded a location. #[serde(default)] pub location: Option, pub firmware: PhysicalDiskFirmware, diff --git a/sled-hardware/src/disk.rs b/sled-hardware/src/disk.rs index a5328435de8..98c4fbb21fe 100644 --- a/sled-hardware/src/disk.rs +++ b/sled-hardware/src/disk.rs @@ -274,6 +274,10 @@ impl UnparsedDisk { /// 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. + /// + /// 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. pub fn pcie_slot(&self) -> i64 { self.pcie_slot } diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index 5470246adfc..0b00cede885 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -22,6 +22,7 @@ use slog::error; use slog::info; use slog::o; use slog::warn; +use slog_error_chain::InlineErrorChain; use std::collections::{HashMap, HashSet}; use tokio::sync::watch; use uuid::Uuid; @@ -186,42 +187,44 @@ impl HardwareSnapshot { } // Now that we know which controllers are present, ask the - // hardware topology where they are. A failure to read the - // topology costs only the locations, never the disks. + // hardware topology where they are. If the topology cannot + // be read, the disks are still reported, without locations. let locations = match topo::read_disk_locations(log) { - Ok(locations) => Some(locations), + Ok(locations) => { + let unlabelled: Vec<_> = found + .iter() + .filter(|(_, instance)| { + !locations.contains_key(instance) + }) + .map(|(_, instance)| instance.to_string()) + .collect(); + if !unlabelled.is_empty() { + warn!( + log, + "hardware topology has no location label \ + for some disk controllers"; + "nvme_instances" => ?unlabelled, + ); + } + locations + } Err(err) => { warn!( log, "failed to read disk locations from hardware \ topology"; - "err" => %err, + InlineErrorChain::new(&err), ); - None + HashMap::new() } }; - let mut unlabelled = Vec::new(); for (disk, instance) in found { - let location = locations - .as_ref() - .and_then(|locations| locations.get(&instance)) - .cloned(); - if location.is_none() && locations.is_some() { - unlabelled.push(instance.to_string()); - } + let location = locations.get(&instance).cloned(); disks.insert( disk.identity().clone(), disk.with_location(location), ); } - if !unlabelled.is_empty() { - warn!( - log, - "hardware topology has no location label for some \ - disk controllers"; - "nvme_instances" => ?unlabelled, - ); - } } ExternalDisks::Hardcoded { vdevs: _, disks: hardcoded_disks } => { @@ -483,7 +486,7 @@ fn poll_blkdev_node( .instance() .ok_or(Error::MissingNvmeDevinfoInstance { node: node.node_name() })?; let nvme_instance = - NvmeInstance::try_from(nvme_instance).map_err(|err| { + NvmeInstance::from_devinfo(nvme_instance).map_err(|err| { Error::InvalidNvmeInstance { node: node.node_name(), err } })?; diff --git a/sled-hardware/src/illumos/topo.rs b/sled-hardware/src/illumos/topo.rs index e9b8f31b26b..2e5f393dd3f 100644 --- a/sled-hardware/src/illumos/topo.rs +++ b/sled-hardware/src/illumos/topo.rs @@ -10,7 +10,7 @@ use crate::nvme_instance::NvmeInstance; use libtopo::{Error, Node, Scheme, TopoHdl, WalkAction, hc}; -use slog::{Logger, debug, error, warn}; +use slog::{Logger, debug, warn}; use std::collections::HashMap; use std::collections::hash_map::Entry; use std::time::Instant; @@ -47,7 +47,7 @@ pub(super) fn read_disk_locations( Entry::Vacant(entry) => { entry.insert(label); } - Entry::Occupied(entry) => error!( + Entry::Occupied(entry) => warn!( log, "hardware topology has two nvme nodes with the same \ instance; keeping the first label"; @@ -98,7 +98,7 @@ fn nvme_instance_of(log: &Logger, node: &Node<'_>) -> Option { return None; } }; - match NvmeInstance::try_from(value) { + match NvmeInstance::from_topo(value) { Ok(instance) => Some(instance), Err(err) => { warn!( diff --git a/sled-hardware/src/nvme_instance.rs b/sled-hardware/src/nvme_instance.rs index a01d474bd7b..47d173cf2f6 100644 --- a/sled-hardware/src/nvme_instance.rs +++ b/sled-hardware/src/nvme_instance.rs @@ -2,16 +2,20 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! The instance number of an `nvme` driver node, the `N` in `nvme`. -//! -//! devinfo reports it on the controller's node and libtopo reports it as -//! the `io/instance` property of its `nvme` node, so it is the key that -//! joins the two views of a controller. sled-hardware uses that join to -//! attach the chassis location topo knows about to the disk devinfo found. +//! The instance number the illumos kernel assigns to each `nvme` driver node. use std::fmt; -/// Instance number of an `nvme` driver node, the `N` in `nvme`. +/// The instance number the illumos kernel assigned to an NVMe controller +/// when it bound the `nvme` driver to it. +/// +/// Every device node that binds to a driver gets an instance number, +/// unique among that driver's nodes and recorded in `/etc/path_to_inst` +/// so it survives reboots. It is the `3` in `nvme3`. devinfo exposes it +/// on the controller's node, libnvme opens controllers by it, and libtopo +/// records it as the `io/instance` property of its `nvme` node. That +/// shared key is how sled-hardware attaches the chassis location topo +/// reports to the controller devinfo found. /// /// Unrelated to the PCIe physical slot number and to NVMe firmware slots. #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, PartialOrd, Ord)] @@ -24,34 +28,27 @@ pub struct NvmeInstance(i32); pub struct InvalidNvmeInstance(pub i64); impl NvmeInstance { - /// The instance number as devinfo and libnvme represent it. - pub fn as_i32(self) -> i32 { - self.0 - } -} - -impl TryFrom for NvmeInstance { - type Error = InvalidNvmeInstance; - - /// From a devinfo instance number, which is signed but never negative. - fn try_from(value: i32) -> Result { + /// From the instance number on a devinfo node, which is signed but + /// never negative for a bound driver. + pub fn from_devinfo(value: i32) -> Result { if value < 0 { Err(InvalidNvmeInstance(value.into())) } else { Ok(Self(value)) } } -} - -impl TryFrom for NvmeInstance { - type Error = InvalidNvmeInstance; - /// From a topo `io/instance` property, which is unsigned. - fn try_from(value: u32) -> Result { + /// From the `io/instance` property of a topo node, which is unsigned. + pub fn from_topo(value: u32) -> Result { i32::try_from(value) .map(Self) .map_err(|_| InvalidNvmeInstance(value.into())) } + + /// The instance number as devinfo and libnvme represent it. + pub fn as_i32(self) -> i32 { + self.0 + } } impl fmt::Display for NvmeInstance { @@ -65,16 +62,16 @@ mod tests { use super::*; fn inst(n: i32) -> NvmeInstance { - NvmeInstance::try_from(n).unwrap() + NvmeInstance::from_devinfo(n).unwrap() } #[test] fn nvme_instance_conversions() { assert_eq!(inst(0).as_i32(), 0); assert_eq!(inst(17).as_i32(), 17); - assert!(NvmeInstance::try_from(-1i32).is_err()); - assert_eq!(NvmeInstance::try_from(3u32).unwrap(), inst(3)); - assert!(NvmeInstance::try_from(u32::MAX).is_err()); + assert!(NvmeInstance::from_devinfo(-1).is_err()); + assert_eq!(NvmeInstance::from_topo(3).unwrap(), inst(3)); + assert!(NvmeInstance::from_topo(u32::MAX).is_err()); assert_eq!(inst(5).to_string(), "nvme5"); } } From 15ee23cf7b679cfd83a8608e708dd1ddea2272fb Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Thu, 17 Sep 2026 15:29:06 -0700 Subject: [PATCH 19/19] Disk location review follow-ups, third round - The rack-setup test sleds give their U.2 disks N-labels instead of no location, like the planning system builder and the inventory examples. - The pcie_slot docs on the sled-agent InventoryDisk API type and on nexus-types PhysicalDisk say the numbering is not guaranteed stable across host OS versions, matching the note on UnparsedDisk::pcie_slot. The sled-agent OpenAPI document is regenerated for the new description. - The comment at the PCIe slot lookup in poll_blkdev_node no longer claims the slot number tells us which bay the disk is in. It says the variant is inferred from a per-board table of slot numbers, and a TODO against #11258 records that the topology's bay and slot nodes are the authoritative source. Test coverage: sled-agent-rack-setup tests pass on Linux, along with cargo fmt and cargo xtask openapi generate. The sled-hardware change is comment-only. --- nexus/types/src/inventory.rs | 3 +++ ....0-c5ef76.json => sled-agent-54.0.0-f8d0db.json} | 2 +- openapi/sled-agent/sled-agent-latest.json | 2 +- sled-agent/rack-setup/src/service.rs | 2 +- .../src/add_disk_location_to_inventory/inventory.rs | 3 +++ sled-hardware/src/illumos/mod.rs | 13 +++++++++---- 6 files changed, 18 insertions(+), 7 deletions(-) rename openapi/sled-agent/{sled-agent-54.0.0-c5ef76.json => sled-agent-54.0.0-f8d0db.json} (99%) diff --git a/nexus/types/src/inventory.rs b/nexus/types/src/inventory.rs index e5f08ee2858..105bf011dbd 100644 --- a/nexus/types/src/inventory.rs +++ b/nexus/types/src/inventory.rs @@ -615,6 +615,9 @@ pub struct PhysicalDisk { /// internal to the board's PCIe topology and board-specific: the same /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the /// location label printed on the chassis; see `location` for that. + /// Nothing guarantees the numbering stays the same across host OS + /// versions either, so anything that needs a stable identifier should + /// use `location`. // // Older serialized state (support bundles, `omdb` exports) called this // field `slot`. Accepting that name lets us still deserialize those files. diff --git a/openapi/sled-agent/sled-agent-54.0.0-c5ef76.json b/openapi/sled-agent/sled-agent-54.0.0-f8d0db.json similarity index 99% rename from openapi/sled-agent/sled-agent-54.0.0-c5ef76.json rename to openapi/sled-agent/sled-agent-54.0.0-f8d0db.json index 6305793621b..e6cc270421f 100644 --- a/openapi/sled-agent/sled-agent-54.0.0-c5ef76.json +++ b/openapi/sled-agent/sled-agent-54.0.0-f8d0db.json @@ -6458,7 +6458,7 @@ "minimum": 0 }, "pcie_slot": { - "description": "PCIe physical slot number of the bridge above this disk.\n\nThis is the `physical-slot#` of the parent `pcieb` device. It is internal to the board's PCIe topology and board-specific: the same U.2 bay is numbered differently on Gimlet and Cosmo. It is not the location label printed on the chassis; see `location` for that.", + "description": "PCIe physical slot number of the bridge above this disk.\n\nThis is the `physical-slot#` of the parent `pcieb` device. It is internal to the board's PCIe topology and board-specific: the same U.2 bay is numbered differently on Gimlet and Cosmo. It is not the location label printed on the chassis; see `location` for that. Nothing guarantees the numbering stays the same across host OS versions either, so anything that needs a stable identifier should use `location`.", "type": "integer", "format": "int64" }, diff --git a/openapi/sled-agent/sled-agent-latest.json b/openapi/sled-agent/sled-agent-latest.json index 34505f7c7d6..c3775b07dc7 120000 --- a/openapi/sled-agent/sled-agent-latest.json +++ b/openapi/sled-agent/sled-agent-latest.json @@ -1 +1 @@ -sled-agent-54.0.0-c5ef76.json \ No newline at end of file +sled-agent-54.0.0-f8d0db.json \ No newline at end of file diff --git a/sled-agent/rack-setup/src/service.rs b/sled-agent/rack-setup/src/service.rs index c9097a8a2ce..a5181c15d45 100644 --- a/sled-agent/rack-setup/src/service.rs +++ b/sled-agent/rack-setup/src/service.rs @@ -1697,7 +1697,7 @@ mod test { }, variant: DiskVariant::U2, pcie_slot: i.try_into().unwrap(), - location: None, + location: Some(format!("N{i}")), active_firmware_slot: 1, next_active_firmware_slot: None, number_of_firmware_slots: 1, diff --git a/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs b/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs index aef6cd2521a..7218673c170 100644 --- a/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs +++ b/sled-agent/types/versions/src/add_disk_location_to_inventory/inventory.rs @@ -38,6 +38,9 @@ pub struct InventoryDisk { /// internal to the board's PCIe topology and board-specific: the same /// U.2 bay is numbered differently on Gimlet and Cosmo. It is not the /// location label printed on the chassis; see `location` for that. + /// Nothing guarantees the numbering stays the same across host OS + /// versions either, so anything that needs a stable identifier should + /// use `location`. pub pcie_slot: i64, /// Where this disk sits in the chassis, as labelled by the platform's /// hardware topology: "N5" for a U.2 bay, "M.2 East" for a boot device. diff --git a/sled-hardware/src/illumos/mod.rs b/sled-hardware/src/illumos/mod.rs index 0b00cede885..81fece24820 100644 --- a/sled-hardware/src/illumos/mod.rs +++ b/sled-hardware/src/illumos/mod.rs @@ -512,10 +512,15 @@ fn poll_blkdev_node( // We expect that the parent of the "nvme" device is a "pcieb" driver. let pcieb_node = get_parent_node(&nvme_node, "pcieb")?; - // The "pcieb" device's PCIe physical slot number tells us which bay or - // M.2 socket the disk is in, and therefore what type of disk it is. The - // numbering is board-specific and internal to the PCIe topology; it is - // not the location label printed on the chassis. + // sled-hardware keeps a per-board table of the PCIe physical slot + // numbers behind which U.2 bays and M.2 sockets sit, and infers the + // disk's variant by finding this number in it. The number itself is + // board-internal and says nothing about the chassis; it is not the + // location label, and it may be renumbered by a future host OS. + // + // TODO(https://github.com/oxidecomputer/omicron/issues/11258): The + // topology's bay and slot nodes are the authoritative source. Derive the + // variant from them instead of from this table. let pcie_slot = i64_from_property( &find_properties(&pcieb_node, ["physical-slot#"])?[0], )?;