Use an unencrypted dataset for local storage #9684
Conversation
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!
| /// 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. |
There was a problem hiding this comment.
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.
| (Some(_), Some(_)) => { | ||
| return Err(Error::internal_error(&format!( | ||
| "LocalStorageDisk {} has multiple allocations!", | ||
| self.id(), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
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?
| format!("{zpool_name}/crypt/local_storage"), | ||
| format!("{dataset_id}/vol"), | ||
| ] | ||
| .join("/"); |
There was a problem hiding this comment.
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)?
| local_storage_dataset_allocation, | ||
| local_storage_unencrypted_dataset_allocation, |
There was a problem hiding this comment.
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(_)?
|
|
||
| /// List one page of unencrypted local storage datasets | ||
| /// | ||
| /// This fetches all datasets, including those that have been tombstoned. |
There was a problem hiding this comment.
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?
| { | ||
| info!(&log, "disk.local_storage_dataset_allocation.is_some()"); | ||
| return Err(SledReservationTransactionError::Reservation( | ||
| SledReservationError::NotFound, |
There was a problem hiding this comment.
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?
| let mut stats = DatasetsRendezvousStats::default(); | ||
|
|
||
| for bp_dataset in blueprint_datasets { | ||
| // Filter down to LocalStorage datasets... |
There was a problem hiding this comment.
| // Filter down to LocalStorage datasets... | |
| // Filter down to LocalStorageUnencrypted datasets... |
| for bp_dataset in blueprint_datasets { | ||
| // Filter down to LocalStorage datasets... | ||
| let dataset = match (&bp_dataset.kind, bp_dataset.address) { | ||
| (DatasetKind::LocalStorageUnencrypted, None) => { |
There was a problem hiding this comment.
Why do we match on an address of None here?
3 participants
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!