osd/OSDMap: do not trust partially simplified pg_upmap_item

If we simplified a partially no-op pg_upmap_item, we shall still
continue to verify that the remaining part is valid.
The bug is introduced by 02e5499b350bcd7d9eac98b2072052a9a4a1f535,
before which we always validate the correctness of a pg_upmap_item
before trying to cancel or simplify it.

Fixes: https://tracker.ceph.com/issues/42052
Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>
(cherry picked from commit 4196b13283144de966eeba40e6765f10b254dac6)

Conflicts:
	src/osd/OSDMap.cc

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index ed2e36f37e1783c1f2505f978756952fd96e93d7..cd8f3191e57624df2594d93af015ba0fb4775afb 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -1637,45 +1637,6 @@ bool OSDMap::check_pg_upmaps(
     }
     vector<int> raw, up;
     pg_to_raw_upmap(pg, &raw, &up);
-    auto i = pg_upmap.find(pg);
-    if (i != pg_upmap.end() && vectors_equal(raw, i->second)) {
-      ldout(cct, 10) << " removing redundant pg_upmap "
-                     << i->first << " " << i->second
-                     << dendl;
-      to_cancel->push_back(pg);
-      continue;
-    }
-    auto j = pg_upmap_items.find(pg);
-    if (j != pg_upmap_items.end()) {
-      mempool::osdmap::vector<pair<int,int>> newmap;
-      for (auto& p : j->second) {
-        if (std::find(raw.begin(), raw.end(), p.first) == raw.end()) {
-          // cancel mapping if source osd does not exist anymore
-          continue;
-        }
-        if (p.second != CRUSH_ITEM_NONE && p.second < max_osd &&
-            p.second >= 0 && osd_weight[p.second] == 0) {
-          // cancel mapping if target osd is out
-          continue;
-        }
-        newmap.push_back(p);
-      }
-      if (newmap.empty()) {
-        ldout(cct, 10) << " removing no-op pg_upmap_items "
-                       << j->first << " " << j->second
-                       << dendl;
-        to_cancel->push_back(pg);
-        continue;
-      } else if (newmap != j->second) {
-        ldout(cct, 10) << " simplifying partially no-op pg_upmap_items "
-                       << j->first << " " << j->second
-                       << " -> " << newmap
-                       << dendl;
-        to_remap->insert({pg, newmap});
-        any_change = true;
-        continue;
-      }
-    }
     auto crush_rule = get_pg_pool_crush_rule(pg);
     auto r = crush->verify_upmap(cct,
                                  crush_rule,
@@ -1720,6 +1681,47 @@ bool OSDMap::check_pg_upmaps(
         break;
       }
     }
+    if (!to_cancel->empty() && to_cancel->back() == pg)
+      continue;
+    // okay, upmap is valid
+    // continue to check if it is still necessary
+    auto i = pg_upmap.find(pg);
+    if (i != pg_upmap.end() && vectors_equal(raw, i->second)) {
+      ldout(cct, 10) << " removing redundant pg_upmap "
+                     << i->first << " " << i->second
+                     << dendl;
+      to_cancel->push_back(pg);
+      continue;
+    }
+    auto j = pg_upmap_items.find(pg);
+    if (j != pg_upmap_items.end()) {
+      mempool::osdmap::vector<pair<int,int>> newmap;
+      for (auto& p : j->second) {
+        if (std::find(raw.begin(), raw.end(), p.first) == raw.end()) {
+          // cancel mapping if source osd does not exist anymore
+          continue;
+        }
+        if (p.second != CRUSH_ITEM_NONE && p.second < max_osd &&
+            p.second >= 0 && osd_weight[p.second] == 0) {
+          // cancel mapping if target osd is out
+          continue;
+        }
+        newmap.push_back(p);
+      }
+      if (newmap.empty()) {
+        ldout(cct, 10) << " removing no-op pg_upmap_items "
+                       << j->first << " " << j->second
+                       << dendl;
+        to_cancel->push_back(pg);
+      } else if (newmap != j->second) {
+        ldout(cct, 10) << " simplifying partially no-op pg_upmap_items "
+                       << j->first << " " << j->second
+                       << " -> " << newmap
+                       << dendl;
+        to_remap->insert({pg, newmap});
+        any_change = true;
+      }
+    }
   }
   any_change = any_change || !to_cancel->empty();
   return any_change;

