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

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

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?

- 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 · 2026-10-29T23:00:33Z

welp, I managed to do some testing, and this PR is blocked by oxidecomputer/helios#273 as well. I'll write more details there.

smklein · 2026-11-02T19:44:30Z

@claude review

claude

Additional findings (outside the current diff — GitHub can't attach inline comments there):

🔴 .cargo/xtask.toml — If the recovery (trampoline) OS image does not ship /usr/lib/fm/amd64/libtopo.so.1, installinator would fail to start and mupdate recovery would stop working. sled-hardware now links libtopo, and installinator is on the new libtopo.so.1 allow-list. That makes libtopo a hard NEEDED dependency, so the runtime linker refuses to load the binary without it, before the soft-failure path in topo.rs runs. Fix: confirm the trampoline image delivers the FM libraries. If it does not, load libtopo lazily or keep it out of installinator's link, so read_disk_locations failing stays the only result.

Why this was flagged

The trigger is a deploy condition this checkout cannot check: whether the installinator host image (the recovery/trampoline image) includes the fault-management package that delivers /usr/lib/fm/amd64/libtopo.so.1. installinator depends on sled-hardware (installinator/Cargo.toml:40). sled-hardware now depends on libtopo (sled-hardware/Cargo.toml:39). .cargo/xtask.toml:144-152 explicitly allows installinator to link libtopo.so.1, and .cargo/config.toml adds -R/usr/lib/fm/amd64. A missing NEEDED library makes ld.so abort the exec, so installinator cannot run at all. On the base branch installinator only needed libnvme and libipcc, and it worked in that image. The graceful handling in sled-hardware/src/illumos/mod.rs (an Err from topo::read_disk_locations gives HashMap::new()) only covers runtime topo failures, not a library that fails to load. The manual testing in the description deployed to a running rack and did not exercise a mupdate through installinator.

Verification: installinator depends on sled-hardware (installinator/Cargo.toml:40), which now links libtopo. .cargo/xtask.toml adds installinator to the libtopo.so.1 allow-list, so it has a hard DT_NEEDED on libtopo.so.1. A missing NEEDED library makes ld.so.1 refuse to start the process before main, so the soft-failure handling in topo.rs never runs. On the base, installinator linked nothing from /usr/lib/fm.

smklein · 2026-11-02T20:01:14Z

Additional findings (outside the current diff — GitHub can't attach inline comments there):

🔴 .cargo/xtask.toml — If the recovery (trampoline) OS image does not ship /usr/lib/fm/amd64/libtopo.so.1, installinator would fail to start and mupdate recovery would stop working. sled-hardware now links libtopo, and installinator is on the new libtopo.so.1 allow-list. That makes libtopo a hard NEEDED dependency, so the runtime linker refuses to load the binary without it, before the soft-failure path in topo.rs runs. Fix: confirm the trampoline image delivers the FM libraries. If it does not, load libtopo lazily or keep it out of installinator's link, so read_disk_locations failing stays the only result.

wow thanks for that if only I had a PR which I linked to that already fixed this and is also merged oxidecomputer/helios#274

hawkw

Okay, great, I'm glad we can finally move forwards with this!

I took the liberty of rewriting some bits that felt particularly gallingly Fable 5-y. Hope you don't mind.

hawkw · 2026-11-05T23:39:47Z

+    /// Note that PCIe slots are specific to the board topology,
+    /// and are unrelated to NVMe firmware slots.


thanks for adding this, i found the preexisting naming here very upsetting...

hawkw · 2026-11-05T23:46:25Z

+/// 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: the wrapper allows one snapshot per
+/// handle (see <https://github.com/oxidecomputer/libtopo/issues/14>), and a
+/// snapshot costs far more than the handle anyway.


My guess is that eventually, once we are using more of libtopo, we are probably going to want to refactor this so that you can reuse one snapshot for multiple things, but I'm fine with keeping it simple for now and doing that later once we actually need it.

hawkw · 2026-11-05T23:47:44Z

+    debug!(
+        log,
+        "read disk locations from hardware topology";
+        "count" => labels.len(),
+        "elapsed" => ?start.elapsed(),
+    );


IIRC sled-agent disables debug! logs in the product build, so if we expect this to be there in production systems...it won't be. Not sure if that's intended or not.

hawkw · 2026-11-05T23:49:32Z

+            None => debug!(
+                log,
+                "nvme topology node has no location label";
+                "nvme_instance" => %instance,
+            ),


this one, I presume, can remain at debug! because there's a log in the calling code with all the NVMe nodes that lack labels?

hawkw · 2026-11-05T23:51:47Z

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


Weird lack of verbs here...how about:

Suggested change

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

/// Returns the driver instance of an `nvme` node, from its `io/instance`.

/// property.

///

/// If the node has no such property, if that property is not a `uint32`

/// (which is the documented type in libtopo), or if the property cannot

/// be read this function returns `None` and logs a message.

hawkw · 2026-11-05T23:55:39Z

+    })
+}
+
+/// A node's label, treating "no label" and an empty label alike.


Suggested change

/// A node's label, treating "no label" and an empty label alike.

