osd/OSDMap: fix clean_temps and test

For both pg_temp and primary_temp, we want to calculate what the
mapping *will* be after the inc is applied, and if the mapping is
redundant (it matches what we would have gotten anyway), remove
it.  Removing the mapping might mean putting a 'remove' entry in
the inc, or it might mean removing the proposed addition in the
inc.

Update the unit test to test both cases.

Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/osd/OSDMap.cc b/src/osd/OSDMap.cc
index a6fcd3810b0fe252de497a6c5d2da6fdff077363..cf1c21528cb864a7d2ee715c0934d5add650740d 100644 (file)
--- a/src/osd/OSDMap.cc
+++ b/src/osd/OSDMap.cc
@@ -1210,45 +1210,43 @@ void OSDMap::clean_temps(CephContext *cct,
       continue;
     }
     // redundant pg_temp?
-    if (pending_inc->new_pg_temp.count(p->first) == 0) {
-      vector<int> raw_up;
-      int primary;
-      tmpmap.pg_to_raw_up(p->first, &raw_up, &primary);
-      if (raw_up == p->second) {
-        ldout(cct, 10) << __func__ << "  removing pg_temp " << p->first << " "
-                      << p->second << " that matches raw_up mapping" << dendl;
-        pending_inc->new_pg_temp[p->first].clear();
-       continue;
-      }
+    vector<int> raw_up;
+    int primary;
+    tmpmap.pg_to_raw_up(p->first, &raw_up, &primary);
+    if (raw_up == p->second) {
+      ldout(cct, 10) << __func__ << "  removing pg_temp " << p->first << " "
+                    << p->second << " that matches raw_up mapping" << dendl;
+      if (osdmap.pg_temp->count(p->first))
+       pending_inc->new_pg_temp[p->first].clear();
+      else
+       pending_inc->new_pg_temp.erase(p->first);
     }
   }
-  map<pg_t,int32_t>::iterator p = tmpmap.primary_temp->begin();
-  while (p != tmpmap.primary_temp->end()) {
+  for (map<pg_t,int32_t>::iterator p = tmpmap.primary_temp->begin();
+       p != tmpmap.primary_temp->end();
+       ++p) {
     // primary down?
     if (tmpmap.is_down(p->second)) {
       ldout(cct, 10) << __func__ << "  removing primary_temp " << p->first
                     << " to down " << p->second << dendl;
       pending_inc->new_primary_temp[p->first] = -1;
-      ++p;
       continue;
     }
     // redundant primary_temp?
-    if (pending_inc->new_primary_temp.count(p->first) == 0) {
-      vector<int> real_up, templess_up;
-      int real_primary, templess_primary;
-      pg_t pgid = p->first;
-      tmpmap.pg_to_acting_osds(pgid, &real_up, &real_primary);
-      tmpmap.primary_temp->erase(p++);
-      tmpmap.pg_to_acting_osds(pgid, &templess_up, &templess_primary);
-      if (real_primary == templess_primary){
-       ldout(cct, 10) << __func__ << "  removing primary_temp "
-                      << pgid << " -> " << real_primary
-                      << " (unnecessary/redundant)" << dendl;
+    vector<int> real_up, templess_up;
+    int real_primary, templess_primary;
+    pg_t pgid = p->first;
+    tmpmap.pg_to_acting_osds(pgid, &real_up, &real_primary);
+    tmpmap.pg_to_raw_up(pgid, &templess_up, &templess_primary);
+    if (real_primary == templess_primary){
+      ldout(cct, 10) << __func__ << "  removing primary_temp "
+                    << pgid << " -> " << real_primary
+                    << " (unnecessary/redundant)" << dendl;
+      if (osdmap.primary_temp->count(pgid))
        pending_inc->new_primary_temp[pgid] = -1;
-      }
-      continue;  // we incremented p above
+      else
+       pending_inc->new_primary_temp.erase(pgid);
     }
-    ++p;
   }
 }
 

diff --git a/src/test/osd/TestOSDMap.cc b/src/test/osd/TestOSDMap.cc
index 7723cb620687b1b171ba81f2f6a593144419379e..3410fe7ccbd166eb26d81d8a2a95c4bea82195c0 100644 (file)
--- a/src/test/osd/TestOSDMap.cc
+++ b/src/test/osd/TestOSDMap.cc
@@ -253,26 +253,37 @@ TEST_F(OSDMapTest, PrimaryTempRespected) {
 TEST_F(OSDMapTest, CleanTemps) {
   set_up_map();
 
-  pg_t rawpg(0, 0, -1);
-  pg_t pgid = osdmap.raw_pg_to_pg(rawpg);
-  vector<int> up_osds, acting_osds;
-  int up_primary, acting_primary;
-
-  osdmap.pg_to_up_acting_osds(pgid, &up_osds, &up_primary,
-                              &acting_osds, &acting_primary);
-
-  // stick calculated values in to temps
   OSDMap::Incremental pgtemp_map(osdmap.get_epoch() + 1);
-  pgtemp_map.new_pg_temp[pgid] = up_osds;
-  pgtemp_map.new_primary_temp[pgid] = up_primary;
+  OSDMap::Incremental pending_inc(osdmap.get_epoch() + 2);
+  pg_t pga = osdmap.raw_pg_to_pg(pg_t(0, 0));
+  {
+    vector<int> up_osds, acting_osds;
+    int up_primary, acting_primary;
+    osdmap.pg_to_up_acting_osds(pga, &up_osds, &up_primary,
+                               &acting_osds, &acting_primary);
+    pgtemp_map.new_pg_temp[pga] = up_osds;
+    pgtemp_map.new_primary_temp[pga] = up_primary;
+  }
+  pg_t pgb = osdmap.raw_pg_to_pg(pg_t(1, 0));
+  {
+    vector<int> up_osds, acting_osds;
+    int up_primary, acting_primary;
+    osdmap.pg_to_up_acting_osds(pgb, &up_osds, &up_primary,
+                               &acting_osds, &acting_primary);
+    pending_inc.new_pg_temp[pgb] = up_osds;
+    pending_inc.new_primary_temp[pgb] = up_primary;
+  }
+
   osdmap.apply_incremental(pgtemp_map);
 
-  OSDMap::Incremental pending_inc(osdmap.get_epoch() + 1);
   OSDMap::clean_temps(g_ceph_context, osdmap, &pending_inc);
 
-  EXPECT_TRUE(pending_inc.new_pg_temp.count(pgid) &&
-             pending_inc.new_pg_temp[pgid].size() == 0);
-  EXPECT_EQ(-1, pending_inc.new_primary_temp[pgid]);
+  EXPECT_TRUE(pending_inc.new_pg_temp.count(pga) &&
+             pending_inc.new_pg_temp[pga].size() == 0);
+  EXPECT_EQ(-1, pending_inc.new_primary_temp[pga]);
+
+  EXPECT_TRUE(!pending_inc.new_pg_temp.count(pgb) &&
+             !pending_inc.new_primary_temp.count(pgb));
 }
 
 TEST_F(OSDMapTest, KeepsNecessaryTemps) {