Skip to content

Conversation

@jgallagher
Copy link
Contributor

This builds on the BlueprintExpungedZoneAccessReason added in #9608, and adds a new set of zone IDs to PlanningInput for "expunged and unreferenced" zones. These are zones that the planner is safe to prune in a future planning iteration, because they are:

  1. Expunged and ready for cleanup
  2. No longer referenced in CRDB, implying any necessary cleanup is done

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's expunged_and_unreferenced set", 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?)

// 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.
Copy link
Contributor Author

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.

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.

2 participants