Skip to content

Conversation

jgallagher
Copy link
Contributor

This PR makes two changes; the first is small and does affect prod (but should be very obviously correct), and the second is large but only affects tests:

  1. Replace LocalStorage::dyn_datasets_config_list() -> Result<DatasetsConfig, _> with LocalStorage::dyn_dataset_config()-> Result<DatasetConfig>. The former returned an entire DatasetsConfig, which no longer exists inside OmicronSledConfig, so the prod implementation of the method had to create one. But the sole caller really only wanted that to look up a single dataset, so replace the method with one that looks up a single dataset.
  2. Convert the tests from using a LocalStorage backed by StorageManagerTestHarness to being backed by sim::Storage. This has a couple pros and one con: pros are that we remove a reference to StorageManager (which is no longer used in prod, but this isn't obvious - ideally we could get rid of all the test consumers and then remove it to avoid confusing) and that the tests can now run on non-illumos systems; con is that we're no longer exercising true ZFS operations in these tests. But I think the point of the LocalStorage trait is that we don't need to exercise ZFS itself, so this con seems pretty mild to me?

Using sim::Storage required adding some tracking of a (fake) mounted property to the "datasets" it keeps track of in memory. I think this is okay? But the nested dataset / dataset split in the APIs is still confusing to me so I could easily have screwed something up in here.

Comment on lines 1232 to 1233
// in-memory property for whether this dataset is mounted; typically `true`,
// but can be explicitly set to false for some tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is a nitpick, but can you humor me?

Suggested change
// in-memory property for whether this dataset is mounted; typically `true`,
// but can be explicitly set to false for some tests
// in-memory property for whether this dataset pretends to be mounted; typically `true`,
// but can be explicitly set to false for some tests

Just to clarify: This boolean only changes how this gets reported; it is not used in any real logic beyond "it's a sticky bool of whatever last value you set".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify: This boolean only changes how this gets reported; it is not used in any real logic beyond "it's a sticky bool of whatever last value you set".

Totally correct, and I wonder if we basically make this the comment instead? Something like

// In-memory flag for whether this dataset pretends to be mounted; defaults
// to true.
//
// Nothing in the simulated storage implementation acts on this value; it is merely
// a sticky bool that remembers the most recent value passed to
// `nested_dataset_set_mounted()`.

@jgallagher jgallagher merged commit 0830569 into main Oct 17, 2025
16 checks passed
@jgallagher jgallagher deleted the john/pruning-storage-manager branch October 17, 2025 14:19
jgallagher added a commit that referenced this pull request Oct 17, 2025
After #9224 (on which this PR is based), there were two remaining users
of `StorageManagerTestHarness`, both of which rely on having real ZFS
pools / datasets available:

* The `zone_bundle` tests exercise production code that wants to
interact with ZFS properties (quota in particular; maybe others)
* The `sled-diagnostics::logs` tests use ZFS snapshots

However, `StorageManager` and the code around it is not in a great spot:
it appears to be real, production code, but it's no longer actually used
by sled-agent - all of its functionality has been absorbed into the
config reconciler.

This PR extracts a `ZfsTestHarness` - this is essentially a subset of
`StorageManagerTestHarness`, and copy/pastes its creation of vdevs for
real ZFS tests, but has a much simpler API: there were several methods
of `StorageManagerTestHarness` that had no callers, and it handles disks
/ datasets / zpools itself (in a very simple way) instead of handing
that off to `StorageManager`. Both of the test modules above are updated
to use `ZfsTestHarness`, which I _think_ clears the way to removing
`StorageManager` and `StorageManagerTestHarness`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants