Merge PR #41509 into master

* refs/pull/41509/head:
	common/cmdparse: fix CephBool validation for tell commands
	mgr/nfs: fix 'nfs export create' argument order
	common/cmdparse: emit proper json
	mon/MonCommands: add -- seperator to example
	qa/tasks/cephfs/test_nfs: fix export create test
	mgr: make mgr commands compat with pre-quincy mon
	doc/_ext/ceph_commands: handle non-positional args in docs
	mgr: fix reweight-by-utilization cephbool flag
	mon/MonCommands: convert some CephChoices to CephBool
	mgr/k8sevents: fix help strings
	pybind/mgr/mgr_module: fix help desc formatting
	mgr/orchestrator: clean up 'orch {daemon add,apply} rgw' args
	mgr/orchestrator: add end_positional to a few methods
	mgr/orchestrator: reformat a few methods
	pybind/ceph_argparse: stop parsing when we run out of positional args
	pybind/ceph_argparse: remove dead code
	pybind/mgr/mgr_module: infer non-positional args
	pybind/mgr/mgr_module: add separator for non-positional args
	command/cmdparse: use -- to separate positional from non-positional args
	pybind/ceph_argparse: adjust help text for non-positional args
	pybind/ceph_argparse: track a 'positional' property on cli args

Reviewed-by: Kefu Chai <kchai@redhat.com>

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 }}

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

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,8 +175,20 @@ 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) {
-      f->dump_string(key, value);
+      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->close_section(); // attribute object for individual desc
   }
@@ -550,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<bool is_vector,
 	 typename T,
 	 typename Value = std::conditional_t<is_vector,
@@ -629,6 +672,9 @@ bool validate_cmd(CephContext* cct,
       } else if (type == "CephFloat") {
 	return !validate_arg<false, double>(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<false, string>(cct, cmdmap, arg_desc,
 					    name, type, os);
@@ -671,4 +717,23 @@ bool cmd_getval(const cmdmap_t& cmdmap,
   }
 }
 
+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;
+  }
+}
+
 }

src/common/cmdparse.h

@@ -62,6 +62,10 @@ struct bad_cmd_get : public std::exception {
 bool cmd_getval(const cmdmap_t& cmdmap,
 		std::string_view k, bool& val);
 
+bool cmd_getval_compat_cephbool(
+  const cmdmap_t& cmdmap,
+  const std::string& k, bool& val);
+
 template <typename T>
 bool cmd_getval(const cmdmap_t& cmdmap,
 		std::string_view k, T& val)

src/mgr/DaemonServer.cc

@@ -1442,7 +1442,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<int32_t, uint32_t> new_weights;
     r = cluster_state.with_osdmap_and_pgmap([&](const OSDMap &osdmap, const PGMap& pgmap) {

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 " \
@@ -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",

src/mgr/MgrStandby.cc

@@ -12,6 +12,7 @@
  */
 
 #include <Python.h>
+#include <boost/algorithm/string/replace.hpp>
 
 #include "common/errno.h"
 #include "common/signal.h"
@@ -258,6 +259,12 @@ void MgrStandby::send_beacon()
       std::vector<MonCommand> commands = mgr_commands;
       std::vector<MonCommand> 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;

src/mon/MgrMonitor.cc

@@ -1184,18 +1184,18 @@ 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;
       goto out;
     }
 
     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)";

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 <name> 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
@@ -474,7 +482,7 @@ COMMAND_WITH_FLAG("mon remove "
 	"remove monitor named <name>", "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 +758,7 @@ COMMAND("osd crush rule rename "
         "rename crush rule <srcname> to <dstname>",
         "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 +1174,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 <tierpool> (the second one) to base pool <pool> (the first one)",
 	"osd", "rw")
 COMMAND("osd tier rm "
@@ -1256,7 +1264,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",
@@ -1286,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",
@@ -1302,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 "
@@ -1352,7 +1360,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))

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.";

src/mon/MonmapMonitor.cc

@@ -395,11 +395,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;
 

src/mon/OSDMonitor.cc

@@ -6652,9 +6652,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<Formatter> f(Formatter::create(format));
     if (f) {
       f->open_object_section("crush_tree");
@@ -12951,11 +12956,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;
@@ -12967,8 +12972,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;