mon: osdmap prune #19331

jecluis · 2017-12-05T08:45:59Z

In the event of an unclean cluster for quite a while, the monitors may end up keeping quite a lot of maps (thousands to tens of thousands). When this happens, the monitors may be unstable due to the size of their store, which may lead to further unrest in the cluster -- and the whole situation can then become a catch-22 kind of thing.
This patch set introduces a new mechanism that will allow an administrator to prune maps. Unlike trim, prune will not be removing maps at the tail; instead, prune will be poking holes in the sequence of full osdmaps, reducing the usage of store space. Prune trades off performance for reduced store size, as the monitors will have to recompute pruned osdmaps from the last known full osdmap and all the incremental maps since.
The patch set also contains a dev doc describing the algorithm.

Signed-off-by: Joao Eduardo Luis <joao@suse.de>

branch-predictor · 2017-12-05T09:46:40Z

I wish every such PR could have a such exhaustive docs. ;)

jecluis · 2017-12-05T10:55:39Z

doc/dev/mon-osdmap-prune.rst

+  upper_pinned(last_pruned) C pinned AND
+  lower_pinned(last_pruned) C pinned AND
+  upper_pinned(last_pruned) != lower_pinned(last_pruned)
+


indentation was messed up for these last two blocks. I'll address it before merging.

liewegas · 2017-12-05T14:29:27Z

src/common/options.cc

