Make nodeset selector not fail for internal logs if configured nodeset_size is smaller than replication factor

Summary: It used to select a nodeset of the given size, then check if it's replicatable and fail because it's not. This diff makes nodeset selector increase nodeset size to 2*replication_factor-1.

Reviewed By: MohamedBassem

Differential Revision: D17372813

fbshipit-source-id: 32859b052db8f0f07d224eaec88307b2c5f57103

Author: al13n321

logdevice/common/nodeset_selection/WeightAwareNodeSetSelector.cpp

@@ -74,6 +74,15 @@ NodeSetSelector::Result WeightAwareNodeSetSelector::getStorageSet(
     min_nodes_per_domain = 0;
   }
 
+  // We'll try to pick at least this many nodes in total.
+  // If target_nodeset_size seems too small compared to replication factor,
+  // increase it to 2*replication_factor - 1, which provides, in some sense,
+  // equal write and read availability.
+  nodeset_size_t min_nodes_total =
+      std::max(target_nodeset_size,
+               static_cast<nodeset_size_t>(
+                   replication_property.getReplicationFactor() * 2 - 1));
+
   struct CandidateNode {
     uint64_t shard_id_hash;
     ShardID shard_id;
@@ -237,7 +246,7 @@ NodeSetSelector::Result WeightAwareNodeSetSelector::getStorageSet(
       int picked_in_domain =
           only_writable ? d->num_picked_writable : d->num_picked;
       if (picked_in_domain >= min_nodes_per_domain &&
-          result_size >= target_nodeset_size) {
+          result_size >= min_nodes_total) {
         // The nodeset is big enough and has enough nodes from each domain.
         break;
       }

logdevice/common/test/RandomNodeSetSelectorTest.cpp

@@ -610,9 +610,9 @@ void basic_test(NodeSetSelectorType ns_type) {
                 logid_t(3),
                 Decision::NEEDS_CHANGE,
                 [&](StorageSet* ss) {
-                  EXPECT_EQ(4, keep_only_writable(*ss).size());
-                  EXPECT_GE(ss->size(), 4);
-                  EXPECT_LE(ss->size(), 10);
+                  EXPECT_EQ(7, keep_only_writable(*ss).size());
+                  EXPECT_GE(ss->size(), 7);
+                  EXPECT_LE(ss->size(), 14);
                 });
 
   verify_result(selector.get(),
@@ -1087,3 +1087,50 @@ TEST(ConsistentHashingWeightAwareNodeSetSelectorTest, Seed) {
   EXPECT_EQ(Decision::KEEP, res6.decision);
   EXPECT_EQ(res5.signature, res6.signature);
 }
+
+TEST(WeightAwareNodeSetSelectorTest, InternalLogsConfiguredTooSmall) {
+  Nodes nodes;
+  addNodes(&nodes, 5, 1, "region.dc3.FB|REGION3|MSB_9.00.ab");
+  addNodes(&nodes, 5, 1, "region.dc3.FB|REGION3|MSB_8.00.ab");
+  addNodes(&nodes, 5, 1, "region.dc3.FB|REGION3|MSB_7.00.ab");
+
+  ASSERT_EQ(15, nodes.size());
+  Configuration::NodesConfig nodes_config(std::move(nodes));
+
+  ReplicationProperty replication(
+      {{NodeLocationScope::CLUSTER, 2}, {NodeLocationScope::NODE, 4}});
+  size_t nodeset_size = 3;
+  auto logs_config = std::make_shared<LocalLogsConfig>();
+
+  InternalLogs il;
+  logsconfig::LogAttributes log_attrs;
+  log_attrs.set_nodeSetSize(nodeset_size);
+  log_attrs.set_replicateAcross(replication.getDistinctReplicationFactors());
+  auto log_group_node = il.insert("event_log_snapshots", log_attrs);
+  ASSERT_NE(nullptr, log_group_node);
+
+  ShapingConfig shaping_cfg(
+      std::set<NodeLocationScope>{NodeLocationScope::NODE},
+      std::set<NodeLocationScope>{NodeLocationScope::NODE});
+  auto config = std::make_shared<Configuration>(
+      ServerConfig::fromDataTest("nodeset_selector_test",
+                                 std::move(nodes_config),
+                                 MetaDataLogsConfig(),
+                                 PrincipalsConfig(),
+                                 SecurityConfig(),
+                                 TraceLoggerConfig(),
+                                 TrafficShapingConfig(),
+                                 shaping_cfg,
+                                 ServerConfig::SettingsConfig(),
+                                 ServerConfig::SettingsConfig(),
+                                 std::move(il)),
+      std::move(logs_config));
+
+  auto selector =
+      NodeSetSelectorFactory::create(NodeSetSelectorType::WEIGHT_AWARE);
+  verify_result(selector.get(),
+                config,
+                InternalLogs::EVENT_LOG_SNAPSHOTS,
+                Decision::NEEDS_CHANGE,
+                [&](StorageSet* ss) { EXPECT_EQ(7, ss->size()); });
+}

logdevice/test/SequencerIntegrationTest.cpp

@@ -432,8 +432,8 @@ TEST_F(SequencerIntegrationTest, WinSizeChangeDelayTest) {
 // nodes changes.
 TEST_F(SequencerIntegrationTest, StorageStateChangeDelayTest) {
   Configuration::Nodes nodes;
-  size_t num_nodes = 7;
-  // 2 sequencer nodes and 5 storage nodes
+  size_t num_nodes = 9;
+  // 2 sequencer nodes and 7 storage nodes
   for (int i = 0; i < num_nodes; ++i) {
     auto& node = nodes[i];
     node.generation = 1;
@@ -512,7 +512,7 @@ TEST_F(SequencerIntegrationTest, StorageStateChangeDelayTest) {
   // nodeset for the written log.
   std::unordered_map<node_index_t, size_t> stores;
   node_index_t to_disable = 1;
-  for (node_index_t n = 2; n <= 6; ++n) {
+  for (node_index_t n = 2; n < num_nodes; ++n) {
     stores[n] = cluster->getNode(n).stats()["message_received.STORE"];
     if (stores[n] > stores[to_disable]) {
       to_disable = n;