-
Notifications
You must be signed in to change notification settings - Fork 63
Add two features for local storage support #9386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
26b7d31
7b7d7c1
1ffd876
f340e58
634ee2a
f7c53ab
78468e6
8724549
79c0bf3
551a528
c6110fc
bf71ddb
7e3d484
b985534
d63ff69
c85bf91
504581b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -220,6 +220,76 @@ pub struct DestroySnapshotError { | |
| err: crate::ExecutionError, | ||
| } | ||
|
|
||
| #[derive(thiserror::Error, Debug)] | ||
| pub enum EnsureDatasetVolumeErrorInner { | ||
| #[error("{0}")] | ||
| Execution(#[from] crate::ExecutionError), | ||
|
|
||
| #[error("{0}")] | ||
| GetValue(#[from] GetValueError), | ||
|
|
||
| #[error("value {value_name} parse error: {value} not a number!")] | ||
| ValueParseError { value_name: String, value: String }, | ||
|
|
||
| #[error("expected {value_name} to be {expected}, but saw {actual}")] | ||
| ValueMismatch { value_name: String, expected: u64, actual: u64 }, | ||
| } | ||
|
|
||
| /// Error returned by [`Zfs::ensure_dataset_volume`]. | ||
| #[derive(thiserror::Error, Debug)] | ||
| #[error("Failed to ensure volume '{name}': {err}")] | ||
| pub struct EnsureDatasetVolumeError { | ||
| name: String, | ||
| #[source] | ||
| err: EnsureDatasetVolumeErrorInner, | ||
| } | ||
|
|
||
| impl EnsureDatasetVolumeError { | ||
| pub fn execution(name: String, err: crate::ExecutionError) -> Self { | ||
| EnsureDatasetVolumeError { | ||
| name, | ||
| err: EnsureDatasetVolumeErrorInner::Execution(err), | ||
| } | ||
| } | ||
|
|
||
| pub fn get_value(name: String, err: GetValueError) -> Self { | ||
| EnsureDatasetVolumeError { | ||
| name, | ||
| err: EnsureDatasetVolumeErrorInner::GetValue(err), | ||
| } | ||
| } | ||
|
|
||
| pub fn value_parse( | ||
| name: String, | ||
| value_name: String, | ||
| value: String, | ||
| ) -> Self { | ||
| EnsureDatasetVolumeError { | ||
| name, | ||
| err: EnsureDatasetVolumeErrorInner::ValueParseError { | ||
| value_name, | ||
| value, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| pub fn value_mismatch( | ||
| name: String, | ||
| value_name: String, | ||
| expected: u64, | ||
| actual: u64, | ||
| ) -> Self { | ||
| EnsureDatasetVolumeError { | ||
| name, | ||
| err: EnsureDatasetVolumeErrorInner::ValueMismatch { | ||
| value_name, | ||
| expected, | ||
| actual, | ||
| }, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Wraps commands for interacting with ZFS. | ||
| pub struct Zfs {} | ||
|
|
||
|
|
@@ -1339,6 +1409,88 @@ impl Zfs { | |
| } | ||
| Ok(result) | ||
| } | ||
|
|
||
| pub async fn ensure_dataset_volume( | ||
| name: String, | ||
| size: ByteCount, | ||
| block_size: u32, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question about newtypes - is any
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the block size argument from the ensure request in 79c0bf3 - these should always only ever be 4096 byte blocks. |
||
| ) -> Result<(), EnsureDatasetVolumeError> { | ||
| let mut command = Command::new(PFEXEC); | ||
| let cmd = command.args(&[ZFS, "create"]); | ||
|
|
||
| cmd.args(&[ | ||
| "-V", | ||
| &size.to_bytes().to_string(), | ||
| "-o", | ||
| &format!("volblocksize={}", block_size), | ||
| &name, | ||
| ]); | ||
|
|
||
| // The command to create a dataset is not idempotent and will fail with | ||
| // "dataset already exists" if the volume is created already. Eat this | ||
| // and return Ok instead. | ||
|
|
||
| match execute_async(cmd).await { | ||
| Ok(_) => Ok(()), | ||
|
|
||
| Err(crate::ExecutionError::CommandFailure(info)) | ||
| if info.stderr.contains("dataset already exists") => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to check it has the expected size and block size?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 8724549 does this - importantly we can't change these after the fact so calling with different parameters is an error. |
||
| { | ||
| // Validate that the total size and volblocksize are what is | ||
| // being requested: these cannot be changed once the volume is | ||
| // created. | ||
|
|
||
| let [actual_size, actual_block_size] = | ||
| Self::get_values(&name, &["volsize", "volblocksize"], None) | ||
| .await | ||
| .map_err(|err| { | ||
| EnsureDatasetVolumeError::get_value( | ||
| name.clone(), | ||
| err, | ||
| ) | ||
| })?; | ||
|
|
||
| let actual_size: u64 = actual_size.parse().map_err(|_| { | ||
| EnsureDatasetVolumeError::value_parse( | ||
| name.clone(), | ||
| String::from("volsize"), | ||
| actual_size, | ||
| ) | ||
| })?; | ||
|
|
||
| let actual_block_size: u32 = | ||
| actual_block_size.parse().map_err(|_| { | ||
| EnsureDatasetVolumeError::value_parse( | ||
| name.clone(), | ||
| String::from("volblocksize"), | ||
| actual_block_size, | ||
| ) | ||
| })?; | ||
|
|
||
| if actual_size != size.to_bytes() { | ||
| return Err(EnsureDatasetVolumeError::value_mismatch( | ||
| name.clone(), | ||
| String::from("volsize"), | ||
| size.to_bytes(), | ||
| actual_size, | ||
| )); | ||
| } | ||
|
|
||
| if actual_block_size != block_size { | ||
| return Err(EnsureDatasetVolumeError::value_mismatch( | ||
| name.clone(), | ||
| String::from("volblocksize"), | ||
| u64::from(block_size), | ||
| u64::from(actual_block_size), | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| Err(err) => Err(EnsureDatasetVolumeError::execution(name, err)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A read-only snapshot of a ZFS filesystem. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit - if there's no useful context for the wrapper, you should use
If you combine
"{0}"with#[from], you'll get one level of doublespeak when printing the error chain (because this error type and its first source will both display theExecutionError);#[error(transparent)]doesn't have that problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changed in c6110fc. I really should re-consult your document about this!