Add sled slot to disks in externals /hardware/disks endpoint - #11221
Open
fakemonster wants to merge 4 commits into
Open
Add sled slot to disks in externals /hardware/disks endpoint#11221fakemonster wants to merge 4 commits into
fakemonster wants to merge 4 commits into
Conversation
Using an IdOrdMap means less scanning for disk slots on calls to the /system/disks API, but it has a few side effects, all of which I believe are benign: 1. While the DiskIdentity strikes me as a reasonable key, it doesn't match the primary key `inv_physical_disk` uses, and so a DB lookup could in principle report the same DiskIdentity for two slots, and we'll drop one of the duplicates now. This deduplication also applies at collection time, meaning if prior to this commit we were recording two disks with the same identity into crdb, that'll no longer happen (which may then hide an unhealthy case we'd want to catch!). But I don't believe this is a problem in principle, because the `physical_disk` table itself has a unique index on the components of DiskIdentity, so a duplicate disk would be unadoptable anyways. 2. Relatedly, since that case seems unlikely, I opted for `from_iter` instead of `from_iter_unique`. It would perhaps be more rigorous to attempt `from_iter_unique`, and loudly fall back to `from_iter`? 3. When omdb lists disks, they'll now be ordered by their identity (and omit duplicates, which as mentioned before, I'm not so sure happens in practice). I figure no one is bothered by this, but it's a difference!
fakemonster
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Kind of like #11077, but for disks! Particularly it's similar in that it's grafting the slot number on from an inventory check. The hope in the linked issue is that it'd be unnecessary in this case because
physical_diskgets recreated when disks are moved between sleds, so we could potentially add aslotcolumn tophsyical_diskand skip the inventory check. But as far as I can tell, a disk readopted to the same sled won't go through the same flow, so we'd need bigger changes to accomplish that.Additionally, this refactors the inventory's disk
Vecinto anIdOrdMapfor a bit of convenience. This is done as its own commit to be easily revertable, in case it's undesirable, but I believe it's a generally reasonable improvement, and the side effects justified (discussed in that commit's message body).Resolves #11088