Merge pull request #7163 from tchaikov/wip-pr-6085

mon: use CephOsdName for osd args instead of CephInt

Reviewed-by: Sage Weil <sage@redhat.com>

qa/workunits/cephtool/test.sh

@@ -1102,7 +1102,12 @@ function test_mon_osd()
   ceph osd thrash 0
 
   ceph osd dump | grep 'osd.0 up'
+  # ceph osd find expects the OsdName, so both ints and osd.n should work.
   ceph osd find 1
+  ceph osd find osd.1
+  expect_false osd find osd.xyz
+  expect_false osd find xyz
+  expect_false osd find 0.1
   ceph --format plain osd find 1 # falls back to json-pretty
   ceph osd metadata 1 | grep 'distro'
   ceph --format plain osd metadata 1 | grep 'distro' # falls back to json-pretty
@@ -1376,7 +1381,7 @@ function test_mon_pg()
 
   ceph osd reweight 0 0.9
   expect_false ceph osd reweight 0 -1
-  ceph osd reweight 0 1
+  ceph osd reweight osd.0 1
 
   ceph osd primary-affinity osd.0 .9
   expect_false ceph osd primary-affinity osd.0 -2
@@ -1384,7 +1389,7 @@ function test_mon_pg()
   ceph osd primary-affinity osd.0 1
 
   ceph osd pg-temp 0.0 0 1 2
-  ceph osd pg-temp 0.0 1 0 2
+  ceph osd pg-temp 0.0 osd.1 osd.0 osd.2
   expect_false ceph osd pg-temp asdf qwer
   expect_false ceph osd pg-temp 0.0 asdf
   expect_false ceph osd pg-temp 0.0

src/common/cmdparse.cc

