Report each disk's chassis location in sled inventory by smklein · Pull Request #11279 · oxidecomputer/omicron

smklein · 2026-09-11T16:36:19Z

Part of #11258. Groundwork for #11088.

Sled-agent identifies disks by the PCIe physical slot number of the bridge above them (physical-slot# on the pcieb node). That number is internal to the board's PCIe topology and board-specific: the same ten U.2 bays are 0x00-0x09 on Gimlet and 0x20-0x29 on Cosmo. It has been flowing into inventory under the name slot, which reads as the bay number printed on the chassis. Unfortunately, the PCI number is not actually the physical bay number.

This PR keeps that prior value of "slot" (though it tries to rename it as pcie_slot, where possible) and adds the value operators actually need: the chassis label from libtopo, such as "N5" for a U.2 bay or "M.2 East". This is the same label nvmeadm list -L and diskinfo -P show. This is reported as location.

What this PR does

Adds a dependency on libtopo
Renames slot to pcie_slot in code, to try to minimize confusion.
Actually uses libtopo to look up the new location information. Uses a small cache to avoid incurring too much cost (it costs about half a second to take a snapshot).
Plumbs this new location info into inventory

What this PR doesn't

The external API. PhysicalDisk views do not yet carry location, and the unadopted-disk list already exposes the raw PCIe slot as slot. Both need an external API version and are the next step for physical disks should know which slot they're in #11088.
Deriving disk variant or boot-disk identity from topo instead of the per-board slot ranges, and dropping the slot column in favor of keying inventory on disk identity. Both are follow-ups once.

Manual Testing

I used fridge to deploy e35b60a to berlin. Once it deployed, I poked at the latest inventory collection, which looked like this:

INV_COLLECTION_ID                    SLED_ID                              PCIE_SLOT LOCATION VENDOR MODEL                     SERIAL       VARIANT FIRMWARE NEXT_FIRMWARE
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 0         N0       1b96   WUS4C6432DSP3X3           A079E23A     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 1         N1       1b96   WUS4C6432DSP3X3           A079E4A3     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 2         N2       1b96   WUS4C6432DSP3X3           A079E56C     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 3         N3       1b96   WUS4C6432DSP3X3           A079DF28     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 4         N4       1b96   WUS4C6432DSP3X3           A079E236     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 5         N5       1b96   WUS4C6432DSP3X3           A079DFC2     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 6         N6       1b96   WUS4C6432DSP3X3           A079DF50     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 7         N7       1b96   WUS4C6432DSP3X3           A079E323     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 8         N8       1b96   WUS4C6432DSP3X3           A079DDE6     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 9         N9       1b96   WUS4C6432DSP3X3           A079E406     U2      R2210010 R2210010
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 17        M.2 East 1344   Micron_7300_MTFDHBG1T9TDF 214132748217 M2      95420280
46089e62-f398-4c48-b6e8-8b999b2af2fd 65bd59eb-f6d5-45b0-9916-c805c3c55ecf 18        M.2 West 1344   Micron_7300_MTFDHBG1T9TDF 214132749518 M2      95420280

As you can see, the new "LOCATION" values are showing up, as they should be.

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).

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.

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.

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.

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.

hawkw · 2026-09-11T17:19:26Z

            variant: DiskVariant::M2,
-            slot: 0,
+            pcie_slot: 0,
+            location: None,


I feel like it would be nicer for the example disks to report locations, instead of None, since real disks are much likelier to report them than not:

Suggested change

location: None,

location: Some("M.2 West".to_string()),

Agreed, done for this (and below)

hawkw · 2026-09-11T17:25:13Z

+            write!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?;
+            if let Some(location) = location {
+                write!(indent2, " ({location})")?;
+            }


hm, I'm a bit on the fence about whether we want to keep displaying things in the original format with the location string just kind of tacked on the end. i feel like saying the disk is "in" the PCIe slot, and then just not printing the location at all if it's unknown, is potentially misleading a reader into thinking that we are telling them the physical label on the chassis. What do you think about reworking this format a bit to make the actual location more "important"-seeming to the reader than the PCIe slot?

Perhaps something like this:

Suggested change

write!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?;

if let Some(location) = location {

write!(indent2, " ({location})")?;

}

let loc = location.as_deref().unwrap_or("unknown location");

writeln!(

indent2,

"{variant:?}: {identity:?} in {loc} (PCIe slot {pcie_slot})",

)?;

Or, really, maybe we shouldn't even be displaying the PCIe slot here? Is that really important information to report in this context?

agreed, I'm going to drop the pcie slot. I kinda think that shouldn't be reported up to Nexus at all, but that'll require some database re-keying to finally drop it.

I kinda think that shouldn't be reported up to Nexus at all, but that'll require some database re-keying to finally drop it.

I also think that, and had been hoping we would discover nothing was using it at all; now that we know it's not, I'm fine with saving that for a subsequent change.

hawkw · 2026-09-11T17:27:50Z

+fn pcie_slot_to_disk_variant(
+    sled: OxideSled,
+    pcie_slot: i64,
+) -> Option<DiskVariant> {
+    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,
    }
 }


Not directly related, but these kinda make me wonder a bit if we might want to do some additional refactoring to make a PcieSlot newtype and turn these into methods, rather than representing it as the untyped i64? Would be fine to make that a follow-up.

Agreed, I'll make a follow-up PR

hawkw · 2026-09-11T17:29:04Z

+// 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.


i feel like this comment is liable to become outdated pretty quickly on a module named topo; i can easily imagine non-disk topo stuff landing in here too...

hawkw · 2026-09-11T17:30:47Z

+/// 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.


What are the consequences/cost of opening a handle? Is there a tradeoff here between opening new handles on every call versus using a Mutex or something around the handle?

Oh, upon a further reading, I think I misunderstood this --- maybe the comment is not saying that it is unsafe to take a snapshot concurrently, it's saying that any reuse of the handle is unsafe? Is that correct?

I'm going to work through this - I originally reported https://www.illumos.org/issues/18110 because it caused issues here, and the remediation was "do not re-use handles", but I think it has since been fixed.

Okay, this is coming back to me. I built protection against this issue into libtopo:

https://github.com/oxidecomputer/libtopo/blob/39fba468f41b752d1b0b38d8d4894f724af84611/libtopo/src/lib.rs#L134-L144

So, "unsafe" is a bit misleading here; we're guarded by the underlying API.

I do think now that 18110 has been resolved, I can probably go remove that in libtopo. but until then, we do need to open new handles. Fortunately, the cost of opening handles is a lot less than the cost of taking a snapshot, so the impact should be pretty small here regardless.

I do think now that 18110 has been resolved, I can probably go remove that in libtopo. but until then, we do need to open new handles. Fortunately, the cost of opening handles is a lot less than the cost of taking a snapshot, so the impact should be pretty small here regardless.

Cool, okay. I'm fine with leaving this code as it is, I just wanted to make sure I understood any nuance of it.

hawkw · 2026-09-11T17:38:57Z

+        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);
+            }
+        };


IMO the nested match is harder to follow than the straight-line code of first matching on the value and then matching on NvmeInstance::TryFrom:

Suggested change

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 instance = {

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);

}

};

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);

}

}

};

sounds good. I can flatten the value out of this too, so I'll do that as well

hawkw · 2026-09-11T17:40:22Z

+        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))
+        });


i feel this could perhaps deserve a comment explaining what the fallback-to-looking-at-the-parent-node behavior here is all about?

It's documented in the function doc, but I'll include something here too.

hawkw · 2026-09-11T17:41:17Z

