[yugabyte#8204] ybase: GetLoadMoveCompletionPercent returns an incorrect 100% if tservers haven't

heartbeated their tablet reports

Summary:
Currently, if tablet servers don't check-in their tablets after a master leader failover, the
GetLoadMoveCompletionPercent incorrectly returns a 100 which can be catastrophic.

If we don't find any load on the blacklisted tservers then it can be because the tservers
are yet to heartbeat their tablet reports to this master. We shouldn't give a false
presumption that the load transfer is complete.
We expect that by load_balancer_initial_delay_secs time, this should go away and if the
load is reported as 0 on the blacklisted tservers after this time then it means that
the transfer is successfully complete.

Test Plan: ybd --cxx-test master_failover-itest --gtest_filter MasterFailoverTest.TestLoadMoveCompletion

Reviewers: nicolas, bogdan, rahuldesirazu

Reviewed By: rahuldesirazu

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D11613

Author: sanketkedia

src/yb/integration-tests/master_failover-itest.cc

@@ -496,18 +496,16 @@ TEST_F(MasterFailoverTest, TestLoadMoveCompletion) {
                 MonoDelta::FromSeconds(60),
                 "Load Balancer Idle check failed"));
 
-  // Delay TS Heartbeats by 40 times of original rate.
-  ASSERT_OK(cluster_->SetFlagOnTServers("heartbeat_interval_ms",
-                            std::to_string(kHeartbeatIntervalMs * 40)));
-
-  // Wait for the delay to take effect.
-  // Approximately let's give it 4 cycles.
-  SleepFor(MonoDelta::FromMilliseconds(4 * kHeartbeatIntervalMs));
+  // Disable TS heartbeats.
+  LOG(INFO) << "Disabled Heartbeats";
+  ASSERT_OK(cluster_->SetFlagOnTServers("tserver_disable_heartbeat_test_only",
+                                        "true"));
 
   // Blacklist a TS.
   ExternalMaster *leader = cluster_->GetLeaderMaster();
   ExternalTabletServer *ts = cluster_->tablet_server(3);
   ASSERT_OK(cluster_->AddTServerToBlacklist(leader, ts));
+  LOG(INFO) << "Blacklisted tserver#3";
 
   // Get the initial load.
   int idx = -1;
@@ -523,6 +521,7 @@ TEST_F(MasterFailoverTest, TestLoadMoveCompletion) {
   int initial_total_load = resp.total();
 
   // Failover the leader.
+  LOG(INFO) << "Failing over master leader.";
   ASSERT_OK(cluster_->StepDownMasterLeaderAndWaitForNewLeader());
 
   // Get the final load and validate.
@@ -534,9 +533,35 @@ TEST_F(MasterFailoverTest, TestLoadMoveCompletion) {
 
   proxy = cluster_->master_proxy(idx);
   ASSERT_OK(proxy->GetLoadMoveCompletion(req, &resp, &rpc));
+  LOG(INFO) << "Initial loads. Before master leader failover: " <<  initial_total_load
+            << " v/s after master leader failover: " << resp.total();
 
   EXPECT_EQ(resp.total(), initial_total_load) << "Expected the initial blacklisted load"
                                   " to be propagated to new leader master.";
+
+  // The progress should be reported as 0 until tservers heartbeat
+  // their tablet reports.
+  EXPECT_EQ(resp.percent(), 0) << "Expected the initial progress"
+                                  " to be zero.";
+
+  // Now enable heartbeats.
+  ASSERT_OK(cluster_->SetFlagOnTServers("tserver_disable_heartbeat_test_only",
+                                        "false"));
+  ASSERT_OK(cluster_->SetFlagOnMasters("blacklist_progress_initial_delay_secs",
+                                        std::to_string((kHeartbeatIntervalMs * 20)/1000)));
+  LOG(INFO) << "Enabled heartbeats";
+
+  ASSERT_OK(LoggedWaitFor(
+    [&]() -> Result<bool> {
+      req.Clear();
+      resp.Clear();
+      rpc.Reset();
+      RETURN_NOT_OK(proxy->GetLoadMoveCompletion(req, &resp, &rpc));
+      return resp.percent() >= 100;
+    },
+    MonoDelta::FromSeconds(300),
+    "Waiting for blacklist load transfer to complete"
+  ));
 }
 
 class MasterFailoverTestWithPlacement : public MasterFailoverTest {

src/yb/master/catalog_manager.cc

@@ -374,6 +374,12 @@ TAG_FLAG(TEST_disable_setting_tablespace_id_at_creation, runtime);
 DEFINE_test_flag(bool, crash_server_on_sys_catalog_leader_affinity_move, false,
                  "When set, crash the master process if it performs a sys catalog leader affinity "
                  "move.");
+DEFINE_int32(blacklist_progress_initial_delay_secs, yb::master::kDelayAfterFailoverSecs,
+             "When a master leader failsover, the time until which the progress of load movement "
+             "off the blacklisted tservers is reported as 0. This initial delay "
+             "gives sufficient time for heartbeats so that we don't report"
+             " a premature incorrect completion.");
+TAG_FLAG(blacklist_progress_initial_delay_secs, runtime);
 
 namespace yb {
 namespace master {
@@ -9098,18 +9104,34 @@ Status CatalogManager::GetLeaderBlacklistCompletionPercent(GetLoadMovePercentRes
 }
 
 Status CatalogManager::GetLoadMoveCompletionPercent(GetLoadMovePercentResponsePB* resp,
-    bool blacklist_leader) {
+                                                    bool blacklist_leader) {
   auto l = cluster_config_->LockForRead();
 
   // Fine to pass in empty defaults if server_blacklist or leader_blacklist is not filled.
   const BlacklistPB& state = blacklist_leader ? l->pb.leader_blacklist() : l->pb.server_blacklist();
   int64_t blacklist_replicas = GetNumRelevantReplicas(state, blacklist_leader);
   int64_t initial_load = (blacklist_leader) ?
                                 state.initial_leader_load(): state.initial_replica_load();
+  // If we are starting up and don't find any load on the tservers, return progress as 0.
+  // We expect that by blacklist_progress_initial_delay_secs time, this should go away and if the
+  // load is reported as 0 on the blacklisted tservers after this time then it means that
+  // the transfer is successfully complete.
+  if (blacklist_replicas == 0 &&
+  TimeSinceElectedLeader() <= MonoDelta::FromSeconds(FLAGS_blacklist_progress_initial_delay_secs)) {
+      LOG(INFO) << "Master leadership has changed. Reporting progress as 0 until the catalog " <<
+                   "manager gets the correct estimates of the remaining load on the blacklisted" <<
+                   "tservers.";
+      resp->set_percent(0);
+      resp->set_total(initial_load);
+      resp->set_remaining(initial_load);
+      return Status::OK();
+  }
 
   // On change of master leader, initial_load_ information may be lost temporarily. Reset to
   // current value to avoid reporting progress percent as 100. Note that doing so will report
   // progress percent as 0 instead.
+  // TODO(Sanket): This might be no longer relevant after we persist and load the initial load
+  // on failover. Need to investigate.
   if (initial_load < blacklist_replicas) {
     LOG(INFO) << Format("Initial load: $0, current load: $1."
               " Initial load is less than the current load. Probably a master leader change."

src/yb/master/catalog_manager.h

@@ -128,6 +128,7 @@ static const char* const kColocatedParentTableIdSuffix = ".colocated.parent.uuid
 static const char* const kColocatedParentTableNameSuffix = ".colocated.parent.tablename";
 static const char* const kTablegroupParentTableIdSuffix = ".tablegroup.parent.uuid";
 static const char* const kTablegroupParentTableNameSuffix = ".tablegroup.parent.tablename";
+static const int32 kDelayAfterFailoverSecs = 120;
 
 using PlacementId = std::string;
 

src/yb/master/catalog_manager_bg_tasks.cc

@@ -50,7 +50,7 @@ DEFINE_int32(catalog_manager_bg_task_wait_ms, 1000,
              "between runs");
 TAG_FLAG(catalog_manager_bg_task_wait_ms, hidden);
 
-DEFINE_int32(load_balancer_initial_delay_secs, 120,
+DEFINE_int32(load_balancer_initial_delay_secs, yb::master::kDelayAfterFailoverSecs,
              "Amount of time to wait between becoming master leader and enabling the load "
              "balancer.");