mon: OSDMonitor: receive CephInt on 'osd pool set' instead on CephString

This partially reverts 2fe0d0d9 in order to allow Emperor monitors to
forward mon command messages to Dumpling monitors without breaking a
cluster.

The need for this patch became obvious after issue #6796 was triggered.
Basically, in a mixed cluster of Emperor/Dumpling monitors, if a client
happens to obtain the command descriptions from an Emperor monitor and
then issue an 'osd pool set' this can turn out in one of two ways:

1. client msg gets forwarded to an Emperor leader and everything's a-okay;
2. client msg gets forwarded to a Dumpling leader and the string fails to
be interpreted without the monitor noticing, thus leaving the monitor with
an uninitialized variable leading to trouble.

If 2 is triggered, a multitude of bad things can happen, such as thousands
of pg splits, due to a simple 'osd set pool foo pg_num 128' turning out
to be interpreted as 109120394 or some other random number.

This patch is such that we make sure the client sends an integer instead
of a string. We also make sure to interpret anything the client sends as
possibly being a string, or an integer.

Fixes: 6796
Backport: emperor

Signed-off-by: Joao Eduardo Luis <joao.luis@inktank.com>
(cherry picked from commit 337195f04653eed8e8f153a5b074f3bd48408998)
Reviewed-by: Sage Weil <sage@inktank.com>
Reviewed-by: Greg Farnum <greg@inktank.com>

diff --git a/src/mon/MonCommands.h b/src/mon/MonCommands.h
index 5a6ca6a471d265dcaba4ca5f621c8990e37f2fe8..881c5a252465e04ceb9badd6036d13ab092f8b01 100644 (file)
--- a/src/mon/MonCommands.h
+++ b/src/mon/MonCommands.h
@@ -508,7 +508,7 @@ COMMAND("osd pool get " \
 COMMAND("osd pool set " \
        "name=pool,type=CephPoolname " \
        "name=var,type=CephChoices,strings=size|min_size|crash_replay_interval|pg_num|pgp_num|crush_ruleset|hashpspool " \
-       "name=val,type=CephString", \
+       "name=val,type=CephInt", \
        "set pool parameter <var> to <val>", "osd", "rw", "cli,rest")
 // 'val' is a CephString because it can include a unit.  Perhaps
 // there should be a Python type for validation/conversion of strings

diff --git a/src/mon/OSDMonitor.cc b/src/mon/OSDMonitor.cc
index 07775fce2bf9a58d24c66770dbbb42931418dca3..31da21d9946dfff93b348f993035402da41dee9f 100644 (file)
--- a/src/mon/OSDMonitor.cc
+++ b/src/mon/OSDMonitor.cc
@@ -2736,6 +2736,7 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
 
   // accept val as a json string or int, and parse out int or float
   // values from the string as needed
+  string raw_val = cmd_vartype_stringify(cmdmap["val"]);
   string val;
   cmd_getval(g_ceph_context, cmdmap, "val", val);
   string interr;
@@ -2749,7 +2750,7 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
 
   if (var == "size") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     if (n == 0 || n > 10) {
@@ -2762,21 +2763,21 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
     ss << "set pool " << pool << " size to " << n;
   } else if (var == "min_size") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     p.min_size = n;
     ss << "set pool " << pool << " min_size to " << n;
   } else if (var == "crash_replay_interval") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     p.crash_replay_interval = n;
     ss << "set pool " << pool << " to crash_replay_interval to " << n;
   } else if (var == "pg_num") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     if (n <= (int)p.get_pg_num()) {
@@ -2795,7 +2796,7 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
     }
   } else if (var == "pgp_num") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     if (n <= 0) {
@@ -2816,7 +2817,7 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
     }
   } else if (var == "crush_ruleset") {
     if (interr.length()) {
-      ss << "error parsing integer value '" << val << "': " << interr;
+      ss << "error parsing integer value '" << raw_val << "': " << interr;
       return -EINVAL;
     }
     if (osdmap.crush->rule_exists(n)) {
@@ -2827,14 +2828,16 @@ int OSDMonitor::prepare_command_pool_set(map<string,cmd_vartype> &cmdmap,
       return -ENOENT;
     }
   } else if (var == "hashpspool") {
-    if (val == "true") {
+    // make sure we only compare against 'n' if we didn't receive a string
+    int flag_val = (!val.empty() ? -1 : n);
+    if (val == "true" || flag_val == 1) {
       p.flags |= pg_pool_t::FLAG_HASHPSPOOL;
       ss << "set";
-    } else if (val == "false") {
+    } else if (val == "false" || flag_val == 0) {
       p.flags ^= pg_pool_t::FLAG_HASHPSPOOL;
       ss << "unset";
     } else {
-      ss << "expecting value true or false";
+      ss << "expecting value 0 or 1";
       return -EINVAL;
     }
     ss << " pool " << pool << " flag hashpspool";