Issue stratisd 3467: Support for filesystem sizes and filesystem size limits in early boot - #4065
Issue stratisd 3467: Support for filesystem sizes and filesystem size limits in early boot#4065alessandralanz wants to merge 4 commits into
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
bmr-cymru
left a comment
There was a problem hiding this comment.
Same basic pattern as #3336 for the JSONRPC plumbing & CLI additions.
One thing to consider: right now stratis-min filesystem set-size-limit with no --size-limit fails with the slightly mysterious:
# ./target/release/stratis-min filesystem set-size-limit p1 fs1
Error: "The requested action had no effect"
Which is the generic error string used when an RPC request is performed but changed == 0 upon return. I think it would be better to either enforce the same arguments as the regular stratis filesystem set-size-limit (which requires the limit as the 3rd positional arg), or to make the --size-limit long option required for the stratis-min filesystem set-size-limit command:
# ./target/release/stratis-min filesystem set-size-limit p1 fs1
error: the following required arguments were not provided:
--size-limit <size_limit>
Usage: stratis-min filesystem set-size-limit --size-limit <size_limit> <pool_name> <fs_name>
For more information, try '--help'.
# ./target/release/stratis-min filesystem set-size-limit p1 fs1
error: the following required arguments were not provided:
<size_limit>
Usage: stratis-min filesystem set-size-limit <pool_name> <fs_name> <size_limit>
For more information, try '--help'.
I think I favor the bottom option. |
mulkieran
left a comment
There was a problem hiding this comment.
Once you've settled the command-line syntax, please add tests in tests/stratis_min.rs and tests/stratisd_min.rs as appropriate.
Agreed: I think the consistency with the regular stratis-cli command syntax is the better UX. |
…t_size_limit handler
…imit subcommand, and add corresponding client functions
5793174 to
59deba6
Compare
| Command::new("set-size-limit") | ||
| .arg(Arg::new("pool_name").required(true)) | ||
| .arg(Arg::new("fs_name").required(true)) | ||
| .arg(Arg::new("size_limit").long("size-limit").num_args(1)), |
There was a problem hiding this comment.
This still isn't required or positional, so it has the same confusing behaviour as before:
# ./target/release/stratis-min filesystem set-size-limit p1 fs1
Error: "The requested action had no effect"
It would be better to enforce this as a parse error than to print the RPC error on the terminal:
# ./target/release/stratis-min filesystem set-size-limit p1 fs1
error: the following required arguments were not provided:
<size_limit>
Usage: stratis-min filesystem set-size-limit <pool_name> <fs_name> <size_limit>
For more information, try '--help'.
Summary
Closes #3467
--sizeand--size-limitparameters tostratis-min filesystem createstratis-min filesystem set-size-limitsubcommandChanges bring early boot JSON-RPC interface to parity with the D-Bus API for filesystem size and size limit support
Files changed
src/jsonrpc/interface.rs— Add size/size_limit fields toFsCreate, addFsSetSizeLimittoStratisParamTypeandStratisRetsrc/jsonrpc/server/filesystem.rs: Accept size params infilesystem_create, addfilesystem_set_size_limithandlersrc/jsonrpc/server/server.rs: Route new parameters andFsSetSizeLimitto handlerssrc/jsonrpc/client/filesystem.rs: Pass size params infilesystem_create, addfilesystem_set_size_limitclient functionsrc/bin/stratis-min/stratis-min.rs: Add--size/--size-limitCLI args andset-size-limitsubcommand@jbaublitz @mulkieran