Blueprints: store sled state #5663
Merged
Conversation
This file contains 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
davepacheco
approved these changes
Apr 30, 2024
| @@ -1603,6 +1604,12 @@ mod tests { | |||
| } | |||
| } | |||
|
|
|||
| fn sled_states_active( | |||
There was a problem hiding this comment.
The repetition of this reinforces the idea that we could use a nice low-level builder interface that could take care of this.
There was a problem hiding this comment.
I thought this would require some serious rework of BlueprintBuilder and the planner to make the builder less plan-y, but I was wrong (although I think there is still a lot of room for improvement there). I reworked this test to use BlueprintBuilder in 4949eac and it went pretty well. There were two spots (creating bp2 and bp4) where it was easier to clone the parent blueprint and manually poke it than to go through the builder, but it still seems like a pretty significant cleanup overall.
andrewjstone
approved these changes
May 8, 2024
jgallagher
added a commit
that referenced
this pull request
May 13, 2024
This builds on (and is currently pointed at) #5663, and is on the path to fixing #5625, although there is still more work to do there. (Should be small, and I'll start on it while this is in review to make sure it really fixes it.) I don't plan on landing this before R8 is out the door, but wanted to go ahead and open it up to review. Much of this is fallout from our discussions about what it means to decommission a sled. Major changes are: * `SledFilter::All` has been renamed to `SledFilter::Commissioned`, and no longer returns sleds that are in `SledState::Decommissioned`. * The blueprint planner will now update the desired sled state to `Decommissioned` for sleds which satisfy our conditions. (See `do_plan_decommission()`.) * The blueprint planner will carry forward the Omicron zones of decommissioned sleds to child blueprints. Pruning these is #5552. * The blueprint planner will _not_ carry forward a desired sled state of `Decommissioned` once the inputs report that the sled has already been decommissioned. * The blueprint executor will decommission sleds that the planner said to. * Decommissioning a sled implicitly decommissions all its disks. (This matches what happens with sled expungement, and should not interfere with region replacement, which keys off of policy, not state.)
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 adds the
sled_statemap described in RFD 457 toBlueprint. The motivating driver here is decommissioning expunged sleds, which is on the path to addressing #5625, but this PR only adds the field - the planner carries forward states from its inputs, but otherwise makes no changes, and the executor doesn't read the new field at all.