mon: Do not increase compatv when using monitor location or stretch mode

For mon_info_t, I first wrote things so that when monitors get a location
added in MonMap::mon_info_t, I bumped the struct_v to 5 and also bumped
the min_compat to 5. This made sure that nobody could decode the
struct and lose the location info, which if it were a monitor
would be very bad.

And for the MonMap, when stretch mode is enabled I bumped up the
comptav (in addition to the always-increased struct_v), for the same reason.

But clients also have to decode these structures, and we can't
disallow older clients from connecting to a stretched cluster.

Happily, usage of any stretch modes already requires a feature
bit and sets it as required in the monmap, so these are already
gated. Therefore, just don't set new compat values in these cases.

While at it, also gate setting the location on the monmap indicating
all monitors are updated.

Reported-by: Travis Nielsen <tnielsen@redhat.com>
Signed-off-by: Greg Farnum <gfarnum@redhat.com>

diff --git a/src/mon/MonMap.cc b/src/mon/MonMap.cc
index efb426f742c276e03bf206fd7af84c733c5fbded..fe417ba44fdd83c6121d23a5ebffaea4cd80c53c 100644 (file)
--- a/src/mon/MonMap.cc
+++ b/src/mon/MonMap.cc
@@ -41,7 +41,11 @@ void mon_info_t::encode(ceph::buffer::list& bl, uint64_t features) const
   uint8_t v = 5;
   uint8_t min_v = 1;
   if (!crush_loc.empty()) {
-    min_v = 5;
+    // we added crush_loc in version 5, but need to let old clients decode it
+    // so just leave the min_v at version 4. Monitors are protected
+    // from misunderstandings about location because setting it is blocked
+    // on FEATURE_PINGING
+    min_v = 4;
   }
   if (!HAVE_FEATURE(features, SERVER_NAUTILUS)) {
     v = 2;
@@ -197,7 +201,6 @@ void MonMap::encode(ceph::buffer::list& blist, uint64_t con_features) const
     return;
   }
 
-  uint8_t new_compat_v = 0;
   ENCODE_START(9, 6, blist);
   ceph::encode_raw(fsid, blist);
   encode(epoch, blist);
@@ -215,10 +218,7 @@ void MonMap::encode(ceph::buffer::list& blist, uint64_t con_features) const
   encode(stretch_mode_enabled, blist);
   encode(tiebreaker_mon, blist);
   encode(stretch_marked_down_mons, blist);
-  if (stretch_mode_enabled) {
-    new_compat_v = 9;
-  }
-  ENCODE_FINISH_NEW_COMPAT(blist, new_compat_v);
+  ENCODE_FINISH(blist);
 }
 
 void MonMap::decode(ceph::buffer::list::const_iterator& p)

diff --git a/src/mon/MonmapMonitor.cc b/src/mon/MonmapMonitor.cc
index f45cbbe8cb4db3a3734a31c6fb6f2f478195f18b..dede84f9ef17d9dc57b0e7dc8cdf3f2cb1eee38d 100644 (file)
--- a/src/mon/MonmapMonitor.cc
+++ b/src/mon/MonmapMonitor.cc
@@ -575,6 +575,19 @@ bool MonmapMonitor::prepare_command(MonOpRequestRef op)
     map<string, string> loc;
     cmd_getval(cmdmap, "location", locationvec);
     CrushWrapper::parse_loc_map(locationvec, &loc);
+    if (locationvec.size() &&
+       !mon->get_quorum_mon_features().contains_all(
+                                       ceph::features::mon::FEATURE_PINGING)) {
+      err = -ENOTSUP;
+      ss << "Not all monitors support adding monitors with a location; please upgrade first!";
+      goto reply;
+    }
+    if (locationvec.size() && !loc.size()) {
+      ss << "We could not parse your input location to anything real; " << locationvec
+        << " turned into an empty map!";
+      err = -EINVAL;
+      goto reply;
+    }
 
     dout(10) << "mon add setting location for " << name << " to " << loc << dendl;
 
@@ -977,6 +990,12 @@ bool MonmapMonitor::prepare_command(MonOpRequestRef op)
     err = 0;
     propose = true;
   } else if (prefix == "mon set_location") {
+    if (!mon->get_quorum_mon_features().contains_all(
+                                       ceph::features::mon::FEATURE_PINGING)) {
+      err = -ENOTSUP;
+      ss << "Not all monitors support monitor locations; please upgrade first!";
+      goto reply;
+    }
     string name;
     if (!cmd_getval(cmdmap, "name", name)) {
       err = -EINVAL;