Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ceph: allow for non-positional optional CLI arguments #41509

Merged
merged 21 commits into from Jun 7, 2021
Merged

ceph: allow for non-positional optional CLI arguments #41509

merged 21 commits into from Jun 7, 2021

Conversation

liewegas
Copy link
Member

Currently any optional argument may be specified as a flag (--arg val) or positionally. The ability to specify flags positionally is confusing, and not something that other CLIs allow. This PR makes it possible to mark the end of positional (and possibly optinoal) arguments and the beginning of optional flags (that must use the '--flagname value' syntax).

Also,

  • fix a few remaining CephChoices args that were never converted to CephBool
  • fix some CLI helptext strings
  • fix orch rgw commands to rearrange positional args and make more args flags
  • misc other CLI cleanup

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@liewegas liewegas requested a review from a team as a code owner May 24, 2021 19:22
@tchaikov tchaikov self-requested a review May 25, 2021 13:44
@tchaikov
Copy link
Contributor

@liewegas this changeset is non-trivial, i will take a closer look at it next week.

@liewegas
Copy link
Member Author

@liewegas this changeset is non-trivial, i will take a closer look at it next week.

Yep! Thanks for taking a look. My main reservation is that the form of the JSON we are generating is wonky.. things like "req": "false" instead of "req": false should probably be cleaned up but it'll be a pain in the but to deal with the compatibility. However... I think that this PR has effectively identified the points where we need to conditionally things encode differently so now is probably the time to do it...

@leseb
Copy link
Member

leseb commented May 31, 2021

@liewegas will this solve https://tracker.ceph.com/issues/50954 too? Or perhaps modules' CLI are treated independently?

tchaikov
tchaikov reviewed May 31, 2021
View reviewed changes
src/common/cmdparse.cc Outdated Show resolved Hide resolved
@tchaikov
Copy link
Contributor

@liewegas this changeset is non-trivial, i will take a closer look at it next week.

Yep! Thanks for taking a look. My main reservation is that the form of the JSON we are generating is wonky.. things like "req": "false" instead of "req": false should probably be cleaned up but it'll be a pain in the but to deal with the compatibility. However... I think that this PR has effectively identified the points where we need to conditionally things encode differently so now is probably the time to do it...

agreed. it should be "req": false. =)

tchaikov
tchaikov reviewed May 31, 2021
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from the nits, i just don't really get one thing: the has_default parameter in:

def _compound_type_to_argdesc(tp, attrs, has_default, positional):

once i am able to get my head around this, it will lgtm.

@github-actions github-actions bot added cephfs Ceph File System nfs tests labels Jun 1, 2021
@liewegas
pybind/ceph_argparse: track a 'positional' property on cli args
165987c 
Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
pybind/ceph_argparse: adjust help text for non-positional args
b9a2a71 
If an arg is non-positional, always show it as

  [--arg-name <value>]

(All non-positional args are optional.)

Signed-off-by: Sage Weil <sage@newdream.net>
tchaikov
tchaikov requested changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

2021-06-04T02:49:23.617 DEBUG:teuthology.orchestra.run.smithi045:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph nfs export create cephfs nfs-cephfs test /cephfs3 /volumes/_nogroup/sub_vol/ed3d73eb-3d51-444b-919a-
f81387b3df6d
2021-06-04T02:49:23.987 DEBUG:teuthology.orchestra.run:got remote process result: 22
2021-06-04T02:49:23.988 INFO:teuthology.orchestra.run.smithi045.stderr:Invalid command: Unexpected argument '/volumes/_nogroup/sub_vol/ed3d73eb-3d51-444b-919a-f81387b3df6d'
2021-06-04T02:49:23.988 INFO:teuthology.orchestra.run.smithi045.stderr:nfs export create cephfs <fsname> <clusterid> <binding> [--readonly] [--path <value>] :  Create a cephfs export
2021-06-04T02:49:23.988 INFO:teuthology.orchestra.run.smithi045.stderr:Error EINVAL: invalid command
2021-06-04T02:49:23.989 DEBUG:teuthology.orchestra.run.smithi045:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph log 'Ended test tasks.cephfs.test_nfs.TestNFS.test_create_multiple_exports'
2021-06-04T02:49:24.313 INFO:journalctl@ceph.mon.a.smithi045.stdout:Jun 04 02:49:24 smithi045 ceph-mon[31481]: from='client.14645 -' entity='client.admin' cmd=[{"prefix": "fs subvolume getpath", "vol_name": "nfs-cephfs", "sub_name": "sub_vol", "target": ["mon-mgr", ""]}]: dispatch
2021-06-04T02:49:24.314 INFO:journalctl@ceph.mon.a.smithi045.stdout:Jun 04 02:49:24 smithi045 ceph-mon[31481]: pgmap v165: 97 pgs: 97 active+clean; 15 KiB data, 20 MiB used, 268 GiB / 268 GiB avail; 1.5 KiB/s rd, 1.6 KiB/s wr, 3 op/s
2021-06-04T02:49:24.314 INFO:journalctl@ceph.mon.a.smithi045.stdout:Jun 04 02:49:24 smithi045 ceph-mon[31481]: from='client.14647 -' entity='client.admin' cmd=[{"prefix": "nfs export create cephfs", "fsname": "nfs-cephfs", "clusterid": "test", "binding": "/cephfs2", "readonly": true, "path": "/volumes/_nogroup/sub_vol/ed3d73eb-3d51-444b-919a-f81387b3df6d", "target": ["mon-mgr", ""]}]: dispatch
2021-06-04T02:49:24.314 INFO:journalctl@ceph.mon.a.smithi045.stdout:Jun 04 02:49:24 smithi045 ceph-mon[31481]: mgrmap e26: a(active, since 4m)
2021-06-04T02:49:24.314 INFO:journalctl@ceph.mon.a.smithi045.stdout:Jun 04 02:49:24 smithi045 ceph-mon[31481]: from='client.? 172.21.15.45:0/2224977036' entity='client.admin' cmd=[{"prefix": "auth ls"}]: dispatch
2021-06-04T02:49:25.146 INFO:tasks.cephfs_test_runner:test_create_multiple_exports (tasks.cephfs.test_nfs.TestNFS) ... ERROR

