Bug #17840

keep-balance should error out on extra unparsed command line flags

Added by Tom Clegg 11 months ago. Updated about 2 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
11/08/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Currently, it does not, which means

keep-balance -commit-pulls false -commit-trash true -dump

is actually interpreted as

keep-balance -commit-pulls

Subtasks

Task #18343: Review 17840-unparsed-argsResolvedWard Vandewege


Related issues

Related to Arvados - Bug #18465: arvados-dispatch-cloud starts compute nodes for "on hold" containersResolved

History

#1 Updated by Tom Clegg 11 months ago

Check other CLI tools for the same bug while we're at it.

#2 Updated by Tom Clegg 11 months ago

  • Status changed from New to In Progress

started at 17840-unparsed-args

#3 Updated by Peter Amstutz 11 months ago

  • Target version deleted (Arvados Future Sprints)

#4 Updated by Tom Clegg 6 months ago

  • Target version set to 2021-11-10 sprint
  • Assigned To set to Tom Clegg

#6 Updated by Ward Vandewege 6 months ago

Tom Clegg wrote:

17840-unparsed-args @ 40f551004ab4e5f1d8ab02ddb55dca225ee8f6ac -- developer-run-tests: #2779

retrying workbench1 tests at developer-run-tests-apps-workbench-integration: #2952

OK, there seems to be a fair bit going on here. Looking at keep-balance, before the patch:

 ./keep-balance -x
flag provided but not defined: -x
Usage of ./keep-balance:
  -commit-confirmed-fields
        update collection fields (replicas_confirmed, storage_classes_confirmed, etc.) (default true)
  -commit-pulls
        send pull requests (make more replicas of blocks that are underreplicated or are not in optimal rendezvous probe order)
  -commit-trash
        send trash requests (delete unreferenced old blocks, and excess replicas of overreplicated blocks)
  -config file
        Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
  -dump
        dump details for each block to stdout
  -legacy-crunch-dispatch-slurm-config file
        Legacy crunch-dispatch-slurm configuration file (default "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml")
  -legacy-git-httpd-config file
        Legacy arv-git-httpd configuration file (default "/etc/arvados/git-httpd/git-httpd.yml")
  -legacy-keepbalance-config file
        Legacy keep-balance configuration file (default "/etc/arvados/keep-balance/keep-balance.yml")
  -legacy-keepproxy-config file
        Legacy keepproxy configuration file (default "/etc/arvados/keepproxy/keepproxy.yml")
  -legacy-keepstore-config file
        Legacy keepstore configuration file (default "/etc/arvados/keepstore/keepstore.yml")
  -legacy-keepweb-config file
        Legacy keep-web configuration file (default "/etc/arvados/keep-web/keep-web.yml")
  -legacy-ws-config file
        Legacy arvados-ws configuration file (default "/etc/arvados/ws/ws.yml")
  -once
        balance once and then exit
  -pprof [addr]:port
        serve Go profile data at [addr]:port
  -skip-legacy
        Don't load legacy config files
  -version
        Write version information to stdout and exit 0

With this patch:

 ./keep-balance -x
flag provided but not defined: -x
Usage of ./keep-balance:
  -commit-confirmed-fields
        update collection fields (replicas_confirmed, storage_classes_confirmed, etc.) (default true)
  -commit-pulls
        send pull requests (make more replicas of blocks that are underreplicated or are not in optimal rendezvous probe order)
  -commit-trash
        send trash requests (delete unreferenced old blocks, and excess replicas of overreplicated blocks)
  -config file
        Site configuration file (default may be overridden by setting an ARVADOS_CONFIG environment variable) (default "/etc/arvados/config.yml")
  -dump
        dump details for each block to stdout
  -legacy-crunch-dispatch-slurm-config file
        Legacy crunch-dispatch-slurm configuration file (default "/etc/arvados/crunch-dispatch-slurm/crunch-dispatch-slurm.yml")
  -legacy-git-httpd-config file
        Legacy arv-git-httpd configuration file (default "/etc/arvados/git-httpd/git-httpd.yml")
  -legacy-keepbalance-config file
        Legacy keep-balance configuration file (default "/etc/arvados/keep-balance/keep-balance.yml")
  -legacy-keepproxy-config file
        Legacy keepproxy configuration file (default "/etc/arvados/keepproxy/keepproxy.yml")
  -legacy-keepstore-config file
        Legacy keepstore configuration file (default "/etc/arvados/keepstore/keepstore.yml")
  -legacy-keepweb-config file
        Legacy keep-web configuration file (default "/etc/arvados/keep-web/keep-web.yml")
  -legacy-ws-config file
        Legacy arvados-ws configuration file (default "/etc/arvados/ws/ws.yml")
  -once
        balance once and then exit
  -pprof [addr]:port
        serve Go profile data at [addr]:port
  -skip-legacy
        Don't load legacy config files
  -version
        Write version information to stdout and exit 0
ERRO[0000] error parsing command line flags: flag provided but not defined: -x 

The error message is now added at the bottom which is nice. Any chance we can easily get rid of the one at the top?

And with an invalid/extra argument, we now get:

./keep-balance x
ERRO[0000] unrecognized command line arguments: [x]     

For consistency's sake, should we also print the help text when this happens?

I'm not sure what to think about the formatting of the errors (the leading ERRO\[0000\] is a bit wonky). Is it easy to omit that? Maybe replace with a more human friendly 'Error: ' ? Maybe with a blank line between the help text and the error message, to make it stand out more visually?

Otherwise, nice cleanup!

#7 Updated by Tom Clegg 6 months ago

  • Target version changed from 2021-11-10 sprint to 2021-11-24 sprint

#8 Updated by Tom Clegg 6 months ago

17840-unparsed-args @ 47c3faf1e26be21190eeee7f266d44eb33a0aeb6 -- developer-run-tests: #2791
  • de-duplicate much of the argument-parsing code and unify behavior across various programs
  • "-h" and "-help" print the usage notes, and exit 0
  • bad flags and missing/extra positional arguments cause an error message on stderr, and exit 2, but do not obscure the error message by printing the usage notes before/after

#9 Updated by Ward Vandewege 6 months ago

Tom Clegg wrote:

17840-unparsed-args @ 47c3faf1e26be21190eeee7f266d44eb33a0aeb6 -- developer-run-tests: #2791
  • de-duplicate much of the argument-parsing code and unify behavior across various programs
  • "-h" and "-help" print the usage notes, and exit 0
  • bad flags and missing/extra positional arguments cause an error message on stderr, and exit 2, but do not obscure the error message by printing the usage notes before/after

This looks great! Any reason `cmd/arvados-package/cmd.go` was not given the same treatment? LGTM otherwise.

#10 Updated by Tom Clegg 6 months ago

17840-unparsed-args @ fb96637bf76fe8779e7a7e58f052b8f55ed76f4f -- developer-run-tests: #2799
  • same treatment to arvados-package
  • merged main

#11 Updated by Ward Vandewege 6 months ago

Tom Clegg wrote:

17840-unparsed-args @ fb96637bf76fe8779e7a7e58f052b8f55ed76f4f -- developer-run-tests: #2799
  • same treatment to arvados-package
  • merged main

LGTM, thanks!

#12 Updated by Tom Clegg 6 months ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados-private:commit:arvados|0f42105b1b59d1b5da764f34e6eb6a1137d7e1cb.

#13 Updated by Ward Vandewege 6 months ago

  • Related to Bug #18465: arvados-dispatch-cloud starts compute nodes for "on hold" containers added

#14 Updated by Peter Amstutz about 2 months ago

  • Release set to 46

Also available in: Atom PDF