Improve `--help` content in both firecracker and jailer #1861

shioyama18 · 2020-05-04T15:46:45Z

Signed-off-by: Shion Yamashita shioyama1118@gmail.com

Reason for This PR

Description of Changes

Improve the following in --help content

Display default values along with the description
Segregate required and optional arguments
Sort and align arguments

Jailer v0.21.0

required arguments:
  --exec-file <exec-file>   File path to exec into.
  --gid <gid>               The group identifier the jailer switches to after exec.
  --id <id>                 Jail ID.
  --node <node>             NUMA node to assign this microVM to.
  --uid <uid>               The user identifier the jailer switches to after exec.

optional arguments:
  --chroot-base-dir <chroot-base-dir>   The base folder where chroot jails are located. [default: "/srv/jailer"]
  --daemonize                           Daemonize the jailer before exec, by invoking setsid(), and redirecting the standard I/O file descriptors to /dev/null.
  --extra-args <extra-args>             Arguments that will be passed verbatim to the exec file.
  --help                                Show the help message.
  --netns <netns>                       Path to the network namespace this microVM should join.

Firecracker v0.21.0

optional arguments:
  --api-sock <api-sock>                     Path to unix domain socket used by the API. [default: "/run/firecracker.socket"]
  --config-file <config-file>               Path to a file that contains the microVM configuration in JSON format.
  --help                                    Show the help message.
  --id <id>                                 MicroVM unique identifier. [default: "anonymous-instance"]
  --level <level>                           Set the logger level. [default: "Warning"]
  --log-path <log-path>                     Path to a fifo or a file used for configuring the logger on startup.
  --no-api                                  Optional parameter which allows starting and using a microVM without an active API socket.
  --seccomp-level <seccomp-level>           Level of seccomp filtering (0: no filter | 1: filter by syscall number | 2: filter by syscall number and argument values) that will be passed to executed path as argument. [default: "2"]
  --show-level                              Whether or not to output the level in the logs.
  --show-log-origin                         Whether or not to include the file path and line number of the log's origin.
  --start-time-cpu-us <start-time-cpu-us>   Process start CPU time (wall clock, microseconds).
  --start-time-us <start-time-us>           Process start time (wall clock, microseconds).

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

aghecenco · 2020-05-13T08:48:28Z

src/utils/src/arg_parser.rs

+                if is_required {
+                    arg.required
+                } else {
+                    !arg.required
+                }


Suggested change

if is_required {

arg.required

} else {

!arg.required

}

is_required == arg.required

aghecenco · 2020-05-13T08:50:53Z

src/firecracker/src/main.rs

