Repository navigation
Conversation
The hardware poll now starts from libtopo's U.2 bay and M.2 socket nodes instead of devinfo, so the topology is authoritative for which disks exist and every disk has a chassis location. libnvme fills in what the topology lacks: the vendor id, the count of usable (not ignored) namespaces, and firmware slots. The disk variant comes from the bay kind, which removes the per-board PCIe slot range tables. Faults are handled per bay. A bay the topology or libnvme can't describe keeps what the previous poll said about it, and the rest of the poll goes ahead. If the bays can't be read at all, the previous disks stand, so sled-agent startup no longer depends on it. HardwareView reports when its picture of the disks is incomplete, and installinator retries until it isn't, rather than writing to only one M.2.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I checked whether removing #[serde(default)] from UnparsedDisk.location breaks reading older serialized data. No persisted or fixture JSON contains UnparsedDisk, and raw_disks.rs still sends inventory location as Some(...), so the wire format is unchanged.
Extended reasoning...
The change moves sled-hardware disk discovery from devinfo to libtopo and libnvme, and makes a disk's chassis location required throughout sled-storage and the config reconciler. The serde ruled-out check found no checked-in JSON that serializes UnparsedDisk, and the inventory conversion still fills in the optional field.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
sled-hardware/src/illumos/topo.rs— Possible regression if libtopo returns a non-PropertyNotFounderror for any one node: all disks, M.2s included, vanish from the view. On a fresh start sled-agent then finds no disks. AnyTopoErrorfrom a bay, nvme, disk, board or ic node stops the walk and failsread_disk_baysfor the whole sled. Fix: report every per-node error, includingUnlabelledBayandBayWithoutSlot, as anObservedOccupant::Unreadablebay (or skip the node) instead of aborting the walk. Abort only when the handle or snapshot itself fails, which matches the PR's per-bay fault promise. [also at: sled-hardware/src/illumos/topo.rs:208 - One bay with no label or nobinding/slotnow makes the whole topology read fail. At startup, sled-agent and installinator then get no disks at all.]Why this was flagged
This is conditional on a libtopo condition that cannot be reproduced here. It applies when
node.property_string/property_u32returns an error other thanPropertyNotFound(for example a type mismatch) on any bay child. It also applies when one bay or M.2 slot node lacks a label orbinding/slot. In those casesoptional_string/optional_u32orvisit_bayreturn Err,BayWalker::visitstores it asfailureand stops the walk, andread_disk_baysreturns Err. read_bays then fails, soHardwareSnapshot.baysisNone. On the first poll inHardwareManager::newthere is no previous view, sodisksstays empty and sled-agent sees neither M.2 nor U.2. On the base branch the topology was only used for labels: a failure there logged a warning, and devinfo still reported every disk. The PR says faults are per bay, but this path turns one bad node into a sled-wide loss of disks.Verification: In sled-hardware/src/illumos/topo.rs,
optional_stringandoptional_u32(lines 389-412) map onlyError::PropertyNotFoundtoOk(None), andvisit_bayreturns hard errors forUnlabelledBay(line 182) andBayWithoutSlot(line 188). Inread_disk_bays(lines 70-79), the walk callback stores the error, returnsWalkAction::Stop, and thenreturn Err(err). Base only logged a warning and reported the disks without locations.
An error reading a node under a bay, or a bay's binding properties, now makes only that bay unreadable, so it keeps its previous state while the rest of the poll goes ahead. Only a bay that cannot be identified (no label, or an unreadable slot type) still fails the whole read, which keeps the previous view rather than dropping that bay's disk.
this isn't really a regression. All that being said, we can make the errors per-bay in certain situations. Done in 3d37f71 |
HardwareSnapshot.bays was an Option, with None meaning the bays could not be read. A Result keeps the reason, and poll_device_tree now makes the choice explicit: an Err leaves the disks as they are.
|
@claude review |
|
I think this PR is blocked by oxidecomputer/helios#273 |
HardwareSnapshot::new now opens the libtopo handle and takes the snapshot, and passes it to read_bays and read_disk_bays. Future readers of the topology can share the same snapshot rather than each taking their own. A failed snapshot still only makes the bays unreadable.
A bay that the topology or libnvme cannot describe is now reported as DiskBayOccupant::Unreadable with the error, rather than as whatever the previous poll saw. The disk last seen at that bay's location stays in the disk list, so a transient fault still does not drop a disk. Installinator fails its hardware scan, and retries, when an M.2 bay is unreadable. Before this it would install to the other M.2 alone.
An unreadable bay carries its error alongside its own location, so the location in the error was repeated wherever the two were shown together. The warning that logs the error now carries the location as a key instead.
40edc37 to
a43217e
Compare
A topology read can succeed without some bays, for example when one enumerator fails partway, and a bay left out that way used to take its disk with it. resolve_bays now takes the poll's result along with the previous bays, and reports every previous bay the poll did not find as unreadable while keeping its disk. A poll that could not read the bays at all goes through the same path, so its bays no longer report the last good poll as current. The bays are now a map keyed by location, and a topology with two bays of the same location fails the read.
|
@claude review |
| pub zpool_name: ZpoolName, | ||
| pub firmware: DiskFirmware, | ||
| /// See [`UnparsedDisk::location`]. | ||
| pub location: Option<String>, |
There was a problem hiding this comment.
In summary, this is the "goal" of this PR. The location information we're reading from libtopo should be non-optional.
|
|
||
| // Monitor for block devices. | ||
| let mut disks = HashMap::new(); | ||
| match external_disks { |
There was a problem hiding this comment.
This is the other "high-level goal" of this PR - note that we aren't walking the blkdev driver anymore, we're looking at the topology tree itself.
smklein
changed the title
smklein
changed the title
| tofino: TofinoSnapshot, | ||
| disks: HashMap<DiskIdentity, UnparsedDisk>, | ||
| /// `Err` if the disk bays could not be read at all this time. | ||
| bays: Result<Vec<PolledBay>, Error>, |
There was a problem hiding this comment.
Worth keeping in mind, this is supposed to be a point-in-time view, so if we fail to read all disk bays, we'll see an error here. If we fail to read some disk bays, they won't show up in the Vec.
However, I want to guard against "we failed to read a single bay, and that messes up the whole view of inventory", or "we hit an OOM or something and couldn't read bays". Both of these are handled in the resolve_bays function below, where we translate this "hardware snapshot" into a "hardware view".
(The gist is that "we avoid mutating things seen in prior snapshots unless they've been read successfully").
Every namespace on a controller shares its DiskIdentity, so sled-agent cannot manage them as separate disks. Rather than silently using the first, report the bay's device as unusable, with the namespace count.
The topology reader now records every `slot` node with its slot type, and `assemble_bays` treats only M.2 slots as disk bays. `bay` and `slot` records share their bay properties through `BayProps`, and `TopoNodeKind::UnknownSlot` is replaced by a `Slot` whose slot type could not be read, which still fails the read with `BayIdentError::SlotType`.
On illumos, TOPO_SLOT_TYPE_M2 and the test-only TOPO_SLOT_TYPE_CEM now come from libtopo::hc. Elsewhere they are stand-ins, so bay classification is still tested off illumos. Also drop a comment that misattributed the storage property names to <fm/topo_hc.h>.
`NodeKey`, `Prop`, `TopoNode`, `TopoNodeKind` and `BayProps` describe what the illumos reader copies out of libtopo, not how bays are assembled from it, so they now live apart from `assemble_bays`. No behaviour change.
It combines what the topology and libnvme reported about a bay into the bay and disk sled-agent uses, which callers already call describing it (`PolledBay::Described`, "Could not describe disk bay"). `ClassifyError` becomes `DescribeError` to match.
|
Recommended reviewing order:
3.. sled-hardware/src/disk_bay.rs: Contains functions for extracting additional information from libnvme ( |
Stacked on #11279. Part of #11319.
sled-agent now finds disks starting from libtopo's U.2 bay and M.2 socket nodes, rather than devinfo. That makes libtopo for which disks exist, and every disk now has a non-optional chassis location.
Testing: unit tests for sled-hardware (bay classification and per-bay resolution), sled-storage, the config reconciler and installinator; the illumos-only code builds on illumos.