osd/PG: invalidate PG if merging with unexpected version

If the source or target PG version is 0'0, we may silently take the max
of the source and target and still leave the PG complete.  This
specifically can happen with an empty PG, as seen with bug 38655.  In
theory we could encounter one of the PGs with some other last_update
that doesn't match what we expect.  If that ever happens, make sure the
result is incomplete so that backfill can clean up.

Additionally check that the pool metadata for the last merge matches the
PGs at all.  This could mismatch if we have an osdmap gap and are forced
to do some merge without merge info at all... in which case we should
definitely invalidate: there should be newer copies of the PG(s), and we
have no idea whether the PGs we are merging are what we want.  If this is
some disaster recovery situation, an operator is always free to use
ceph-objectstore-tool to re-mark a PG complete (at their own peril!).

Fixes: http://tracker.ceph.com/issues/38655
Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/qa/standalone/osd/pg-split-merge.sh b/qa/standalone/osd/pg-split-merge.sh
index 97583da048b69d0f053413f34934dbec7ccae3d9..ad697a9fc3c88182ee764c2bef37bfaf7320bfef 100755 (executable)
--- a/qa/standalone/osd/pg-split-merge.sh
+++ b/qa/standalone/osd/pg-split-merge.sh
@@ -58,19 +58,30 @@ function TEST_a_merge_empty() {
 
     wait_for_clean || return 1
 
-    # now 1.0 is there but 1.1 is not
+    # osd.2: now 1.0 is there but 1.1 is not
+
+    # instantiate 1.1 on osd.2 with last_update=0'0 ('empty'), which is
+    # the problematic state... then let it merge with 1.0
     ceph tell osd.2 config set osd_debug_no_acting_change true
     ceph osd out 0 1
     ceph osd pool set foo pg_num 1
     sleep 5
     ceph tell osd.2 config set osd_debug_no_acting_change false
+
+    # go back to osd.1 being primary, and 3x so the osd.2 copy doesn't get
+    # removed
     ceph osd in 0 1
     ceph osd pool set foo size 3
 
     wait_for_clean || return 1
+
+    # scrub to ensure the osd.3 copy of 1.0 was incomplete (vs missing
+    # half of its objects).
     ceph pg scrub 1.0
-    sleep 5
-    ceph -s | grep inconsistent || return 1
+    sleep 10
+    ceph log last debug
+    ceph pg ls
+    ceph pg ls | grep ' active.clean ' || return 1
 }
 
 function TEST_import_after_merge_and_gap() {

diff --git a/src/osd/PG.cc b/src/osd/PG.cc
index 0fce841bafdd8ea2432a78ff790d9b4631316b97..f68c4ba8ce0a325f54f93acf581eb372b8a471dc 100644 (file)
--- a/src/osd/PG.cc
+++ b/src/osd/PG.cc
@@ -2718,6 +2718,18 @@ void PG::merge_from(map<spg_t,PGRef>& sources, RecoveryCtx *rctx,
     dout(10) << __func__ << " target incomplete" << dendl;
     incomplete = true;
   }
+  if (info.pgid.pgid != last_pg_merge_meta.source_pgid.get_parent()) {
+    dout(10) << __func__ << " target doesn't match expected parent "
+            << last_pg_merge_meta.source_pgid.get_parent()
+            << " of source_pgid " << last_pg_merge_meta.source_pgid
+            << dendl;
+    incomplete = true;
+  }
+  if (info.last_update != last_pg_merge_meta.target_version) {
+    dout(10) << __func__ << " target version doesn't match expected "
+            << last_pg_merge_meta.target_version << dendl;
+    incomplete = true;
+  }
 
   PGLogEntryHandler handler{this, rctx->transaction};
   pg_log.roll_forward(&handler);
@@ -2741,6 +2753,17 @@ void PG::merge_from(map<spg_t,PGRef>& sources, RecoveryCtx *rctx,
               << dendl;
       incomplete = true;
     }
+    if (source->info.pgid.pgid != last_pg_merge_meta.source_pgid) {
+      dout(10) << __func__ << " source " << source->info.pgid.pgid
+              << " doesn't match expected source pgid "
+              << last_pg_merge_meta.source_pgid << dendl;
+      incomplete = true;
+    }
+    if (source->info.last_update != last_pg_merge_meta.source_version) {
+      dout(10) << __func__ << " source version doesn't match expected "
+              << last_pg_merge_meta.target_version << dendl;
+      incomplete = true;
+    }
 
     // prepare log
     PGLogEntryHandler handler{source.get(), rctx->transaction};