From 165987c4c9028136c6e41e43e0198091913c4366 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 May 2021 18:00:15 -0400 Subject: [PATCH 01/21] pybind/ceph_argparse: track a 'positional' property on cli args Signed-off-by: Sage Weil --- src/pybind/ceph_argparse.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index f58fca3afd800..77e31e228602d 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -168,32 +168,34 @@ def complete(self, s): return [] @staticmethod - def _compound_type_to_argdesc(tp, attrs): + def _compound_type_to_argdesc(tp, attrs, positional): # generate argdesc from Sequence[T], Tuple[T,..] and Optional[T] orig_type = get_origin(tp) type_args = get_args(tp) if orig_type in (abc.Sequence, Sequence, List, list): assert len(type_args) == 1 attrs['n'] = 'N' - return CephArgtype.to_argdesc(type_args[0], attrs) + return CephArgtype.to_argdesc(type_args[0], attrs, positional=positional) elif orig_type is Tuple: assert len(type_args) >= 1 inner_tp = type_args[0] assert type_args.count(inner_tp) == len(type_args), \ f'all elements in {tp} should be identical' attrs['n'] = str(len(type_args)) - return CephArgtype.to_argdesc(inner_tp, attrs) + return CephArgtype.to_argdesc(inner_tp, attrs, positional=positional) elif get_origin(tp) is Union: # should be Union[t, NoneType] assert len(type_args) == 2 and isinstance(None, type_args[1]) - return CephArgtype.to_argdesc(type_args[0], attrs, True) + return CephArgtype.to_argdesc(type_args[0], attrs, True, positional) else: raise ValueError(f"unknown type '{tp}': '{attrs}'") @staticmethod - def to_argdesc(tp, attrs, has_default=False): + def to_argdesc(tp, attrs, has_default=False, positional=True): if has_default: attrs['req'] = 'false' + if not positional: + attrs['positional'] = 'false' CEPH_ARG_TYPES = { str: CephString, int: CephInt, @@ -208,7 +210,7 @@ def to_argdesc(tp, attrs, has_default=False): elif isinstance(tp, type) and issubclass(tp, enum.Enum): return CephChoices(tp=tp).argdesc(attrs) else: - return CephArgtype._compound_type_to_argdesc(tp, attrs) + return CephArgtype._compound_type_to_argdesc(tp, attrs, positional) def argdesc(self, attrs): attrs['type'] = type(self).__name__ @@ -758,7 +760,8 @@ def complete(self, s): class argdesc(object): """ argdesc(typename, name='name', n=numallowed|N, - req=False, helptext=helptext, **kwargs (type-specific)) + req=False, positional=True, + helptext=helptext, **kwargs (type-specific)) validation rules: typename: type(**kwargs) will be constructed @@ -767,6 +770,7 @@ class argdesc(object): name is used for parse errors and for constructing JSON output n is a numeric literal or 'n|N', meaning "at least one, but maybe more" req=False means the argument need not be present in the list + positional=False means the argument name must be specified, e.g. "--myoption value" helptext is the associated help for the command anything else are arguments to pass to the type constructor. @@ -775,15 +779,19 @@ class argdesc(object): valid() will later be called with input to validate against it, and will store the validated value in self.instance.val for extraction. """ - def __init__(self, t, name=None, n=1, req=True, **kwargs): + def __init__(self, t, name=None, n=1, req=True, positional=True, **kwargs): if isinstance(t, basestring): self.t = CephPrefix self.typeargs = {'prefix': t} self.req = True + self.positional = True else: self.t = t self.typeargs = kwargs self.req = req in (True, 'True', 'true') + self.positional = positional in (True, 'True', 'true') + if not positional: + assert not req self.name = name self.N = (n in ['n', 'N']) @@ -917,12 +925,13 @@ def parse_funcsig(sig: Sequence[Union[str, Dict[str, str]]]) -> List[argdesc]: kwargs = dict() for key, val in desc.items(): - if key not in ['type', 'name', 'n', 'req']: + if key not in ['type', 'name', 'n', 'req', 'positional']: kwargs[key] = val newsig.append(argdesc(t, name=desc.get('name', None), n=desc.get('n', 1), req=desc.get('req', True), + positional=desc.get('positional', True), **kwargs)) return newsig From b9a2a71402921df762a828373d99761c28e5320e Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 May 2021 18:01:03 -0400 Subject: [PATCH 02/21] pybind/ceph_argparse: adjust help text for non-positional args If an arg is non-positional, always show it as [--arg-name ] (All non-positional args are optional.) Signed-off-by: Sage Weil --- src/pybind/ceph_argparse.py | 79 +++++++++++++++++++++++++------------ 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index 77e31e228602d..c5b813ddcaa97 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -836,34 +836,61 @@ def helpstr(self): like str(), but omit parameter names (except for CephString, which really needs them) """ - if self.t == CephBool: - chunk = "--{0}".format(self.name.replace("_", "-")) - elif self.t == CephPrefix: - chunk = str(self.instance) - elif self.t == CephChoices: - if self.name == 'format': - chunk = f'--{self.name} {{{str(self.instance)}}}' - else: + if self.positional: + if self.t == CephBool: + chunk = "--{0}".format(self.name.replace("_", "-")) + elif self.t == CephPrefix: chunk = str(self.instance) - elif self.t == CephOsdName: - # it just so happens all CephOsdName commands are named 'id' anyway, - # so is perfect. - chunk = '' - elif self.t == CephName: - # CephName commands similarly only have one arg of the - # type, so is good. - chunk = '' - elif self.t == CephInt: - chunk = '<{0}:int>'.format(self.name) - elif self.t == CephFloat: - chunk = '<{0}:float>'.format(self.name) + elif self.t == CephChoices: + if self.name == 'format': + # this is for talking to legacy clusters only; new clusters + # should properly mark format args as non-positional. + chunk = f'--{self.name} {{{str(self.instance)}}}' + else: + chunk = f'<{self.name}:{self.instance}>' + elif self.t == CephOsdName: + # it just so happens all CephOsdName commands are named 'id' anyway, + # so is perfect. + chunk = '' + elif self.t == CephName: + # CephName commands similarly only have one arg of the + # type, so is good. + chunk = '' + elif self.t == CephInt: + chunk = '<{0}:int>'.format(self.name) + elif self.t == CephFloat: + chunk = '<{0}:float>'.format(self.name) + else: + chunk = '<{0}>'.format(self.name) + s = chunk + if self.N: + s += '...' + if not self.req: + s = '[' + s + ']' else: - chunk = '<{0}>'.format(self.name) - s = chunk - if self.N: - s += '...' - if not self.req: - s = '[' + s + ']' + # non-positional + if self.t == CephBool: + chunk = "--{0}".format(self.name.replace("_", "-")) + elif self.t == CephPrefix: + chunk = str(self.instance) + elif self.t == CephChoices: + chunk = f'--{self.name} {{{str(self.instance)}}}' + elif self.t == CephOsdName: + chunk = f'--{self.name} ' + elif self.t == CephName: + chunk = f'--{self.name} ' + elif self.t == CephInt: + chunk = f'--{self.name} ' + elif self.t == CephFloat: + chunk = f'--{self.name} ' + else: + chunk = f'--{self.name} ' + s = chunk + if self.N: + s += '...' + if not self.req: # req should *always* be false + s = '[' + s + ']' + return s def complete(self, s): From c441d35bc19aa876e3b87770015f5606e65c0ef0 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 May 2021 18:01:35 -0400 Subject: [PATCH 03/21] command/cmdparse: use -- to separate positional from non-positional args In a command definition, separate the non-positional args with "--". Signed-off-by: Sage Weil --- src/common/cmdparse.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/common/cmdparse.cc b/src/common/cmdparse.cc index f1756a292a521..03f634123eb16 100644 --- a/src/common/cmdparse.cc +++ b/src/common/cmdparse.cc @@ -138,13 +138,20 @@ dump_cmd_to_json(Formatter *f, uint64_t features, const string& cmd) stringstream ss(cmd); std::string word; + bool positional = true; while (std::getline(ss, word, ' ')) { + if (word == "--") { + positional = false; + continue; + } + // if no , or =, must be a plain word to put out if (word.find_first_of(",=") == string::npos) { f->dump_string("arg", word); continue; } + // accumulate descriptor keywords in desckv auto desckv = cmddesc_get_args(word); // name the individual desc object based on the name key @@ -168,7 +175,13 @@ dump_cmd_to_json(Formatter *f, uint64_t features, const string& cmd) } // dump all the keys including name into the array + if (!positional) { + desckv["positional"] = "false"; + } for (auto [key, value] : desckv) { + if (key == "positional" && !HAVE_FEATURE(features, SERVER_QUINCY)) { + continue; + } f->dump_string(key, value); } f->close_section(); // attribute object for individual desc From bcd821e1b0a35e9eb1c055e2ece8431d44915a08 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 May 2021 18:02:10 -0400 Subject: [PATCH 04/21] pybind/mgr/mgr_module: add separator for non-positional args Signed-off-by: Sage Weil --- src/pybind/mgr/mgr_module.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/pybind/mgr/mgr_module.py b/src/pybind/mgr/mgr_module.py index 8c7e4710c64f0..f78b45493b893 100644 --- a/src/pybind/mgr/mgr_module.py +++ b/src/pybind/mgr/mgr_module.py @@ -324,15 +324,20 @@ def load_func_metadata(f: HandlerFuncType) -> Tuple[str, Dict[str, Any], int, st if full_argspec.defaults: first_default -= len(full_argspec.defaults) args = [] + positional = True for index, arg in enumerate(full_argspec.args): if arg in CLICommand.KNOWN_ARGS: continue + if arg == '_end_positional_': + positional = False + continue assert arg in arg_spec, \ f"'{arg}' is not annotated for {f}: {full_argspec}" has_default = index >= first_default args.append(CephArgtype.to_argdesc(arg_spec[arg], dict(name=arg), - has_default)) + has_default, + positional)) return desc, arg_spec, first_default, ' '.join(args) def store_func_metadata(self, f: HandlerFuncType) -> None: From d2e869353e8fbc456bb57a611b7fa42ad705346e Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 21 May 2021 18:02:42 -0400 Subject: [PATCH 05/21] pybind/mgr/mgr_module: infer non-positional args Once we have an Optional[bool], we can always transition to non-positional, since we never have a non-optional bool. Same goes for the 'format' arg. Signed-off-by: Sage Weil --- src/pybind/mgr/mgr_module.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/pybind/mgr/mgr_module.py b/src/pybind/mgr/mgr_module.py index f78b45493b893..42469fc9a61e2 100644 --- a/src/pybind/mgr/mgr_module.py +++ b/src/pybind/mgr/mgr_module.py @@ -331,6 +331,14 @@ def load_func_metadata(f: HandlerFuncType) -> Tuple[str, Dict[str, Any], int, st if arg == '_end_positional_': positional = False continue + if ( + arg == 'format' + or arg_spec[arg] is Optional[bool] + or arg_spec[arg] is bool + ): + # implicit switch to non-positional on any + # Optional[bool] or the --format option + positional = False assert arg in arg_spec, \ f"'{arg}' is not annotated for {f}: {full_argspec}" has_default = index >= first_default From 747eb7d1429a4903892069387588ed70f518503f Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 12:59:14 -0400 Subject: [PATCH 06/21] pybind/ceph_argparse: remove dead code Signed-off-by: Sage Weil --- src/pybind/ceph_argparse.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index c5b813ddcaa97..e401ef0c3428b 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -1159,10 +1159,6 @@ def validate(args: List[str], # argdesc for the keyword argument, if we find one kwarg_desc = None - # Track whether we need to push value back onto - # myargs in the case that this isn't a valid k=v - consumed_next = False - # Try both styles of keyword argument kwarg_match = re.match(KWARG_EQUALS, myarg) if kwarg_match: From fb80427ec1cc8ba61fe75bb45819baa857674ee6 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 13:00:24 -0400 Subject: [PATCH 07/21] pybind/ceph_argparse: stop parsing when we run out of positional args If we encouter an arg that is not a named flag/arg, and the next item in the command description is non-positional, then raise an 'unexpected argument' exception. Signed-off-by: Sage Weil --- src/pybind/ceph_argparse.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index e401ef0c3428b..dd70156c97ec3 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -1196,6 +1196,10 @@ def validate(args: List[str], store_arg(kwarg_desc, d) continue + if not desc.positional: + # No more positional args! + raise ArgumentValid(f"Unexpected argument '{myarg}'") + # Don't handle something as a positional argument if it # has a leading "--" unless it's a CephChoices (used for # "--yes-i-really-mean-it") From 27c2b83a8e4c7efd319fcd6c1b80698f76dea6c4 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 13:21:04 -0400 Subject: [PATCH 08/21] mgr/orchestrator: reformat a few methods Signed-off-by: Sage Weil --- src/pybind/mgr/orchestrator/module.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index 90d540a398df0..866d34494d859 100644 --- a/src/pybind/mgr/orchestrator/module.py +++ b/src/pybind/mgr/orchestrator/module.py @@ -329,7 +329,11 @@ def _select_orchestrator(self) -> str: return cast(str, self.get_module_option("orchestrator")) @_cli_write_command('orch host add') - def _add_host(self, hostname: str, addr: Optional[str] = None, labels: Optional[List[str]] = None, maintenance: Optional[bool] = False) -> HandleCommandResult: + def _add_host(self, + hostname: str, + addr: Optional[str] = None, + labels: Optional[List[str]] = None, + maintenance: Optional[bool] = False) -> HandleCommandResult: """Add a host""" _status = 'maintenance' if maintenance else '' @@ -970,7 +974,9 @@ def _daemon_action(self, action: DaemonAction, name: str) -> HandleCommandResult return HandleCommandResult(stdout=completion.result_str()) @_cli_write_command('orch daemon redeploy') - def _daemon_action_redeploy(self, name: str, image: Optional[str] = None) -> HandleCommandResult: + def _daemon_action_redeploy(self, + name: str, + image: Optional[str] = None) -> HandleCommandResult: """Redeploy a daemon (with a specifc image)""" if '.' not in name: raise OrchestratorError('%s is not a valid daemon name' % name) From b64e6dda58ee4b80a3fc60617d540458e8a8343e Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 13:26:42 -0400 Subject: [PATCH 09/21] mgr/orchestrator: add end_positional to a few methods Signed-off-by: Sage Weil --- src/pybind/mgr/orchestrator/module.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index 866d34494d859..faa4671707e27 100644 --- a/src/pybind/mgr/orchestrator/module.py +++ b/src/pybind/mgr/orchestrator/module.py @@ -611,6 +611,7 @@ def ukn(s: Optional[str]) -> str: @_cli_read_command('orch ps') def _list_daemons(self, hostname: Optional[str] = None, + _end_positional_: int = 0, service_name: Optional[str] = None, daemon_type: Optional[str] = None, daemon_id: Optional[str] = None, @@ -1342,6 +1343,7 @@ def _upgrade_status(self) -> HandleCommandResult: @_cli_write_command('orch upgrade start') def _upgrade_start(self, image: Optional[str] = None, + _end_positional_: int = 0, ceph_version: Optional[str] = None) -> HandleCommandResult: """Initiate upgrade""" self._upgrade_check_image_name(image, ceph_version) From 5d8cfb149b34d068b4dd6942e9a38e40aced299f Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 13:28:37 -0400 Subject: [PATCH 10/21] mgr/orchestrator: clean up 'orch {daemon add,apply} rgw' args Make placement the only optional positional. This means that the "usual" 'orch orch apply 3' will do placement=3. Signed-off-by: Sage Weil --- src/pybind/mgr/orchestrator/module.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/pybind/mgr/orchestrator/module.py b/src/pybind/mgr/orchestrator/module.py index faa4671707e27..f3c8d2ccf92b2 100644 --- a/src/pybind/mgr/orchestrator/module.py +++ b/src/pybind/mgr/orchestrator/module.py @@ -901,9 +901,10 @@ def _mds_add(self, @_cli_write_command('orch daemon add rgw') def _rgw_add(self, svc_id: str, + placement: Optional[str] = None, + _end_positional_: int = 0, port: Optional[int] = None, ssl: bool = False, - placement: Optional[str] = None, inbuf: Optional[str] = None) -> HandleCommandResult: """Start RGW daemon(s)""" if inbuf: @@ -1091,11 +1092,12 @@ def _apply_mds(self, @_cli_write_command('orch apply rgw') def _apply_rgw(self, svc_id: str, + placement: Optional[str] = None, + _end_positional_: int = 0, realm: Optional[str] = None, zone: Optional[str] = None, port: Optional[int] = None, ssl: bool = False, - placement: Optional[str] = None, dry_run: bool = False, format: Format = Format.plain, unmanaged: bool = False, From 4bc37ba3b53930589f2fcd3f137b45ea9872f9e6 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 22 May 2021 13:33:40 -0400 Subject: [PATCH 11/21] pybind/mgr/mgr_module: fix help desc formatting Signed-off-by: Sage Weil --- src/pybind/mgr/mgr_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pybind/mgr/mgr_module.py b/src/pybind/mgr/mgr_module.py index 42469fc9a61e2..c89511dfc7d9d 100644 --- a/src/pybind/mgr/mgr_module.py +++ b/src/pybind/mgr/mgr_module.py @@ -317,7 +317,7 @@ def __init__(self, @staticmethod def load_func_metadata(f: HandlerFuncType) -> Tuple[str, Dict[str, Any], int, str]: - desc = inspect.getdoc(f) or '' + desc = (inspect.getdoc(f) or '').replace('\n', ' ') full_argspec = inspect.getfullargspec(f) arg_spec = full_argspec.annotations first_default = len(arg_spec) From fd77020e73bb43f254ec8a4ee7093dccd1b63ff9 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sun, 23 May 2021 10:34:26 -0400 Subject: [PATCH 12/21] mgr/k8sevents: fix help strings Signed-off-by: Sage Weil --- src/pybind/mgr/k8sevents/module.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pybind/mgr/k8sevents/module.py b/src/pybind/mgr/k8sevents/module.py index afdd5ae77b9a1..1e12d1f27a25c 100644 --- a/src/pybind/mgr/k8sevents/module.py +++ b/src/pybind/mgr/k8sevents/module.py @@ -1026,12 +1026,12 @@ class Module(MgrModule): }, { "cmd": "k8sevents set-access name=key,type=CephString", - "desc": "Set kubernetes access credentials. must be cacrt or token and use -i syntax.\ne.g. ceph k8sevents set-access cacrt -i /root/ca.crt", + "desc": "Set kubernetes access credentials. must be cacrt or token and use -i syntax (e.g., ceph k8sevents set-access cacrt -i /root/ca.crt).", "perm": "rw" }, { "cmd": "k8sevents set-config name=key,type=CephString name=value,type=CephString", - "desc": "Set kubernetes config paramters. must be server or namespace.\ne.g. ceph k8sevents set-config server https://localhost:30433", + "desc": "Set kubernetes config paramters. must be server or namespace (e.g., ceph k8sevents set-config server https://localhost:30433).", "perm": "rw" }, { From bd242c4997cd934c8f11022ec362d143c314e4fa Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sun, 23 May 2021 10:47:35 -0400 Subject: [PATCH 13/21] mon/MonCommands: convert some CephChoices to CephBool These are old bool options that never got converted to CephBool. Signed-off-by: Sage Weil --- src/common/cmdparse.cc | 19 +++++++++++++++++++ src/common/cmdparse.h | 4 ++++ src/mon/MgrMonitor.cc | 8 ++++---- src/mon/MonCommands.h | 10 +++++----- src/mon/Monitor.cc | 12 +++++++++--- src/mon/MonmapMonitor.cc | 6 +----- src/mon/OSDMonitor.cc | 21 +++++++++++++-------- 7 files changed, 55 insertions(+), 25 deletions(-) diff --git a/src/common/cmdparse.cc b/src/common/cmdparse.cc index 03f634123eb16..9b296f8fbf4e0 100644 --- a/src/common/cmdparse.cc +++ b/src/common/cmdparse.cc @@ -682,4 +682,23 @@ bool cmd_getval(const cmdmap_t& cmdmap, return false; } +bool cmd_getval_compat_cephbool( + const cmdmap_t& cmdmap, + const std::string& k, bool& val) +{ + try { + return cmd_getval(cmdmap, k, val); + } catch (bad_cmd_get& e) { + // try as legacy/compat CephChoices + std::string t; + if (!cmd_getval(cmdmap, k, t)) { + return false; + } + std::string expected = "--"s + k; + std::replace(expected.begin(), expected.end(), '_', '-'); + val = (t == expected); + return true; + } +} + } diff --git a/src/common/cmdparse.h b/src/common/cmdparse.h index 112c249305321..4d5160d3333b2 100644 --- a/src/common/cmdparse.h +++ b/src/common/cmdparse.h @@ -59,6 +59,10 @@ struct bad_cmd_get : public std::exception { bool cmd_getval(const cmdmap_t& cmdmap, const std::string& k, bool& val); +bool cmd_getval_compat_cephbool( + const cmdmap_t& cmdmap, + const std::string& k, bool& val); + template bool cmd_getval(const cmdmap_t& cmdmap, const std::string& k, T& val) diff --git a/src/mon/MgrMonitor.cc b/src/mon/MgrMonitor.cc index 4c9cfb6efd9db..f3647ca29c5d1 100644 --- a/src/mon/MgrMonitor.cc +++ b/src/mon/MgrMonitor.cc @@ -1186,10 +1186,10 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op) ss << "module '" << module << "' is already enabled (always-on)"; goto out; } - string force; - cmd_getval(cmdmap, "force", force); + bool force = false; + cmd_getval_compat_cephbool(cmdmap, "force", force); if (!pending_map.all_support_module(module) && - force != "--force") { + !force) { ss << "all mgr daemons do not support module '" << module << "', pass " << "--force to force enablement"; r = -ENOENT; @@ -1197,7 +1197,7 @@ bool MgrMonitor::prepare_command(MonOpRequestRef op) } std::string can_run_error; - if (force != "--force" && !pending_map.can_run_module(module, &can_run_error)) { + if (!force && !pending_map.can_run_module(module, &can_run_error)) { ss << "module '" << module << "' reports that it cannot run on the active " "manager daemon: " << can_run_error << " (pass --force to force " "enablement)"; diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h index e10a4129f085b..2ca0d0e8f1c40 100644 --- a/src/mon/MonCommands.h +++ b/src/mon/MonCommands.h @@ -474,7 +474,7 @@ COMMAND_WITH_FLAG("mon remove " "remove monitor named ", "mon", "rw", FLAG(DEPRECATED)) COMMAND("mon feature ls " - "name=with_value,type=CephChoices,strings=--with-value,req=false", + "name=with_value,type=CephBool,req=false", "list available mon map features to be set/unset", "mon", "r") COMMAND("mon feature set " @@ -750,7 +750,7 @@ COMMAND("osd crush rule rename " "rename crush rule to ", "osd", "rw") COMMAND("osd crush tree " - "name=shadow,type=CephChoices,strings=--show-shadow,req=false", + "name=show_shadow,type=CephBool,req=false", "dump crush buckets and items in a tree view", "osd", "r") COMMAND("osd crush ls name=node,type=CephString,goodchars=[A-Za-z0-9-_.]", @@ -1166,7 +1166,7 @@ COMMAND("osd force_recovery_stretch_mode " \ COMMAND("osd tier add " "name=pool,type=CephPoolname " "name=tierpool,type=CephPoolname " - "name=force_nonempty,type=CephChoices,strings=--force-nonempty,req=false", + "name=force_nonempty,type=CephBool,req=false", "add the tier (the second one) to base pool (the first one)", "osd", "rw") COMMAND("osd tier rm " @@ -1256,7 +1256,7 @@ COMMAND("mgr services", "mgr", "r") COMMAND("mgr module enable " "name=module,type=CephString " - "name=force,type=CephChoices,strings=--force,req=false", + "name=force,type=CephBool,req=false", "enable mgr module", "mgr", "rw") COMMAND("mgr module disable " "name=module,type=CephString", @@ -1352,7 +1352,7 @@ COMMAND_WITH_FLAG("connection scores reset", "mon", "rwx", FLAG(TELL)) COMMAND_WITH_FLAG("sync_force " - "name=validate,type=CephChoices,strings=--yes-i-really-mean-it,req=false", + "name=yes_i_really_mean_it,type=CephBool,req=false", "force sync of and clear monitor store", "mon", "rw", FLAG(TELL)) diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 5f02325bb089d..e0fb3786e879d 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -337,9 +337,15 @@ int Monitor::do_admin_command( } else if (command == "quorum_status") { _quorum_status(f, out); } else if (command == "sync_force") { - string validate; - if ((!cmd_getval(cmdmap, "validate", validate)) || - (validate != "--yes-i-really-mean-it")) { + bool validate = false; + if (!cmd_getval(cmdmap, "yes_i_really_mean_it", validate)) { + std::string v; + if (cmd_getval(cmdmap, "validate", v) && + v == "--yes-i-really-mean-it") { + validate = true; + } + } + if (!validate) { err << "are you SURE? this will mean the monitor store will be erased " "the next time the monitor is restarted. pass " "'--yes-i-really-mean-it' if you really do."; diff --git a/src/mon/MonmapMonitor.cc b/src/mon/MonmapMonitor.cc index daeb4b572fd7b..4fd3a94cf7749 100644 --- a/src/mon/MonmapMonitor.cc +++ b/src/mon/MonmapMonitor.cc @@ -397,11 +397,7 @@ bool MonmapMonitor::preprocess_command(MonOpRequestRef op) } else if (prefix == "mon feature ls") { bool list_with_value = false; - string with_value; - if (cmd_getval(cmdmap, "with_value", with_value) && - with_value == "--with-value") { - list_with_value = true; - } + cmd_getval_compat_cephbool(cmdmap, "with_value", list_with_value); MonMap *p = mon.monmap; diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc index 7e8c3450bc60a..d04a8f28d0573 100644 --- a/src/mon/OSDMonitor.cc +++ b/src/mon/OSDMonitor.cc @@ -6656,9 +6656,14 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op) rs << "\n"; rdata.append(rs.str()); } else if (prefix == "osd crush tree") { - string shadow; - cmd_getval(cmdmap, "shadow", shadow); - bool show_shadow = shadow == "--show-shadow"; + bool show_shadow = false; + if (!cmd_getval_compat_cephbool(cmdmap, "show_shadow", show_shadow)) { + std::string shadow; + if (cmd_getval(cmdmap, "shadow", shadow) && + shadow == "--show-shadow") { + show_shadow = true; + } + } boost::scoped_ptr f(Formatter::create(format)); if (f) { f->open_object_section("crush_tree"); @@ -12961,11 +12966,11 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, } // make sure new tier is empty - string force_nonempty; - cmd_getval(cmdmap, "force_nonempty", force_nonempty); + bool force_nonempty = false; + cmd_getval_compat_cephbool(cmdmap, "force_nonempty", force_nonempty); const pool_stat_t *pstats = mon.mgrstatmon()->get_pool_stat(tierpool_id); if (pstats && pstats->stats.sum.num_objects != 0 && - force_nonempty != "--force-nonempty") { + !force_nonempty) { ss << "tier pool '" << tierpoolstr << "' is not empty; --force-nonempty to force"; err = -ENOTEMPTY; goto reply; @@ -12977,8 +12982,8 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, goto reply; } if ((!tp->removed_snaps.empty() || !tp->snaps.empty()) && - ((force_nonempty != "--force-nonempty") || - (!g_conf()->mon_debug_unsafe_allow_tier_with_nonempty_snaps))) { + (!force_nonempty || + !g_conf()->mon_debug_unsafe_allow_tier_with_nonempty_snaps)) { ss << "tier pool '" << tierpoolstr << "' has snapshot state; it cannot be added as a tier without breaking the pool"; err = -ENOTEMPTY; goto reply; From c70a4f583c7bca7d240d13495b8c1571514847f2 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Mon, 24 May 2021 15:17:58 -0400 Subject: [PATCH 14/21] mgr: fix reweight-by-utilization cephbool flag This was broken, since it was trying to get a CephChoices as a bool. Signed-off-by: Sage Weil --- src/mgr/DaemonServer.cc | 2 +- src/mgr/MgrCommands.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index ea41ba0588709..479ea7a80cb09 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -1443,7 +1443,7 @@ bool DaemonServer::_handle_command( return true; } bool no_increasing = false; - cmd_getval(cmdctx->cmdmap, "no_increasing", no_increasing); + cmd_getval_compat_cephbool(cmdctx->cmdmap, "no_increasing", no_increasing); string out_str; mempool::osdmap::map new_weights; r = cluster_state.with_osdmap_and_pgmap([&](const OSDMap &osdmap, const PGMap& pgmap) { diff --git a/src/mgr/MgrCommands.h b/src/mgr/MgrCommands.h index bc3350da448eb..11607446b84bc 100644 --- a/src/mgr/MgrCommands.h +++ b/src/mgr/MgrCommands.h @@ -111,7 +111,7 @@ COMMAND("osd reweight-by-utilization " \ "name=oload,type=CephInt,req=false " \ "name=max_change,type=CephFloat,req=false " \ "name=max_osds,type=CephInt,req=false " \ - "name=no_increasing,type=CephChoices,strings=--no-increasing,req=false",\ + "name=no_increasing,type=CephBool,req=false",\ "reweight OSDs by utilization [overload-percentage-for-consideration, default 120]", \ "osd", "rw") COMMAND("osd test-reweight-by-utilization " \ From 295a5f282710efa7c3815faaa3919f1018d04066 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 25 May 2021 10:53:01 -0400 Subject: [PATCH 15/21] doc/_ext/ceph_commands: handle non-positional args in docs Signed-off-by: Sage Weil --- doc/_ext/ceph_commands.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/_ext/ceph_commands.py b/doc/_ext/ceph_commands.py index eec031ee3d2ab..046a5e09c398c 100644 --- a/doc/_ext/ceph_commands.py +++ b/doc/_ext/ceph_commands.py @@ -82,7 +82,7 @@ class CmdParam(object): def __init__(self, type, name, who=None, n=None, req=True, range=None, strings=None, - goodchars=None): + goodchars=None, positional=True): self.type = type self.name = name self.who = who @@ -91,6 +91,7 @@ def __init__(self, type, name, self.range = range.split('|') if range else [] self.strings = strings.split('|') if strings else [] self.goodchars = goodchars + self.positional = positional != 'false' assert who == None @@ -200,7 +201,7 @@ def parse_args(args): {%- if command.params %} :Parameters:{% for param in command.params -%} -{{" -" | indent(12, not loop.first) }} **{{param.name}}**: {{ param.help() }} +{{" -" | indent(12, not loop.first) }} **{% if param.positional %}{{param.name}}{% else %}--{{param.name}}{% endif %}**: {{ param.help() }} {% endfor %} {% endif %} :Ceph Module: {{ command.module }} From 218f4cefea087eadcd8ddf47d719beda1cb6370b Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 28 May 2021 22:19:27 -0400 Subject: [PATCH 16/21] mgr: make mgr commands compat with pre-quincy mon If the mon is not yet quincy, do not share the quincy-ism of 'positional=false'. Signed-off-by: Sage Weil --- src/mgr/MgrStandby.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/mgr/MgrStandby.cc b/src/mgr/MgrStandby.cc index 1ba6eaea4f61e..14d95ab5158b4 100644 --- a/src/mgr/MgrStandby.cc +++ b/src/mgr/MgrStandby.cc @@ -12,6 +12,7 @@ */ #include +#include #include "common/errno.h" #include "common/signal.h" @@ -258,6 +259,12 @@ void MgrStandby::send_beacon() std::vector commands = mgr_commands; std::vector py_commands = py_module_registry.get_commands(); commands.insert(commands.end(), py_commands.begin(), py_commands.end()); + if (monc.monmap.min_mon_release < ceph_release_t::quincy) { + dout(10) << " stripping out positional=false quincy-ism" << dendl; + for (auto& i : commands) { + boost::replace_all(i.cmdstring, ",positional=false", ""); + } + } m->set_command_descs(commands); dout(4) << "going active, including " << m->get_command_descs().size() << " commands in beacon" << dendl; From 8683cccd06077bfbfd39e9e5ca3750078937dc0e Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 1 Jun 2021 13:39:03 -0400 Subject: [PATCH 17/21] qa/tasks/cephfs/test_nfs: fix export create test Everything after --readonly is non-positional. Signed-off-by: Sage Weil --- qa/tasks/cephfs/test_nfs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qa/tasks/cephfs/test_nfs.py b/qa/tasks/cephfs/test_nfs.py index b6fcd33b492ba..df3756a2198fc 100644 --- a/qa/tasks/cephfs/test_nfs.py +++ b/qa/tasks/cephfs/test_nfs.py @@ -348,7 +348,8 @@ def test_create_multiple_exports(self): # Export-3 for subvolume with r only self._cmd('fs', 'subvolume', 'create', self.fs_name, 'sub_vol') fs_path = self._cmd('fs', 'subvolume', 'getpath', self.fs_name, 'sub_vol').strip() - self._create_export(export_id='3', extra_cmd=[self.pseudo_path+'2', '--readonly', fs_path]) + self._create_export(export_id='3', extra_cmd=[self.pseudo_path+'2', '--readonly', + '--path', fs_path]) # Export-4 for subvolume self._create_export(export_id='4', extra_cmd=[self.pseudo_path+'3', fs_path]) # Check if exports gets listed From b6b5ecc210d7e4044cefd83fb4e59dbe88aaef12 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 1 Jun 2021 14:43:47 -0400 Subject: [PATCH 18/21] mon/MonCommands: add -- seperator to example Signed-off-by: Sage Weil --- src/mon/MonCommands.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h index 2ca0d0e8f1c40..df03ca4a8a8a1 100644 --- a/src/mon/MonCommands.h +++ b/src/mon/MonCommands.h @@ -77,7 +77,9 @@ * * COMMAND("auth add " * "name=entity,type=CephString " - * "name=caps,type=CephString,n=N,req=false", + * "name=caps,type=CephString,n=N,req=false " + * "-- " + * "name=some_option,type=CephString,req=false", * "add auth info for from input file, or random key " * "if no input given, and/or any caps specified in the command") * @@ -88,6 +90,12 @@ * enters auth add client.admin 'mon rwx' 'osd *'. The result will be a * JSON object like {"prefix":"auth add", "entity":"client.admin", * "caps":["mon rwx", "osd *"]}. + * + * The -- separates positional from non-positional (and, by implication, + * optional) arguments. Note that CephBool is assumed to be non-positional + * and will also implicitly mark that any following arguments are + * non-positional. + * * Note that * - string literals are accumulated into 'prefix' * - n=1 descriptors are given normal string or int object values From ec293f34b1ca74243f0af70180d5025e5866f231 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Tue, 1 Jun 2021 16:55:44 -0400 Subject: [PATCH 19/21] common/cmdparse: emit proper json 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 --- src/common/cmdparse.cc | 12 +++++++++--- src/mgr/MgrCommands.h | 4 ++-- src/mon/MonCommands.h | 4 ++-- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/common/cmdparse.cc b/src/common/cmdparse.cc index 9b296f8fbf4e0..7052b235fb995 100644 --- a/src/common/cmdparse.cc +++ b/src/common/cmdparse.cc @@ -179,10 +179,16 @@ dump_cmd_to_json(Formatter *f, uint64_t features, const string& cmd) desckv["positional"] = "false"; } for (auto [key, value] : desckv) { - if (key == "positional" && !HAVE_FEATURE(features, SERVER_QUINCY)) { - continue; + if (key == "positional") { + if (!HAVE_FEATURE(features, SERVER_QUINCY)) { + continue; + } + f->dump_bool(key, value == "true" || value == "True"); + } else if (key == "req" && HAVE_FEATURE(features, SERVER_QUINCY)) { + f->dump_bool(key, value == "true" || value == "True"); + } else { + f->dump_string(key, value); } - f->dump_string(key, value); } f->close_section(); // attribute object for individual desc } diff --git a/src/mgr/MgrCommands.h b/src/mgr/MgrCommands.h index 11607446b84bc..439f07c67019f 100644 --- a/src/mgr/MgrCommands.h +++ b/src/mgr/MgrCommands.h @@ -181,7 +181,7 @@ COMMAND("service status", "dump service state", "service", "r") COMMAND("config show " \ - "name=who,type=CephString name=key,type=CephString,req=False", + "name=who,type=CephString name=key,type=CephString,req=false", "Show running configuration", "mgr", "r") COMMAND("config show-with-defaults " \ @@ -203,7 +203,7 @@ COMMAND("device ls-by-host name=host,type=CephString", "mgr", "r") COMMAND("device set-life-expectancy name=devid,type=CephString "\ "name=from,type=CephString "\ - "name=to,type=CephString,req=False", + "name=to,type=CephString,req=false", "Set predicted device life expectancy", "mgr", "rw") COMMAND("device rm-life-expectancy name=devid,type=CephString", diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h index df03ca4a8a8a1..d2d00ddfc7ccc 100644 --- a/src/mon/MonCommands.h +++ b/src/mon/MonCommands.h @@ -1294,7 +1294,7 @@ COMMAND("config rm" "config", "rw") COMMAND("config get " "name=who,type=CephString " - "name=key,type=CephString,req=False", + "name=key,type=CephString,req=false", "Show configuration option(s) for an entity", "config", "r") COMMAND("config dump", @@ -1310,7 +1310,7 @@ COMMAND("config ls", COMMAND("config assimilate-conf", "Assimilate options from a conf, and return a new, minimal conf file", "config", "rw") -COMMAND("config log name=num,type=CephInt,req=False", +COMMAND("config log name=num,type=CephInt,req=false", "Show recent history of config changes", "config", "r") COMMAND("config reset " From ffce51af8e7be9835ff01c8d9a5f378f19d4d746 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Fri, 4 Jun 2021 13:35:46 -0400 Subject: [PATCH 20/21] mgr/nfs: fix 'nfs export create' argument order Put path before --readonly so that it can still be positional. Signed-off-by: Sage Weil --- src/pybind/mgr/nfs/module.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pybind/mgr/nfs/module.py b/src/pybind/mgr/nfs/module.py index a3d4272c98b9f..06342aae900a1 100644 --- a/src/pybind/mgr/nfs/module.py +++ b/src/pybind/mgr/nfs/module.py @@ -28,8 +28,8 @@ def _cmd_nfs_export_create_cephfs(self, fsname: str, clusterid: str, binding: str, - readonly: bool = False, - path: str = '/') -> Tuple[int, str, str]: + path: str = '/', + readonly: bool = False) -> Tuple[int, str, str]: """Create a cephfs export""" # TODO Extend export creation for rgw. return self.export_mgr.create_export(fsal_type='cephfs', fs_name=fsname, From 6bd4df3573f645163d0f2d4212da9ae265389dd7 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Sat, 5 Jun 2021 11:23:35 -0400 Subject: [PATCH 21/21] common/cmdparse: fix CephBool validation for tell commands The tell/asok validation uses a different validation path; make it work for CephBool arguments. Signed-off-by: Sage Weil --- src/common/cmdparse.cc | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/common/cmdparse.cc b/src/common/cmdparse.cc index 7052b235fb995..9d28a5c57dbf6 100644 --- a/src/common/cmdparse.cc +++ b/src/common/cmdparse.cc @@ -569,6 +569,30 @@ bool validate_str_arg(std::string_view value, } } +bool validate_bool(CephContext *cct, + const cmdmap_t& cmdmap, + const arg_desc_t& desc, + const std::string_view name, + const std::string_view type, + std::ostream& os) +{ + bool v; + try { + if (!cmd_getval(cmdmap, string(name), v)) { + if (auto req = desc.find("req"); + req != end(desc) && req->second == "false") { + return true; + } else { + os << "missing required parameter: '" << name << "'"; + return false; + } + } + return true; + } catch (const bad_cmd_get& e) { + return false; + } +} + template(cct, cmdmap, arg_desc, name, type, os); + } else if (type == "CephBool") { + return !validate_bool(cct, cmdmap, arg_desc, + name, type, os); } else { return !validate_arg(cct, cmdmap, arg_desc, name, type, os);