[sled-agent] Encrypt a specific set of U.2 datasets #4853
Conversation
| @@ -2930,6 +2948,35 @@ impl ServiceManager { | |||
| // Currently, the zone filesystem should be destroyed between | |||
| // reboots, so it's fine to make this decision locally. | |||
| let root = if let Some(dataset) = zone.dataset_name() { | |||
| // Check that the dataset is actually ready to be used. | |||
There was a problem hiding this comment.
This is where, on the "zone launching pathway", we validate these properties before starting a zone with a dataset.
I believe this should prevent us from using a dataset before the sled-storage code has "finalized" it with expected properties, and should be a guard against using unencrypted datasets generally in the future.
| }; | ||
| check_property("zoned", zoned, "on")?; | ||
| check_property("canmount", canmount, "on")?; | ||
| if dataset.dataset().dataset_should_be_encrypted() { |
There was a problem hiding this comment.
Need this check because crucible's dataset is still unencrypted.
| // | ||
| // Crucible already performs encryption internally, and we | ||
| // avoid double-encryption. | ||
| DatasetKind::Crucible => false, |
There was a problem hiding this comment.
If we remove this line, Crucible will also become encrypted, and Crucible datasets will be automatically migrated.
I think that's the only change which would be necessary to make that move
| /// | ||
| /// If this dataset should be encrypted, this automatically adds the | ||
| /// "crypt" dataset component. | ||
| pub fn full_name(&self) -> String { |
There was a problem hiding this comment.
If you're looking for "how are we changing the provisioning of new datasets to become encrypted?", it's here.
There's some code in sled-storage/src/manager.rs that invokes this function to find the full path of the dataset -- since we return encrypted names for "should-be-encrypted" datasets, that's where they get placed.
| // TODO: Could do this in parallel? | ||
| for dataset in unencrypted_datasets { | ||
| let log = &log.new(slog::o!("dataset" => dataset.clone())); | ||
| info!(log, "Found unencrypted dataset"); | ||
|
|
||
| ensure_zpool_dataset_is_encrypted(&log, &zpool_name, &dataset).await?; | ||
| } |
There was a problem hiding this comment.
I'm not sure if we actually want to parallelize this, but it would be easy to do.
Seems like we should make disk addition parallel at least, so we can fire up all U.2s at once -- but we might get less benefit from putting extra workload on a U.2 already performing a migration. That's all speculative though.
There was a problem hiding this comment.
I'm going to start adding some concurrency support later today, or Monday at the latest. Got a touch delayed.
There was a problem hiding this comment.
All good - I think it would be nice to have, at any time, but I don't think it needs to be a blocker here.
There was a problem hiding this comment.
To be clear: I meant concurrency support for Disk::new, not ensure_zpool_dataset_is_encrypted. I agree with your speculation here, and if actual migrations are too slow we can revisit. I'm hesitant to complicate this unnecessarily.
| source: illumos_utils::zfs::GetValueError, | ||
| }, | ||
|
|
||
| #[error("Cannot launch {zone} with {dataset} (saw {prop_name} = {prop_value}, expected {prop_value_expected})")] |
There was a problem hiding this comment.
Love the detail in this error
| // TODO: Could do this in parallel? | ||
| for dataset in unencrypted_datasets { | ||
| let log = &log.new(slog::o!("dataset" => dataset.clone())); | ||
| info!(log, "Found unencrypted dataset"); | ||
|
|
||
| ensure_zpool_dataset_is_encrypted(&log, &zpool_name, &dataset).await?; | ||
| } |
There was a problem hiding this comment.
To be clear: I meant concurrency support for Disk::new, not ensure_zpool_dataset_is_encrypted. I agree with your speculation here, and if actual migrations are too slow we can revisit. I'm hesitant to complicate this unnecessarily.
This PR does the following:
ensure_zpool_datasets_are_encryptedis invoked. This identifies all datasets which should be encrypted (cockroachdb,clickhouse,internal_dns,external_dns,clickhouse_keeper) and performs a one-way migration from unencrypted to encrypted dataset.