-
Notifications
You must be signed in to change notification settings - Fork 66
Blueprint planner/builder: require reason when accessing expunged zones #9608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Blippy has had checks on sled subnet consistency for a while, but they were implemented by inferring each sled's subnet from its zones' IPs. We added explicit sled subnets to blueprints in #9416; this changes blippy's sled-subnet-related checks to use that new field. (This is work that fell out of a larger PR that isn't ready yet, and I'm opening it separately to reduce diff clutter in that PR.)
| Either::Left(editor.in_service_zones()) | ||
| } | ||
| InnerSledEditor::Decommissioned(_) => { | ||
| // A decommissioned sled cannot have any in-service zones! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly suspect we could pretty significantly simplify SledEditor here by getting rid of this internal decommissioned state entirely, with some minor changes to the builder. I put this here when I first started doing the builder cleanup and didn't have a great handle on commissioned vs decommissioned sleds; I'll take a crack at this simplification soon.
| for (zone, nexus) in self | ||
| .blueprint | ||
| .current_zones(BlueprintZoneDisposition::any) | ||
| .current_in_service_zones() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this a change which no longer considers expunged zones, but that seems fine
This is a followup to #9521, specifically driven by @smklein's note that the planner also accesses expunged zones, and we need to track those reasons too. Doing this work revealed one case where the planner was accessing them in a way that was going to cause problems with pruning; that's fixed by #9550, and this PR is staged on top of it.