BlueprintBuilder: remove internal resource_allocator()
#9347
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
jgallagher
commented
Nov 5, 2025
| // similar case to the previous paragraph: a zone with networking resources was | ||
| // expunged, the database doesn't realize it yet, but can still move forward and | ||
| // make planning decisions that reuse those resources for new zones. | ||
| pub(super) fn ensure_input_networking_records_appear_in_parent_blueprint( |
Contributor
Author
There was a problem hiding this comment.
I deleted this for a couple reasons:
- I removed its only caller (
resource_allocator()), and it's not obvious where a new caller would be. This is more of a blippy-level check, except that all the existing blippy checks only look at theBlueprintin isolation, and this is checking for consistency between aBlueprintand aPlanningInput. - This is here to bail out of planning early if we have some kind of mismatch between the
external_ipCRDB table and the current targetBlueprint. I don't believe it's ever actually caught a problem, and we don't have similar checks for other tables that blueprint execution updates.
But if this makes folks uneasy and have a suggestion for the first bullet, I don't feel strongly about it.
Contributor
There was a problem hiding this comment.
FWIW I'd love to have blippy (optionally?) accept a PlanningInput at some point. There's at least one check with the mupdate-update work (host phase 2 image sources should be set to current if a mupdate override is set for that sled) that would be unblocked by that.
sunshowers
approved these changes
Nov 5, 2025
Comment on lines
123
to
126
| .context("constructing ExternalNetworkingAllocator")? | ||
| .for_new_nexus() | ||
| .context("choosing external IP for new Nexus")?; | ||
| builder |
Contributor
There was a problem hiding this comment.
nit: the context messages here don't quite match up with the ones above
| // similar case to the previous paragraph: a zone with networking resources was | ||
| // expunged, the database doesn't realize it yet, but can still move forward and | ||
| // make planning decisions that reuse those resources for new zones. | ||
| pub(super) fn ensure_input_networking_records_appear_in_parent_blueprint( |
Contributor
There was a problem hiding this comment.
FWIW I'd love to have blippy (optionally?) accept a PlanningInput at some point. There's at least one check with the mupdate-update work (host phase 2 image sources should be set to current if a mupdate override is set for that sled) that would be unblocked by that.
This was referenced Nov 6, 2025
jgallagher
added a commit
that referenced
this pull request
Nov 10, 2025
This PR adds a new method to (optionally) check a blueprint against a given planning input. The only check included in this PR is a line-for-line port of the builder's `ensure_input_networking_records_appear_in_parent_blueprint()` function (which I'm removing from the builder in #9347). I updated most of the `Blippy` call sites to use this method; I skipped the one in an RSS test because it has no planning input available - RSS just generates a raw blueprint to hand off to Nexus.
jgallagher
added a commit
that referenced
this pull request
Nov 10, 2025
…o planner (#9370) 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 `collection` argument from all the callers of `BlueprintBuilder::new_based_on()`.
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 finishes the set of PRs that tackle the first bullet of #9238:
This requires callers to pass in external networking decisions when adding zones that need them (Nexus, External DNS, and Boundary NTP). IMO this is a big win to the planner / builder split - the planner creates an allocator at an appropriate point during planning, and uses that to make decisions about external networking - but it makes all the other
BlueprintBuilderconsumers (tests) a fair bit noisier, because they all have to do the same thing. I think this is still a win overall, but I'm very interested if others feel differently (and how we could clean up the test consumers too, if possible!).A couple ideas on that last bit:
BlueprintBuilder::available_external_networking(policy: &ExternalIpPolicy) -> Result<ExternalNetworkingAllocator>)method that just passes itself toExternalNetworkingAllocator::from_current_zones(). (No real semantic change, but slightly shorter call sites?)BlueprintBuilder::next_{nexus,external_dns,boundary_ntp}_external_ip(policy: &ExternalIpPolicy) -> Result<ExternalIp>methods that internally create anExternalNetworkingAllocator, use it to generate a single IP, then throw it away. This would be correct but wasteful if used by the few callers that allocate multiple IPs, so they could still go with the "construct an allocator, use it several times" they have as the PR is now, but it would shorten many of the tests that only want to create one or two and that don't care about a bit of waste.