Use an unencrypted dataset for local storage #9684

jmpesp · 2026-01-20T18:03:40Z

Using a dataset that inherits encryption for raw zvols isn't possible, so create a new unencrypted parent dataset for local storage, and switch over to using that.

Note that there isn't a migration plan for existing disks backed by local storage. The sled-agent API simply switches over to using the unencrypted dataset instead, and Nexus' allocation routines similarly switch over. All existing disks backed by local storage should be deleted before this PR lands!

Using a dataset that inherits encryption for raw zvols isn't possible, so create a new unencrypted parent dataset for local storage, and switch over to using that. Note that there isn't a migration plan for existing disks backed by local storage. The sled-agent API simply switches over to using the unencrypted dataset instead, and Nexus' allocation routines similarly switch over. All existing disks backed by local storage should be deleted before this PR lands!

jgallagher · 2026-01-22T16:02:13Z

common/src/api/internal/shared/mod.rs

-    /// Delegate a slice of the local storage dataset present on this pool into
-    /// the zone.
+    /// Delegate a slice of the unencrypted local storage dataset present on
+    /// this pool into the zone.


Is it okay to change the meaning of this variant as opposed to adding a LocalStorageUnencrypted variant to match the two dataset kinds? I'm wondering if this will be a pain when we get back to DatasetKind::LocalStorage (encrypted) support.

Putting both encrypted and unencrypted variants in here required an API rev because DelegatedZvol is passed through InstanceSledLocalConfig in the InstanceEnsureBody, and this turned into more support for both encrypted and unencrypted local storage datasets, in particular deleting now works for both. I have the changes staged in my branch but would really like oxidecomputer/dropshot#1520 to land (and be pulled in here!) so that the resulting code isn't too duplicative.

done in ad79d19

jgallagher · 2026-01-22T16:09:56Z

nexus/db-queries/src/db/datastore/disk.rs

+            (Some(_), Some(_)) => {
+                return Err(Error::internal_error(&format!(
+                    "LocalStorageDisk {} has multiple allocations!",
+                    self.id(),
+                )));
+            }


Can we encode this requirement in the type system? E.g., instead of two Option fields, a single field with a enum with variants spelling out the valid options?

done in dc51a95 - I should have done this in the first place, the code is much nicer.

jgallagher · 2026-01-22T16:11:11Z

nexus/db-queries/src/db/datastore/disk.rs

+                    format!("{zpool_name}/crypt/local_storage"),
+                    format!("{dataset_id}/vol"),
+                ]
+                .join("/");


I realize this was already here but I guess I missed in the last PR. It seems very fishy for Nexus to be building up paths to devices on sleds - can this level of detail be left to sled-agent, and Nexus deal with higher level things (like just the zpool / dataset IDs)?

This function is currently only consumed by Nexus when building the instance spec to send to sled-agent. I'm mildly uncomfortable with the idea that sled-agent has to reach into the prepared spec to edit something, though we could introduce an abstraction in the form of a Nexus-level instance spec that gets converted by the sled-agent into a proper Propolis instance spec, and that would allow Nexus to deal only in the higher level UUIDs.

Fair enough; if this is gross/painful either way and it's better to leave it as-is, that's fine. I've been trying to be a little more critical of how we treat paths/strings in sled-agent, and was quite surprised to see this in Nexus. Might be worth a discussion after R18 is out the door?

jgallagher · 2026-01-22T16:12:59Z

nexus/db-queries/src/db/datastore/disk.rs

+            local_storage_dataset_allocation,
+            local_storage_unencrypted_dataset_allocation,


It looks like this function can return Ok(_) with both of these fields set to Some(_) or None(_), which will end up causing errors above. I feel more strongly this should be a single field, now, but if for some reason it can't, should this function check that exactly one of these fields is Some(_)?

right, also done in dc51a95

jgallagher · 2026-01-22T16:18:37Z

nexus/db-queries/src/db/datastore/local_storage.rs

+
+    /// List one page of unencrypted local storage datasets
+    ///
+    /// This fetches all datasets, including those that have been tombstoned.


Including tombstoned datasets seems unusual for a _list function - should we make the name scarier sounding so it's more obvious to callers that they may need to consider each item's status?

This was basically all copied from the existing rendezvous code, which similarly returns tombstoned datasets - hopefully callers would be aware of this convention already! :)

If not then yeah we'll need to establish a naming convention for "also returns deleted and/or tombstoned stuff".

jgallagher · 2026-01-22T16:23:02Z

nexus/db-queries/src/db/datastore/sled.rs