/// Returns a node's label, or `None` if it has no label or the label is an

/// empty string.

hawkw · 2026-11-06T00:23:20Z

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


Suggested change

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

/// Every device node that binds to a driver gets an instance number, which is

/// unique among that driver's nodes. Instance numbers are recorded in

/// `/etc/path_to_inst`, so they are persistant across reboots.

///

/// Instance numbers are exposed by devinfo on the controller's node, and

/// reported by libtopo as the `io/instance` property of its `nvme` node. In

/// libnvme, controllers are opened by instance number. Therefore,

/// sled-hardware uses instance numbers as a shared key to associate the chassis

/// locations reported by libtopo to the controller reported by devinfo.

///

/// Instance numbers are unrelated to the PCIe physical slot number and to NVMe

/// firmware slots.

hawkw · 2026-11-06T00:23:34Z

+#[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


Suggested change

/// An instance number outside the range devinfo and libnvme use, which is

/// An instance number outside the range used by devinfo and libnvme, which is

hawkw · 2026-11-06T00:30:37Z

+            .map_err(|_| InvalidNvmeInstance(value.into()))
+    }
+
+    /// The instance number as devinfo and libnvme represent it.


Suggested change

/// The instance number as devinfo and libnvme represent it.

/// Returns the instance number as devinfo and libnvme represent it.

hawkw · 2026-11-06T00:33:10Z

+    /// The file must be named `m2_<serial>.vdev` or `u2_<serial>.vdev`. When
+    /// `<serial>` 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.


Suggested change

/// The file must be named `m2_<serial>.vdev` or `u2_<serial>.vdev`. When

/// `<serial>` 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.

/// The file must be named `m2_<serial>.vdev` or `u2_<serial>.vdev`. When

/// `<serial>` is an integer, it also determines the disk's chassis

/// location. For example, `u2_3.vdev` sits in bay N3, while `m2_0.vdev`

/// and `m2_1.vdev` are M.2 East and M.2 West. Otherwise, the location is

/// determined by `pcie_slot`.

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

Comment thread sled-hardware/src/illumos/topo.rs Resolved

Comment thread sled-hardware/src/illumos/topo.rs Resolved

Comment thread sled-hardware/src/illumos/topo.rs Resolved

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

Comment thread nexus/db-model/src/inventory.rs Resolved

Comment thread sled-hardware/src/illumos/mod.rs Resolved

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

smklein added 3 commits October 8, 2026 11:24

merge

b253d2e

merge

ed8b4f3

merge

d45b283

This was referenced Oct 23, 2026

Find disks through the hardware topology, and report disk bays in inventory #11587

Closed

Use topo instead of devinfo for disks, make location required #11590

Open

smklein added 2 commits October 23, 2026 15:14

merge

6cecf2f

merge

a1ae16d

smklein mentioned this pull request Oct 29, 2026

Recovery image construction prevents us from using libtopo instead of libdevinfo to monitor for disks in omicron oxidecomputer/helios#273

Closed

smklein mentioned this pull request Oct 30, 2026

image: keep libtopo dependencies in the recovery image oxidecomputer/helios#274

Merged

merge

8923b45

claude Bot reviewed Nov 2, 2026

View reviewed changes

hawkw approved these changes Nov 6, 2026

View reviewed changes

		/// Note that PCIe slots are specific to the board topology,
		/// and are unrelated to NVMe firmware slots.

-/// 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.
+/// Returns the driver instance of an `nvme` node, from its `io/instance`.
+/// property.
+///
+/// If the node has no such property, if that property is not a `uint32`
+/// (which is the documented type in libtopo), or if the property cannot
+/// be read this function returns `None` and logs a message.

	/// A node's label, treating "no label" and an empty label alike.
	/// Returns a node's label, or `None` if it has no label or the label is an
	/// empty string.

-/// 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.
+/// Every device node that binds to a driver gets an instance number, which is
+/// unique among that driver's nodes. Instance numbers are recorded in
+/// `/etc/path_to_inst`, so they are persistant across reboots.
+///
+/// Instance numbers are exposed by devinfo on the controller's node, and
+/// reported by libtopo as the `io/instance` property of its `nvme` node. In
+/// libnvme, controllers are opened by instance number. Therefore,
+/// sled-hardware uses instance numbers as a shared key to associate the chassis
+/// locations reported by libtopo to the controller reported by devinfo.
+///
+/// Instance numbers are unrelated to the PCIe physical slot number and to NVMe
+/// firmware slots.

	/// An instance number outside the range devinfo and libnvme use, which is
	/// An instance number outside the range used by devinfo and libnvme, which is

	/// The instance number as devinfo and libnvme represent it.
	/// Returns the instance number as devinfo and libnvme represent it.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

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!

Uh oh!

Uh oh!

smklein commented Sep 18, 2026

Uh oh!

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

Uh oh!

Uh oh!

smklein commented Nov 2, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

smklein commented Nov 2, 2026

Uh oh!

hawkw left a comment

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

smklein commented Sep 11, 2026 •

edited

Loading

smklein commented Sep 16, 2026 •

edited

Loading

smklein commented Oct 29, 2026 •

edited

Loading