Report each disk's chassis location in sled inventory - #11279
Conversation
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.
| variant: DiskVariant::M2, | ||
| slot: 0, | ||
| pcie_slot: 0, | ||
| location: None, |
There was a problem hiding this comment.
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:
| location: None, | |
| location: Some("M.2 West".to_string()), |
There was a problem hiding this comment.
Agreed, done for this (and below)
| write!(indent2, "{variant:?}: {identity:?} in {pcie_slot}")?; | ||
| if let Some(location) = location { | ||
| write!(indent2, " ({location})")?; | ||
| } |
There was a problem hiding this comment.
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:
| 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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, | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agreed, I'll make a follow-up PR
| // 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. |
There was a problem hiding this comment.
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...
| /// 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay, this is coming back to me. I built protection against this issue into libtopo:
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.
There was a problem hiding this comment.
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.
| identity: DiskIdentity, | ||
| is_boot_disk: bool, | ||
| firmware: DiskFirmware, | ||
| /// See [`Self::location`]. |
| 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); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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:
| 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); | |
| } | |
| } | |
| }; |
There was a problem hiding this comment.
sounds good. I can flatten the value out of this too, so I'll do that as well
| 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)) | ||
| }); |
There was a problem hiding this comment.
i feel this could perhaps deserve a comment explaining what the fallback-to-looking-at-the-parent-node behavior here is all about?
There was a problem hiding this comment.
It's documented in the function doc, but I'll include something here too.
| }); | ||
| match label { | ||
| Some(label) => { | ||
| labels.insert(instance, label); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
2 participants
Part of #11258. Groundwork for #11088.
Sled-agent identifies disks by the PCIe physical slot number of the bridge above them (
physical-slot#on thepciebnode). 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 nameslot, which reads as the bay number printed on the chassis and is not.This PR keeps that number, names it what it is, and adds the value operators actually need: the chassis label from the platform's hardware topology (libtopo), "N5" for a U.2 bay or "M.2 East" for a boot device. This is the same label
nvmeadm list -Landdiskinfo -Pshow, and helios owns the mapping in its topology maps, so omicron stops maintaining a copy.Commits
Add libtopo dependency to sled-hardware. Adds
libtopo0.1.0 from crates.io as an illumos-only dependency. libtopo needsillumos-nvpair0.4, so the workspace pin moves from 0.3 (the 0.4 release is purely additive; oxlog, the only in-tree user, compiles unchanged) andfmd-admis bumped to its current main revision, which differs from the old pin only by the same nvpair bump. Cargo's re-resolution wanted to flip several unrelated wide-range dependencies (windows-sys, socket2, heck, itertools) to newer versions already in the lock; those were reverted so the lockfile diff is limited to these packages.Test coverage: manifest-only. Verified with cargo check on Linux and illumos (sled-hardware, oxlog, omdb, sled-agent),
cargo xtask check-workspace-deps, andcargo hakari generate(no workspace-hack changes).Rename disk "slot" to "pcie_slot" in sled-hardware and Nexus types. Pure rename on
UnparsedDisk,PooledDisk,RawDisk,Disk,RawSyntheticDisk, andnexus_types::inventory::PhysicalDisk, plus the per-board range helpers onOxideSled, with doc comments saying what the number is and is not. Inventory display output is unchanged. The sled-agent API type and the database columns are deliberately left for the last commit.Test coverage: no behavior change;
cargo check --workspace --all-targetson Linux andcargo check --all-targetsof the illumos-gated crates on a Helios host.Report each disk's chassis location from the hardware topology. sled-hardware asks libtopo where each NVMe controller sits and records the label as a new
location: Option<String>onUnparsedDiskandPooledDisk. The lookup mirrors nvmeadm: walk thehcscheme, find eachnvmenode byio/instance, take its label or its parent's when the parent is abayorslot. The devinfo nvme instance sled-hardware already reads is the join key, carried as anNvmeInstancenewtype.A topo snapshot costs about half a second on a sled and the device tree is polled every five seconds, so a
DiskLocationCacheremembers what topo said per controller. Topo is consulted on the first poll after boot and again only when a controller appears that it has not been asked about; unlabelled controllers and outright topo failures are retried at most once a minute. A controller's label cannot change while the system is up (instance number fixed by devinfo path, label fixed by the static topology map), so learned labels are kept for the life of the process. Location is best-effort and never prevents a disk from being used; synthetic and hardcoded disks reportNone.Test coverage: unit tests in
sled_hardware::disk_locationdrive the cache against a stand-in for the topo snapshot and cover first sight, steady state, throttled retry of unlabelled controllers, a new controller bypassing the throttle, and a topo failure keeping known labels. The topo walk itself is illumos-only; it is ported from thedisk_locationsexample in the libtopo repo, which was run on a dogfood Gimlet and a dogfood Cosmo and matcheddiskinfo -Pon every disk (U.2s 0-9 and 32-41 both resolving to N0-N9, M.2s to "M.2 East"/"M.2 West"). Also the sled-agent-config-reconciler suite, and illumos cargo check of the sled-agent crates.Carry disk chassis location through the sled-agent API into inventory. Sled Agent API version 54 (
ADD_DISK_LOCATION_TO_INVENTORY) addslocationtoInventoryDiskand renames itsslottopcie_slot; older clients are served through the usual provided-method conversion chain. Nexus stores the label in a new nullablelocationcolumn oninv_physical_disk(schema 300), shows it in the inventory display when present, and prints it inomdb db inventory physical-disks.The inventory columns holding the PCIe slot number keep the name
slotin the database, since the schema README rules out idempotent column renames; the Rust fields arepcie_slotvia#[diesel(column_name = slot)]and thedbinit.sqlcomments now say what the number is. The simulated sled-agent reports no location.Test coverage:
cargo xtask openapi checkpasses, and the version 54 document differs from version 53 by exactly the twoInventoryDiskfield changes. The omicron-nexus schema suite passes, including idempotent re-application of the new migration and the dbinit-versus-migrations comparison; the nexus-db-model verification-file test confirms no backfill file is needed. Inventory and physical-disk datastore tests in nexus-db-queries pass. Alsocargo check --workspace --all-targets, illumos cargo check of the sled-agent crates,cargo xtask clippy, andcargo fmt --check.Not in this PR
PhysicalDiskviews do not yet carrylocation, and the unadopted-disk list already exposes the raw PCIe slot asslot. Both need an external API version and are the next step for physical disks should know which slot they're in #11088.slotcolumn in favor of keying inventory on disk identity. Both are follow-ups once the label has been in production inventory for a while.omdb db inventory physical-disksfrom a deployed sled-agent is still to do.