From 67019f5b02772daac62c7f9ea305d783aaccbd01 Mon Sep 17 00:00:00 2001
From: Vitaliy Filippov <vitalif@yourcmc.ru>
Date: Sat, 31 Dec 2022 02:30:19 +0300
Subject: [PATCH] Make OSD sort & sanitize PG history items

---
 src/etcd_state_client.cpp | 24 +++++++++++++++++++-----
 src/osd_cluster.cpp       |  1 +
 src/osd_peering.cpp       | 16 ++++++++++++----
 src/osd_peering_pg.cpp    | 11 +++++++----
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/src/etcd_state_client.cpp b/src/etcd_state_client.cpp
index bd28d5cb..a753fabf 100644
--- a/src/etcd_state_client.cpp
+++ b/src/etcd_state_client.cpp
@@ -871,19 +871,33 @@ void etcd_state_client_t::parse_state(const etcd_kv_t & kv)
             pg_cfg.target_history.clear();
             pg_cfg.all_peers.clear();
             // Refuse to start PG if any set of the <osd_sets> has no live OSDs
-            for (auto hist_item: value["osd_sets"].array_items())
+            for (auto & hist_item: value["osd_sets"].array_items())
             {
                 std::vector<osd_num_t> history_set;
-                for (auto pg_osd: hist_item.array_items())
+                for (auto & pg_osd: hist_item.array_items())
                 {
-                    history_set.push_back(pg_osd.uint64_value());
+                    osd_num_t pg_osd_num = pg_osd.uint64_value();
+                    if (pg_osd_num != 0)
+                    {
+                        auto it = std::lower_bound(history_set.begin(), history_set.end(), pg_osd_num);
+                        if (it == history_set.end() || *it != pg_osd_num)
+                            history_set.insert(it, pg_osd_num);
+                    }
                 }
-                pg_cfg.target_history.push_back(history_set);
+                auto it = std::lower_bound(pg_cfg.target_history.begin(), pg_cfg.target_history.end(), history_set);
+                if (it == pg_cfg.target_history.end() || *it != history_set)
+                    pg_cfg.target_history.insert(it, history_set);
             }
             // Include these additional OSDs when peering the PG
             for (auto pg_osd: value["all_peers"].array_items())
             {
-                pg_cfg.all_peers.push_back(pg_osd.uint64_value());
+                osd_num_t pg_osd_num = pg_osd.uint64_value();
+                if (pg_osd_num != 0)
+                {
+                    auto it = std::lower_bound(pg_cfg.all_peers.begin(), pg_cfg.all_peers.end(), pg_osd_num);
+                    if (it == pg_cfg.all_peers.end() || *it != pg_osd_num)
+                        pg_cfg.all_peers.insert(it, pg_osd_num);
+                }
             }
             // Read epoch
             pg_cfg.epoch = value["epoch"].uint64_value();
diff --git a/src/osd_cluster.cpp b/src/osd_cluster.cpp
index 4ff3ac52..52a17861 100644
--- a/src/osd_cluster.cpp
+++ b/src/osd_cluster.cpp
@@ -885,6 +885,7 @@ void osd_t::report_pg_states()
             if (pg.history_changed)
             {
                 // Prevent race conditions (for the case when the monitor is updating this key at the same time)
+                // FIXME: target_history updates may be lost on PG re-peering
                 pg.history_changed = false;
                 std::string history_key = base64_encode(st_cli.etcd_prefix+"/pg/history/"+std::to_string(pg.pool_id)+"/"+std::to_string(pg.pg_num));
                 json11::Json::object history_value = {
diff --git a/src/osd_peering.cpp b/src/osd_peering.cpp
index 69a3b1a7..b51cc9bc 100644
--- a/src/osd_peering.cpp
+++ b/src/osd_peering.cpp
@@ -560,13 +560,17 @@ void osd_t::report_pg_state(pg_t & pg)
         pg.history_changed = true;
         pg.target_history.clear();
         pg.all_peers = pg.target_set;
+        std::sort(pg.all_peers.begin(), pg.all_peers.end());
         pg.cur_peers = pg.target_set;
     }
     else if (pg.state == (PG_ACTIVE|PG_LEFT_ON_DEAD))
     {
         // Clear history of active+left_on_dead PGs, but leave dead OSDs in all_peers
-        pg.history_changed = true;
-        pg.target_history.clear();
+        if (pg.target_history.size())
+        {
+            pg.history_changed = true;
+            pg.target_history.clear();
+        }
         std::set<osd_num_t> dead_peers;
         for (auto pg_osd: pg.all_peers)
         {
@@ -583,8 +587,12 @@ void osd_t::report_pg_state(pg_t & pg)
                 dead_peers.insert(pg_osd);
             }
         }
-        pg.all_peers.clear();
-        pg.all_peers.insert(pg.all_peers.begin(), dead_peers.begin(), dead_peers.end());
+        auto new_all_peers = std::vector<osd_num_t>(dead_peers.begin(), dead_peers.end());
+        if (pg.all_peers != new_all_peers)
+        {
+            pg.history_changed = true;
+            pg.all_peers = new_all_peers;
+        }
         pg.cur_peers.clear();
         for (auto pg_osd: pg.target_set)
         {
diff --git a/src/osd_peering_pg.cpp b/src/osd_peering_pg.cpp
index 4ebc1642..8f3ba184 100644
--- a/src/osd_peering_pg.cpp
+++ b/src/osd_peering_pg.cpp
@@ -92,12 +92,15 @@ void pg_obj_state_check_t::walk()
         for (auto peer_osd: pg->cur_set)
         {
             if (peer_osd != 0)
-            {
                 history_set.push_back(peer_osd);
-            }
         }
-        pg->target_history.push_back(history_set);
-        pg->history_changed = true;
+        std::sort(history_set.begin(), history_set.end());
+        auto it = std::lower_bound(pg->target_history.begin(), pg->target_history.end(), history_set);
+        if (it == pg->target_history.end() || *it != history_set)
+        {
+            pg->target_history.insert(it, history_set);
+            pg->history_changed = true;
+        }
     }
     else
     {