diff --git a/src/test/osd/TestOSDMap.cc b/src/test/osd/TestOSDMap.cc
index 622c35c8e185584d3ae47ad72dc81226948ce278..42c1bc590d57987cd6d491684712790deb78ff3b 100644 (file)
--- a/src/test/osd/TestOSDMap.cc
+++ b/src/test/osd/TestOSDMap.cc
@@ -1361,6 +1361,92 @@ TEST_F(OSDMapTest, BUG_40104) {
   }
 }
 
+TEST_F(OSDMapTest, BUG_42052) {
+  // https://tracker.ceph.com/issues/42052
+  set_up_map(6, true);
+  const string pool_name("pool");
+  // build customized crush rule for "pool"
+  CrushWrapper crush;
+  get_crush(osdmap, crush);
+  string rule_name = "rule";
+  int rule_type = pg_pool_t::TYPE_REPLICATED;
+  ASSERT_TRUE(!crush.rule_exists(rule_name));
+  int rno;
+  for (rno = 0; rno < crush.get_max_rules(); rno++) {
+    if (!crush.rule_exists(rno) && !crush.ruleset_exists(rno))
+      break;
+  }
+  int min_size = 3;
+  int max_size = 3;
+  int steps = 8;
+  crush_rule *rule = crush_make_rule(steps, rno, rule_type, min_size, max_size);
+  int step = 0;
+  crush_rule_set_step(rule, step++, CRUSH_RULE_SET_CHOOSELEAF_TRIES, 5, 0);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_SET_CHOOSE_TRIES, 100, 0);
+  // always choose osd.0, osd.1, osd.2
+  crush_rule_set_step(rule, step++, CRUSH_RULE_TAKE, 0, 0);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_EMIT, 0, 0);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_TAKE, 0, 1);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_EMIT, 0, 0);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_TAKE, 0, 2);
+  crush_rule_set_step(rule, step++, CRUSH_RULE_EMIT, 0, 0);
+  ASSERT_TRUE(step == steps);
+  auto r = crush_add_rule(crush.get_crush_map(), rule, rno);
+  ASSERT_TRUE(r >= 0);
+  crush.set_rule_name(rno, rule_name);
+  {
+    OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
+    pending_inc.crush.clear();
+    crush.encode(pending_inc.crush, CEPH_FEATURES_SUPPORTED_DEFAULT);
+    osdmap.apply_incremental(pending_inc);
+  }
+
+  // create "pool"
+  OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
+  pending_inc.new_pool_max = osdmap.get_pool_max();
+  auto pool_id = ++pending_inc.new_pool_max;
+  pg_pool_t empty;
+  auto p = pending_inc.get_new_pool(pool_id, &empty);
+  p->size = 3;
+  p->min_size = 1;
+  p->set_pg_num(1);
+  p->set_pgp_num(1);
+  p->type = pg_pool_t::TYPE_REPLICATED;
+  p->crush_rule = rno;
+  p->set_flag(pg_pool_t::FLAG_HASHPSPOOL);
+  pending_inc.new_pool_names[pool_id] = pool_name;
+  osdmap.apply_incremental(pending_inc);
+  ASSERT_TRUE(osdmap.have_pg_pool(pool_id));
+  ASSERT_TRUE(osdmap.get_pool_name(pool_id) == pool_name);
+  pg_t rawpg(0, pool_id);
+  pg_t pgid = osdmap.raw_pg_to_pg(rawpg);
+  {
+    // pg_upmap 1.0 [2,3,5]
+    vector<int32_t> new_up{2,3,5};
+    OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
+    pending_inc.new_pg_upmap[pgid] = mempool::osdmap::vector<int32_t>(
+      new_up.begin(), new_up.end());
+    osdmap.apply_incremental(pending_inc);
+  }
+  {
+    // pg_upmap_items 1.0 [0,3,4,5]
+    vector<pair<int32_t,int32_t>> new_pg_upmap_items;
+    new_pg_upmap_items.push_back(make_pair(0, 3));
+    new_pg_upmap_items.push_back(make_pair(4, 5));
+    OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
+    pending_inc.new_pg_upmap_items[pgid] =
+    mempool::osdmap::vector<pair<int32_t,int32_t>>(
+      new_pg_upmap_items.begin(), new_pg_upmap_items.end());
+    osdmap.apply_incremental(pending_inc);
+  }
+  {
+    OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
+    clean_pg_upmaps(g_ceph_context, osdmap, pending_inc);
+    osdmap.apply_incremental(pending_inc);
+    ASSERT_FALSE(osdmap.have_pg_upmaps(pgid));
+  }
+}
+
 TEST(PGTempMap, basic)
 {
   PGTempMap m;