+        {
+            info!(&log, "disk.local_storage_dataset_allocation.is_some()");
+            return Err(SledReservationTransactionError::Reservation(
+                SledReservationError::NotFound,


Should this error indicate that we can't reserve this disk because there's an older local storage dataset that needs to be cleaned up?

yeah, dc51a95 changed this to "disk has an unsupported encrypted dataset allocation"

jgallagher · 2026-01-22T18:17:26Z

nexus/reconfigurator/rendezvous/src/local_storage_unencrypted_dataset.rs

+    let mut stats = DatasetsRendezvousStats::default();
+
+    for bp_dataset in blueprint_datasets {
+        // Filter down to LocalStorage datasets...


Suggested change

// Filter down to LocalStorage datasets...

// Filter down to LocalStorageUnencrypted datasets...

jgallagher · 2026-01-22T18:17:56Z

nexus/reconfigurator/rendezvous/src/local_storage_unencrypted_dataset.rs

+    for bp_dataset in blueprint_datasets {
+        // Filter down to LocalStorage datasets...
+        let dataset = match (&bp_dataset.kind, bp_dataset.address) {
+            (DatasetKind::LocalStorageUnencrypted, None) => {


Why do we match on an address of None here?

my mental model of this is that the only datasets with addresses are the Crucible ones, maybe that's wrong?

No, that's not wrong, but if we have an explicit DatasetKind::LocalStorageUnencrypted, isn't address guaranteed to always be None anyway?

I'd hope so haha - I'm inclined to leave the None match in as a double-check for this guarantee though.

Should we have a CHECK constraint that guarantees the address is only set for crucible datasets? On the Rust side it'd be awfully nice if we modeled this differently, but that gets into the whole DatasetName / DatasetKind redesign thing :(

My concern with matching on the address here is that it gives the appearance that we could have DatasetKind::LocalStorageUnencrypted datasets with an address, and that we're intentionally choosing to ignore those. I'd drop that part of the match for that reason alone, personally, but it's not a hill I'm willing to die on.

Thinking about it for a bit, I agree with you about dropping the match on address: in the future if we have addresses here, we'll still want the rendezvous tables management to work, or we'll have changed these types and it won't matter. Dropped in 849942e

jgallagher · 2026-01-28T18:33:01Z

common/src/api/internal/shared/mod.rs

+                );
+
+                format!("{}/{}", local_storage_parent.full_name(), dataset_id)
+            }


Small style nit - for matches like this, I think it'd be clearer to try to shrink them down to only the bits that are distinct. In this case, it looks like the only change is the DatasetKind we pass to DatasetName::new()? So maybe with some helper methods like zpool_id() for common fields this could be

let dataset_kind = match self { DelegatedZvol::LocalStorageUnencrypted { .. } => DatasetKind::LocalStorageUnencrypted, DelegatedZvol::LocalStorageEncrypted { .. } => DatasetKind::LocalStorage, }; let local_storage_parent = DatasetName::new( ZpoolName::External(self.zpool_id()), dataset_kind, ); format!("{}/{}", local_storage_parent.full_name(), self.dataset_id())

or even make a dataset_kind() method that does the match internally, making this

let local_storage_parent = DatasetName::new( ZpoolName::External(self.zpool_id()), self.dataset_kind(), ); format!("{}/{}", local_storage_parent.full_name(), self.dataset_id())

shrunk in 4ce5bc2

jgallagher · 2026-01-28T18:39:11Z

nexus/reconfigurator/rendezvous/src/local_storage_unencrypted_dataset.rs

+    for bp_dataset in blueprint_datasets {
+        // Filter down to LocalStorage datasets...
+        let dataset = match (&bp_dataset.kind, bp_dataset.address) {
+            (DatasetKind::LocalStorageUnencrypted, None) => {


Should we have a CHECK constraint that guarantees the address is only set for crucible datasets? On the Rust side it'd be awfully nice if we modeled this differently, but that gets into the whole DatasetName / DatasetKind redesign thing :(

My concern with matching on the address here is that it gives the appearance that we could have DatasetKind::LocalStorageUnencrypted datasets with an address, and that we're intentionally choosing to ignore those. I'd drop that part of the match for that reason alone, personally, but it's not a hill I'm willing to die on.

jgallagher · 2026-01-28T18:42:20Z

nexus/src/app/sagas/disk_delete.rs

-    let dataset_id = allocation.id();
-    let pool_id = allocation.pool_id();
-    let sled_id = allocation.sled_id();
+    let (sled_id, request) = match allocation {


Similar style nit as the match above - could we shrink the match down to just the encrypted_at_rest field?

let encrypted_at_rest = match allocation { datastore::LocalStorageAllocation::Unencrypted(_) => false, datastore::LocalStorageAllocation::Encrypted(_) => true, }; let sled_id = allocation.sled_id(); let request = LocalStorageDatasetDeleteRequest { zpool_id: allocation.pool_id(), dataset_id: allocation.id(), encrypted_at_rest, };

also 4ce5bc2

jmpesp requested a review from jgallagher January 20, 2026 18:03

expectorate change for sled_insert_resource_query_with_local_storage.sql

cd7d8b0

leftwo added the local storage relating to the local storage feature label Jan 20, 2026

leftwo added this to the 18 milestone Jan 20, 2026

jmpesp self-assigned this Jan 20, 2026

jgallagher reviewed Jan 22, 2026

View reviewed changes

jmpesp added 6 commits January 26, 2026 17:07

use the type system = LocalStorageAllocation enum

dc51a95

copy and paste, missed it

773a214

Support two types of DelegatedZvol

ad79d19

Merge branch 'main' into local_storage_use_unencrypted_dataset

99d5be3

fmt

679a0e7

hakari

73efb59

jgallagher approved these changes Jan 28, 2026

View reviewed changes

jmpesp added 3 commits January 28, 2026 21:46

do not match on address = None

849942e

shrink or eliminate matches

4ce5bc2

revert changes to openapi/nexus/, why did this happen?

56ad7a0

jmpesp enabled auto-merge (squash) January 28, 2026 22:08

		local_storage_dataset_allocation,
		local_storage_unencrypted_dataset_allocation,

	// Filter down to LocalStorage datasets...
	// Filter down to LocalStorageUnencrypted datasets...

Use an unencrypted dataset for local storage #9684

Are you sure you want to change the base?

Use an unencrypted dataset for local storage #9684

Uh oh!

Conversation

jmpesp commented Jan 20, 2026

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!

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!

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