HDDS-6882. Correct exit code for invalid arguments passed to command-line tools.

[duongkame]

What changes were proposed in this pull request?

Command-line tools don't take exit-code from picocli and set the exit-code accordingly. That makes exit-codes for cases like invalid argument be always 0.

This change fixes that by replacing the use of the deprecated parseWithHandler method with the execute method that returns correct exit-code for parsing/invalid input cases.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6882

How was this patch tested?

Regression is checked by unit test.

Manually tests to verify the expected status code for error-cases.

ozone sh bucket create /vol1/bucket1 -l 123
Invalid value for option '--layout': expected one of [FILE_SYSTEM_OPTIMIZED, OBJECT_STORE, LEGACY] (case-sensitive) but was '123'
Usage: ozone sh bucket create [-ghV] [-k=<bekName>] [-l=<allowedBucketLayout>]
                              [--namespace-quota=<quotaInNamespace>]
                              [--quota=<quotaInBytes>] [-r=<replication>]
                              [-t=<type>] [-u=<ownerName>] <value>
creates a bucket in a given volume
      <value>              URI of the volume/bucket.
                           Ozone URI could start with o3:// or without prefix.
                             URI may contain the host/serviceId and port of the
                             OM server. Both are optional. If they are not
                             specified it will be identified from the config
                             files.
  -g, --enforcegdpr        if true, indicates GDPR enforced bucket,
                             false/unspecified indicates otherwise
  -h, --help               Show this help message and exit.
  -k, --bucketkey=<bekName>
                           bucket encryption key name
  -l, --layout=<allowedBucketLayout>
                           Allowed Bucket Layouts: FILE_SYSTEM_OPTIMIZED,
                             OBJECT_STORE, LEGACY
      --namespace-quota=<quotaInNamespace>
                           For volume this parameter represents the number of
                             buckets, and for buckets represents the number of
                             keys (eg. 5)
      --quota, --space-quota=<quotaInBytes>
                           The maximum space quota can be used (eg. 1GB)
  -r, --replication=<replication>
                           Replication definition. Valid values are replication
                             type-specific.  For RATIS: ONE or THREE. In case
                             of EC, pass CODEC-DATA-PARITY-CHUNKSIZE,  e.g.
                             rs-3-2-1024k, rs-6-3-1024k, rs-10-4-1024k
  -t, --type, --replication-type=<type>
                           Replication type. Supported types are: RATIS, EC
  -u, --user=<ownerName>   Owner of the bucket. Defaults to current user if not
                             specified
  -V, --version            Print version information and exit.
sh-4.2$ echo $?
2

sh-4.2$ ozone sh bucket create /vol1/bucket_1
INVALID_BUCKET_NAME Bucket or Volume name has an unsupported character : _
sh-4.2$ echo $?
255

sh-4.2$ ozone sh bucket create /vol1/bucket1
2022-06-14 23:06:01,060 INFO  rpc.RpcClient (RpcClient.java:createVolume(466)) - Creating Volume: vol1, with duong as owner and space quota set to -1 bytes, counts quota set to -1
sh-4.2$ echo $?
0

[adoroszlai]

Thanks @DuongNguyen0 for working on this. Looks like there is an unused import reported by checkstyle and unit test failure in TestStorageContainerManagerStarter seems related. Acceptance test failures are probably unrelated (I remember seeing them elsewhere, too).

[duongkame]

Thanks @DuongNguyen0 for working on this. Looks like there is an unused import reported by checkstyle and unit test failure in TestStorageContainerManagerStarter seems related. Acceptance test failures are probably unrelated (I remember seeing them elsewhere, too).

Thanks for having a look. I fixed the checkstyle problems and made necessary changes to the unit-tests.

[kerneltime]

Change makes sense. Can you look into adding a test as this was not caught in our testing.

[adoroszlai]

Thanks @DuongNguyen0 for updating the patch. TestOzoneManagerStarter failure was masked by being skipped in previous run, sorry about that.

[duongkame]

Can you look into adding a test as this was not caught in our testing.

Right, I added a simple robot test-suite to ensure correct CLI exit code.

[duongkame]

TestOzoneManagerStarter failure was masked by being skipped in previous run, sorry about that.

I updated the pull-requests with a new commit and that should trigger a rerun. Not sure if the TestOzoneManagerStarter would run properly this time. Please give me a hint if the test is still masked failure.

[adoroszlai]

TestOzoneManagerStarter failure was masked by being skipped in previous run, sorry about that.

Not sure if the TestOzoneManagerStarter would run properly this time. Please give me a hint if the test is still masked failure.

Let me clarify my previous comment: TestOzoneManagerStarter was skipped in previous run because it is in a module that depends on the module where TestStorageContainerManagerStarter failed. Thus it was executed in the next run, triggered by 80c4421, which fixed the failing test. So we still need TestOzoneManagerStarter to be fixed.

[duongkame]

Let me clarify my previous comment: TestOzoneManagerStarter was skipped in previous run because it is in a module that depends on the module where TestStorageContainerManagerStarter failed. Thus it was executed in the next run, triggered by 80c4421, which fixed the failing test. So we still need TestOzoneManagerStarter to be fixed.

My bad. I tried running the same unit-test script in local but somehow it didn't catch this. Updated TestOzoneManagerStarter and manually checked all other places.

[rakeshadr]

+1 LGTM

[swagle]

Thanks for thre review @kerneltime and @rakeshadr. Merging this