osd: change in up set primary constitutes a peering interval change

In several places, a change in the up_primary triggers a new peering
interval, but the palces that actually generate the new past intervals,
including check_new_interval(), did not enforce that.  This becomes
somewhat obvious when you see that those callers are ignoring the
up_primary output argument for pg_to_up_acting_osds().

Fix this by adding arguments to check_new_interval and fixing the callers
to pass them in properly.  Add a unit test case to verify this.

Note that the past interval struct itself does not record who the
up_primary was; possibly it should.

Fixes: #8139
Signed-off-by: Sage Weil <sage@inktank.com>

diff --git a/src/osd/OSD.cc b/src/osd/OSD.cc
index 4b9f3a36946caf446c11745f03b03466dc7ae6b6..2d6379055b23dbd5438363eff28e47a5999b3cdb 100644 (file)
--- a/src/osd/OSD.cc
+++ b/src/osd/OSD.cc
@@ -2156,6 +2156,7 @@ struct pistate {
   vector<int> old_acting, old_up;
   epoch_t same_interval_since;
   int primary;
+  int up_primary;
 };
 
 void OSD::build_past_intervals_parallel()
@@ -2207,9 +2208,10 @@ void OSD::build_past_intervals_parallel()
        continue;
 
       vector<int> acting, up;
+      int up_primary;
       int primary;
       cur_map->pg_to_up_acting_osds(
-       pg->info.pgid.pgid, &up, 0, &acting, &primary);
+       pg->info.pgid.pgid, &up, &up_primary, &acting, &primary);
 
       if (p.same_interval_since == 0) {
        dout(10) << __func__ << " epoch " << cur_epoch << " pg " << pg->info.pgid
@@ -2219,6 +2221,7 @@ void OSD::build_past_intervals_parallel()
        p.old_up = up;
        p.old_acting = acting;
        p.primary = primary;
+       p.up_primary = up_primary;
        continue;
       }
       assert(last_map);
@@ -2228,6 +2231,8 @@ void OSD::build_past_intervals_parallel()
        p.primary,
        primary,
        p.old_acting, acting,
+       p.up_primary,
+       up_primary,
        p.old_up, up,
        p.same_interval_since,
        pg->info.history.last_epoch_clean,

diff --git a/src/osd/PG.cc b/src/osd/PG.cc
index 32a7becd5b9d8a50aeea68c1888b8584e594c38c..db61ae5693e20b8ddeba3f089ed7a22698e83641 100644 (file)
--- a/src/osd/PG.cc
+++ b/src/osd/PG.cc
@@ -638,17 +638,19 @@ void PG::generate_past_intervals()
 
   OSDMapRef last_map, cur_map;
   int primary = -1;
