-
Notifications
You must be signed in to change notification settings - Fork 67
Clean up a variety of tests that create blueprints #9393
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
Conversation
| // If used for RSS tests (most of the time!), the blueprint built by this | ||
| // builder will need to have its `parent_blueprint_id` manually set to | ||
| // `None` to erase the link to the empty parent. | ||
| fn blueprint_builder_with_empty_parent( |
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 is still kinda janky, and was earlier in the stack of work that produced this PR. I think I could go through these tests again and replace most or all uses of this with more ExampleSystemBuilder usage, if folks think that'd be better.
…omoting internal NTP
e7417cd to
263cd1f
Compare
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! I only skimmed the changes to rack.rs. I love the removal of explicit construction of BlueprintZoneConfig and Blueprint.
| to: blueprint 86db3308-f817-4626-8838-4085949a6a41 | ||
|
|
||
| MODIFIED SLEDS: | ||
| ADDED SLEDS: |
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 isn't that worrisome but I'm not clear on why this happened. This test doesn't appear to explicitly add any sleds.
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.
Above what github shows, these are the current blueprints in the simulated system:
> blueprint-list
T ENA ID PARENT TIME_CREATED
02697f74-b14a-4418-90f0-c28b2a3a6aa9 <none> <REDACTED_TIMESTAMP>
* yes ade5749d-bdf3-4fab-a8ae-00bea01b3a5a 02697f74-b14a-4418-90f0-c28b2a3a6aa9 <REDACTED_TIMESTAMP>
86db3308-f817-4626-8838-4085949a6a41 ade5749d-bdf3-4fab-a8ae-00bea01b3a5a <REDACTED_TIMESTAMP>
The diff here is between 02697f74-b14a-4418-90f0-c28b2a3a6aa9 and 86db3308-f817-4626-8838-4085949a6a41 (although diffing from 02697f74-b14a-4418-90f0-c28b2a3a6aa9 to ade5749d-bdf3-4fab-a8ae-00bea01b3a5a would show the same behavior). Prior to this change, the initial blueprint 02697f74-b14a-4418-90f0-c28b2a3a6aa9 with was built via build_empty_with_sleds(...), so it had all the sleds present (but they themselves were all empty). On this PR, it's now built via build_empty(), so it has no sleds at all, hence the diff shows the sleds being ADDED instead of MODIFIED. The details within the sled didn't change, because in both cases we're adding all the disks, datasets, and zones.
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.
Oh, I didn't actually answer your question. The test does this:
> load-example --seed test-basic --nsleds 1 --ndisks-per-sled 4
which still creates two blueprints:
> blueprint-list
T ENA ID PARENT TIME_CREATED
02697f74-b14a-4418-90f0-c28b2a3a6aa9 <none> <REDACTED_TIMESTAMP>
* yes ade5749d-bdf3-4fab-a8ae-00bea01b3a5a 02697f74-b14a-4418-90f0-c28b2a3a6aa9 <REDACTED_TIMESTAMP>
We used to add the sleds in the first and then flesh out their details in the second; now the first is truly empty, and we add them with their details already fleshed out in the second.
I'd like to add a couple new fields to
BlueprintSledConfig, and doing so broke a variety of tests in annoying ways. Many of those tests have required annoying changes in the past, too. This PR doesn't add any new fields, but instead attempts to clean up those tests so changes to Reconfigurator structures are less likely to cause problems (and hopefully any problems will be easier to fix). A couple techniques:BlueprintBuilderorExampleSystemBuilderBlueprintBuilder::build_empty_with_sleds{,_seeded}withBlueprintBuilder::build_empty{,_seeded}. The former added some sleds with nothing on them; the latter adds no sleds at all. I updated all the callers ofbuild_empty_with_sledsto either usebuild_empty(actually fine for most of them) or the example system infrastructure.This does add a few new helper methods on real types; e.g.,
BlueprintBuilder::sled_add_zone_boundary_ntp()(for directly adding a boundary NTP instead of promoting an existing internal NTP zone). But currently only tests call these new methods, and I don't think they're prone to misuse; e.g.,sled_add_zone_boundary_ntp()checks and fails if called on a sled that already has an in-service NTP zone of either flavor.(This is staged on top of #9392 to pick up that fix for
main.)