[reconfigurator] BlueprintBuilder cleanup 4/5 - when choosing a zpool, it must be from a disk present in the blueprint
#7106
Conversation
| // NOTE: We'll probably need to actually add disks here | ||
| // when the Blueprint contains "which disks back zones". | ||
| // | ||
| // However, for now, this isn't necessary. |
There was a problem hiding this comment.
Thank you for fixing this, the way you've done so makes a ton of sense
| // TODO Should we know the dataset kind from inventory? We could | ||
| // probably infer it from the name, but yuck. | ||
| kind: None, |
There was a problem hiding this comment.
Yeah, I didn't do this already, because I did not want inventory collection to fail if we see a dataset with a new "kind" value. So I can't actually map this directly to a DatasetKind value without having an Unknown variant, or without treating it as an optional.
There was a problem hiding this comment.
Would this not fall under the nexus -> sled-agent API compatibility?
There was a problem hiding this comment.
I suppose it could? I don't know if this is a rational concern, but the scope of inventory feels a little more like it could be arbitrary than other more-structured aspects of the API. For example, I wanted to ensure that "regardless of what shows up within hardware / datasets / etc, we can return something reasonable back to Nexus".
Basically, if it was possible for a "new, arbitrary dataset" to show up, I would still want that to be reportable through inventory, as much as possible -- even if Nexus can't request such a dataset, because it isn't a part of the strongly-typed Nexus -> Sled Agent API.
| @@ -1220,8 +1220,49 @@ stdout: | |||
| blueprint ......<REDACTED_BLUEPRINT_ID>....... | |||
| parent: <none> | |||
|
|
|||
| !..........<REDACTED_UUID>........... | |||
There was a problem hiding this comment.
Wow, it is nice to get rid of this!
…isk that's in our blueprint
jgallagher
force-pushed
the
john/reconfigurator-storage-4
branch
from
a0c5837 to
f3652cc
Compare
… ensure_zone_multiple_* to add_zone_* (#7107) All of the callers of the `ensure_zone_multiple_*` methods to add discretionary zones were, in practice, wanting to _add_ a specific number of zones. This rewords all of those methods to be imperative, allowing us to remove a pretty significant chunk of now-unnecessary checks and logic. Builds on and is staged on top of #7106.
Builds and is staged on top of #7105.
The intended change here is in the first commit (8274174): In
BlueprintBuilder::sled_select_zpool(), instead of only looking at thePlanningInput, we also look at the disks present in the blueprint, and only select a zpool that the planning input says is in service and that we have in the blueprint.This had a surprisingly-large blast radius in terms of tests - we had many tests which were adding zones (which implicitly selects a zpool) from a
BlueprintBuilderwhere there were no disks configured at all, causing them to emit invalid blueprints. These should all be fixed as of this PR, but I'm a little worried about test fragility in general, particularly with an eye toward larger changes like #7078. Nothing to do about that at the moment, but something to keep an eye on.Fixes #7079.