this change breaks "test_create_multiple_exports " in tasks.cephfs.test_nfs.TestNFS, which executes

$ ceph --cluster ceph nfs export create cephfs nfs-cephfs test /cephfs3 /volumes/_nogroup/sub_vol/ed3d73eb-3d51-444b-919a-f81387b3df6d'

see https://pulpito.ceph.com/kchai-2021-06-04_02:22:09-rados-wip-kefu-testing-2021-06-03-2320-distro-basic-smithi/6150856/

@liewegas liewegas force-pushed the cli-positional branch 2 times, most recently from 2052a4b to 4b908a2 Compare June 4, 2021 20:53
@liewegas
mon/MonCommands: convert some CephChoices to CephBool
bd242c4 
These are old bool options that never got converted to CephBool.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
mgr: fix reweight-by-utilization cephbool flag
c70a4f5 
This was broken, since it was trying to get a CephChoices as a bool.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
doc/_ext/ceph_commands: handle non-positional args in docs
295a5f2 
Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
mgr: make mgr commands compat with pre-quincy mon
218f4ce 
If the mon is not yet quincy, do not share the quincy-ism of
'positional=false'.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
qa/tasks/cephfs/test_nfs: fix export create test
8683ccc 
Everything after --readonly is non-positional.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
mon/MonCommands: add -- seperator to example
b6b5ecc 
Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
common/cmdparse: emit proper json
ec293f3 
Instead of '"req": "false"', emit '"req": false'.  Same for conditional.

Luckily, the clients don't really care about this change, as
ceph_argparse.py argdesc interpets the JSON like so:

            self.req = req in (True, 'True', 'true')
            self.positional = positional in (True, 'True', 'true')

Clean up command definitions to use lowercase 'false', but tolerate
both for backward compat during upgrade and to tolerate future errors.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas
mgr/nfs: fix 'nfs export create' argument order
ffce51a 
Put path before --readonly so that it can still be positional.

Signed-off-by: Sage Weil <sage@newdream.net>
@tchaikov
Copy link
Contributor

tchaikov commented Jun 5, 2021

@liewegas
common/cmdparse: fix CephBool validation for tell commands
6bd4df3 
The tell/asok validation uses a different validation path; make it work
for CephBool arguments.

Signed-off-by: Sage Weil <sage@newdream.net>
@liewegas liewegas merged commit b18427d into ceph:master Jun 7, 2021
@tchaikov tchaikov mentioned this pull request Jun 7, 2021
Merged
3 tasks
@leseb leseb mentioned this pull request Jun 7, 2021
Closed
3 tasks
@leseb
Copy link
Member

leseb commented Jul 1, 2021

@liewegas @tchaikov is this going to be backported to Pacific?

@tchaikov
Copy link
Contributor

tchaikov commented Jul 1, 2021

@leseb i have no idea if we want to backport it or not. presumably, it's not a bug fix or a prerequisite for a critical feature required by pacific. am i right?

@leseb
Copy link
Member

leseb commented Jul 1, 2021

@leseb i have no idea if we want to backport it or not. presumably, it's not a bug fix or a prerequisite for a critical feature required by pacific. am i right?

Well @liewegas added this as a fix for https://tracker.ceph.com/issues/50954. So it'd be nice to have some kind of backport.

@sebastian-philipp sebastian-philipp mentioned this pull request Sep 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

Assignees
No one assigned
Labels
cephfs Ceph File System common core documentation mgr mon nfs orchestrator pybind tests wip-sage3-testing
Projects
None yet
Milestone
No milestone
4 participants
@liewegas @tchaikov @leseb @huww98