BlueprintZoneDisposition::Expunged - track additional properties#7558
Merged
jgallagher merged 9 commits intomainfrom Feb 20, 2025
Merged
BlueprintZoneDisposition::Expunged - track additional properties#7558jgallagher merged 9 commits intomainfrom
jgallagher merged 9 commits intomainfrom
Conversation
The `Expunged` zone disposition can now also track: * the config generation in which the zone was expunged * whether we've confirmed that the zone has been shut down Currently `confirmed_shut_down` is always false; the planner will fill it in to `true` in a subsequent PR. While I was modifying this enum: removed the `Quiesced` state, which we don't use yet (and it's not clear how that will need to be represented when we get to it in the future).
andrewjstone
approved these changes
Feb 19, 2025
Contributor
andrewjstone
left a comment
andrewjstone
left a comment
There was a problem hiding this comment.
Looks great. After all our discussion today, this lines up with expectations. It also provides us with the ability now to fix bugs like #7380.
I like the name change of the bool from confirmed_shut_down to ready_for_cleanup. Please be sure to change the commit message to reflect this on merge.
…ition-expunged-cleanup
Contributor
Author
|
Couple small but nontrivial edits since the review: |
jgallagher
added a commit
that referenced
this pull request
Feb 20, 2025
This fell out of discussion around and is staged on top of #7558. Since that PR removes the diesel `blueprint_zones_filter()` extension (now that `BlueprintZoneDisposition::Expunged` carries extra data with it that has to be spread across multiple columns), the biggest motivator for `BlueprintZoneFilter` is gone. We still want to force ourselves to consider zone dispositions, so this PR keeps the `filter` argument to `Blueprint::all_omicron_zones()` and related functions, but that filter is now a closure instead of a `BlueprintZoneFilter`. All the call sites boiled down to one of: * `all_omicron_zones(BlueprintZoneDisposision::is_in_service)` (helper method that wraps `matches!(self, BlueprintZoneDisposition::InService)`) * `all_omicron_zones(BlueprintZoneDisposision::is_expunged)` (helper method that wraps `matches!(self, BlueprintZoneDisposition::Expunged { .. })`) * `all_omicron_zones(BlueprintZoneDisposition::any)` (helper that always returns `true`) With followup work to have the planner fill in `BlueprintZoneDisposition::Expunged { ready_for_cleanup: true }`, some of the `is_expunged` callers will change to something like `is_ready_for_cleanup()`. But for now, this PR should introduce no behavioral changes at all.
hawkw
pushed a commit
that referenced
this pull request
Feb 21, 2025
The `Expunged` zone disposition can now also track: * the config generation in which the zone was expunged * whether we've confirmed that the zone is ready for any post-expungment cleanup Currently `ready_for_cleanup` is always false; the planner will fill it in to `true` in a subsequent PR. While I was modifying this enum: removed the `Quiesced` state, which we don't use yet (and it's not clear how that will need to be represented when we get to it in the future).
hawkw
pushed a commit
that referenced
this pull request
Feb 21, 2025
This fell out of discussion around and is staged on top of #7558. Since that PR removes the diesel `blueprint_zones_filter()` extension (now that `BlueprintZoneDisposition::Expunged` carries extra data with it that has to be spread across multiple columns), the biggest motivator for `BlueprintZoneFilter` is gone. We still want to force ourselves to consider zone dispositions, so this PR keeps the `filter` argument to `Blueprint::all_omicron_zones()` and related functions, but that filter is now a closure instead of a `BlueprintZoneFilter`. All the call sites boiled down to one of: * `all_omicron_zones(BlueprintZoneDisposision::is_in_service)` (helper method that wraps `matches!(self, BlueprintZoneDisposition::InService)`) * `all_omicron_zones(BlueprintZoneDisposision::is_expunged)` (helper method that wraps `matches!(self, BlueprintZoneDisposition::Expunged { .. })`) * `all_omicron_zones(BlueprintZoneDisposition::any)` (helper that always returns `true`) With followup work to have the planner fill in `BlueprintZoneDisposition::Expunged { ready_for_cleanup: true }`, some of the `is_expunged` callers will change to something like `is_ready_for_cleanup()`. But for now, this PR should introduce no behavioral changes at all.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The
Expungedzone disposition can now also track:Currently
confirmed_shut_downis always false; the planner will fill it in totruein a subsequent PR.While I was modifying this enum: removed the
Quiescedstate, which we don't use yet (and it's not clear how that will need to be represented when we get to it in the future).The followup work to fill in
confirmed_shut_downand use it in the executor is important, and I'll hold off merging this until that work is up in a PR. But I think this is far enough along that it's ready for review, and I'd like to get some eyes on it to see what we think abouta) storing cleanup eligibility in
Expungedas opposed to a separatestatefieldb) storing the
as_of_generationto help the planner know when cleanup can proceedStoring these inside the disposition required getting rid of the
blueprint_zone_dispositiondiesel filter (which itself relied onstrumenum iteration, which isn't supported for a data-bearing variant like the newExpunged). I think this is not just okay but expected: we're saying the expunged disposition is more complex than a single column can represent. The diesel filter was to help downstream consumers of the blueprint, but we've recently changed directions there and said downstream consumers really ought to be reading rendezvous tables instead. There was only one user of theblueprint_zone_dispositionfilter; I changed it to just look at thedispositioncolumn directly for now, but noted it should use a rendezvous table at some point.