@@ -897,6 +897,36 @@ std::vector<Option> get_global_options() {
    .set_default(true)
    .set_description(""),

+    /* -- mon: osdmap prune (begin) -- */
+    Option("mon_osdmap_full_prune_enabled", Option::TYPE_BOOL, Option::LEVEL_ADVANCED)
+    .set_default(false)


liewegas · 2017-12-05T14:33:18Z

src/mon/OSDMonitor.cc

+ * for more information, please refer to doc/dev/mon-osdmap-prune.rst
+ */
+
+void OSDMonitor::update_osdmap_manifest()


load_osdmap_manifest()? i wasn't sure when i got here whether this was making some change or refreshing state.

liewegas · 2017-12-05T14:36:36Z

src/mon/OSDMonitor.cc

+  dout(10) << __func__
+           << " interval [" << first << ".." << last << "]" << dendl;
+  for (version_t v = first; v <= last; ++v) {
+


unnecessary whitespace

liewegas · 2017-12-05T14:36:43Z

src/mon/OSDMonitor.cc

+  version_t next = osdmap_manifest.get_next_prunable();
+
+  if (g_conf->get_val<bool>("mon_debug_extra_checks") && next > 0) {
+


unnecessary whitespace

liewegas · 2017-12-05T14:36:48Z

src/mon/OSDMonitor.cc

+
+    dout(20) << __func__
+             << " dropping osdmap manifest from memory." << dendl;
+


unnecessary whitespace

liewegas · 2017-12-05T14:39:35Z

src/mon/OSDMonitor.cc

+        return ret;
+      }
+    } else if (ret != 0) {
+      return ret;
    }


simpler?

if (ret == -ENOENT) { // build map? ret = get_full_from_pinned_map(ver, bl); } if (ret != 0) { return ret; }

liewegas · 2017-12-05T14:43:16Z

src/mon/OSDMonitor.h

+    return get_next_prunable(last_pruned);
+  }
+
+  version_t get_next_prunable(version_t v) const


I wonder if it would be simpler to just drop this last_pruned, in_progress thing. Instead, when we prune, we (in a single transaction) (1) add the pinned epoch and (2) delete all maps between the new pinned epoch and the next one. I don't think the gap should be more then a few 10s of epochs anyway, and deletes aren't expensive to include in a transaction.

liewegas · 2017-12-05T14:45:46Z

src/mon/OSDMonitor.cc

+    osdm->encode(tbl, encode_features | CEPH_FEATURE_RESERVED);
+    tosdm->decode(tbl);
+    delete osdm;
+    osdm = tosdm;


why do the switcheroo to tosdm here? paranoia?

pretty much; I don't want to rely on squashing existing values when decoding to an already existing map. And afaict we need the whole encode/decode dance to get updated crcs out of the maps.

I think you should drop this. In the normal path(s) we apply multiple incrementals to one map. I think that's safe.

If you want you can add an if (mon_debug_extra_checks) branch that does a paranoid check.

liewegas · 2017-12-05T14:47:21Z

src/mon/Monitor.cc

@@ -306,6 +306,35 @@ void Monitor::do_admin_command(string command, cmdmap_t& cmdmap, string format,
      f->flush(ss);
    }

+  } else if (command == "test generate osdmap") {


this whole thing makes me nervous... it seems inevitable that it will rot. can we instead do a loop like 'for f in seq 1 100 ; do ceph osd unset nodown ; done'?

i still don't like this patch; i'd rather do it the slow way (for loop in bash doing some trivial change)

liewegas · 2017-12-05T14:48:50Z

qa/standalone/mon/mon-osdmap-prune.sh

+    '--mon-debug-block-osdmap-trim '\
+    '--mon-debug-extra-checks' || return 1
+
+  ceph daemon $(get_asok_path mon.a) test generate osdmap 1000 || return 1


ok, i guess it won't bit rot. :)

liewegas · 2017-12-05T15:28:57Z

The other thing that would be nice here is if one of the thrashing test cases set things such that this is regularly triggered. Like, min epochs = 25, prune max = 5, prune tx = 1, something like that.

tchaikov · 2017-12-06T09:13:46Z

src/common/options.cc

+    Option("mon_osdmap_full_prune_min", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
+    .set_default(10000)
+    .set_description("Minimum number of versions in the store to trigger full map pruning")
+    .add_see_also("mon_osdmap_full_prune_enabled")


nit, i think we can assume the yet-written tool will take Option("foo").add_see_also("bar") as a link from "foo" to "bar", and hence "bar" will be linked to "foo" also.

I do not know what tool is this you are talking about, but -- even if it is in the works -- it seems that assumption to be weird. I understand the add_see_also() as a unidirectional link, to further instruct the user to know more about a given topic; sometimes we'll have bidirectional links, but not necessarily.
In any case, I presume this tool will be smart enough to either take it as is, or complain about it and having us remove one of the links. When the time comes, I'll be all for fixing this to follow said tool's semantics.

okay. i think it will be bidirectional. but, agreed. we can fix it when the tool is around. BTW, for the "tool", see https://trello.com/c/wBc0R9HV/6-config-options-docs and http://pad.ceph.com/p/config-annotation

script to generate .rst docs for docs.ceph.com

tchaikov

will continue reviewing tomorrow.

tchaikov · 2017-12-06T09:25:41Z

src/mon/OSDMonitor.cc

+
+bool OSDMonitor::should_prune() const
+{
+  version_t first = get_first_committed();


i'd suggest move the definition of first, last, min_osdmap_epochs, prune_to and last_pruned after the following if block.

@liewegas @tchaikov sorry, where to? Somehow, it looks like these comments have been kept, even though the code itself has changed (heck, github tells me the commit this review is on has changed). Not only there's no following if block atm (it's a bunch of if-elseif-else that actually depend on these variables to be declared and initialized), but everything pertaining to last_pruned has been dropped from the current algorithm.

tchaikov · 2017-12-06T09:33:24Z

src/mon/OSDMonitor.h

+  {
+    set<version_t>::const_reverse_iterator it = pinned.crbegin();
+    if (it == pinned.crend()) {
+      ceph_assert(pinned.empty());


i don't think we need this.

tchaikov · 2017-12-06T09:37:14Z

src/mon/OSDMonitor.h

+
+  version_t get_lower_closest_pinned(version_t v) const
+  {
+    set<version_t>::const_iterator p = pinned.lower_bound(v);


nit, could just use auto. less type.

tchaikov · 2017-12-06T09:41:04Z

src/common/options.cc

+    Option("mon_osdmap_full_prune_min", Option::TYPE_UINT, Option::LEVEL_ADVANCED)
+    .set_default(10000)
+    .set_description("Minimum number of versions in the store to trigger full map pruning")
+    .add_see_also("mon_osdmap_full_prune_enabled")


okay. i think it will be bidirectional. but, agreed. we can fix it when the tool is around. BTW, for the "tool", see https://trello.com/c/wBc0R9HV/6-config-options-docs and http://pad.ceph.com/p/config-annotation

script to generate .rst docs for docs.ceph.com

tchaikov · 2017-12-06T09:56:22Z

src/mon/OSDMonitor.cc

+           << dendl;
+
+  if (!osdmap_manifest.is_pinned(first)) {
+    osdmap_manifest.pinned.insert(first);


might want to use the helper method instead. i.e.

osdmap_manifest.pin(first);

tchaikov · 2017-12-06T10:00:29Z

src/mon/OSDMonitor.cc

@@ -3425,6 +4020,12 @@ void OSDMonitor::dump_info(Formatter *f)
  f->open_object_section("crushmap");
  osdmap.crush->dump(f);
  f->close_section();
+
+  if (has_osdmap_manifest) {
+    f->open_object_section("osdmap_manifest");


nit, use f.dump_object("osdmap_manifest", osdmap_manifest) instead.

tchaikov · 2017-12-15T07:27:21Z

@jecluis ping?

jecluis · 2017-12-15T11:56:07Z

@tchaikov busy week; not forgotten ;)

shinobu-x · 2017-12-16T02:22:03Z

src/mon/OSDMonitor.cc

+  bool store_has_manifest =
+    mon->store->exists(get_service_name(), "full_manifest");
+
+  if (!store_has_manifest) {


nit

if (!store_has_manifest && !has_osdmap_manifest) return;

shinobu-x · 2017-12-16T02:28:08Z

src/mon/OSDMonitor.cc

+
+  if (prune_interval > prune_min) {
+    derr << __func__
+         << " impossible to ascertain proper prune interval because "


nit: unnecessary white space after because

shinobu-x · 2017-12-16T02:34:28Z

src/mon/OSDMonitor.cc

+  // this allows us the monitor to trim maps without caring too much about
+  // pinned maps, and then allow us to use another ceph-mon without these
+  // capabilities, without having to repair the store.
+  version_t last_to_pin = last - g_conf->mon_min_osdmap_epochs;


g_conf->get_val<int64_t>("mon_min_osdmap_epochs");

shinobu-x · 2017-12-16T02:39:07Z

src/mon/OSDMonitor.cc

+  version_t last_to_pin = last - g_conf->mon_min_osdmap_epochs;
+  version_t last_pinned = osdmap_manifest.get_last_pinned();
+  version_t last_pruned = osdmap_manifest.get_last_pruned();
+  uint64_t prune_interval =


nit: might be better to consistent with version_t?

jecluis · 2018-02-07T16:11:44Z

@liewegas mind taking another looks? I'm now working on getting a teuthology test along with the PR, but I haven't done this in a while and I'm still trying to figure out how (sigh).

liewegas · 2018-02-07T21:28:38Z

@jecluis don't forget to update the cli test: /home/jenkins-build/build/workspace/ceph-pull-requests/src/test/cli/monmaptool/feature-set-unset-list.t failed

jecluis · 2018-02-07T22:27:59Z

@liewegas yeah, already got a patch for it; will push that later squashed with the appropriate commit, as to avoid voiding any comments on the current state.

jecluis · 2018-03-27T06:01:00Z

http://pulpito.ceph.com/rdias-2018-03-26_09:00:32-rados:monthrash-wip-mon-osdmap-prune-distro-basic-smithi/

Just one of the latest runs (more: http://pulpito.ceph.com/compare/?branch=wip-mon-osdmap-prune&suite=rados:monthrash ). One of the jobs was marked dead for no obvious reason, but the workunit succeeded (and the job had a passing mark in a previous run).

Fairly confident this is ready for merge.

Anyone has any further objections? @liewegas @tchaikov ?

tchaikov · 2018-03-27T07:14:09Z

doc/dev/mon-osdmap-prune.rst

+[1..50000], we would allow the pruning algorithm to operate only over
+osdmap epochs [1..49500); but, if have on-disk versions [1..10200], we
+won't be pruning because the algorithm would only operate on versions
+[1..700), and this interval contains less versions than the minimum


s/700/9700/

fixed, will rebase and squash before merge.

jecluis · 2018-04-06T02:58:18Z

@liewegas ping

liewegas · 2018-04-06T03:06:52Z

@jecluis looks good, can you squash those commits down?

jecluis · 2018-04-06T03:08:46Z

@liewegas on it! :)