From 2e3643647bfbe955b54c62c8aaf114744dedb86e Mon Sep 17 00:00:00 2001 From: Greg Farnum Date: Thu, 29 Oct 2020 05:58:56 +0000 Subject: [PATCH] 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 Signed-off-by: Greg Farnum --- src/mon/MonMap.cc | 12 ++++++------ src/mon/MonmapMonitor.cc | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/mon/MonMap.cc b/src/mon/MonMap.cc index efb426f742c..fe417ba44fd 100644 --- 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 f45cbbe8cb4..dede84f9ef1 100644 --- a/src/mon/MonmapMonitor.cc +++ b/src/mon/MonmapMonitor.cc @@ -575,6 +575,19 @@ bool MonmapMonitor::prepare_command(MonOpRequestRef op) map 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; -- 2.47.3