+  int up_primary = -1;
   vector<int> acting, up, old_acting, old_up;
 
   cur_map = osd->get_map(cur_epoch);
   cur_map->pg_to_up_acting_osds(
-    get_pgid().pgid, &up, 0, &acting, &primary);
+    get_pgid().pgid, &up, &up_primary, &acting, &primary);
   epoch_t same_interval_since = cur_epoch;
   dout(10) << __func__ << " over epochs " << cur_epoch << "-"
           << end_epoch << dendl;
   ++cur_epoch;
   for (; cur_epoch <= end_epoch; ++cur_epoch) {
     int old_primary = primary;
+    int old_up_primary = up_primary;
     last_map.swap(cur_map);
     old_up.swap(up);
     old_acting.swap(acting);
@@ -657,7 +659,7 @@ void PG::generate_past_intervals()
     pg_t pgid = get_pgid().pgid;
     if (cur_map->get_pools().count(pgid.pool()))
       pgid = pgid.get_ancestor(cur_map->get_pg_num(pgid.pool()));
-    cur_map->pg_to_up_acting_osds(pgid, &up, 0, &acting, &primary);
+    cur_map->pg_to_up_acting_osds(pgid, &up, &up_primary, &acting, &primary);
 
     std::stringstream debug;
     bool new_interval = pg_interval_t::check_new_interval(
@@ -665,6 +667,8 @@ void PG::generate_past_intervals()
       primary,
       old_acting,
       acting,
+      old_up_primary,
+      up_primary,
       old_up,
       up,
       same_interval_since,
@@ -4686,6 +4690,8 @@ void PG::start_peering_interval(
       old_acting_primary.osd,
       new_acting_primary,
       oldacting, newacting,
+      old_up_primary.osd,
+      new_up_primary,
       oldup, newup,
       info.history.same_interval_since,
       info.history.last_epoch_clean,

diff --git a/src/osd/osd_types.cc b/src/osd/osd_types.cc
index 33b1b9c345962c08231aa35bff915f36d14c206b..288527b74584fac6c075d618dda89a4abd92f283 100644 (file)
--- a/src/osd/osd_types.cc
+++ b/src/osd/osd_types.cc
@@ -2157,10 +2157,12 @@ void pg_interval_t::generate_test_instances(list<pg_interval_t*>& o)
 }
 
 bool pg_interval_t::check_new_interval(
-  int old_primary,
-  int new_primary,
+  int old_acting_primary,
+  int new_acting_primary,
   const vector<int> &old_acting,
   const vector<int> &new_acting,
+  int old_up_primary,
+  int new_up_primary,
   const vector<int> &old_up,
   const vector<int> &new_up,
   epoch_t same_interval_since,
@@ -2173,8 +2175,13 @@ bool pg_interval_t::check_new_interval(
   std::ostream *out)
 {
   // remember past interval
-  if (old_primary != new_primary ||
-      new_acting != old_acting || new_up != old_up ||
+  //  NOTE: a change in the up set primary triggers an interval
+  //  change, even though the interval members in the pg_interval_t
+  //  do not change.
+  if (old_acting_primary != new_acting_primary ||
+      new_acting != old_acting ||
+      old_up_primary != new_up_primary ||
+      new_up != old_up ||
       (!(lastmap->get_pools().count(pool_id))) ||
       (lastmap->get_pools().find(pool_id)->second.min_size !=
        osdmap->get_pools().find(pool_id)->second.min_size)  ||
@@ -2185,7 +2192,7 @@ bool pg_interval_t::check_new_interval(
     i.last = osdmap->get_epoch() - 1;
     i.acting = old_acting;
     i.up = old_up;
-    i.primary = old_primary;
+    i.primary = old_acting_primary;
 
     if (!i.acting.empty() && i.primary != -1 &&
        i.acting.size() >=

diff --git a/src/osd/osd_types.h b/src/osd/osd_types.h
index 7874977ea92c3fcd4649ff3d2920a9eec08d1977..c8b7ff8675e8d6a2d6e4eb2e5efdf1d3f096b5aa 100644 (file)
--- a/src/osd/osd_types.h
+++ b/src/osd/osd_types.h
@@ -1696,10 +1696,12 @@ struct pg_interval_t {
    * if an interval was closed out.
    */
   static bool check_new_interval(
-    int old_primary,                            ///< [in] primary as of lastmap
-    int new_primary,                            ///< [in] primary as of lastmap
+    int old_acting_primary,                     ///< [in] primary as of lastmap
+    int new_acting_primary,                     ///< [in] primary as of lastmap
     const vector<int> &old_acting,              ///< [in] acting as of lastmap
     const vector<int> &new_acting,              ///< [in] acting as of osdmap
+    int old_up_primary,                         ///< [in] up primary of lastmap
+    int new_up_primary,                         ///< [in] up primary of osdmap
     const vector<int> &old_up,                  ///< [in] up as of lastmap
     const vector<int> &new_up,                  ///< [in] up as of osdmap
     epoch_t same_interval_since,                ///< [in] as of osdmap

diff --git a/src/test/osd/types.cc b/src/test/osd/types.cc
index ac218fa80d3d878d6ffe2c9b58a81585a206d4b8..a04f2cba6132404f05b4c4428eca9a4ba8b3a436 100644 (file)
--- a/src/test/osd/types.cc
+++ b/src/test/osd/types.cc
@@ -155,6 +155,8 @@ TEST(pg_interval_t, check_new_interval)
   int new_primary = osd_id;
   vector<int> new_up;
   new_up.push_back(osd_id);
+  int old_up_primary = osd_id;
+  int new_up_primary = osd_id;
   vector<int> old_up = new_up;
   pg_t pgid;
   pgid.set_pool(pool_id);
@@ -172,6 +174,8 @@ TEST(pg_interval_t, check_new_interval)
                                                   new_primary,
                                                   old_acting,
                                                   new_acting,
+                                                  old_up_primary,
+                                                  new_up_primary,
                                                   old_up,
                                                   new_up,
                                                   same_interval_since,
@@ -200,6 +204,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -231,6 +237,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -263,6 +271,40 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
+                                                 old_up,
+                                                 new_up,
+                                                 same_interval_since,
+                                                 last_epoch_clean,
+                                                 osdmap,
+                                                 lastmap,
+                                                 pool_id,
+                                                 pgid,
+                                                 &past_intervals));
+    ASSERT_EQ((unsigned int)1, past_intervals.size());
+    ASSERT_EQ(same_interval_since, past_intervals[same_interval_since].first);
+    ASSERT_EQ(osdmap->get_epoch() - 1, past_intervals[same_interval_since].last);
+    ASSERT_EQ(osd_id, past_intervals[same_interval_since].acting[0]);
+    ASSERT_EQ(osd_id, past_intervals[same_interval_since].up[0]);
+  }
+
+  //
+  // The up primary has changed
+  //
+  {
+    vector<int> new_up;
+    int _new_up_primary = osd_id + 1;
+
+    map<epoch_t, pg_interval_t> past_intervals;
+
+    ASSERT_TRUE(past_intervals.empty());
+    ASSERT_TRUE(pg_interval_t::check_new_interval(old_primary,
+                                                 new_primary,
+                                                 old_acting,
+                                                 new_acting,
+                                                 old_up_primary,
+                                                 _new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -300,6 +342,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -337,6 +381,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -369,6 +415,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -419,6 +467,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -452,6 +502,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -495,6 +547,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,
@@ -542,6 +596,8 @@ TEST(pg_interval_t, check_new_interval)
                                                  new_primary,
                                                  old_acting,
                                                  new_acting,
+                                                 old_up_primary,
+                                                 new_up_primary,
                                                  old_up,
                                                  new_up,
                                                  same_interval_since,