Retry marking shards as PROVISIONED on VERSION_MISMATCH

Summary: Marking shards as provisioned can fail because of races with other shards during cluster startup. Let's harden it against that by retries with exponential backoffs and jitters.

Reviewed By: AhmedSoliman

Differential Revision: D17761397

fbshipit-source-id: 6382c0dbff52380a9832be4a1244505815902b39

Author: MohamedBassem

logdevice/server/RebuildingCoordinator.cpp

@@ -306,7 +306,6 @@ int RebuildingCoordinator::checkMarkers() {
   RebuildingMarkerChecker checker(
       storage_mem->getShardStates(getMyNodeID().index()),
       getMyNodeID(),
-      storage_mem->getVersion(),
       processor_->getNodesConfigurationManager(),
       shardedStore_);
 

logdevice/server/RebuildingMarkerChecker.cpp

@@ -10,17 +10,17 @@
 
 #include <tuple>
 
+#include "logdevice/common/RetryHandler.h"
+
 namespace facebook { namespace logdevice {
 
 RebuildingMarkerChecker::RebuildingMarkerChecker(
     const std::unordered_map<shard_index_t, membership::ShardState>& shards,
     NodeID my_node_id,
-    membership::MembershipVersion::Type storage_version,
     configuration::NodesConfigurationAPI* nc_api,
     ShardedLocalLogStore* sharded_store)
     : shards_(shards),
       my_node_id_(std::move(my_node_id)),
-      storage_version_(storage_version),
       nc_api_(nc_api),
       sharded_store_(sharded_store) {
   ld_check(sharded_store_);
@@ -124,37 +124,59 @@ Status RebuildingMarkerChecker::markShardsAsProvisioned(
     return Status::OK;
   }
 
-  ld_info("Will mark shards %s as provisioned in the NodesConfiguration",
-          toString(to_be_updated).c_str());
-
-  using namespace configuration::nodes;
-  using namespace membership;
-
-  // Build the MARK_SHARD_PROVISIONED update
-  NodesConfiguration::Update update;
-  update.storage_config_update = std::make_unique<StorageConfig::Update>();
-  update.storage_config_update->membership_update =
-      std::make_unique<StorageMembership::Update>(storage_version_);
-  for (auto shard : shards) {
-    update.storage_config_update->membership_update->addShard(
-        ShardID(my_node_id_.index(), shard),
-        {
-            StorageStateTransition::MARK_SHARD_PROVISIONED,
-            Condition::EMPTY_SHARD | Condition::LOCAL_STORE_READABLE |
-                Condition::NO_SELF_REPORT_MISSING_DATA,
-        });
-  }
-
-  // Apply the update
-  Status update_status;
-  folly::Baton<> b;
-  nc_api_->update(std::move(update),
-                  [&](Status st, std::shared_ptr<const NodesConfiguration>) {
-                    update_status = st;
-                    b.post();
-                  });
-  b.wait();
-  return update_status;
+  auto result = RetryHandler<Status>::syncRun(
+      [&](size_t trial_num) {
+        ld_info("Will mark shards %s as PROVISIONED in the NodesConfiguration "
+                "(trial #%ld)",
+                toString(to_be_updated).c_str(),
+                trial_num);
+
+        using namespace configuration::nodes;
+        using namespace membership;
+
+        auto storage_membership = nc_api_->getConfig()->getStorageMembership();
+        auto storage_version = storage_membership->getVersion();
+
+        // Build the MARK_SHARD_PROVISIONED update
+        NodesConfiguration::Update update;
+        update.storage_config_update =
+            std::make_unique<StorageConfig::Update>();
+        update.storage_config_update->membership_update =
+            std::make_unique<StorageMembership::Update>(storage_version);
+        for (auto shard : shards) {
+          auto shard_id = ShardID(my_node_id_.index(), shard);
+          if (storage_membership->getShardState(shard_id)->storage_state !=
+              StorageState::PROVISIONING) {
+            continue;
+          }
+          update.storage_config_update->membership_update->addShard(
+              shard_id,
+              {
+                  StorageStateTransition::MARK_SHARD_PROVISIONED,
+                  Condition::EMPTY_SHARD | Condition::LOCAL_STORE_READABLE |
+                      Condition::NO_SELF_REPORT_MISSING_DATA,
+              });
+        }
+
+        // Apply the update
+        Status update_status;
+        folly::Baton<> b;
+        nc_api_->update(
+            std::move(update),
+            [&](Status st, std::shared_ptr<const NodesConfiguration>) {
+              update_status = st;
+              b.post();
+            });
+        b.wait();
+        return update_status;
+      },
+      [](const Status& st) { return st == Status::VERSION_MISMATCH; },
+      /* num_tries */ 10,
+      /* backoff_min */ std::chrono::seconds(1),
+      /* backoff_max */ std::chrono::seconds(60),
+      /* jitter_param */ 0.25);
+
+  return result.hasValue() ? result.value() : result.error();
 }
 
 }} // namespace facebook::logdevice

logdevice/server/RebuildingMarkerChecker.h

@@ -57,7 +57,6 @@ class RebuildingMarkerChecker {
   RebuildingMarkerChecker(
       const std::unordered_map<shard_index_t, membership::ShardState>& shards,
       NodeID my_node_id,
-      membership::MembershipVersion::Type storage_version,
       configuration::NodesConfigurationAPI* nc_api,
       ShardedLocalLogStore* sharded_store);
 
@@ -84,7 +83,6 @@ class RebuildingMarkerChecker {
   // Shards to checkm
   std::unordered_map<shard_index_t, membership::ShardState> shards_;
   NodeID my_node_id_;
-  membership::MembershipVersion::Type storage_version_;
   // The NCAPI to apply the NodesConfiguration update. The update is ignored
   // if it's a nullptr (meaning that the NCM is disabled).
   // If the NCAPI is set, it has to outlive the RebuildingMarkerChecker.

logdevice/server/test/RebuildingMarkerCheckerTest.cpp

@@ -48,7 +48,6 @@ TEST(RebuildingMarkerCheckerTest, testAllProvisioning) {
   RebuildingMarkerChecker checker(
       nc.getStorageMembership()->getShardStates(node_idx),
       NodeID(node_idx, generation),
-      nc.getStorageMembership()->getVersion(),
       nc_api.get(),
       store.get());
 
@@ -93,7 +92,6 @@ TEST(RebuildingMarkerCheckerTest, testFirstGeneration) {
   RebuildingMarkerChecker checker(
       nc.getStorageMembership()->getShardStates(node_idx),
       NodeID(node_idx, generation),
-      nc.getStorageMembership()->getVersion(),
       nullptr /* simulating a disabled NCM */,
       store.get());
 
@@ -145,7 +143,6 @@ TEST(RebuildingMarkerCheckerTest, testMissingData) {
   RebuildingMarkerChecker checker(
       nc.getStorageMembership()->getShardStates(node_idx),
       NodeID(node_idx, generation),
-      nc.getStorageMembership()->getVersion(),
       nc_api.get(),
       store.get());