[reconfigurator] BlueprintBuilder cleanup 1/5 - move BlueprintDisksEditor to its own module
#7103
Conversation
Changes the API style to be more imperative. One behavioral change is that "generation 1" is only ever the set of empty disks; if any disks are added, that becomes "generation 2". This is consistent with how we define the OmicronZonesConfig generation, but is a change from what's on main.
| Some(&config.disks) | ||
| } | ||
|
|
||
| pub fn build( |
There was a problem hiding this comment.
Could we include some docs about why we're passing sled_ids again here?
(e.g., why are we passing this here, when BlueprintDisksEditor is already taking a map of sleds as input?)
There was a problem hiding this comment.
I agree with Sean that a comment is a good idea.
To check my understanding, the sleds we take in the constructor are all from the parent blueprint, but these sleds are the InService sleds from the PlanningInput which may be different. This allows us to add a DiskConfig::Empty for any new sleds, and also to ignore sleds that are no longer in service but may have configurations.
There was a problem hiding this comment.
I think the last bit is the most relevant one: we do indeed pass only InService sleds, so in practice the incoming sled IDs here allow us to drop sleds that were present in the parent blueprint but are no longer in service.
TBH this is confusing, there were related bits that confused Sean and me in the original datasets PR (#6229 (comment)). We'll have to address this when we start squishing these maps back together, at which point the sled IDs here probably go away?
I'll add the comment though!
| parent_changed_set: &'a mut BTreeSet<SledUuid>, | ||
| } | ||
|
|
||
| impl Drop for SledDisksEditor<'_> { |
There was a problem hiding this comment.
Neat, so the mutable borrows here require that SledDisksEditor is dropped before BlueprintDisksEditor::build gets called, right?
There was a problem hiding this comment.
Correct.
You could always leak or mem::forget() a SledDisksEditor, but at that point you get what you deserve, I think? The alternative would be for sled_disks_editor to take an FnOnce(&mut SledDisksEditor), but that's much more awkward from a caller's point of view. It seems fine to say "if you leak a SledDisksEditor your edits won't be fully reflected".
| Some(&config.disks) | ||
| } | ||
|
|
||
| pub fn build( |
There was a problem hiding this comment.
I agree with Sean that a comment is a good idea.
To check my understanding, the sleds we take in the constructor are all from the parent blueprint, but these sleds are the InService sleds from the PlanningInput which may be different. This allows us to add a DiskConfig::Empty for any new sleds, and also to ignore sleds that are no longer in service but may have configurations.
This is mostly moving code around; specifically,
BlueprintDisksEditoris now in its own module. The non-"moving code around" changes are:OmicronPhysicalDisksConfigis now only the empty set of disks; if there are disks present, that will be at least generation 2. This caused some expectorate churn but seems probably fine? It matches howOmicronZonesConfiggenerations are numbered.