Blueprint execution: Split up top-level cleanup implementation #9512
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
Before this change, the high-level structure of this was "for each
ready-for-cleanup zone in the blueprint, match on its type and then
maybe perform some cleanup action depending on the type". After this
change, this is inverted: "for each zone type we care about, for each
ready-for-cleanup zone of that type in the blueprint, perform the
relevant cleanup action".
On its face this is a little worse: we now loop over the zones in the
blueprint twice times (currently, because we have two zone types we
care about for cleanup, although a third is implemented and compiled out
for now) instead of once. The intent here is that we're going to attach
a `reason` for why a given caller wants to act on expunged zones, and
it's much clearer if it's broken out this way. For example, in future
work we'll change this:
```rust
let zones_to_clean_up = blueprint
.all_omicron_zones(BlueprintZoneDisposition::is_ready_for_cleanup)
.filter(|(_sled_id, zone)| zone.zone_type.is_oximeter());
```
to something like
```rust
let zones_to_clean_up = blueprint.expunged_oximeter_zones_ready_to_clean_up(
Reason::OximeterProducerReassignment,
);
```
davepacheco
approved these changes
Dec 19, 2025
Collaborator
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
The only comment I had here is tiny and really on the pre-existing code.
jgallagher
added a commit
that referenced
this pull request
Jan 7, 2026
…xpunged methods (#9521) The primary change in this PR is that `Blueprint::all_omicron_zones(filter)` has been replaced by two new methods: * `Blueprint::in_service_zones()` (no arguments) * `Blueprint::expunged_zones(ready_for_cleanup, reason)` where `ready_for_cleanup` allows filtering on that field, and `reason` is a not-used-at-runtime marker for "why are we looking for expunged zones" The `reason` is the driver for this change. One of my big fears for #5552 is that we do the work today to make the planner account for any cleanup we do on expunged zones, but then some future work adds a new kind of cleanup without realizing the planner now has to consider that too. This led to the new `BlueprintExpungedZoneAccessReason` enum, which I'm hoping serves as both a static guard (because it has to be passed to `expunged_zone()`) and as a point of documentation (describing all the different things the planner has to consider when expunging zones). File ordering puts it toward the end, but I'd recommend reviewing `nexus/types/src/deployment.rs` first - the changes to the rest of the files are largely straightforward fallout from that. (One bit that's slightly surprising is that a bunch of `all_omicron_zones(BlueprintZoneDisposition::any)` calls were replaced by `in_service_zones()`; this is mostly in tests that were using `::any` but didn't actually need to access expunged zones.) This is staged on top of #9512.
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.
Before this change, the high-level structure of this was "for each ready-for-cleanup zone in the blueprint, match on its type and then maybe perform some cleanup action depending on the type". After this change, this is inverted: "for each zone type we care about, for each ready-for-cleanup zone of that type in the blueprint, perform the relevant cleanup action".
On its face this is a little worse: we now loop over the zones in the blueprint twice (currently, because we have two zone types we care about for cleanup, although a third is implemented and compiled out for now) instead of once. The intent here is that we're going to attach a
reasonfor why a given caller wants to act on expunged zones, and it's much clearer if it's broken out this way. For example, in future work we'll change this:to something like
and it's harder to express some kind of "composite reason" if we were to keep the original structure.