Skip to content

Commit

Permalink
[reconfigurator] Introduce a combined SledEditor inside `BlueprintB…
Browse files Browse the repository at this point in the history
…uilder` (#7204)

This is a step on the road to #7078. The current `BlueprintBuilder`
internals would not at all be amenable to squishing the disparate maps
in `Blueprint` together, so this PR tries to rework those internals.
`BlueprintBuilder` now does its own map squishing (inside
`new_based_on()`), combining state+zones+disks+datasets into one helper
(`SledEditor`). All modifications are preformed via those editors, then
`BlueprintBuilder::build()` breaks the combined editors' results back
out into four maps for `Blueprint`.

There should be only one functional change in this PR (marked with a
`TODO-john`): previously, expunging a zone checked that that zone had
not been modified in the current planning cycle. I'm not sure that's
really necessary; it felt like some defensiveness born out of the
complexity of the builder itself, maybe? I think we could put it back
(inside `ZonesEditor`, presumably) if folks think it would be good to
keep.

My intention was to change the tests as little as possible to ensure I
didn't break anything as I was moving functionality around, so
`new_based_on()` and `build()` have some arguably-pretty-gross
concessions to behave exactly the way the builder did before these
changes. I'd like to _remove_ those concessions, but those will be
nontrivial behavioral changes that I don't want to try to roll in with
all of this cleanup. I think I landed this pretty well; there are only a
few expectorate changes (due to the slightly reworked `ZonesEditor`
producing a different ordering for a few tests), and none in `omdb`
(which is where I've often seen incidental planner changes bubble out).

Apologies for the size of the PR. I'd lightly recommend that the new
`blueprint_editor/` module and its subcomponents should be pretty
quickly reviewable and should be looked at first; they're _supposed_ to
be simple and obvious, and are largely ported over from the prior
storage / disks / datasets / zones editors. I did rework how we're
handling #6645 backwards compat to try to reduce how much we need to
pass `SledResources` around, so that complicates things in
`DatasetsEditor` some, but not too bad IMO. (I also added a test for
this, since I changed it and I don't think we had one before.) The
`BlueprintBuilter` changes should then be more or less the natural way
one would use a collection of `SledEditor`s without changing its API or
behavior (yet!).
  • Loading branch information
jgallagher authored Dec 5, 2024
1 parent 1922c8e commit e73a30e
Show file tree
Hide file tree
Showing 18 changed files with 1,620 additions and 1,751 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions common/src/disk.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ impl DatasetName {
Self { pool_name, kind }
}

pub fn into_parts(self) -> (ZpoolName, DatasetKind) {
(self.pool_name, self.kind)
}

pub fn pool(&self) -> &ZpoolName {
&self.pool_name
}
Expand Down
Loading

0 comments on commit e73a30e

Please sign in to comment.