@@ -155,16 +155,35 @@ cmdmap_from_json(vector<string> cmd, map<string, cmd_vartype> *mapp, stringstrea
       case json_spirit::array_type:
 	{
 	  // array is a vector of values.  Unpack it to a vector
-	  // of strings, the only type we handle.
-	  vector<json_spirit::mValue> spvals = it->second.get_array();
-	  vector<string> outv;
-	  for (vector<json_spirit::mValue>::iterator sv = spvals.begin();
-	       sv != spvals.end(); ++sv) {
-	    if (sv->type() != json_spirit::str_type)
-	      throw(runtime_error("Can't handle arrays of non-strings"));
-	    outv.push_back(sv->get_str());
+	  // of strings or int64_t, the only types we handle.
+	  const vector<json_spirit::mValue>& spvals = it->second.get_array();
+	  if (spvals.empty()) {
+	    // if an empty array is acceptable, the caller should always check for
+	    // vector<string> if the expected value of "vector<int64_t>" in the
+	    // cmdmap is missing.
+	    (*mapp)[it->first] = std::move(vector<string>());
+	  } else if (spvals.front().type() == json_spirit::str_type) {
+	    vector<string> outv;
+	    for (const auto& sv : spvals) {
+	      if (sv.type() != json_spirit::str_type) {
+		throw(runtime_error("Can't handle arrays of multiple types"));
+	      }
+	      outv.push_back(sv.get_str());
+	    }
+	    (*mapp)[it->first] = std::move(outv);
+	  } else if (spvals.front().type() == json_spirit::int_type) {
+	    vector<int64_t> outv;
+	    for (const auto& sv : spvals) {
+	      if (spvals.front().type() != json_spirit::int_type) {
+		throw(runtime_error("Can't handle arrays of multiple types"));
+	      }
+	      outv.push_back(sv.get_int64());
+	    }
+	    (*mapp)[it->first] = std::move(outv);
+	  } else {
+	    throw(runtime_error("Can't handle arrays of types other than "
+				"int or string"));
 	  }
-	  (*mapp)[it->first] = outv;
 	}
 	break;
       case json_spirit::str_type:

src/common/cmdparse.h

@@ -17,7 +17,12 @@ class CephContext;
 /* this is handy; can't believe it's not standard */
 #define ARRAY_SIZE(a)	(sizeof(a) / sizeof(*a))
 
-typedef boost::variant<std::string, bool, int64_t, double, std::vector<std::string> > cmd_vartype;
+typedef boost::variant<std::string,
+		       bool,
+		       int64_t,
+		       double,
+		       std::vector<std::string>,
+		       std::vector<int64_t>>  cmd_vartype;
 typedef std::map<std::string, cmd_vartype> cmdmap_t;
 
 void dump_cmd_to_json(ceph::Formatter *f, const std::string& cmd);

src/mon/MonCommands.h

@@ -468,11 +468,11 @@ COMMAND("osd blocked-by", \
 	"osd", "r", "cli,rest")
 COMMAND("osd getmaxosd", "show largest OSD id", "osd", "r", "cli,rest")
 COMMAND("osd find " \
-	"name=id,type=CephInt,range=0", \
+	"name=id,type=CephOsdName", \
 	"find osd <id> in the CRUSH map and show its location", \
 	"osd", "r", "cli,rest")
 COMMAND("osd metadata " \
-	"name=id,type=CephInt,range=0,req=false", \
+	"name=id,type=CephOsdName,req=false", \
 	"fetch metadata for osd {id} (default all)", \
 	"osd", "r", "cli,rest")
 COMMAND("osd map " \
@@ -648,17 +648,17 @@ COMMAND("osd rm " \
 	"name=ids,type=CephString,n=N", \
 	"remove osd(s) <id> [<id>...] in", "osd", "rw", "cli,rest")
 COMMAND("osd reweight " \
-	"name=id,type=CephInt,range=0 " \
+	"name=id,type=CephOsdName " \
 	"type=CephFloat,name=weight,range=0.0|1.0", \
 	"reweight osd to 0.0 < <weight> < 1.0", "osd", "rw", "cli,rest")
 COMMAND("osd pg-temp " \
 	"name=pgid,type=CephPgid " \
-	"name=id,type=CephString,n=N,req=false", \
+	"name=id,type=CephOsdName,n=N,req=false", \
 	"set pg_temp mapping pgid:[<id> [<id>...]] (developers only)", \
         "osd", "rw", "cli,rest")
 COMMAND("osd primary-temp " \
 	"name=pgid,type=CephPgid " \
-	"name=id,type=CephString", \
+	"name=id,type=CephOsdName", \
         "set primary_temp mapping pgid:<id>|-1 (developers only)", \
         "osd", "rw", "cli,rest")
 COMMAND("osd primary-affinity " \
@@ -667,13 +667,13 @@ COMMAND("osd primary-affinity " \
 	"adjust osd primary-affinity from 0.0 <= <weight> <= 1.0", \
 	"osd", "rw", "cli,rest")
 COMMAND("osd lost " \
-	"name=id,type=CephInt,range=0 " \
+	"name=id,type=CephOsdName " \
 	"name=sure,type=CephChoices,strings=--yes-i-really-mean-it,req=false", \
 	"mark osd as permanently lost. THIS DESTROYS DATA IF NO MORE REPLICAS EXIST, BE CAREFUL", \
 	"osd", "rw", "cli,rest")
 COMMAND("osd create " \
 	"name=uuid,type=CephUUID,req=false " \
-	"name=id,type=CephInt,range=0,req=false", \
+	"name=id,type=CephOsdName,req=false", \
 	"create new osd (with optional UUID and ID)", "osd", "rw", "cli,rest")
 COMMAND("osd blacklist " \
 	"name=blacklistop,type=CephChoices,strings=add|rm " \

src/mon/OSDMonitor.cc

@@ -6406,7 +6406,7 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
       return true;
     }
 
-    vector<string> id_vec;
+    vector<int64_t> id_vec;
     vector<int32_t> new_pg_temp;
     if (!cmd_getval(g_ceph_context, cmdmap, "id", id_vec)) {
       ss << "unable to parse 'id' value(s) '"
@@ -6414,18 +6414,12 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
       err = -EINVAL;
       goto reply;
     }
-    for (unsigned i = 0; i < id_vec.size(); i++) {
-      int32_t osd = parse_osd_id(id_vec[i].c_str(), &ss);
-      if (osd < 0) {
-        err = -EINVAL;
-        goto reply;
-      }
+    for (auto osd : id_vec) {
       if (!osdmap.exists(osd)) {
         ss << "osd." << osd << " does not exist";
         err = -ENOENT;
         goto reply;
       }
-
       new_pg_temp.push_back(osd);
     }
 
@@ -6453,27 +6447,17 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
       goto reply;
     }
 
-    string id;
-    int32_t osd;
-    if (!cmd_getval(g_ceph_context, cmdmap, "id", id)) {
+    int64_t osd;
+    if (!cmd_getval(g_ceph_context, cmdmap, "id", osd)) {
       ss << "unable to parse 'id' value '"
          << cmd_vartype_stringify(cmdmap["id"]) << "'";
       err = -EINVAL;
       goto reply;
     }
-    if (strcmp(id.c_str(), "-1")) {
-      osd = parse_osd_id(id.c_str(), &ss);
-      if (osd < 0) {
-        err = -EINVAL;
-        goto reply;
-      }
-      if (!osdmap.exists(osd)) {
-        ss << "osd." << osd << " does not exist";
-        err = -ENOENT;
-        goto reply;
-      }
-    } else {
-      osd = -1;
+    if (osd != -1 && !osdmap.exists(osd)) {
+      ss << "osd." << osd << " does not exist";
+      err = -ENOENT;
+      goto reply;
     }
 
     if (!g_conf->mon_osd_allow_primary_temp) {

src/pybind/ceph_argparse.py

@@ -423,6 +423,8 @@ class CephOsdName(CephArgtype):
             i = int(i)
         except:
             raise ArgumentFormat('osd id ' + i + ' not integer')
+        if i < 0:
+            raise ArgumentFormat('osd id {0} is less than 0'.format(i))
         self.nametype = t
         self.nameid = i
         self.val = i