[sled-agent-config-reconciler] Flesh out ExternalDisks type
#8111
Conversation
jgallagher
force-pushed
the
john/sled-agent-config-reconciler-external-disks
branch
from
733eddb to
4a56c9c
Compare
|
|
||
| impl DiskAdopter for RealDiskAdopter<'_> { | ||
| async fn adopt_disk( | ||
| &mut self, |
There was a problem hiding this comment.
Does this need to be &mut self? Or is that just a convenience for tests?
| // Take ownership of our current disk state (if we have one), then | ||
| // immediately put back the new state. | ||
| let maybe_current = self.disks.remove(&config.id); | ||
| self.disks.insert( | ||
| self.try_ensure_disk_managed( | ||
| maybe_current, | ||
| config, | ||
| raw_disk, | ||
| disk_adopter, | ||
| log, | ||
| ) | ||
| .await, | ||
| ); |
There was a problem hiding this comment.
Given that "disk adoption" might involve creating partitions on the underlying disks, it seems like it might be nice to do this operation concurrently?
This appears to currently be:
- Take out the Option for the disk
- Call "try_ensure_disk_managed" to transform that state (or create it, if it was None)
- Put that state back into
self.disks
Each operation seems to be acting on independent parts of self.disks, and I don't think the disk_adopter actually needs mutability. WDYT about doing this concurrently?
There was a problem hiding this comment.
(I care less about this in the removal path, because that's significantly cheaper)
|
|
||
| #[derive(Debug, Default)] | ||
| struct TestDiskAdopter { | ||
| requests: Vec<RawDisk>, |
There was a problem hiding this comment.
AFAICT, wrapping this in an Arc<Mutex<..>>> should enable us to operate on &self instead of &mut self within adopt_disk
Staged on top of #8103, and is pretty similar to it. The biggest difference is that #8103 adds a tokio task responsible for adopting internal disks automatically, and this adds an
ExternalDiskstype that will be used by the reconciler task to adopt external disks only when asked to viaOmicronSledConfig.