+        });
+        match label {
+            Some(label) => {
+                labels.insert(instance, label);


what happens if we saw two nodes with the same instance and it's already in labels? should we at least log an error or something about that?

I think this would imply a corrupt topology tree, but I'm happy to make this log an error rather than do silent replacement.

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.

Review feedback on #11279: printing "in <pcie slot>" 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.

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.

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.

…ad 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.

libtopo 0.2.0 re-exports the <fm/topo_hc.h> 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.

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.

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.

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.

hawkw · 2026-09-16T20:21:03Z

Seems like the Helios CI run is dying because it can't link with libtopo: https://buildomat.eng.oxide.computer/wg/0/details/01M2NVS1RRZV9770QMCPV51356/lrB1Flk6awKzqgRCrhjBflw4E3Na2I5TpHsx6kckYmyOYrNF/01M2NVSW37JP5RYKM6QV8SWT32#S5781

smklein · 2026-09-16T20:35:55Z

Seems like the Helios CI run is dying because it can't link with libtopo: https://buildomat.eng.oxide.computer/wg/0/details/01M2NVS1RRZV9770QMCPV51356/lrB1Flk6awKzqgRCrhjBflw4E3Na2I5TpHsx6kckYmyOYrNF/01M2NVSW37JP5RYKM6QV8SWT32#S5781

Ugh, working through this now.

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.

smklein · 2026-09-16T21:13:10Z

As a follow-up: I tried looking into what we could do to remove the column we're now calling pcie_slot.

As a reminder: pcie_slot is the old value we used for slot, which we're supplanting with location (e.g. "N1").

The main problem for this is inventory. The inv_physical_disk table uses (inv_collection_id, sled_id, and slot) as it's primary key -- it uses this instead of the "disk identity" (vendor/model/serial combo) used elsewhere so that we can cope with two disks reporting the same serial without blowing up. The "slot" was used as the distinguisher because, well, you should not be able to have two disks using the same slot.

In this new world, we can't trivially replace slot with location, because it's fallible through libtopo.

That leaves us with some options:

Leave the pcie_slot where it is; it's part of the inventory primary key
Make it a harder error when we cannot observe the label, so that the value is non-optional. This option would require jumping through a couple releases though, because the value of location will be "NULL" on inventories across all existing deployments after this PR's schema change runs

smklein · 2026-09-17T00:23:29Z

As a follow-up: I tried looking into what we could do to remove the column we're now calling pcie_slot.

As a reminder: pcie_slot is the old value we used for slot, which we're supplanting with location (e.g. "N1").

The main problem for this is inventory. The inv_physical_disk table uses (inv_collection_id, sled_id, and slot) as it's primary key -- it uses this instead of the "disk identity" (vendor/model/serial combo) used elsewhere so that we can cope with two disks reporting the same serial without blowing up. The "slot" was used as the distinguisher because, well, you should not be able to have two disks using the same slot.

In this new world, we can't trivially replace slot with location, because it's fallible through libtopo.

That leaves us with some options:

Leave the pcie_slot where it is; it's part of the inventory primary key

Make it a harder error when we cannot observe the label, so that the value is non-optional. This option would require jumping through a couple releases though, because the value of location will be "NULL" on inventories across all existing deployments after this PR's schema change runs

and FWIW my preference, as usual, is "Merge this PR and then do nothing for now". I think I'm okay-ish with the "eliminate optional" pathway but we'd need bake-time across releases to go that route anyway

hawkw · 2026-09-16T20:34:38Z

-            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";


turbo nitpick: perhaps this should now also say:

Suggested change

log, "skipping M.2 drive with unexpected slot number";

log, "skipping M.2 drive with unexpected PCIe slot number";

hawkw · 2026-09-16T20:52:33Z

+    /// See [`nexus_types::inventory::PhysicalDisk::pcie_slot`]. The column
+    /// keeps its original name because columns cannot be renamed
+    /// idempotently.


I don't love the comment "the column keeps its original name" without explicitly saying "this used to be named xxx". Perhaps:

Suggested change

/// See [`nexus_types::inventory::PhysicalDisk::pcie_slot`]. The column

/// keeps its original name because columns cannot be renamed

/// idempotently.

/// See [`nexus_types::inventory::PhysicalDisk::pcie_slot`]. The

/// database column was originally named `slot`, but this field

/// is renamed to make its meaning clearer. The database column

/// itself cannot be renamed idempotently.

or...something?

This seemed implied to me by the column_name = slot line below, but I can be more explicit.

hawkw · 2026-09-16T20:55:20Z

    inv_collection_id: DbTypedUuid<CollectionKind>,
    sled_id: DbTypedUuid<SledKind>,
-    slot: i64,
+    /// See [`InvPhysicalDisk::pcie_slot`].


is this really providing value? i'd prefer we either have a comment that's useful (here it might be worth explaining what the difference between this and active_slot, which is totally unrelated but also includes the word "slot"), or not have comment at all; the other fields that are also present on InvPhysicalDisk don't have corresponding "See InvPhysicalDisk::name_of_field" comments...

I'll rework comments on this struct. I totally agree, the line of "pcie slot" vs "nvme slot" seems really easy to fudge, so I'll just self-describe, rather than referencing other shit.

hawkw · 2026-09-16T20:56:06Z

                })?;
                paginator =
