Skip to content

Conversation

@jgallagher
Copy link
Contributor

The high-level summary of this change is that it moves the logic for handling expunged sleds out of BlueprintBuilder::expunge_zones_for_sled() (which is now gone entirely) and into a new method on the planner (do_plan_expunge_for_commissioned_sled(), naming suggestions welcome).

This fixes two problems; one stylistic, and one semantic:

  1. BlueprintBuilder::expunge_zones_for_sled() was maybe the worst offender for "is this a builder API or a planner API", because a big chunk of its logic was actually delegated back to zone_needs_expungement(), a helper function defined in the planner. The builder now has more explicit and direct methods to expunge specific things, and it's the planner that decides based on the planning input which things to expunge.
  2. BlueprintBuilder::expunge_zones_for_sled() only handled expunging zones. When a sled was expunged, at no point did the planner expunge the disks or datasets on that sled. We haven't been bitten by this yet because the builder currently omits expunged sleds from blueprint_disks and blueprint_datasets entirely, but that's a major roadblock in the path to joining the maps together (which is a prereq for fixing Reconfigurator "deploy datasets then zones" ordering will break for zone expungement #7309 by merging sled-agent endpoints together). do_plan_expunge_for_commissioned_sled() expunges disks, datasets, and zones. For now we still omit the expunged sleds from blueprint_disks and blueprint_datasets, but when we stop doing that we'll at least have set the dispositions correctly.

Copy link
Contributor

@andrewjstone andrewjstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it immediately. I'm going to pull it into #7286 to deal with disk expungement instead of what I have for that in the planner/builder.

@jgallagher jgallagher merged commit 42d86cd into main Feb 6, 2025
16 checks passed
@jgallagher jgallagher deleted the john/planner-builder-expunge branch February 6, 2025 21:28
jgallagher added a commit that referenced this pull request Feb 18, 2025
These were supposed to be fixed before merging #7495, but were not.

Both of these are of the flavor "should never happen unless there's an
internal bug" and could arguably be `debug_assert`s, but having the
option for Nexus panics due to internal bugs here is not awesome.
jgallagher added a commit that referenced this pull request Feb 18, 2025
These were supposed to be fixed before merging #7495, but were not.

Both of these are of the flavor "should never happen unless there's an
internal bug" and could arguably be `debug_assert`s, but having the
option for Nexus panics due to internal bugs here is not awesome.
hawkw pushed a commit that referenced this pull request Feb 21, 2025
These were supposed to be fixed before merging #7495, but were not.

Both of these are of the flavor "should never happen unless there's an
internal bug" and could arguably be `debug_assert`s, but having the
option for Nexus panics due to internal bugs here is not awesome.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants