-
Notifications
You must be signed in to change notification settings - Fork 67
[reconfigurator] Add "pruneable zones" to PlanningInput
#9730
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
| // We have no way to confirm that this zone is "unreferenced" - that's a | ||
| // property of the system at large, mostly CRDB state - but we can | ||
| // confirm that it's expunged and ready for cleanup by looking at the | ||
| // parent blueprint. |
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 was originally going to add a couple blippy lints that any zone in expunged_and_unreferenced is (a) present and (b) actually expunged, but with the guard in this method and the fact that PlanningInput's fields are private, I don't believe it's actually possible to construct a PlanningInput with any such zone IDs in expunged_and_unreferenced, so it was impossible to write a test to confirm the blippy lints triggered.
davepacheco
left a comment
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.
Nice! This is really clean. I've commented on one possible bug here and the rest is nitty feedback.
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
| clickhouse_config.keepers.contains_key(&zone_id) | ||
| || clickhouse_config.servers.contains_key(&zone_id) |
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 don't know enough about this to evaluate this case -- may want to ask @karencfv or @andrewjstone to double-check.
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 think it should be fine? I'd like to get @andrewjstone's thoughts on this though just to be sure
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.
This looks good to me. I just read some of the code to remind me what is going on, and if there is no zone there, it means it must have been expunged. Since that's mainly what this PR is about, the same zone with random uuid is not coming back magically.
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
nexus/reconfigurator/preparation/src/expunged_and_unreferenced.rs
Outdated
Show resolved
Hide resolved
|
Bunch of changes here (enough it's probably easier to rereview than look at the diff?):
|
PlanningInputPlanningInput
This builds on the
BlueprintExpungedZoneAccessReasonadded in #9608, and adds a new set of zone IDs toPlanningInputfor "pruneable zones". These are zones that the planner is safe to prune in a future planning iteration, because they are:This PR only adds the new set of zones to
PlanningInput. It will be nearly trivial to add a step to the planner to "prune all zones present in the planning input'spruneable_zonesset", but out of an abundance of caution I'd prefer to land this first, collect a reconfigurator state from some deployed racks, and confirm the contents match what we'd expect on some real systems.This is a little weird in that the logic for whether a zone can be pruned lives in the "prepare a planning input" crate rather than the planner itself, but I think this is reasonable given the implementation of several of the checks (e.g., "query CRDB for this expunged zone specifically" to see whether it's still referenced, which the planner can't do since it doesn't have access to CRDB). Happy to discuss alternatives if this seems wrong upon review.
(We may also want to wait on #7278 before adding the actual prune step?)