From: John Spray Date: Wed, 31 Oct 2018 16:59:20 +0000 (-0400) Subject: ceph_argparse: introduce CephBool arguments X-Git-Tag: v14.1.0~969^2~4 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=525623b6cd335ce4dd84879567bde214668c38de;p=ceph.git ceph_argparse: introduce CephBool arguments 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 --- diff --git a/src/common/admin_socket.cc b/src/common/admin_socket.cc index a88ce1a5cee8..c7f21f7b0f20 100644 --- a/src/common/admin_socket.cc +++ b/src/common/admin_socket.cc @@ -566,6 +566,7 @@ public: ostringstream secname; secname << "cmd" << setfill('0') << std::setw(3) << cmdnum; dump_cmd_and_help_to_json(&jf, + CEPH_FEATURES_ALL, secname.str().c_str(), info.desc, info.help); diff --git a/src/common/cmdparse.cc b/src/common/cmdparse.cc index 2e1c2ce21f2b..518112facf4e 100644 --- a/src/common/cmdparse.cc +++ b/src/common/cmdparse.cc @@ -72,7 +72,7 @@ arg_desc_t cmddesc_get_args(const String& cmddesc) */ void -dump_cmd_to_json(Formatter *f, const string& cmd) +dump_cmd_to_json(Formatter *f, uint64_t features, const string& cmd) { // put whole command signature in an already-opened container // elements are: "name", meaning "the typeless name that means a literal" @@ -91,6 +91,23 @@ dump_cmd_to_json(Formatter *f, const string& cmd) auto desckv = cmddesc_get_args(word); // name the individual desc object based on the name key f->open_object_section(string(desckv["name"]).c_str()); + + // Compatibility for pre-nautilus clients that don't know about CephBool + if (!HAVE_FEATURE(features, SERVER_NAUTILUS)) { + auto i = desckv.find("type"); + if (i != desckv.end() && i->second == "CephBool") { + // Instruct legacy clients to send --foo-bar string in place + // of a 'true'/'false' value + std::ostringstream oss; + oss << std::string("--") << desckv["name"]; + std::string val = oss.str(); + std::replace(val.begin(), val.end(), '_', '-'); + + desckv["type"] = "CephChoices"; + desckv["strings"] = val; + } + } + // dump all the keys including name into the array for (auto [key, value] : desckv) { f->dump_string(string(key).c_str(), string(value)); @@ -101,13 +118,14 @@ dump_cmd_to_json(Formatter *f, const string& cmd) void dump_cmd_and_help_to_json(Formatter *jf, + uint64_t features, const string& secname, const string& cmdsig, const string& helptext) { jf->open_object_section(secname.c_str()); jf->open_array_section("sig"); - dump_cmd_to_json(jf, cmdsig); + dump_cmd_to_json(jf, features, cmdsig); jf->close_section(); // sig array jf->dump_string("help", helptext.c_str()); jf->close_section(); // cmd @@ -115,6 +133,7 @@ dump_cmd_and_help_to_json(Formatter *jf, void dump_cmddesc_to_json(Formatter *jf, + uint64_t features, const string& secname, const string& cmdsig, const string& helptext, @@ -124,7 +143,7 @@ dump_cmddesc_to_json(Formatter *jf, { jf->open_object_section(secname.c_str()); jf->open_array_section("sig"); - dump_cmd_to_json(jf, cmdsig); + dump_cmd_to_json(jf, features, cmdsig); jf->close_section(); // sig array jf->dump_string("help", helptext.c_str()); jf->dump_string("module", module.c_str()); @@ -560,3 +579,37 @@ bool validate_cmd(CephContext* cct, } }); } + +bool cmd_getval(CephContext *cct, const cmdmap_t& cmdmap, + const std::string& k, bool& val) +{ + /* + * Specialized getval for booleans. CephBool didn't exist before Nautilus, + * so earlier clients are sent a CephChoices argdesc instead, and will + * send us a "--foo-bar" value string for boolean arguments. + */ + if (cmdmap.count(k)) { + try { + val = boost::get(cmdmap.find(k)->second); + return true; + } catch (boost::bad_get&) { + try { + std::string expected = "--" + k; + std::replace(expected.begin(), expected.end(), '_', '-'); + + std::string v_str = boost::get(cmdmap.find(k)->second); + if (v_str == expected) { + val = true; + return true; + } else { + throw bad_cmd_get(k, cmdmap); + } + } catch (boost::bad_get&) { + throw bad_cmd_get(k, cmdmap); + } + } + } + return false; +} + + diff --git a/src/common/cmdparse.h b/src/common/cmdparse.h index 9c3f857f87dd..550851b216a8 100644 --- a/src/common/cmdparse.h +++ b/src/common/cmdparse.h @@ -23,12 +23,15 @@ typedef boost::variant> cmdmap_t; std::string cmddesc_get_prefix(const std::string &cmddesc); -void dump_cmd_to_json(ceph::Formatter *f, const std::string& cmd); +void dump_cmd_to_json(ceph::Formatter *f, uint64_t features, + const std::string& cmd); void dump_cmd_and_help_to_json(ceph::Formatter *f, + uint64_t features, const std::string& secname, const std::string& cmd, const std::string& helptext); void dump_cmddesc_to_json(ceph::Formatter *jf, + uint64_t features, const std::string& secname, const std::string& cmdsig, const std::string& helptext, @@ -52,6 +55,9 @@ struct bad_cmd_get : public std::exception { } }; +bool cmd_getval(CephContext *cct, const cmdmap_t& cmdmap, + const std::string& k, bool& val); + template bool cmd_getval(CephContext *cct, const cmdmap_t& cmdmap, const std::string& k, T& val) diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 5d1e76f16581..079227cb63ec 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -727,7 +727,8 @@ int MDSDaemon::_handle_command( for (auto& c : get_commands()) { ostringstream secname; secname << "cmd" << setfill('0') << std::setw(3) << cmdnum; - dump_cmddesc_to_json(f.get(), secname.str(), c.cmdstring, c.helpstring, + dump_cmddesc_to_json(f.get(), m->get_connection()->get_features(), + secname.str(), c.cmdstring, c.helpstring, c.module, "*", 0); cmdnum++; } diff --git a/src/mgr/DaemonServer.cc b/src/mgr/DaemonServer.cc index 3f69ff76915c..b5c404e23778 100644 --- a/src/mgr/DaemonServer.cc +++ b/src/mgr/DaemonServer.cc @@ -830,10 +830,11 @@ bool DaemonServer::_handle_command( JSONFormatter f; f.open_object_section("command_descriptions"); - auto dump_cmd = [&cmdnum, &f](const MonCommand &mc){ + auto dump_cmd = [&cmdnum, &f, m](const MonCommand &mc){ ostringstream secname; secname << "cmd" << setfill('0') << std::setw(3) << cmdnum; - dump_cmddesc_to_json(&f, secname.str(), mc.cmdstring, mc.helpstring, + dump_cmddesc_to_json(&f, m->get_connection()->get_features(), + secname.str(), mc.cmdstring, mc.helpstring, mc.module, mc.req_perms, 0); cmdnum++; }; diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h index 8039a79fb58c..d7637a52093d 100644 --- a/src/mon/MonCommands.h +++ b/src/mon/MonCommands.h @@ -738,7 +738,8 @@ COMMAND("osd pause", "pause osd", "osd", "rw") COMMAND("osd unpause", "unpause osd", "osd", "rw") COMMAND("osd erasure-code-profile set " \ "name=name,type=CephString,goodchars=[A-Za-z0-9-_.] " \ - "name=profile,type=CephString,n=N,req=false", \ + "name=profile,type=CephString,n=N,req=false " \ + "name=force,type=CephBool,req=false", \ "create erasure code profile with [ ...] pairs. Add a --force at the end to override an existing profile (VERY DANGEROUS)", \ "osd", "rw") COMMAND("osd erasure-code-profile get " \ @@ -942,16 +943,16 @@ COMMAND("osd pool create " \ COMMAND_WITH_FLAG("osd pool delete " \ "name=pool,type=CephPoolname " \ "name=pool2,type=CephPoolname,req=false " \ - "name=sure,type=CephChoices,strings=--yes-i-really-really-mean-it|" \ - "--yes-i-really-really-mean-it-not-faking,req=false", \ + "name=yes_i_really_really_mean_it,type=CephBool,req=false " + "name=yes_i_really_really_mean_it_not_faking,type=CephBool,req=false ", \ "delete pool", \ "osd", "rw", \ FLAG(DEPRECATED)) COMMAND("osd pool rm " \ "name=pool,type=CephPoolname " \ "name=pool2,type=CephPoolname,req=false " \ - "name=sure,type=CephChoices,strings=--yes-i-really-really-mean-it|" \ - "--yes-i-really-really-mean-it-not-faking,req=false", \ + "name=yes_i_really_really_mean_it,type=CephBool,req=false " + "name=yes_i_really_really_mean_it_not_faking,type=CephBool,req=false ", \ "remove pool", \ "osd", "rw") COMMAND("osd pool rename " \ diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 3522f5b24341..54500d8fc051 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -2869,6 +2869,7 @@ bool Monitor::_allowed_command(MonSession *s, const string &module, void Monitor::format_command_descriptions(const std::vector &commands, Formatter *f, + uint64_t features, bufferlist *rdata) { int cmdnum = 0; @@ -2877,7 +2878,7 @@ void Monitor::format_command_descriptions(const std::vector &command unsigned flags = cmd.flags; ostringstream secname; secname << "cmd" << setfill('0') << std::setw(3) << cmdnum; - dump_cmddesc_to_json(f, secname.str(), + dump_cmddesc_to_json(f, features, secname.str(), cmd.cmdstring, cmd.helpstring, cmd.module, cmd.req_perms, flags); cmdnum++; @@ -2979,7 +2980,8 @@ void Monitor::handle_command(MonOpRequestRef op) } } - format_command_descriptions(commands, f, &rdata); + auto features = m->get_connection()->get_features(); + format_command_descriptions(commands, f, features, &rdata); delete f; reply_command(op, 0, "", rdata, 0); return; diff --git a/src/mon/Monitor.h b/src/mon/Monitor.h index 1c8cc3229db9..065917139305 100644 --- a/src/mon/Monitor.h +++ b/src/mon/Monitor.h @@ -963,6 +963,7 @@ private: public: static void format_command_descriptions(const std::vector &commands, Formatter *f, + uint64_t features, bufferlist *rdata); const std::vector &get_local_commands(mon_feature_t f) { diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc index 4952188b0363..e56612aed316 100644 --- a/src/mon/OSDMonitor.cc +++ b/src/mon/OSDMonitor.cc @@ -9589,13 +9589,10 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, cmd_getval(cct, cmdmap, "name", name); vector profile; cmd_getval(cct, cmdmap, "profile", profile); - bool force; - if (profile.size() > 0 && profile.back() == "--force") { - profile.pop_back(); - force = true; - } else { - force = false; - } + + bool force = false; + cmd_getval(cct, cmdmap, "force", force); + map profile_map; err = parse_erasure_code_profile(profile, &profile_map, &ss); if (err) @@ -11591,7 +11588,6 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, string poolstr, poolstr2, sure; cmd_getval(cct, cmdmap, "pool", poolstr); cmd_getval(cct, cmdmap, "pool2", poolstr2); - cmd_getval(cct, cmdmap, "sure", sure); int64_t pool = osdmap.lookup_pg_pool_name(poolstr.c_str()); if (pool < 0) { ss << "pool '" << poolstr << "' does not exist"; @@ -11599,9 +11595,12 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op, goto reply; } - bool force_no_fake = sure == "--yes-i-really-really-mean-it-not-faking"; + bool force_no_fake = false; + cmd_getval(cct, cmdmap, "yes_i_really_really_mean_it", force_no_fake); + bool force = false; + cmd_getval(cct, cmdmap, "yes_i_really_really_mean_it_not_faking", force); if (poolstr2 != poolstr || - (sure != "--yes-i-really-really-mean-it" && !force_no_fake)) { + (!force && !force_no_fake)) { ss << "WARNING: this will *PERMANENTLY DESTROY* all data stored in pool " << poolstr << ". If you are *ABSOLUTELY CERTAIN* that is what you want, pass the pool name *twice*, " << "followed by --yes-i-really-really-mean-it."; diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc index 4c4763c01628..42c90fedd7c1 100644 --- a/src/osd/OSD.cc +++ b/src/osd/OSD.cc @@ -6092,7 +6092,8 @@ int OSD::_do_command( ostringstream secname; secname << "cmd" << setfill('0') << std::setw(3) << cmdnum; - dump_cmddesc_to_json(f, secname.str(), cp->cmdstring, cp->helpstring, + dump_cmddesc_to_json(f, con->get_features(), + secname.str(), cp->cmdstring, cp->helpstring, cp->module, cp->perm, 0); cmdnum++; } diff --git a/src/pybind/ceph_argparse.py b/src/pybind/ceph_argparse.py index 1c06e99b2390..38f85cf314f4 100644 --- a/src/pybind/ceph_argparse.py +++ b/src/pybind/ceph_argparse.py @@ -499,6 +499,27 @@ class CephChoices(CephArgtype): return all_elems +class CephBool(CephArgtype): + """ + A boolean argument, values may be case insensitive 'true', 'false', '0', + '1'. In keyword form, value may be left off (implies true). + """ + def __init__(self, strings='', **kwargs): + self.strings = strings.split('|') + + def valid(self, s, partial=False): + lower_case = s.lower() + if lower_case in ['true', '1']: + self.val = True + elif lower_case in ['false', '0']: + self.val = False + else: + raise ArgumentValid("{0} not one of 'true', 'false'".format(s)) + + def __str__(self): + return '' + + class CephFilepath(CephArgtype): """ Openable file @@ -681,6 +702,8 @@ class argdesc(object): """ if self.t == CephString: chunk = '<{0}>'.format(self.name) + elif self.t == CephBool: + chunk = "--{0}".format(self.name.replace("_", "-")) else: chunk = str(self.instance) s = chunk @@ -943,7 +966,6 @@ def validate(args, signature, flags=0, partial=False): # A keyword argument? if myarg: - # argdesc for the keyword argument, if we find one kwarg_desc = None @@ -956,15 +978,31 @@ def validate(args, signature, flags=0, partial=False): if kwarg_match: # We have a "--foo=bar" style argument kwarg_k, kwarg_v = kwarg_match.groups() + + # Either "--foo-bar" or "--foo_bar" style is accepted + kwarg_k = kwarg_k.replace('-', '_') + kwarg_desc = arg_descs_by_name.get(kwarg_k, None) - elif len(myargs): # Some trailing arguments exist - # Maybe this is a "--foo bar" style argument + else: + # Maybe this is a "--foo bar" or "--bool" style argument key_match = re.match(KWARG_SPACE, myarg) if key_match: kwarg_k = key_match.group(1) + + # Permit --foo-bar=123 form or --foo_bar=123 form, + # assuming all command definitions use foo_bar argument + # naming style + kwarg_k = kwarg_k.replace('-', '_') + kwarg_desc = arg_descs_by_name.get(kwarg_k, None) if kwarg_desc: - kwarg_v = myargs.pop(0) + if kwarg_desc.t == CephBool: + kwarg_v = 'true' + elif len(myargs): # Some trailing arguments exist + kwarg_v = myargs.pop(0) + else: + # Forget it, this is not a valid kwarg + kwarg_desc = None if kwarg_desc: validate_one(kwarg_v, kwarg_desc) @@ -976,18 +1014,16 @@ def validate(args, signature, flags=0, partial=False): # "--yes-i-really-mean-it") if myarg and myarg.startswith("--"): # Special cases for instances of confirmation flags - # that were defined as CephString instead of CephChoices + # that were defined as CephString/CephChoices instead of CephBool # in pre-nautilus versions of Ceph daemons. is_value = desc.t == CephChoices \ or myarg == "--yes-i-really-mean-it" \ or myarg == "--yes-i-really-really-mean-it" \ or myarg == "--yes-i-really-really-mean-it-not-faking" \ + or myarg == "--force" \ or injectargs if not is_value: - sys.stderr.write("desc={0} t={1}".format( - desc, desc.t)) - # Didn't get caught by kwarg handling, but has a "--", so # we must assume it's something invalid, to avoid naively # passing through mis-typed options as the values of diff --git a/src/test/common/get_command_descriptions.cc b/src/test/common/get_command_descriptions.cc index c5dde7fd782a..108b3deec9da 100644 --- a/src/test/common/get_command_descriptions.cc +++ b/src/test/common/get_command_descriptions.cc @@ -45,7 +45,8 @@ static void json_print(const std::vector &mon_commands) { bufferlist rdata; Formatter *f = Formatter::create("json"); - Monitor::format_command_descriptions(mon_commands, f, &rdata); + Monitor::format_command_descriptions(mon_commands, f, + CEPH_FEATURES_ALL, &rdata); delete f; string data(rdata.c_str(), rdata.length()); cout << data << std::endl; diff --git a/src/test/pybind/test_ceph_argparse.py b/src/test/pybind/test_ceph_argparse.py index ec4465b83dbe..64321c7a70ce 100755 --- a/src/test/pybind/test_ceph_argparse.py +++ b/src/test/pybind/test_ceph_argparse.py @@ -999,6 +999,62 @@ class TestOSD(TestArgparse): assert_equal({}, validate_command(sigdict, ['osd', 'pool', 'create', "foo", "8", "--foo=bar"])) + def test_foo(self): + # Long form of a boolean argument (--foo=true) + assert_equal( + { + "prefix": "osd pool delete", + "pool": "foo", + "pool2": "foo", + "yes_i_really_really_mean_it": True + }, validate_command(sigdict, [ + 'osd', 'pool', 'delete', "foo", "foo", + "--yes-i-really-really-mean-it=true"])) + + def test_pool_bool_args(self): + """ + Use pool deletion to exercise boolean arguments since it has + the --yes-i-really-really-mean-it flags + """ + + # Short form of a boolean argument (--foo) + assert_equal( + { + "prefix": "osd pool delete", + "pool": "foo", + "pool2": "foo", + "yes_i_really_really_mean_it": True + }, validate_command(sigdict, [ + 'osd', 'pool', 'delete', "foo", "foo", + "--yes-i-really-really-mean-it"])) + + # Long form of a boolean argument (--foo=true) + assert_equal( + { + "prefix": "osd pool delete", + "pool": "foo", + "pool2": "foo", + "yes_i_really_really_mean_it": True + }, validate_command(sigdict, [ + 'osd', 'pool', 'delete', "foo", "foo", + "--yes-i-really-really-mean-it=true"])) + + # Negative form of a boolean argument (--foo=false) + assert_equal( + { + "prefix": "osd pool delete", + "pool": "foo", + "pool2": "foo", + "yes_i_really_really_mean_it": False + }, validate_command(sigdict, [ + 'osd', 'pool', 'delete', "foo", "foo", + "--yes-i-really-really-mean-it=false"])) + + # Invalid value boolean argument (--foo=somethingelse) + assert_equal({}, validate_command(sigdict, [ + 'osd', 'pool', 'delete', "foo", "foo", + "--yes-i-really-really-mean-it=rhubarb"])) + def test_pool_create(self): self.assert_valid_command(['osd', 'pool', 'create', 'poolname', '128'])