-                    p.found_batch(&batch, &|row| (row.sled_id, row.slot));
+                    p.found_batch(&batch, &|row| (row.sled_id, row.pcie_slot));


i presume this is not going to change because we are still using pcie slot as a pkey?

Yeah, exactly.

hawkw · 2026-09-16T20:57:56Z

                        variant: DiskVariant::U2,
-                        slot: i64::try_from(i).unwrap(),
+                        pcie_slot: i64::try_from(i).unwrap(),
+                        location: Some(format!("N{i}")),


I note that we are not making fake M.2s here, but that was the case before so I guess it's fine?

yeah, variant was U2 for all these beforehand, so... it's fine I suppose.

We can start making M2, but I don't want this PR to introduce that

hawkw · 2026-09-16T21:17:33Z

+                Entry::Vacant(entry) => {
+                    entry.insert(label);
+                }
+                Entry::Occupied(entry) => error!(


hm, is this an error or a warning? it seems like we just keep chugging along, so. but on the other hand i guess it's implying the topo tree is somehow corrupted...

Yeah, I'm not sure there's a great answer here.

If we read a topo tree with multiple duplicate labels, it probably is corrupted...

... if we throw an error, then this case stops us from reading the topo tree at all.

... if we warn and "do something" (like ignore the subsequent label, etc), we'll get some topo tree back, but an incomplete one.

Do we think it's better to stop topo from being read here, or to tweak the output and "do the best we can"?

(in the meanwhile, I'm dropping this to warn, because we currently aren't stopping the world)

hawkw · 2026-09-16T21:20:28Z

+    /// 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.


per #11258 (comment), it sounds like we also shouldn't expect these to remain stable across future host OS versions, so we may want to note that as well?

Added a note

hawkw · 2026-09-16T21:24:49Z

+
+use std::fmt;
+
+/// Instance number of an `nvme` driver node, the `N` in `nvme<N>`.


"the N in nvme" feels a bit...weird as a way to define this. i would maybe want to define this in terms of who in the system is assigning these to things?

hawkw · 2026-09-16T21:25:06Z

+//! 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.


nitpicky: i feel like all of this description belongs on the type, not the module?

Sure, I'll move it.

hawkw · 2026-09-16T21:52:31Z

+impl TryFrom<i32> for NvmeInstance {
+    type Error = InvalidNvmeInstance;
+
+    /// From a devinfo instance number, which is signed but never negative.
+    fn try_from(value: i32) -> Result<Self, Self::Error> {
+        if value < 0 {
+            Err(InvalidNvmeInstance(value.into()))
+        } else {
+            Ok(Self(value))
+        }
+    }
+}
+
+impl TryFrom<u32> for NvmeInstance {
+    type Error = InvalidNvmeInstance;
+
+    /// From a topo `io/instance` property, which is unsigned.
+    fn try_from(value: u32) -> Result<Self, Self::Error> {
+        i32::try_from(value)
+            .map(Self)
+            .map_err(|_| InvalidNvmeInstance(value.into()))
+    }
+}


Hm, I'm not sure if I love that these are just TryFroms and not some more domain specific from_devinfo and from_topo or something? It would be the same code, but it might be a bit better at representing the semantic behavior of the conversion? Not a big deal though.

I'll use from_devinfo, since I believe it's more a property of driver assignment

This reverts 901fb2a and 831a0ef, which routed libtopo's runpath through omicron-rpaths, and returns to the arrangement 603073f 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.

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.

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.

hawkw

thought of some additional pain and/or fear, with my sincerest apologies

hawkw · 2026-09-17T20:10:00Z

+
+    /// 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) {


so, um, i ... don't like update_mutable_properties very much, but i am also struggling to come up with something i dislike less, so maybe this is fine haha.

hawkw · 2026-09-17T20:37:16Z

        match self {
            Disk::Real(pooled_disk) => {
                pooled_disk.firmware = raw_disk.firmware().clone();
+                pooled_disk.location = raw_disk.location().map(str::to_string);


hm, this logic is mildly sketchy to me; it looks like if we read a location successfully, and then a subsequent attempt to read it fails in libtopo someplace, we blip the location out of existence again? is that a potential issue here? i wonder if we might want to do something like, if it's currently Some, and we are merging with a new RawDisk where the location is None, we leave it Some if and only if the PCIe slot number is the same (which we would hope indicates the thing has not moved?). or uh the NVMe driver instance number or something is the same?

This feels contingent on libtopo's output being optional, while libdevinfo's output is required. I think that's what this PR currently does, but I think it's the wrong disposition. I'd like to experiment with changing that first before building a tiny cache here.

This feels contingent on libtopo's output being optional, while libdevinfo's output is required. I think that's what this PR currently does, but I think it's the wrong disposition.

Agreed, as we discussed yesterday!

I'd like to experiment with changing that first before building a tiny cache here.

Sounds good, I was mostly just thinking about things in the current approach. If we're able to change this to require topo to work, I'll be happier anyway!

...sorry, just to clarify, are we planning to do that in this PR, or subsequently as per #11319?

I was going to try to whip up a follow-up PR if that's possible, but I can merge it into this one if they end up making breaking changes in (separately) uncomfortable ways

I'm fine with whatever you think makes sense to do here. Happy to wait to re-review this one until you've investigated further?

hawkw · 2026-09-17T20:43:49Z

    pub serial: String,
    pub variant: PhysicalDiskKind,
-    /// See [`nexus_types::inventory::PhysicalDisk::location`].
+    /// The location of the disk in the chassis, as identified by libtopo.


considered unimportant, but github's diff highlighting for this is extremely funny:

hawkw · 2026-09-17T20:44:52Z

+                            .filter(|(_, instance)| {
+                                !locations.contains_key(instance)
+                            })
+                            .map(|(_, instance)| instance.to_string())


somewhat surprised that the clippy "why aren't you using filter_map" lint doesn't fire on this, not that I personally care

- 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.

smklein · 2026-09-18T17:34:34Z

Follow-ups for this PR:

physical disks should know which slot they're in #11088 - actually plumb this through the external API
libtopo should probably be "not optional" to view disks #11319 - poking at this now
sled-agent physical_disk inventory should include topo's label, rather than pcieb's physical-slot# #11258 - basically tracking the "Removal of pcie_slot to disk type" mapping altogether

smklein added 4 commits September 11, 2026 09:19

smklein marked this pull request as draft September 11, 2026 16:54

hawkw reviewed Sep 11, 2026

View reviewed changes

smklein added 4 commits September 11, 2026 11:36

This was referenced Sep 15, 2026

Expose hc scheme names as &str constants oxidecomputer/libtopo#15

Closed

Add typed property getters mirroring topo_prop_get_* oxidecomputer/libtopo#16

Closed

smklein added 6 commits September 15, 2026 13:48

merge

f69fbb6

smklein marked this pull request as ready for review September 16, 2026 19:41

hawkw reviewed Sep 17, 2026

View reviewed changes

smklein mentioned this pull request Sep 17, 2026

sled-agent physical_disk inventory should include topo's label, rather than pcieb's physical-slot# #11258

Open

smklein added 2 commits September 17, 2026 11:29

hawkw reviewed Sep 17, 2026

View reviewed changes

smklein added 2 commits September 18, 2026 08:41

merge

8dbe94d

hawkw added the fault-management Everything related to the fault-management initiative (RFD480 and others) label Sep 18, 2026

smklein mentioned this pull request Sep 18, 2026

libtopo should probably be "not optional" to view disks #11319

Open

	log, "skipping M.2 drive with unexpected slot number";
	log, "skipping M.2 drive with unexpected PCIe slot number";


		use std::fmt;

		/// Instance number of an `nvme` driver node, the `N` in `nvme<N>`.

Conversation

smklein commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

What this PR doesn't

Manual Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw commented Sep 16, 2026

Uh oh!

smklein commented Sep 16, 2026

Uh oh!

smklein commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smklein commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein commented Sep 11, 2026 •

edited

Loading

smklein commented Sep 16, 2026 •

edited

Loading