jewel: osd: condition OSDMap encoding on features #12167

liewegas · 2016-11-23T19:59:55Z

http://tracker.ceph.com/issues/18015

Align this with decode. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 01d9e8a)

If the target features are missing the new OSDOp encoding, the first feature we added post-hammer, encode like hammer. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 2f8cfb6)

ghost · 2016-11-23T20:26:08Z

make check will fail on flake8: this is expected and harmless (environmental)

athanatos · 2016-11-23T20:43:42Z

src/mon/OSDMonitor.cc

@@ -3269,7 +3269,7 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
      rdata.append(osdmap_bl);
      ss << "got osdmap epoch " << p->get_epoch();
    } else if (prefix == "osd getcrushmap") {
-      p->crush->encode(rdata);
+      p->crush->encode(rdata, CEPH_FEATURES_SUPPORTED_DEFAULT);


Why not quorum features here?

they're going back to the cli caller; doesn't matter i guess

athanatos · 2016-11-23T20:43:52Z

src/mon/OSDMonitor.cc

@@ -97,7 +97,7 @@ void OSDMonitor::_get_pending_crush(CrushWrapper& newcrush)
  if (pending_inc.crush.length())
    bl = pending_inc.crush;
  else
-    osdmap.crush->encode(bl);
+    osdmap.crush->encode(bl, CEPH_FEATURES_SUPPORTED_DEFAULT);


It turns out the features for the pending OSDMap::Incremental don't matter. The canonical mon map and the osdmap full map both encode from OSDMap.. they don't use the encoded crush in the incremental.

athanatos · 2016-11-23T20:44:31Z

src/mon/OSDMonitor.cc

@@ -5569,7 +5569,7 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
    }

    pending_inc.crush.clear();
-    newcrush.encode(pending_inc.crush);
+    newcrush.encode(pending_inc.crush, CEPH_FEATURES_SUPPORTED_DEFAULT);


Why not quorum features here, looks like you're updating the pending_inc, right?

Meh.. okay, i'll make these consistent

athanatos · 2016-11-23T20:45:07Z

src/osd/OSDMap.cc

-  ::encode(*o->crush, oc);
-  ::encode(*n->crush, nc);
+  ::encode(*o->crush, oc, CEPH_FEATURES_SUPPORTED_DEFAULT);
+  ::encode(*n->crush, nc, CEPH_FEATURES_SUPPORTED_DEFAULT);


Would this work if DEFAULT != the features actually used to encode the map?

yeah; all we're doing in an operator== on teh map itself; encoding is just our lame way to implementing that

Oh, we're encoding both rather than comparing to an existing encoded buffer. Ok

athanatos · 2016-11-23T21:16:35Z

I withdraw my questions, makes sense.

liewegas · 2016-11-23T22:55:37Z

Updated OSDMonitor.cc to be more consistent.

ghost · 2016-11-24T08:13:15Z

jenkins test this please (ceph-disk test timedout https://jenkins.ceph.com/job/ceph-pull-requests/14870/console verifying this is real)

ghost · 2016-11-24T08:17:08Z

A reminder to update the commit message at "crush/CrushWrapper: encode with features" and " crush/CrushWrapper: drop unused 'lean' encode() argument" to explain how the conflicts were resolved.

ghost · 2016-11-24T13:42:14Z

jenkins test this please (unittest_pglog)

ghost · 2016-11-24T14:52:08Z

make check passes at https://jenkins.ceph.com/job/ceph-pull-requests/14905/ with one expected failure (flake8)

liewegas · 2016-11-26T15:49:20Z

this is incomplete; still seeing an encoding disparity i need to track down

liewegas · 2016-11-26T20:11:36Z

I take it back--I think the last issue is fixed, this should be ready.

Also merge this with ceph-qa-suite: ceph/ceph-qa-suite#1285

liewegas · 2016-11-26T20:16:58Z

http://pulpito.ceph.com/sage-2016-11-26_20:15:21-upgrade:hammer-x-jewel---basic-smithi/

liewegas · 2016-11-28T19:31:18Z

http://pulpito.ceph.com/sage-2016-11-28_16:58:23-upgrade:hammer-x-jewel---basic-smithi/

calling this a pass wrt to the osdmap encoding changes.. 👍 on merging this!

theanalyst · 2016-11-28T19:33:51Z

@liewegas do you want this in for 10.2.4

No callers, no users. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 638a38b) Conflicts: src/crush/CrushWrapper.h [trivial conflict due to removal of write_file and read_file]

No behavior change yet; just fixing callers. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit b7c9e05) [Updated write_file to use all feaetures] [Updated OSDMonitor.cc to use mon->quorum_features instead of the mon->get_quorum_con_featuers() helper] [trivial conflict from removed write_file and read_file] Conflicts: src/crush/CrushWrapper.h src/mgr/PyModules.cc src/mon/OSDMonitor.cc src/tools/ceph_monstore_tool.cc

