Add two features for local storage support #9386

jmpesp · 2025-11-11T20:22:09Z

Bump the sled-agent API to add two new features for local storage support:

Add an endpoint for Nexus to create and destroy local storage datasets. These will be allocated and deallocated as part of the higher level Disk lifecycle for the forthcoming local storage disk type.
Add the ability to delegate a specific zvol to a Propolis zone. This required accepting a new DelegatedZvol parameter during vmm registration.

Bump the sled-agent API to add two new features for local storage support: - Add an endpoint for Nexus to create and destroy local storage datasets. These will be allocated and deallocated as part of the higher level Disk lifecycle for the forthcoming local storage disk type. - Add the ability to delegate a specific zvol to a Propolis zone. This required accepting a new `DelegatedZvol` parameter during vmm registration.

hawkw · 2025-11-11T22:36:58Z

sled-agent/src/instance.rs

+        let mut devices = vec![
+            zone::Device { name: "/dev/vmm/*".to_string() },
+            zone::Device { name: "/dev/vmmctl".to_string() },
+            zone::Device { name: "/dev/viona".to_string() },
+        ];
+
+        for delegated_zvol in &self.delegated_zvols {


nit, take it or leave it: this probably doesn't actually matter, but seems like a nice place for a

let mut devices = Vec::with_capacity(self.delegated_zvols.len() + 3); // ... push the devices

to preallocate the necessary size

hawkw · 2025-11-11T22:37:46Z

sled-agent/src/sim/sled_agent.rs

+                return Err(Error::internal_error(&format!(
+                    "request block_size {} does not match FileStorageBackend \
+                    block_size {block_size}",
+                    request.block_size,
+                )));


perhaps this error ought to include the ID of the invalid backend?

hawkw · 2025-11-11T22:38:13Z

sled-agent/src/sim/storage.rs

-            .expect("Failed to get the dataset we just inserted");
+            .expect("Failed to get the dataset we just inserted")
+        else {
+            panic!("just inserted this variant!");


obnoxious turbo nit: feels like a place for

Suggested change

panic!("just inserted this variant!");

unreachable!("just inserted this variant!");

jgallagher · 2025-11-13T22:28:36Z

common/src/api/internal/shared.rs

+    /// The fully qualified name of the parent dataset
+    pub parent_dataset: String,
+
+    /// The volume name
+    pub name: String,


Do we have anything with a stronger type than String we could use for either of these fields? I know a bunch of the dataset methods in sled-agent and illumos-utils work directly on strings, but I think that's been a source of confusion at times.

If not, would it make sense to add some newtypes and start using them? I don't know if it's worth doing any kind of minimal validation (e.g., can a volume name be the empty string? can it contain arbitrary utf8?), but that'd be a place to hang that.

I didn't find a "zfs dataset name" struct that was permissive enough - there's DatasetName in common/src/disk.rs but didn't seem generic enough to support the children of LocalStorage. We talked on a meet and I'm going to go with splitting this up to separate parent_dataset (aka the LocalStorage dataset), and a child dataset (that is the parent of the zvol itself).

For moving away from straight Strings, I did manage to find https://github.com/oxidecomputer/illumos-gate/blob/ebc7b84196d53327d76f6194783e79c0afcc86f2/usr/src/lib/libzfs/common/libzfs_dataset.c#L102-L110 which validates names for zfs datasets. We probably won't be able to use that function directly but we could copy the rules there to start that newtype you're talking about.

jgallagher · 2025-11-13T22:30:12Z

illumos-utils/src/zfs.rs

+            Ok(_) => Ok(()),
+
+            Err(crate::ExecutionError::CommandFailure(info))
+                if info.stderr.contains("dataset already exists") =>


Do we need to check it has the expected size and block size?

jgallagher · 2025-11-13T22:32:33Z

illumos-utils/src/zfs.rs

+    pub async fn ensure_dataset_volume(
+        name: String,
+        size: ByteCount,
+        block_size: u32,


Same question about newtypes - is any u32 valid for a block size, or should this be a BlockSize(u32) (or even a BlockSize::* enum if there are only a few choices that we allow)?

jgallagher · 2025-11-13T22:35:51Z

sled-agent/src/sim/sled_agent.rs

+            // The FileStorageBackend path will be the full device path, so
+            // strip the beginning, including the first part of the external
+            // pool name.
+            let dataset = path.strip_prefix("/dev/zvol/rdsk/oxp_").unwrap();


Could this string be a constant somewhere?

jgallagher · 2025-11-13T22:40:51Z

sled-agent/src/sim/storage.rs

            .datasets
            .get(&id)
-            .expect("Failed to get the dataset we just inserted");
+            .expect("Failed to get the dataset we just inserted")


Untested, but I think we could get rid of this expect by replacing the insert() / get() with entry() / insert_entry():

let DatasetContents::Crucible(crucible) = self .datasets .entry(id) .insert_entry(DatasetContents::Crucible(/* ... blah blah ...*/)) .into_mut() else { unreachable!("..."); };

(but +1 on @hawkw's suggestion for unreachable!() over panic!())

jgallagher · 2025-11-13T22:43:02Z

sled-agent/src/sim/storage.rs

        match self.get_dataset(zpool_id, dataset_id) {
            DatasetContents::Crucible(crucible) => crucible.data.clone(),
+            DatasetContents::LocalStorage(_) => {
+                panic!("asked for Crucible, got LocalStorage!")


Maybe this is fine since it's the simulator, but should get_crucible_dataset() return an Option<Arc<CrucibleData>> and let the caller unwrap should they so choose? (Same question about get_local_storage_dataset() below)

jgallagher · 2025-11-13T22:48:38Z

sled-agent/src/sled_agent.rs

+        dataset_id: DatasetUuid,
+        request: sled_agent_api::LocalStorageDatasetEnsureRequest,
+    ) -> Result<(), HttpError> {
+        let zpool_name = ZpoolName::External(zpool_id);


Should this confirm that zpool_id is a disk with a local storage dataset our config says is okay to access before attempting to call Zfs::ensure_dataset()? The config reconciler exposes a available_datasets_rx() watch channel that several sled-agent subsystems consume; we could give AvailableDatasetReceiver a has_local_storage_dataset(&self, zpool_id) method or something like that?

This would catch two potential problems:

Disks our config has said we can't use (it would be surprising to get a request from Nexus for such a disk, but it's possible if we're racing, perhaps?)

Disks our config says we should use, but we haven't been able to ensure the local storage dataset exists (could be a disk problem or a zfs error or ...)

jgallagher · 2025-11-13T22:50:18Z

sled-agent/src/sled_agent.rs

+    ) -> Result<(), HttpError> {
+        let zpool_name = ZpoolName::External(zpool_id);
+
+        let name = format!("{zpool_name}/crypt/local_storage/{dataset_id}");


Similar to the above, I think I'd try to attach this to AvailableDatasetsReceiver somehow to keep the "build up dataset strings" more confined than it is here in an HTTP handler. Might be able to squish this into the has_local_storage_dataset() method I suggested above; local_storage_dataset_name(&self, zpool_id, dataset_id) -> Result<_, _> or something like that maybe?

jmpesp requested a review from jgallagher November 11, 2025 20:22

hawkw reviewed Nov 11, 2025

View reviewed changes

jgallagher reviewed Nov 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add two features for local storage support #9386

Add two features for local storage support #9386

jmpesp commented Nov 11, 2025

Uh oh!

hawkw Nov 11, 2025

Uh oh!

hawkw Nov 11, 2025

Uh oh!

hawkw Nov 11, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jmpesp Nov 14, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

jgallagher Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	panic!("just inserted this variant!");
	unreachable!("just inserted this variant!");

Add two features for local storage support #9386

Are you sure you want to change the base?

Add two features for local storage support #9386

Conversation

jmpesp commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants