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

sunshowers · 2024-03-15T01:07:52Z

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.

TODO:

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt

jgallagher

Looks great to me!

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt

clients/sled-agent-client/src/lib.rs

nexus/types/src/deployment.rs

jgallagher · 2024-03-15T14:35:43Z

nexus/types/src/deployment.rs

+            //    parameters inside changed. We need to add additional text to
+            //    make that clearer.
+            //
+            // TODO: also print out the exact bits of configuration that


Do you want to do this on this PR, in a followup, or file an issue for someone to pick up later? (I have no preference between these - the old format also didn't show the details here, right?)

Definitely in a follow up, will file an issue for this. A question that immediately occurs to me is: why are diffs special, why not display all the configs all the time within blueprints? Maybe we should have an extended mode in the displayer to make that possible.

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

davepacheco

Thanks for doing this. I was ambivalent but I think I've come around.

I really like that we're testing the blueprint diff output.

Some parts definitely look nicer.

My complaints are all nitty preference on the output format (but then again, the point of the PR is to change the output format for clarity so hopefully this isn't too out of place). My first reaction was: this looks a lot less like diff(1) output and I find it quite a lot harder to skim and scan it. I had tried to write up why but then took another look at the raw files instead of the GitHub diff and I think it's not so bad.

I do prefer left-aligned (or even right-aligned) columns to center-aligned. I think this is probably just convention burned into my brain more than anything -- I don't think I've seen many terminal tools that center-align columns and I'm not used to searching for tokens (like nexus) in the middle of a column.

clients/sled-agent-client/src/lib.rs

nexus/reconfigurator/planning/tests/output/planner_basic_add_sled_2_3.txt

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_bp2.txt

nexus/types/src/deployment.rs

sunshowers · 2024-03-21T01:01:20Z

My complaints are all nitty preference on the output format (but then again, the point of the PR is to change the output format for clarity so hopefully this isn't too out of place).

Absolutely! I really want to make things better for all of us, and feedback is really welcome. I've already incorporated some of your feedback. (And yes -- I do think it looks better in the terminal when I'm actually developing against it, the visual separation makes things really easy to parse.) And as always, we can keep iterating on this. Thanks for being so receptive :)

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

…on enum next to each zone (#5238) 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.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers · 2024-03-27T07:59:17Z

This ended up being expanded quite substantially after all the feedback was incorporated, but I'm pretty happy overall with the way it generally looks.

I'll go over with some final ideas tomorrow morning, then put it up for another round of review.

I do prefer left-aligned (or even right-aligned) columns to center-aligned. I think this is probably just convention burned into my brain more than anything -- I don't think I've seen many terminal tools that center-align columns and I'm not used to searching for tokens (like nexus) in the middle of a column.

Ended up agreeing with this in the end -- everything is now left-aligned. This led to:

I noticed that with left-aligned text, it looked better if all the zones were together.
So I sorted the output by (zone type, zone id) rather than just zone id.
After discussing this with Dave, we decided that we should move the zone type to the left of the zone ID.

Created using spr 1.3.6-beta.1

sunshowers · 2024-03-27T21:32:09Z

Showed this to @smklein this morning, who had a bunch of really great suggestions. In particular, we figured out a much better way to show modified zones -- really happy with the current iteration.

Created using spr 1.3.6-beta.1

sunshowers · 2024-03-27T21:42:11Z

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt

+     ├─                                                   in service    fd00:1122:3344:105::22             
+*     └─ zone type config changed                                                                           


I skipped over all the columns that must inherently be the same over here.

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

jgallagher

Changes LGTM - just a few questions on some of the output files

nexus/reconfigurator/planning/tests/output/blueprint_builder_initial_diff.txt

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_2_2a.txt

…er use it (#5341) In #5270, we need determinism not just from blueprints but also collections. So move the UuidRng into a common place. As part of that, I also decided to make it its own crate and write some documentation about it, making it more generic along the way. I think this should be a pretty clean representation of what this is trying to do.

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

Created using spr 1.3.6-beta.1 [skip ci]

Created using spr 1.3.6-beta.1

sunshowers · 2024-03-29T01:58:58Z

@jgallagher accepted this over chat (doesn't have access to GH right now), so landing.

sunshowers added 2 commits March 14, 2024 18:07

[𝘀𝗽𝗿] changes to main this commit is based on

a88f90d

Created using spr 1.3.6-beta.1 [skip ci]

[𝘀𝗽𝗿] initial version

b89b6a1

Created using spr 1.3.6-beta.1

sunshowers marked this pull request as draft March 15, 2024 01:08

sunshowers mentioned this pull request Mar 15, 2024

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

Merged

4 tasks

sunshowers added 3 commits March 14, 2024 23:06

[𝘀𝗽𝗿] changes introduced through rebase

35addac

Created using spr 1.3.6-beta.1 [skip ci]

Ready for review

d5f04b8

Created using spr 1.3.6-beta.1

self-review

3552ff2

Created using spr 1.3.6-beta.1

sunshowers commented Mar 15, 2024

View reviewed changes

nexus/reconfigurator/planning/tests/output/planner_nonprovisionable_1_2.txt Outdated Show resolved Hide resolved

sunshowers marked this pull request as ready for review March 15, 2024 06:12

sunshowers requested review from andrewjstone, davepacheco, jgallagher and smklein March 15, 2024 06:12

jgallagher reviewed Mar 15, 2024

View reviewed changes

sunshowers added 3 commits March 15, 2024 17:43

rebase

435616c

Created using spr 1.3.6-beta.1

[𝘀𝗽𝗿] changes introduced through rebase

dc25657

Created using spr 1.3.6-beta.1 [skip ci]

removing border line, makes it look less busy

b75546b

Created using spr 1.3.6-beta.1

davepacheco reviewed Mar 20, 2024

View reviewed changes

sunshowers added 2 commits March 22, 2024 20:17

[𝘀𝗽𝗿] changes introduced through rebase

9d9bb5c

Created using spr 1.3.6-beta.1 [skip ci]

continue work, TODO

ba3df67

Created using spr 1.3.6-beta.1

rcgoodfellow and others added 3 commits March 26, 2024 12:26

[𝘀𝗽𝗿] changes introduced through rebase

f9f2f91

Created using spr 1.3.6-beta.1 [skip ci]

almost done, just a couple things remaining

ad8a0d8

Created using spr 1.3.6-beta.1

fix up clippy and docs

76d4615

Created using spr 1.3.6-beta.1

sunshowers changed the base branch from sunshowers/spr/main.reconfigurator-use-tabled-to-display-blueprints-and-diffs to main March 26, 2024 19:49

sunshowers added 3 commits March 26, 2024 13:08

warning on modified sleds with unchanged generation number

101e256

Created using spr 1.3.6-beta.1

Almost done

c27abf3

Created using spr 1.3.6-beta.1

A couple more changes

f610044

Created using spr 1.3.6-beta.1

sunshowers added 2 commits March 27, 2024 01:07

Add a snapshot test for collection -> blueprint diff

f087991

Created using spr 1.3.6-beta.1

Incorporate Sean's suggestions

8a6e21e

Created using spr 1.3.6-beta.1

simply don't need the ZONE TABLE at the top, it's obvious

ce6bc3a

Created using spr 1.3.6-beta.1

sunshowers commented Mar 27, 2024

View reviewed changes

sunshowers requested review from davepacheco and jgallagher March 27, 2024 21:42

sunshowers added 2 commits March 27, 2024 14:50

Reduce width slightly

7efedbf

Created using spr 1.3.6-beta.1

Fix docs

65d71a7

Created using spr 1.3.6-beta.1

sunshowers mentioned this pull request Mar 28, 2024

[nexus] move UuidRng code out to a common crate, make CollectionBuilder use it #5341

Merged

sunshowers added 2 commits March 28, 2024 00:16

[𝘀𝗽𝗿] changes introduced through rebase

4ea57cb

Created using spr 1.3.6-beta.1 [skip ci]

Rebase on 5341

e4f9289

Created using spr 1.3.6-beta.1

sunshowers changed the base branch from main to sunshowers/spr/main.reconfigurator-use-tabled-to-display-blueprints-and-diffs-1 March 28, 2024 07:16

jgallagher reviewed Mar 28, 2024

View reviewed changes

sunshowers added 4 commits March 28, 2024 15:45

[𝘀𝗽𝗿] changes introduced through rebase

0fc8014

Created using spr 1.3.6-beta.1 [skip ci]

rebase, address review comments, switch to BlueprintMetadata

2150271

Created using spr 1.3.6-beta.1

[𝘀𝗽𝗿] changes introduced through rebase

f1f3207

Created using spr 1.3.6-beta.1 [skip ci]

Rebase, ready for landing

8a813d2

Created using spr 1.3.6-beta.1

sunshowers changed the base branch from sunshowers/spr/main.reconfigurator-use-tabled-to-display-blueprints-and-diffs-1 to main March 28, 2024 22:49

sunshowers requested a review from jgallagher March 29, 2024 01:21

sunshowers merged commit e5094dc into main Mar 29, 2024

sunshowers deleted the sunshowers/spr/reconfigurator-use-tabled-to-display-blueprints-and-diffs branch March 29, 2024 01:59

		+ ├─ in service fd00:1122:3344:105::22
		* └─ zone type config changed

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

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

Uh oh!

Conversation

sunshowers commented Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgallagher Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

sunshowers Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunshowers commented Mar 21, 2024

Uh oh!

sunshowers commented Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunshowers commented Mar 27, 2024

Uh oh!

sunshowers Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunshowers commented Mar 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sunshowers commented Mar 15, 2024 •

edited

Loading

sunshowers Mar 15, 2024 •

edited

Loading

sunshowers commented Mar 27, 2024 •

edited

Loading