-
Notifications
You must be signed in to change notification settings - Fork 62
BlueprintBuilder: don't hold on to PlanningInput
#9374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| // We'll check both the disks available to this sled per our current | ||
| // blueprint and the list of all in-service zpools on this sled per our | ||
| // planning input, and only pick zpools that are available in both. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the logical change I mentioned in the PR description: I removed this second check that we only pick a zpool that's also InService according to the planning input.
I believe this change should not affect any planner behavior. It always decommissions disks before potentially adding new zones, so any disks that are no longer in service in the planning input will also no longer be in service according to the BlueprintPhysicalDiskDisposition by the time we get here.
This change could affect some weirder tests that only go through BlueprintBuilder, but I'm hoping we don't have any that trip over that particular case. If we do I think I'd claim the test is probably in the wrong: it means they'd be decommissioning disks, not updating the disk config to match, then trying to add zones that require a trip through this method to pick an available disk.
f2abab5 to
ce13a32
Compare
ce13a32 to
430665d
Compare
sunshowers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems great to me, thanks for taking care of it. The logical change seems pretty reasonable to me.
| // These fields are used to allocate resources for sleds. | ||
| input: &'a PlanningInput, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we ensure the PlanningInput doesn't creep its way in here in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any way to guarantee it, but: I'm working on a few more PRs to remove the need to pass it into BlueprintBuilder::new_based_on() at all. Hopefully at that point the bar for adding it back is pretty high.
This builds on top of #9370 and is a big piece of #9238: as of this PR,
BlueprintBuilderno longer holds aPlanningInput, which should reduce confusion on where a given bit of logic should live. Anything that needs the planning input to be decided now must be implemented in the planner.BlueprintBuilder::new_based_on(..)still takes aPlanningInputargument, which I don't love but need to do some more work to figure out how (if?) to address. It uses it for a few things; I think I'd group them as:BaseboardIdand subnet of each active sleds. This is the one that seems trickiest to tackle.)SledEditors for commissioned sleds that are present inPlanningInputbut not the parent blueprint. (I think I'd make the case thatnew_based_on()shouldn't do this at all, and the planner should do it?)PlanningInput. (Specifically: the cockroachdb fingerprint and the internal/external DNS generations. I think it would be fine forBlueprintBuilderto only copy the parent blueprint, and provide setters that the planner could use to update these to newer values.)There's one hopefully-small-and-fine logical change in this PR that I'll point out below with some extra details.