[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

sunshowers · 2024-03-09T05:56:56Z

This greatly simplifies the handling of the expunged disposition in #5211. It also is more understandable, and less prone to making mistakes.

This PR only changes the in-memory representation of what is currently zones_in_service. I've skipped doing the DB migration in this PR because it'll be easier to just do it wholesale in #5211.

There are some small differences in diff output that I think make sense. But I took a bigger swing at it in #5270, which depends on this.

TODO:

Fix code.
Turn Blueprint's Debug impl into a display_detailed() method (done in [reconfigurator] improvements to blueprint displays and tests #5248)
Add snapshot tests for diff output (done in [reconfigurator] improvements to blueprint displays and tests #5248)
Fix tests.

Created using spr 1.3.6-beta.1

dev-tools/reconfigurator-cli/tests/output/cmd-complex-stdout

Created using spr 1.3.6-beta.1

jgallagher

Thanks, this is much cleaner. And my apologies that most of my comments apply to the existing code this PR touches 😬.

nexus/db-queries/src/db/datastore/vpc.rs

nexus/reconfigurator/execution/src/omicron_zones.rs

nexus/reconfigurator/execution/src/resource_allocation.rs

nexus/reconfigurator/planning/src/planner.rs

nexus/src/app/background/blueprint_execution.rs

nexus/types/src/deployment.rs

Created using spr 1.3.6-beta.1

sunshowers · 2024-03-22T05:03:12Z

I've put in the changes we discussed today in this PR:

Renamed BlueprintZonePolicy to BlueprintZoneDisposition.
Added a BlueprintZoneFilter enum which can be used to query sets of dispositions in various ways.

I also have a question: Should all_omicron_zones also take a BlueprintZoneFilter as an argument? I'd do that in a subsequent PR since the blast radius of that would be bigger.

I think this will benefit from another round of review since the changes are substantial. GitHub code review change tracking is completely useless, so here's an interdiff generated by Jujutsu of the changes since last review.

nexus/reconfigurator/execution/src/omicron_zones.rs

nexus/reconfigurator/planning/src/blueprint_builder.rs

nexus/src/app/background/blueprint_execution.rs

nexus/types/src/deployment.rs

jgallagher · 2024-03-22T14:26:50Z

I also have a question: Should all_omicron_zones also take a BlueprintZoneFilter as an argument? I'd do that in a subsequent PR since the blast radius of that would be bigger.

After reading the changes, I think I would very strongly vote "yes", and at that point we could probably drop all_blueprint_zones altogether? It looks like all the current callers of that are only using it because all_omicron_zones doesn't take a filter.

sunshowers · 2024-03-23T00:31:42Z

at that point we could probably drop all_blueprint_zones altogether?

Depends, I think some of the users want the full BlueprintZoneConfig. But it can definitely be de-emphasized.

Created using spr 1.3.6-beta.1

While developing #5238, I noticed that the output was getting significantly busier and less aligned. I decided to prototype out using `tabled` to display outputs, and I really liked the results. Examples that cover all of the cases are included in the PR. In the future I'd also like to add color support on the CLI, and expand it to inventory and `omdb` (it's similar except it doesn't have the zone policy table). Some other changes that are bundled into this PR: * Sort by (zone type, zone ID) rather than zone ID, to keep zones of the same type grouped together. * Moved unchanged data to the top to allow users to see less scrollback. * Moved metadata to the bottom for the same reason. * Add information about the zone config being changed. * Change `Blueprint::diff_sleds` and `Blueprint::diff_sleds_from_collection` to `Blueprint::diff_since_blueprint` and `diff_since_collection` recently. * Reordered `diff_since_blueprint`'s arguments so that `self` is after and the argument is before, to align with `diff_since_collection`. (I found that surprising!) * Renamed the diff type from `OmicronZonesDiff` to `BlueprintDiff`, since it's going to contain a lot more than zones. * Return an error from the diff methods, specifically if the before and after have the same zone ID but different types. Depends on #5238 and #5341.

sunshowers added 4 commits March 8, 2024 21:56

[𝘀𝗽𝗿] initial version

4e4bd36

Created using spr 1.3.6-beta.1

Fix code and tests

1f84a23

Created using spr 1.3.6-beta.1

Address dap's comment + some more changes

349ba6c

Created using spr 1.3.6-beta.1

Ready for review

30a9152

Created using spr 1.3.6-beta.1

sunshowers marked this pull request as ready for review March 14, 2024 21:50

cleanup output

0499853

Created using spr 1.3.6-beta.1

sunshowers commented Mar 14, 2024

View reviewed changes

dev-tools/reconfigurator-cli/tests/output/cmd-complex-stdout Outdated Show resolved Hide resolved

sunshowers requested review from jgallagher, davepacheco and andrewjstone March 14, 2024 22:51

sunshowers mentioned this pull request Mar 15, 2024

[reconfigurator] use tabled to display blueprints and diffs #5270

Merged

6 tasks

Rebase

44524fb

Created using spr 1.3.6-beta.1

sunshowers changed the title ~~[WIP] replace Blueprint::zones_in_service with a policy enum next to each zone~~ [reconfigurator] replace Blueprint::zones_in_service with a policy enum next to each zone Mar 15, 2024

Rebase

f33bdd5

Created using spr 1.3.6-beta.1

jgallagher approved these changes Mar 19, 2024

View reviewed changes

Ready to land

8448c5d

Created using spr 1.3.6-beta.1

sunshowers enabled auto-merge (squash) March 21, 2024 00:10

small comment fixes

e4fcde6

Created using spr 1.3.6-beta.1

sunshowers disabled auto-merge March 21, 2024 19:32

Discussed changes, may need another review

26b1a38

Created using spr 1.3.6-beta.1

sunshowers changed the title ~~[reconfigurator] replace Blueprint::zones_in_service with a policy enum next to each zone~~ [reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone Mar 22, 2024

sunshowers requested a review from jgallagher March 22, 2024 04:55

jgallagher approved these changes Mar 22, 2024

View reviewed changes

sunshowers mentioned this pull request Mar 23, 2024

Make Blueprint::all_omicron_zones take BlueprintZoneFilter as an argument #5315

Closed

Camera-ready

a09a012

Created using spr 1.3.6-beta.1

sunshowers enabled auto-merge (squash) March 23, 2024 02:11

does this work?

e617e5f

Created using spr 1.3.6-beta.1

sunshowers merged commit 2349feb into main Mar 24, 2024
23 checks passed

sunshowers deleted the sunshowers/spr/wip-replace-blueprintzones_in_service-with-a-policy-enum-next-to-each-zone branch March 24, 2024 01:59

jgallagher mentioned this pull request Apr 10, 2024

Add BlueprintZoneFilter to all_omicron_zones #5348

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

sunshowers commented Mar 9, 2024 •

edited

Loading

jgallagher left a comment

sunshowers commented Mar 22, 2024

jgallagher commented Mar 22, 2024

sunshowers commented Mar 23, 2024

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone #5238

Conversation

sunshowers commented Mar 9, 2024 • edited Loading

jgallagher left a comment

Choose a reason for hiding this comment

sunshowers commented Mar 22, 2024

jgallagher commented Mar 22, 2024

sunshowers commented Mar 23, 2024

sunshowers commented Mar 9, 2024 •

edited

Loading