sled-agent: Move InternalDisks and friends from sled-agent-config-reconciler to sled-agent-types
#8497
Conversation
| /// disk properties (e.g., as created by `Self::fake_static()`). It is also | ||
| /// only exposed to this crate; external callers should only operate on the | ||
| /// view provided by `InternalDisks`. Internal-to-this-crate callers should | ||
| /// take care not to hold the returned reference too long (as this keeps the | ||
| /// watch channel locked). | ||
| pub(crate) fn borrow_and_update_raw_disks( | ||
| /// disk properties (e.g., as created by `Self::fake_static()`). | ||
| /// | ||
| /// This method is very low-level and should only be used by | ||
| /// sled-agent-config-reconciler's inner workings. Other sled-agent | ||
| /// consumers should not use this method and should prefer operating on the | ||
| /// `InternalDisks` returned by `current()`. | ||
| /// | ||
| /// Callers should take care not to hold the returned reference too long (as | ||
| /// this keeps the watch channel locked). |
There was a problem hiding this comment.
This is the only change I don't love; this was pub(crate), and it would've been nice to keep it restricted to only the config reconciler. Not the end of the world, though.
There was a problem hiding this comment.
I think this is fine -- maybe change the name to have internal in it or something.
| sled-hardware.workspace = true | ||
| sled-hardware-types.workspace = true | ||
| sled-storage.workspace = true |
There was a problem hiding this comment.
Ah crap, this does introduce a libnvme.so.1 dependency to sled-agent-types, sorry...
One option is to move it to sled-storage, next to Disk. Another is to have a separate "sled-agent-storage-types" crate.
|
You mentioned this in chat:
and wow I don't know how I missed that. I'm not sure any of this work is necessary; I think from the config-reconciler's point of view, all it wants is a way to ask sled-agent for the current Lemme give that a whirl and see if I can close this altogether. If not, yeah moving this to |
I don't think we need this at all. That might change when we incorporate the mupdate override / config-reconciler, but I think it's unlikely. (#8514 introduces the first integration point without needing any of this.) |
2 participants
This allows us to break the crate dependency sled-agent-zone-images has on sled-agent-config-reconciler, which will allow us to introduce a dependency in the other direction (needed to address #8463 and implement various mupdate-override-related things).
I changed all the
IdMaps involved toiddqd::IdOrdMap, which also allowed dropping some extra ID wrapper types that introduced cheaply-cloneable keys, sinceIdOrdMapallows borrowed keys. ❤️