Add two features for local storage support #9386
Conversation
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.
| 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 { |
There was a problem hiding this comment.
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 devicesto preallocate the necessary size
| return Err(Error::internal_error(&format!( | ||
| "request block_size {} does not match FileStorageBackend \ | ||
| block_size {block_size}", | ||
| request.block_size, | ||
| ))); |
There was a problem hiding this comment.
perhaps this error ought to include the ID of the invalid backend?
| .expect("Failed to get the dataset we just inserted"); | ||
| .expect("Failed to get the dataset we just inserted") | ||
| else { | ||
| panic!("just inserted this variant!"); |
There was a problem hiding this comment.
obnoxious turbo nit: feels like a place for
| panic!("just inserted this variant!"); | |
| unreachable!("just inserted this variant!"); |
| /// The fully qualified name of the parent dataset | ||
| pub parent_dataset: String, | ||
|
|
||
| /// The volume name | ||
| pub name: String, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Ok(_) => Ok(()), | ||
|
|
||
| Err(crate::ExecutionError::CommandFailure(info)) | ||
| if info.stderr.contains("dataset already exists") => |
There was a problem hiding this comment.
Do we need to check it has the expected size and block size?
| pub async fn ensure_dataset_volume( | ||
| name: String, | ||
| size: ByteCount, | ||
| block_size: u32, |
There was a problem hiding this comment.
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)?
| // 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(); |
There was a problem hiding this comment.
Could this string be a constant somewhere?
| .datasets | ||
| .get(&id) | ||
| .expect("Failed to get the dataset we just inserted"); | ||
| .expect("Failed to get the dataset we just inserted") |
There was a problem hiding this comment.
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!())
| match self.get_dataset(zpool_id, dataset_id) { | ||
| DatasetContents::Crucible(crucible) => crucible.data.clone(), | ||
| DatasetContents::LocalStorage(_) => { | ||
| panic!("asked for Crucible, got LocalStorage!") |
There was a problem hiding this comment.
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)
| dataset_id: DatasetUuid, | ||
| request: sled_agent_api::LocalStorageDatasetEnsureRequest, | ||
| ) -> Result<(), HttpError> { | ||
| let zpool_name = ZpoolName::External(zpool_id); |
There was a problem hiding this comment.
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 ...)
| ) -> Result<(), HttpError> { | ||
| let zpool_name = ZpoolName::External(zpool_id); | ||
|
|
||
| let name = format!("{zpool_name}/crypt/local_storage/{dataset_id}"); |
There was a problem hiding this comment.
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?
3 participants
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
DelegatedZvolparameter during vmm registration.