This avoids throwing hammer OSDMap encodings off. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 9e5ff86)

liewegas · 2016-11-28T19:36:29Z

updated commit messages about (trivial) conflicts

liewegas · 2016-11-28T20:14:51Z

yes please

theanalyst · 2016-11-28T22:23:22Z

test this please

ghost · 2016-12-05T08:36:22Z

test this please (jenkins hasty expiration of logs)

theanalyst · 2016-12-05T11:51:57Z

@liewegas @dachary this has passed the rados run (with exception of valgrind issue) and details captured here http://tracker.ceph.com/issues/17487#note-37, Do you think it is ready to be merged into jewel yet?

ghost · 2016-12-05T13:49:53Z

the make check failure is only about flake8 and is expected (https://jenkins.ceph.com/job/ceph-pull-requests/15330/)

liewegas added 2 commits November 23, 2016 14:38

osd/osd_types: conditional pg_pool_t encoding

85caf34

Align this with decode. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 01d9e8a)

osd/osd_types: encode pg_pool_t like hammer if features indicate hammer

c66c556

If the target features are missing the new OSDOp encoding, the first feature we added post-hammer, encode like hammer. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 2f8cfb6)

liewegas added this to the jewel milestone Nov 23, 2016

liewegas added the core label Nov 23, 2016

ghost changed the title ~~osd: condition OSDMap encoding on features (jewel)~~ jewel: osd: condition OSDMap encoding on features (jewel) Nov 23, 2016

ghost self-assigned this Nov 23, 2016

ghost added the bug-fix label Nov 23, 2016

ghost changed the title ~~jewel: osd: condition OSDMap encoding on features (jewel)~~ jewel: osd: condition OSDMap encoding on features Nov 23, 2016

athanatos reviewed Nov 23, 2016

View reviewed changes

liewegas force-pushed the wip-osdmap-encoding-jewel branch from a4bb664 to aaf4a7f Compare November 23, 2016 21:46

liewegas changed the title ~~jewel: osd: condition OSDMap encoding on features~~ DNM: jewel: osd: condition OSDMap encoding on features Nov 26, 2016

liewegas changed the title ~~DNM: jewel: osd: condition OSDMap encoding on features~~ jewel: osd: condition OSDMap encoding on features Nov 26, 2016

liewegas added 3 commits November 28, 2016 14:35

crush/CrushWrapper: drop unused 'lean' encode() argument

c5f5b94

No callers, no users. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 638a38b) Conflicts: src/crush/CrushWrapper.h [trivial conflict due to removal of write_file and read_file]

crush: condition latest tunable encoding on features

4d6f848

This avoids throwing hammer OSDMap encodings off. Signed-off-by: Sage Weil <sage@redhat.com> (cherry picked from commit 9e5ff86)

liewegas force-pushed the wip-osdmap-encoding-jewel branch from aaf4a7f to 4d6f848 Compare November 28, 2016 19:36

ghost merged commit a944491 into ceph:jewel Dec 5, 2016

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jewel: osd: condition OSDMap encoding on features #12167

jewel: osd: condition OSDMap encoding on features #12167

liewegas commented Nov 23, 2016 •

edited by ghost

ghost commented Nov 23, 2016

athanatos Nov 23, 2016

liewegas Nov 23, 2016

athanatos Nov 23, 2016

liewegas Nov 23, 2016

athanatos Nov 23, 2016

liewegas Nov 23, 2016

athanatos Nov 23, 2016

liewegas Nov 23, 2016

athanatos Nov 23, 2016

athanatos commented Nov 23, 2016

liewegas commented Nov 23, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 28, 2016

theanalyst commented Nov 28, 2016

liewegas commented Nov 28, 2016

liewegas commented Nov 28, 2016 via email

theanalyst commented Nov 28, 2016

ghost commented Dec 5, 2016

theanalyst commented Dec 5, 2016

ghost commented Dec 5, 2016

jewel: osd: condition OSDMap encoding on features #12167

jewel: osd: condition OSDMap encoding on features #12167

Conversation

liewegas commented Nov 23, 2016 • edited by ghost

ghost commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athanatos commented Nov 23, 2016

liewegas commented Nov 23, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

ghost commented Nov 24, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 26, 2016

liewegas commented Nov 28, 2016

theanalyst commented Nov 28, 2016

liewegas commented Nov 28, 2016

liewegas commented Nov 28, 2016 via email

theanalyst commented Nov 28, 2016

ghost commented Dec 5, 2016

theanalyst commented Dec 5, 2016

ghost commented Dec 5, 2016

liewegas commented Nov 23, 2016 •

edited by ghost