[nexus] Make it 'more default' for Debug datasets to exist in test env #7982
Conversation
| // - DEFAULT_ZPOOL_COUNT zpools, each of which contains: | ||
| // - A crucible dataset | ||
| // - A debug dataset | ||
| DiskTest::new(&cptestctx).await; |
There was a problem hiding this comment.
As an example of where this is useful:
We can create (and store) support bundles on the simulated system.
| @@ -799,8 +801,8 @@ pub(crate) mod test { | |||
| async fn test_region_replacement_start_saga( | |||
| cptestctx: &ControlPlaneTestContext, | |||
| ) { | |||
| let mut disk_test = DiskTest::new(cptestctx).await; | |||
| disk_test.add_zpool_with_dataset(cptestctx.first_sled_id()).await; | |||
There was a problem hiding this comment.
This isn't strictly necessary, but IMO it's a little clearer -- we're calling "add_zpool_with_dataset" to make a fourth zpool, on top of the three which already exist.
| @@ -209,14 +210,16 @@ mod test { | |||
| datastore.clone(), | |||
| ); | |||
|
|
|||
| // Record which datasets map to which zpools for later | |||
| // Record which crucible datasets map to which zpools for later | |||
|
|
|||
| let mut dataset_to_zpool: BTreeMap<ZpoolUuid, DatasetUuid> = | |||
| BTreeMap::default(); | |||
|
|
|||
| for zpool in disk_test.zpools() { | |||
| for dataset in &zpool.datasets { | |||
There was a problem hiding this comment.
Take it or leave it: it might be useful to make zpool.datasets non-pub and have a method that iterates just the Crucible datasets. That would ensure that we've found all possible uses of datasets that needed to be updated in this PR and also makes it easier in the future (e.g., if we add some other kind of dataset, we won't need to go change all these callers again).
There was a problem hiding this comment.
Done in 36d2d04.
I built this originally as an iterator, before realizing: Actually, tests kinda are assuming that we'll have at most one dataset kind per zpool.
So I'm exposing a helper function that provides that lookup of "find a dataset of a specific kind", validating that there is exactly one. If that assumption breaks, tests will break pretty clearly, and we can keep updating the TestZpool implementation.
| // Also call a similar method to invoke the "omicron_physical_disks_ensure" API. Right now, | ||
| // we aren't calling this at all for the simulated sled agent, which only works because | ||
| // the simulated sled agent simply treats this as a stored config, rather than processing it | ||
| // to actually provide a different view of storage. |
There was a problem hiding this comment.
Is the 2nd part of the old comment no longer applicable because of the update on line 1325 above?
There was a problem hiding this comment.
No, I think this comment is still accurate -- dataset config is different from physical disk config.
This comment is now saying "we should do something similar to this for disks", even though the simulated sled agent is basically ignoring the provided disks config, for now.
Before this PR:
DiskTest::newdid not create any debug datasetsomicron-dev run-alldid not initialize any disksdisk_test.propagate_datasets_to_sleds().After this PR:
DiskTest::newdoes creates debug datasets on all zpoolsomicron-dev run-allinitializes disks on the "first sled agent", with datasetsDiskTest::newwill get them by default