Skip to content

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

Merged
sunshowers merged 12 commits into
mainfrom
sunshowers/spr/wip-replace-blueprintzones_in_service-with-a-policy-enum-next-to-each-zone
Mar 24, 2024
Merged

[reconfigurator] replace Blueprint::zones_in_service with a disposition enum next to each zone#5238
sunshowers merged 12 commits into
mainfrom
sunshowers/spr/wip-replace-blueprintzones_in_service-with-a-policy-enum-next-to-each-zone

Conversation

@sunshowers

@sunshowers sunshowers commented Mar 9, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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:

@sunshowers
[𝘀𝗽𝗿] initial version
4e4bd36 
Created using spr 1.3.6-beta.1
@sunshowers
Fix code and tests
1f84a23 
Created using spr 1.3.6-beta.1
@sunshowers
Address dap's comment + some more changes
349ba6c 
Created using spr 1.3.6-beta.1
@sunshowers
Ready for review
30a9152 
Created using spr 1.3.6-beta.1
@sunshowers sunshowers marked this pull request as ready for review March 14, 2024 21:50
@sunshowers
cleanup output
0499853 
Created using spr 1.3.6-beta.1
sunshowers
sunshowers commented Mar 14, 2024
View reviewed changes
Comment thread dev-tools/reconfigurator-cli/tests/output/cmd-complex-stdout Outdated
@sunshowers sunshowers mentioned this pull request Mar 15, 2024
Merged
6 tasks
@sunshowers
Rebase
44524fb 
Created using spr 1.3.6-beta.1
@sunshowers 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
@sunshowers
Rebase
f33bdd5 
Created using spr 1.3.6-beta.1
jgallagher
jgallagher approved these changes Mar 19, 2024
View reviewed changes

@jgallagher jgallagher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread nexus/db-queries/src/db/datastore/vpc.rs Outdated
Comment thread nexus/reconfigurator/execution/src/omicron_zones.rs Outdated
Comment thread nexus/reconfigurator/execution/src/resource_allocation.rs
Comment thread nexus/reconfigurator/planning/src/planner.rs Outdated
Comment thread nexus/reconfigurator/planning/src/planner.rs Outdated
Comment thread nexus/src/app/background/blueprint_execution.rs Outdated
Comment thread nexus/types/src/deployment.rs
@sunshowers
Ready to land
8448c5d 
Created using spr 1.3.6-beta.1
@sunshowers sunshowers enabled auto-merge (squash) March 21, 2024 00:10
@sunshowers
small comment fixes
e4fcde6 
Created using spr 1.3.6-beta.1
@sunshowers sunshowers disabled auto-merge March 21, 2024 19:32
@sunshowers
Discussed changes, may need another review
26b1a38 
Created using spr 1.3.6-beta.1
@sunshowers 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 sunshowers requested a review from jgallagher March 22, 2024 04:55
@sunshowers

sunshowers commented Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

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.

jgallagher
jgallagher approved these changes Mar 22, 2024
View reviewed changes
Comment thread nexus/reconfigurator/execution/src/omicron_zones.rs Outdated
Comment thread nexus/reconfigurator/execution/src/omicron_zones.rs Outdated
Comment thread nexus/reconfigurator/planning/src/blueprint_builder.rs
Comment thread nexus/src/app/background/blueprint_execution.rs Outdated
Comment thread nexus/types/src/deployment.rs Outdated
Comment thread nexus/types/src/deployment.rs
@jgallagher

jgallagher commented Mar 22, 2024

Copy link
Copy Markdown
Contributor

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

sunshowers commented Mar 23, 2024

Copy link
Copy Markdown
Contributor Author

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.

@sunshowers sunshowers mentioned this pull request Mar 23, 2024
Closed
@sunshowers
Camera-ready
a09a012 
Created using spr 1.3.6-beta.1
@sunshowers sunshowers enabled auto-merge (squash) March 23, 2024 02:11
@sunshowers
does this work?
e617e5f 
Created using spr 1.3.6-beta.1
@sunshowers sunshowers merged commit 2349feb into main Mar 24, 2024
@sunshowers sunshowers deleted the sunshowers/spr/wip-replace-blueprintzones_in_service-with-a-policy-enum-next-to-each-zone branch March 24, 2024 01:59
sunshowers added a commit that referenced this pull request Mar 29, 2024
@sunshowers
[reconfigurator] use tabled to display blueprints and diffs (#5270)
e5094dc 
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.
@jgallagher jgallagher mentioned this pull request Apr 10, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jgallagher jgallagher jgallagher approved these changes
@davepacheco davepacheco Awaiting requested review from davepacheco
@andrewjstone andrewjstone Awaiting requested review from andrewjstone

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sunshowers @jgallagher