@@ -106,11 +106,13 @@ fn main() {
        )
        .arg(
            Argument::new("start-time-us")
-                .takes_value(true),
+                .takes_value(true)
+                .help("Wall clock time calculated by the jailer that it spent doing its work.")


Suggested change

.help("Wall clock time calculated by the jailer that it spent doing its work.")

.help("Process start time (wall clock, microseconds).")

aghecenco · 2020-05-13T08:51:36Z

src/firecracker/src/main.rs

        )
        .arg(
            Argument::new("start-time-cpu-us")
-                .takes_value(true),
+                .takes_value(true)
+                .help("CPU time calculated by the jailer that it spent doing its work."),


Suggested change

.help("CPU time calculated by the jailer that it spent doing its work."),

.help("Process start CPU time (wall clock, microseconds)."),

shioyama18 · 2020-05-13T13:06:48Z

@aghecenco
Thank you for the review. I updated the help message and filter predicates.

lauralt

Changes look really nice 👏.

The git history though is not accurate at this moment. You are fixing things from a previous commit in another one and the commit descriptions are not so clear anymore. Also, the changes are kind of mixed up.

I think the easy way out would be to squash the commits into a single one and enumerate in the commit message all the changes (except the ones that fix previous things or unit tests, fixing unit tests for a change that you introduce is implied and shouldn't be mentioned in a git message IMO). If you want to have separate commits, please keep them as clean as possible (for example, if you want a commit for the BTreeMap change, that commit should contain only this change or if you want one for the unit testing part, it should contain only unit test changes).

lauralt · 2020-05-14T06:31:43Z

src/utils/src/arg_parser.rs

        help_builder.concat()
    }
+
+    fn arg_with_opt(&self) -> String {


I would rename this function, the arg part should be already implied by the fact that this function belongs to Argument, so we don't need to include it in the function name. Also, opt seems a little bit ambiguous.
How about format_name()?

lauralt · 2020-05-14T06:35:10Z

src/utils/src/arg_parser.rs

-        help_builder.concat()
+        let optional_arguments = self.format_arguments(false);
+        if !optional_arguments.is_empty() {
+            // Add line break if `required_arguments` is pushed


nit:

Suggested change

// Add line break if `required_arguments` is pushed

// Add line break if `required_arguments` is pushed.

lauralt · 2020-05-14T06:40:07Z

src/utils/src/arg_parser.rs

+            .filter(|arg| is_required == arg.required)
+            .collect::<Vec<_>>();
+
+        let arg_width = filtered_arguments


Can we rename arg_width to max_arg_width to better suggest its purpose?

lauralt · 2020-05-14T06:53:46Z

src/utils/src/arg_parser.rs

        let mut help_builder = vec![];

-        help_builder.push(format!("--{}", self.name));
+        let arg = self.arg_with_opt();
+        help_builder.push(format!("{:<arg_width$}", arg, arg_width = arg_width));


lauralt · 2020-05-14T07:02:33Z

src/utils/src/arg_parser.rs

-        }
+        match (self.help, &self.default_value) {
+            (Some(help), Some(default_value)) => {
+                help_builder.push(format!("   {} (default {})", help, default_value))


Suggested change

help_builder.push(format!(" {} (default {})", help, default_value))

help_builder.push(format!("{} [default: {}]", help, default_value))

The three spaces from the beginning can be pushed before the match so you won't need to add them for every match branch.
The default formatting applies for the third match branch too.

lauralt · 2020-05-14T07:47:57Z

src/utils/src/arg_parser.rs


-        assert_eq!(argument.format_help(), "--exec-file <exec-file>");
+        assert_eq!(argument.format_help(width), "  --exec-file <exec-file>");


width parameter is somehow unrelated to the exact length of an argument (in this case the current argument), so I wouldn't test format_help() with that length (which is updated for every arg in this test, so it's kind of a variable width).
I know you are checking at the end of this test the fixed width scenario too, but my suggestion would be to have two fixed widths defined at the beginning of the test (width and other_width or something like that) and for every argument to test format_help() with those two widths.
It would be interesting to test format_help() also with a smaller width than the minimum one that would be required for an argument to fit in (this check could be done I think for just an argument at the end of this test :-?).
What do you say?

shioyama18 · 2020-05-14T13:27:08Z

@lauralt
Thank you for the review. I squashed the commits and fixed all issues mentioned in comments.
For the test, I defined width that is longer than the width of the argument and short_width that is shorter than the width of the argument, and tested each case with parameters, help message, and default value.

lauralt

Looks good, I have only two more nits.
Also, please update the coverage target value with the new one as it seems the test is failing now and shorten a little the commit title in order to follow the 50/72 rule (you can remove both word for example).

lauralt · 2020-05-14T14:12:57Z

src/utils/src/arg_parser.rs

-        if self.takes_value {
-            help_builder.push(format!(" <{}>", self.name));
-        }
+        // Add three whitespaces between command and help message for readablity.


Suggested change

// Add three whitespaces between command and help message for readablity.

// Add three whitespaces between the argument and its help message for readability.

lauralt · 2020-05-14T14:20:33Z

src/utils/src/arg_parser.rs

@@ -150,20 +186,40 @@ impl<'a> Argument<'a> {
        self
    }

-    fn format_help(&self) -> String {
+    fn format_help(&self, max_arg_width: usize) -> String {


Hmm, for this function context, I don't think max_arg_width is an accurate name, it's more of a fixed width than max. I think I prefer the old version (arg_width if I remember correctly).

Done. Thank you for the comment.

shioyama18 · 2020-05-14T23:43:56Z

@lauralt
I updated the commit message and coverage target value, but I am now failing the tests in test_rate_limiter.py. Is MAX_BYTES_DIFF_PERCENTAGE also a value to be updated?

lauralt · 2020-05-15T07:17:07Z

@lauralt
I updated the commit message and coverage target value, but I am now failing the tests in test_rate_limiter.py. Is MAX_BYTES_DIFF_PERCENTAGE also a value to be updated?

No.. that test is failing intermittently, we have an open issue for it: #1809 and a PR that is trying to fix it: #1898. So, another push -f should be enough.

Anyway, it looks like you have a conflict in the coverage file. Meantime, there were merged some PRs which increased the coverage target pretty much, so the value added by you is not accurate anymore.
I suggest you to rebase your branch against our master when there are many commits merged since you opened the PR and maybe run locally the coverage test: tools/devtool test -- integration_tests/build/test_coverage.py::test_coverage. I did that earlier for your branch (updated with the changes from master) and the new value seems to be: 84.56, you can try with this one even though in the CI this value varies slightly (you still need to do the rebase).

shioyama18 · 2020-05-15T10:41:29Z

@lauralt
Thank you for the comment, I rebased to master and all tests passed successfully.

lauralt · 2020-05-18T13:06:31Z

Hi, @shioyama18! The coverage test is failing again :(. Can you please update the target value to 84.59? Sorry for that.

shioyama18 · 2020-05-18T13:46:17Z

Hi @lauralt, I added commit to update target value to 84.59.

lauralt · 2020-05-18T13:53:38Z

Thanks, one more little thing :D: would be awesome if you could squash the commits into a single one, we don't usually have a separate commit just for the coverage change, sorry again :(.

- Display default values along with the description - Segregate required and optional arguments - Sort and align arguments Signed-off-by: Shion Yamashita <shioyama1118@gmail.com>

shioyama18 · 2020-05-18T14:14:45Z

No problem. Thank you for your kind review.

edigaryev mentioned this pull request May 4, 2020

Improve command-line arguments #1862

Merged

8 tasks

lauralt self-requested a review May 11, 2020 07:24

aghecenco assigned shioyama18 May 13, 2020

aghecenco added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label May 13, 2020

aghecenco reviewed May 13, 2020

View reviewed changes

lauralt reviewed May 14, 2020

View reviewed changes

shioyama18 force-pushed the refactoring/help branch from 1a0b890 to ea3017a Compare May 14, 2020 13:21

lauralt reviewed May 14, 2020

View reviewed changes

shioyama18 force-pushed the refactoring/help branch from ea3017a to 65930b0 Compare May 14, 2020 23:06

shioyama18 force-pushed the refactoring/help branch from 65930b0 to 1bbf4c5 Compare May 15, 2020 10:18

lauralt previously approved these changes May 15, 2020

View reviewed changes

lauralt requested a review from aghecenco May 15, 2020 14:47

aghecenco previously approved these changes May 18, 2020

View reviewed changes

shioyama18 dismissed stale reviews from aghecenco and lauralt via 30abe2c May 18, 2020 13:32

Improve --help content in firecracker and jailer

fef9b11

- Display default values along with the description - Segregate required and optional arguments - Sort and align arguments Signed-off-by: Shion Yamashita <shioyama1118@gmail.com>

shioyama18 force-pushed the refactoring/help branch from 30abe2c to fef9b11 Compare May 18, 2020 14:03

lauralt approved these changes May 18, 2020

View reviewed changes

aghecenco approved these changes May 18, 2020

View reviewed changes

aghecenco merged commit 484e5e9 into firecracker-microvm:master May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `--help` content in both firecracker and jailer #1861

Improve `--help` content in both firecracker and jailer #1861

shioyama18 commented May 4, 2020 •

edited

Loading

aghecenco May 13, 2020

aghecenco May 13, 2020

aghecenco May 13, 2020

shioyama18 commented May 13, 2020

lauralt left a comment •

edited

Loading

lauralt May 14, 2020

lauralt May 14, 2020

lauralt May 14, 2020

lauralt May 14, 2020

lauralt May 14, 2020

lauralt May 14, 2020

shioyama18 commented May 14, 2020

lauralt left a comment •

edited

Loading

lauralt May 14, 2020

shioyama18 May 14, 2020

lauralt May 14, 2020

shioyama18 May 14, 2020

shioyama18 commented May 14, 2020

lauralt commented May 15, 2020

shioyama18 commented May 15, 2020

lauralt commented May 18, 2020

shioyama18 commented May 18, 2020

lauralt commented May 18, 2020

shioyama18 commented May 18, 2020

	.help("Wall clock time calculated by the jailer that it spent doing its work.")
	.help("Process start time (wall clock, microseconds).")

	.help("CPU time calculated by the jailer that it spent doing its work."),
	.help("Process start CPU time (wall clock, microseconds)."),

	// Add line break if `required_arguments` is pushed
	// Add line break if `required_arguments` is pushed.

	help_builder.push(format!(" {} (default {})", help, default_value))
	help_builder.push(format!("{} [default: {}]", help, default_value))


		assert_eq!(argument.format_help(), "--exec-file <exec-file>");
		assert_eq!(argument.format_help(width), " --exec-file <exec-file>");

	// Add three whitespaces between command and help message for readablity.
	// Add three whitespaces between the argument and its help message for readability.

Improve --help content in both firecracker and jailer #1861

Improve --help content in both firecracker and jailer #1861

Conversation

shioyama18 commented May 4, 2020 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shioyama18 commented May 13, 2020

lauralt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shioyama18 commented May 14, 2020

lauralt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shioyama18 commented May 14, 2020

lauralt commented May 15, 2020

shioyama18 commented May 15, 2020

lauralt commented May 18, 2020

shioyama18 commented May 18, 2020

lauralt commented May 18, 2020

shioyama18 commented May 18, 2020

Improve `--help` content in both firecracker and jailer #1861

Improve `--help` content in both firecracker and jailer #1861

shioyama18 commented May 4, 2020 •

edited

Loading

lauralt left a comment •

edited

Loading

lauralt left a comment •

edited

Loading