Move ClickhouseClusterConfig construction from BlueprintBuilder to planner
#9370
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewjstone
approved these changes
Nov 7, 2025
Contributor
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
Looks great. Thanks!
| InlineErrorChain::new(&err), | ||
| ); | ||
|
|
||
| // If planning the new config failed, carry forward our |
Contributor
There was a problem hiding this comment.
Thanks for adding this extra comment here.
Base automatically changed from
john/prune-blueprint-resource-allocator
to
main
November 10, 2025 19:49
jgallagher
added a commit
that referenced
this pull request
Nov 12, 2025
This builds on top of #9370 and is a big piece of #9238: as of this PR, `BlueprintBuilder` no longer holds a `PlanningInput`, 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 a `PlanningInput` argument, 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: 1. Reassembling information about sleds that we knew in previous iterations but that aren't stored in the blueprint. (Specifically: the `BaseboardId` and subnet of each active sleds. This is the one that seems trickiest to tackle.) 2. Adding new, empty `SledEditor`s for commissioned sleds that are present in `PlanningInput` but not the parent blueprint. (I think I'd make the case that `new_based_on()` shouldn't do this at all, and the planner should do it?) 3. Making a copy of a few fields that are present in the blueprint, but that are updated each time the planner runs based on potentially-newer values in `PlanningInput`. (Specifically: the cockroachdb fingerprint and the internal/external DNS generations. I think it would be fine for `BlueprintBuilder` to 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is staged on top of #9347 and is a part of #9238. In particular, by moving this from the builder to the planner, we can remove the builder's need for an inventory collection entirely.
There should be no functional changes here; it's just moving code around (with some reformatting to break it up a bit) and then removing the
collectionargument from all the callers ofBlueprintBuilder::new_based_on().