Remove the REQUIRES_RESTART restriction from the NCM source of truth flag

Summary: As per our discussion, we will allow the source of truth flag to be switched on/off during runtime without requiring a restart.

Reviewed By: shixiao

Differential Revision: D17450554

fbshipit-source-id: 84257552850354c814a56fb743f2afac6aa51ab8

Author: MohamedBassem

docs/settings.md

@@ -74,7 +74,7 @@ sidebar_label: Settings
 | server-default-dscp | Use default DSCP to setup to server sockets at Sender.Range was defined by https://tools.ietf.org/html/rfc4594#section-1.4.4 | 0 | requires restart, server only |
 | server\_based\_nodes\_configuration\_store\_polling\_extra\_requests | how many extra requests to send for server based Nodes Configuration Store polling in addition to the required response for each wave | 1 |  |
 | shutdown-on-my-node-id-mismatch | Gracefully shutdown whenever the server's NodeID changes | true | server only |
-| use-nodes-configuration-manager-nodes-configuration | If true and enable\_nodes\_configuration\_manager is set, logdevice will use the nodes configuration from the NodesConfigurationManager. | false | requires restart |
+| use-nodes-configuration-manager-nodes-configuration | If true and enable\_nodes\_configuration\_manager is set, logdevice will use the nodes configuration from the NodesConfigurationManager. | false |  |
 | zk-config-polling-interval | polling and retry interval for Zookeeper config source | 1000ms | CLI only |
 
 ## Core settings

logdevice/common/settings/Settings.cpp

@@ -2626,7 +2626,7 @@ void Settings::defineSettings(SettingEasyInit& init) {
        nullptr, // no custom validation necessary
        "If true and enable_nodes_configuration_manager is set, logdevice will "
        "use the nodes configuration from the NodesConfigurationManager.",
-       CLIENT | SERVER | REQUIRES_RESTART,
+       CLIENT | SERVER,
        SettingsCategory::Configuration);
 
   init("nodes-configuration-manager-store-polling-interval",

logdevice/test/UpdateableNodesConfigurationPublisherIntegrationTest.cpp

@@ -6,6 +6,8 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+#include "logdevice/admin/if/gen-cpp2/AdminAPI.h"
+#include "logdevice/admin/if/gen-cpp2/nodes_constants.h"
 #include "logdevice/common/configuration/nodes/FileBasedNodesConfigurationStore.h"
 #include "logdevice/common/configuration/nodes/NodesConfigurationCodec.h"
 #include "logdevice/common/configuration/nodes/NodesConfigurationManagerFactory.h"
@@ -27,6 +29,8 @@ class NodesConfigurationPublisherIntegrationTest : public IntegrationTestBase {
     // Initialize the cluster
     cluster_ = IntegrationTestUtils::ClusterFactory{}
                    .doNotSyncServerConfigToNodesConfiguration()
+                   .setNodesConfigurationSourceOfTruth(
+                       NodesConfigurationSourceOfTruth::SERVER_CONFIG)
                    .create(5);
 
     // Initialize a NCS pointing to the same data as the cluster to be able
@@ -63,7 +67,7 @@ class NodesConfigurationPublisherIntegrationTest : public IntegrationTestBase {
   std::shared_ptr<Client> client_;
 };
 
-TEST_F(NodesConfigurationPublisherIntegrationTest, Publish) {
+TEST_F(NodesConfigurationPublisherIntegrationTest, ClientPublish) {
   auto client_impl = dynamic_cast<ClientImpl*>(client_.get());
   ASSERT_NE(nullptr, client_impl);
   auto processor = client_impl->getProcessorPtr();
@@ -185,3 +189,48 @@ TEST_F(NodesConfigurationPublisherIntegrationTest, Publish) {
     EXPECT_EQ(2, processor->getNodesConfiguration()->getVersion().val());
   }
 }
+
+TEST_F(NodesConfigurationPublisherIntegrationTest, ServerPublish) {
+  auto admin_client = cluster_->getNode(0).createAdminClient();
+
+  auto wait_until_version = [&](uint64_t version) {
+    facebook::logdevice::thrift::NodesConfigResponse resp;
+    wait_until(folly::sformat("NC version is: {}", version).c_str(), [&]() {
+      admin_client->sync_getNodesConfig(
+          resp, facebook::logdevice::thrift::NodesFilter{});
+      ld_info("Current version: %ld", resp.version);
+      return resp.version == version;
+    });
+    return resp.version;
+  };
+
+  auto current_nc =
+      cluster_->getConfig()->getNodesConfigurationFromServerConfigSource();
+  EXPECT_EQ(current_nc->getVersion().val(),
+            wait_until_version(current_nc->getVersion().val()));
+
+  // For the correctness of this test, let's make sure that the current NC
+  // version is not equal to our arbitrary number.
+  ASSERT_NE(123, current_nc->getVersion().val());
+
+  // Force the NCM NC to diverge
+  auto new_nc =
+      current_nc->withVersion(membership::MembershipVersion::Type{123});
+  auto serialized = NodesConfigurationCodec::serialize(*new_nc);
+  ASSERT_EQ(
+      Status::OK, ncs_->updateConfigSync(std::move(serialized), folly::none));
+
+  // Switch the source of truth to the NCM NC and make sure that it's what
+  // you get back from querying the server.
+  cluster_->getNode(0).sendCommand(
+      "set use-nodes-configuration-manager-nodes-configuration true --ttl max");
+  EXPECT_EQ(123, wait_until_version(123));
+
+  // Flip the switch back to the server config and make sure you get the old
+  // version back.
+  cluster_->getNode(0).sendCommand(
+      "set use-nodes-configuration-manager-nodes-configuration false --ttl "
+      "max");
+  EXPECT_EQ(current_nc->getVersion().val(),
+            wait_until_version(current_nc->getVersion().val()));
+}