ceph.in: clean up and enable --foo=bar style arguments #24333

jcsp · 2018-09-28T14:54:13Z

Currently, our CLI only provides positional arguments -- if there are lots of optional fields for an operation, we can only specify a field by specifying all the optional fields before it too. That's awkward, and it's hard to be sure you've got all your values in the order you intended.

For example, specifying "pool_type" on a created pool, previously we had to know the right order of args, and also specify the pgp_num field (because it comes before pool type in the command definition):

ceph osd pool create mypool 8 8 replicated

With this change, we can use a simpler form without extra unnecessary args:

ceph osd pool create mypool 8 --pool_type=replicated

In general, I would expect most people to continue using positional arguments for the mandatory arguments, and switch to keyword arguments for optional arguments.

Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

dmick · 2018-10-05T21:53:09Z

src/pybind/ceph_argparse.py

@@ -851,30 +853,20 @@ def matchnum(args, signature, partial=False):
    return matchcnt


-def get_next_arg(desc, args):
+def _get_next_arg(desc, args):
    '''
    Get either the value matching key 'desc.name' or the next arg in


I guess this comment changes with the reduced function too

Yeah -- I think actually I'll remove this function entirely, the part where it considers one of the args being a list seems to be redundant as we're always parsing a simple list of strings from the CLI.

liewegas · 2018-10-05T22:08:49Z

Can/do we allow "--foo bar" style too? (i.e., space instead of =)

dmick · 2018-10-06T02:05:27Z

src/ceph.in

@@ -600,12 +599,6 @@ def new_style_command(parsed_args, cmdargs, target, sigdict, inbuf, verbose):
            # Non interactive mode
            ret, outbuf, outs = do_command(parsed_args, target, cmdargs, sigdict, inbuf, verbose)
        else:
-            # Interactive mode (ceph cli)


how do we get to remove this? Is readline support now builtin?..

Is just importing readline meant to have an effect? It seems like e.g. pressing the up arrow to see last command already doesn't work, so I'm not sure what it was meant to be doing

It had been the case that, when raw_input() was used, "import readline" made it support line-editing-and-recall features. Perhaps the port to Py3 lost that behavior (as raw_input disappeared too)?

https://docs.python.org/2/library/functions.html#raw_input

confusingly, both Py2 and Py3 pre-import readline for me, and thus raw_input() on py2 and input() on py3 have the recall behavior. odd that you don't observe it with ceph CLI. Digging.

https://gist.github.com/dmick/90b43cb15464417870147d953d7a2412 works as I'd expect for me with py2 and py3. ?

Huh, yeah, that snippet does show importing readline having the side effect on raw_input, which is presumably why it was in ceph.in. I'll reinstate it. Thanks for setting me straight!

(my "pre-import" comment was about the IDE, which of course makes sense since it also supports commandline editing)

dmick · 2018-10-06T02:06:55Z

src/ceph.in

+    """
+    Consume generic arguments from the start of the ``args``
+    list.  Call this first to handle arguments that are not
+    part of a server-side command's parameters.


I know it's hard to describe this, but...the 'generic args' are server-side too. hm. Maybe just call them "generic args that are not part of the 'argdesc' parameters" as a signpost?

I'll update this comment

src/pybind/ceph_argparse.py

jcsp · 2018-10-10T14:07:11Z

This now has "--foo=bar" style support. This introduces a constraint that string arguments need to not start with --, so that we don't eat mistyped keyword arguments if they appear in a place where they could also be a string.

There were a few CephString instances for --yes-i-really-mean-it etc, so I've made them all CephChoices, and added a special case for those strings when talking to an old daemon.

jcsp · 2018-10-10T14:08:37Z

I still have one annoying failure for the test that invokes "osd reweight-by-utilization --no-increasing" with the expectation that the parser will skip over the numeric arguments and match --no-increasing to the first string arg -- this doesn't work with the new code because --no-increasing just looks like a bogus keyword argument, so I'm inclined to change the rules to say that positional arguments have to be in order, rather than allowing this "skip through args until you find one that's a match for the type" case.

liewegas · 2018-10-17T19:13:45Z

needs rebase

jcsp · 2018-10-18T16:06:34Z

rebased

liewegas · 2018-10-19T14:02:38Z

src/mgr/MgrCommands.h

@@ -108,14 +108,14 @@ COMMAND("osd test-reweight-by-pg " \

 COMMAND("osd destroy "	    \
        "name=id,type=CephOsdName " \
-        "name=sure,type=CephString,req=False",
+	"name=sure,type=CephChoices,strings=--force|--yes-i-really-mean-it,req=false", \


Does this mean you can do

ceph osd destroy osd.123 --sure=--force

? It seems like what we maybe want instead is a new option,

name=force,type=CephBool,default=False

Then we'd update the code to look for the new field, and leave the optional positional one there for 2+ releases for backward compatibility?

I agree with wanting to introduce a CephBool type, but I think it can come later? The CephChoices style is consistent with other commands that already used that for the confirmation flag.

I agree with wanting to introduce a CephBool type, but I think it can come later? The CephChoices style is consistent with other commands that already used that for the confirmation flag.

I'm kind of hoping that nobody would actually use the weird --sure=--force style, and/or the cleaned up CephBool thing would happen before releasing Nautilus

Sounds good

tchaikov · 2018-10-29T11:27:47Z

2018-10-26T15:13:45.598 INFO:tasks.workunit.client.0.smithi175.stderr:/home/ubuntu/cephtest/clone.client.0/qa/workunits/cephtool/test.sh:2192: test_mon_osd_erasure_code:ceph osd erasure-code-profile set fooprofile a=b c=d e=f --force
2018-10-26T15:13:45.888 INFO:tasks.workunit.client.0.smithi175.stderr:desc={profile(<string>) [<string>...]} t=<class 'ceph_argparse.CephString'>Invalid command: Unexpected argument '--force'
2018-10-26T15:13:45.888 INFO:tasks.workunit.client.0.smithi175.stderr:osd erasure-code-profile set <name> {<profile> [<profile>...]} :  create erasure code profile <name> with [<key[=value]> ...] pairs. Add a --force at the end to overrid
e an existing profile (VERY DANGEROUS)
2018-10-26T15:13:45.889 INFO:tasks.workunit.client.0.smithi175.stderr:Error EINVAL: invalid command

http://pulpito.ceph.com/kchai-2018-10-26_14:26:38-rados-wip-kefu-testing-2018-10-25-1035-distro-basic-smithi/3186771/

jcsp · 2018-11-02T08:37:27Z

retest this please

Signed-off-by: John Spray <john.spray@redhat.com>

This was introduced for the now-removed ceph-rest-api gateway. It enabled limiting certain commands to be CLI-only or rest-only, but in practice almost everything just said "cli,rest" here. Now that ceph-rest-api is gone, let's remove this field. The CLI client code already tolerated the absence of this field, so older CLI clients will not mind. Signed-off-by: John Spray <john.spray@redhat.com>

Get this pyflakes-clean ahead of making changes for keyword arguments. Signed-off-by: John Spray <john.spray@redhat.com>

Add some docstrings to functions, remove some dead code and places where e.g. dicts were used unncessary. Signed-off-by: John Spray <john.spray@redhat.com>

This is a simple implementation that treats anything that matches the "--X=Y" pattern as separate from positional arguments. This works well for optional arguments. Mandatory arguments still need to be specified positionally, or the parsing code will think the command's argument description has not been satisfied. Signed-off-by: John Spray <john.spray@redhat.com>

This had existed in a disabled state (by having an empty string for the cli/rest field in the command definition) for a long time. Now that that field is gone, we don't have a concept of "disabled" commands any more, so let's just clean up this loose end. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

...so that arg parsing can rely on strings satisfying a "doesn't start with --" convention. Signed-off-by: John Spray <john.spray@redhat.com>

This relied on a behaviour where positional arguments could be omitted if the subsequent argument was of a different type. This was pretty weird, and in any case the reweight-by-utilization command is likely to go away soon. Signed-off-by: John Spray <john.spray@redhat.com>

This replaces use of CephChoices for cases like --yes-i-really-really-mean-it. It's a relatively invasive change, because both client and server need to handle backward compatibility: - Clients are easy, they just cope with the server's use of CephChoices/CephString with a few special case strings to recognise --yes-i-really-mean-it and similar - Servers are harder, they have to convert CephBool arguments into a similar CephChoices argument that older clients will understand. This involves propagating feature flags into some code paths that couldn't see them before. Signed-off-by: John Spray <john.spray@redhat.com>

Signed-off-by: John Spray <john.spray@redhat.com>

liewegas · 2018-11-06T21:48:00Z

retest this please

Signed-off-by: John Spray <john.spray@redhat.com>

jcsp · 2018-11-07T12:09:14Z

teuthology run here: http://pulpito.ceph.com/jspray-2018-11-02_14:40:18-rados-wip-kwargs-distro-basic-smithi/

test_python.sh failure was because of a lingering --yes-i-really-mean-it in the application_enable path through librados. I've updated it, but that is also going to show up for anyone who is using an old librados with a newer Ceph cluster -- we shouldn't really have been hardcoding that CLI syntax into librados :-/

Other librados test failures were from old style --yes-i-really-mean-it instances in the test code.

liewegas · 2018-11-07T12:56:54Z

retest this please

liewegas · 2018-11-07T13:02:35Z

src/mgr/MgrCommands.h

-        "name=sure,type=CephString,req=False",
+	"name=force,type=CephBool,req=false "
+        // backward compat synonym for --force
+	"name=yes_i_really_mean_it,type=CephBool,req=false", \


What if, instead, we simply s/sure/force/, and s/CephString/CephBool/. Then the compat code would only need to rewrite CephBool to CephString in the general case, and the code for each of these commands in the mon would need to tolerate getting either a boolean or a "--yes-i-really-mean-it" style string (it already does the latter). I think that would make it so that the librados test case code would only need s/sure/force/? And we would end up with only one field defined here in MonCommands. We would lose the CephChoices that would tell you the magic force argument on older clients in ceph -h, but that is small and maybe even a good thing anyway?

Oh, I see... this way lets you still do --yes-i-really-mean-it on the new code while also making it a non-positional flag.

In that case, I think the checks in the mon need to be adjusted so that they tolerate a string being passed for the compat field. Probably a helper like bool cmdmap_compat_confirm(cmdmap, "force", "yes_i_really_mean_it") or something would make this easier and capture all of the compat hackery.

This particular command definition already had both --force and --yes-i-really-mean-it, the two lines here are just synonym flags rather than anything related to CephBool vs. CephChoices

jcsp · 2018-11-08T11:58:35Z

Rerun with fixes: http://pulpito.ceph.com/jspray-2018-11-08_09:46:03-rados-wip-kwargs-distro-basic-smithi/

Looks good so far (failures in rados/test_librados_build.sh and rados/test_envlibrados_for_rocksdb.sh are unrelated to this)

tchaikov · 2018-11-08T15:08:27Z

FWIW, #24896 is supposed to address rados/test_librados_build.sh and rados/test_envlibrados_for_rocksdb.sh failures.

* refs/pull/24333/head: test: update librados tests for CLI arg syntax librados: update for CLI arg format change pybind: update python callers of force flags mon: convert remaining confirmation flags to CephBool ceph_argparse: introduce CephBool arguments test: remove quirky argparse case mgr,mon: use CephChoices for confirmation flags test: add cases for CLI's --key=val style mon: remove dead "cluster_snap" command pybind: enable --keyword=arguments in ceph_argparse ceph.in: some cleanups ceph.in: misc cleanups common: remove unused 'avail' field from commands mon: fix help string for `osd crush rule create-replicated` Reviewed-by: Sage Weil <sage@redhat.com>

jcsp requested review from liewegas and dmick September 28, 2018 14:54

jcsp added core pybind labels Sep 28, 2018

dmick reviewed Oct 5, 2018

View reviewed changes

dmick reviewed Oct 6, 2018

View reviewed changes

src/pybind/ceph_argparse.py Show resolved Hide resolved

jcsp force-pushed the wip-kwargs branch from b37d3fc to 54980ff Compare October 10, 2018 14:05

jcsp changed the title ~~[RFC] ceph.in: clean up and enable --foo=bar style arguments~~ ceph.in: clean up and enable --foo=bar style arguments Oct 10, 2018

jcsp force-pushed the wip-kwargs branch 5 times, most recently from 7e9c4ed to fe7d4ef Compare October 17, 2018 11:33

jcsp force-pushed the wip-kwargs branch from fe7d4ef to ea79de1 Compare October 17, 2018 19:21

liewegas requested a review from dmick October 19, 2018 13:58

liewegas reviewed Oct 19, 2018

View reviewed changes

liewegas added the needs-qa label Oct 22, 2018

jcsp force-pushed the wip-kwargs branch from ea79de1 to 62de958 Compare October 24, 2018 10:56

tchaikov added the wip-kefu-testing label Oct 25, 2018

tchaikov removed the wip-kefu-testing label Oct 29, 2018

liewegas added wip-sage2-testing and removed wip-sage2-testing labels Oct 30, 2018

jcsp force-pushed the wip-kwargs branch 2 times, most recently from 681da6e to 55e44b1 Compare November 2, 2018 00:05

John Spray added 12 commits November 2, 2018 06:57

mon: fix help string for osd crush rule create-replicated

f82372f

Signed-off-by: John Spray <john.spray@redhat.com>

ceph.in: misc cleanups

917fc3d

Get this pyflakes-clean ahead of making changes for keyword arguments. Signed-off-by: John Spray <john.spray@redhat.com>

ceph.in: some cleanups

bd548c0

Add some docstrings to functions, remove some dead code and places where e.g. dicts were used unncessary. Signed-off-by: John Spray <john.spray@redhat.com>

test: add cases for CLI's --key=val style

61d2ad0

Signed-off-by: John Spray <john.spray@redhat.com>

mgr,mon: use CephChoices for confirmation flags

0da65ed

...so that arg parsing can rely on strings satisfying a "doesn't start with --" convention. Signed-off-by: John Spray <john.spray@redhat.com>

mon: convert remaining confirmation flags to CephBool

98c2511

Signed-off-by: John Spray <john.spray@redhat.com>

pybind: update python callers of force flags

a3e1a3e

Signed-off-by: John Spray <john.spray@redhat.com>

jcsp force-pushed the wip-kwargs branch from 55e44b1 to a3e1a3e Compare November 2, 2018 10:58

liewegas mentioned this pull request Nov 6, 2018

[RFC] mgr: poolsets module #24178

Closed

2 tasks

liewegas added the wip-sage2-testing label Nov 6, 2018

John Spray added 2 commits November 7, 2018 07:02

librados: update for CLI arg format change

e95eb0e

Signed-off-by: John Spray <john.spray@redhat.com>

test: update librados tests for CLI arg syntax

4df08eb

Signed-off-by: John Spray <john.spray@redhat.com>

liewegas reviewed Nov 7, 2018

View reviewed changes

liewegas approved these changes Nov 7, 2018

View reviewed changes

liewegas merged commit 4df08eb into ceph:master Nov 8, 2018

jcsp deleted the wip-kwargs branch November 9, 2018 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ceph.in: clean up and enable --foo=bar style arguments #24333

ceph.in: clean up and enable --foo=bar style arguments #24333

jcsp commented Sep 28, 2018

dmick Oct 5, 2018

jcsp Oct 9, 2018

liewegas commented Oct 5, 2018

dmick Oct 6, 2018

jcsp Oct 9, 2018

dmick Oct 13, 2018

dmick Oct 13, 2018

dmick Oct 13, 2018

jcsp Oct 15, 2018

dmick Oct 15, 2018

dmick Oct 6, 2018

jcsp Oct 9, 2018

jcsp commented Oct 10, 2018

jcsp commented Oct 10, 2018

liewegas commented Oct 17, 2018

jcsp commented Oct 18, 2018

liewegas Oct 19, 2018

jcsp Oct 22, 2018

jcsp Oct 22, 2018

jcsp Oct 22, 2018

liewegas Oct 22, 2018

tchaikov commented Oct 29, 2018

jcsp commented Nov 2, 2018

liewegas commented Nov 6, 2018

jcsp commented Nov 7, 2018

liewegas commented Nov 7, 2018

liewegas Nov 7, 2018

liewegas Nov 7, 2018

jcsp Nov 7, 2018

jcsp commented Nov 8, 2018

tchaikov commented Nov 8, 2018

ceph.in: clean up and enable --foo=bar style arguments #24333

ceph.in: clean up and enable --foo=bar style arguments #24333

Conversation

jcsp commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Oct 5, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp commented Oct 10, 2018

jcsp commented Oct 10, 2018

liewegas commented Oct 17, 2018

jcsp commented Oct 18, 2018

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

tchaikov commented Oct 29, 2018

jcsp commented Nov 2, 2018

liewegas commented Nov 6, 2018

jcsp commented Nov 7, 2018

liewegas commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcsp commented Nov 8, 2018

tchaikov commented Nov 8, 2018