SOLR-17877: fix overseerEnabled cluster prop (#3627)

ZkController:
* fix parsing of overseerEnabled cluster property
* fix sequence of reading cluster property
* reorder some initialization for clarity

MiniSolrCloudCluster:
* Remove flipOverseerEnablement
* Fix MiniSolrCloudCluster application of overseer enablement

TestDistributedTracing requires Overseer
MockSimpleZkController: unused

* Test overseerEnabled property (unit test)

Author: dsmiley

solr/core/src/java/org/apache/solr/cloud/ZkController.java

@@ -300,7 +300,7 @@ public ZkController(
       String zkServerAddress,
       int zkClientConnectTimeout,
       CloudConfig cloudConfig)
-      throws InterruptedException, TimeoutException, IOException {
+      throws InterruptedException, TimeoutException, IOException, KeeperException {
 
     if (cc == null) throw new IllegalArgumentException("CoreContainer cannot be null.");
     this.cc = cc;
@@ -375,26 +375,38 @@ public ZkController(
     this.overseerFailureMap = Overseer.getFailureMap(zkClient);
     this.asyncIdsMap = Overseer.getAsyncIdsMap(zkClient);
 
+    createClusterZkNodes(zkClient);
+
     zkStateReader =
         new ZkStateReader(
             zkClient,
             () -> {
               if (cc != null) cc.securityNodeChanged();
             });
 
+    zkStateReader.createClusterStateWatchersAndUpdate(); // and reads cluster properties
+
+    // note: Can't read cluster properties until createClusterState ^ is called
+    final String urlSchemeFromClusterProp =
+        zkStateReader.getClusterProperty(ZkStateReader.URL_SCHEME, ZkStateReader.HTTP);
+    // this must happen after zkStateReader has initialized the cluster props
+    this.baseURL = URLUtil.getBaseUrlForNodeName(this.nodeName, urlSchemeFromClusterProp);
+
     // Now that zkStateReader is available, read OVERSEER_ENABLED.
-    // When overseerEnabled is false, both distributed features should be enabled
-    Boolean overseerEnabled =
-        zkStateReader.getClusterProperty(ZkStateReader.OVERSEER_ENABLED, null);
-    if (overseerEnabled == null) {
-      overseerEnabled = EnvUtils.getPropertyAsBool("solr.cloud.overseer.enabled", true);
-    }
+    final boolean overseerEnabled =
+        Boolean.parseBoolean(
+            String.valueOf(
+                zkStateReader.getClusterProperty(
+                    ZkStateReader.OVERSEER_ENABLED,
+                    EnvUtils.getPropertyAsBool("solr.cloud.overseer.enabled", true))));
+
     if (overseerEnabled) {
       log.info("The Overseer is enabled.  It will process all cluster commands & state updates.");
     } else {
       log.info(
           "The Overseer is disabled.  Cluster commands & state updates will happen on any/all nodes.");
     }
+    // These "distributed" things replace the Overseer when that's disabled
     this.distributedClusterStateUpdater = new DistributedClusterStateUpdater(!overseerEnabled);
     this.distributedCommandRunner =
         !overseerEnabled
@@ -1068,16 +1080,6 @@ private static void repairSecurityJson(SolrZkClient zkClient)
 
   private void init() {
     try {
-      createClusterZkNodes(zkClient);
-      zkStateReader.createClusterStateWatchersAndUpdate();
-
-      // note: Can't read cluster properties until createClusterState ^ is called
-      final String urlSchemeFromClusterProp =
-          zkStateReader.getClusterProperty(ZkStateReader.URL_SCHEME, ZkStateReader.HTTP);
-
-      // this must happen after zkStateReader has initialized the cluster props
-      this.baseURL = Utils.getBaseUrlForNodeName(this.nodeName, urlSchemeFromClusterProp);
-
       checkForExistingEphemeralNode();
       registerLiveNodesListener();
       checkClusterVersionCompatibility();
@@ -1109,10 +1111,6 @@ private void init() {
 
       // Do this last to signal we're up.
       createEphemeralLiveNode();
-    } catch (IOException e) {
-      log.error("", e);
-      throw new SolrException(
-          SolrException.ErrorCode.SERVER_ERROR, "Can't create ZooKeeperController", e);
     } catch (InterruptedException e) {
       // Restore the interrupted status
       Thread.currentThread().interrupt();

solr/core/src/test/org/apache/solr/cloud/CreateCollectionCleanupTest.java

@@ -66,7 +66,6 @@ public static void createCluster() throws Exception {
         .addConfig(
             "conf1", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
         .withSolrXml(CLOUD_SOLR_XML_WITH_10S_CREATE_COLL_WAIT)
-        .flipOverseerEnablement()
         .configure();
   }
 

solr/core/src/test/org/apache/solr/cloud/DeleteReplicaTest.java

@@ -62,12 +62,7 @@ public void setUp() throws Exception {
     System.setProperty("distribUpdateSoTimeout", "15000");
 
     // these tests need to be isolated, so we don't share the minicluster
-    configureCluster(4)
-        .addConfig("conf", configset("cloud-minimal"))
-        .flipOverseerEnablement()
-        // Some tests (this one) use "the other" cluster Collection API execution strategy to
-        // increase coverage
-        .configure();
+    configureCluster(4).addConfig("conf", configset("cloud-minimal")).configure();
   }
 
   @After

solr/core/src/test/org/apache/solr/cloud/MockSimpleZkController.java

@@ -682,6 +682,96 @@ public void testCheckNoOldClusterstate() throws Exception {
     }
   }
 
+  @Test
+  public void testOverseerEnabledClusterPropertyRespected() throws Exception {
+    // Do not use MiniSolrCloudCluster
+    Path zkDir = createTempDir("testOverseerEnabledClusterPropertyRespected");
+    ZkTestServer server = new ZkTestServer(zkDir);
+    try {
+      server.run();
+
+      // Create cluster ZK nodes and set the overseerEnabled cluster property to false BEFORE
+      // ZkController starts
+      try (SolrZkClient client =
+          new SolrZkClient.Builder()
+              .withUrl(server.getZkAddress())
+              .withTimeout(TIMEOUT, TimeUnit.MILLISECONDS)
+              .build()) {
+
+        ZkController.createClusterZkNodes(client);
+        ClusterProperties cp = new ClusterProperties(client);
+        cp.setClusterProperty(ZkStateReader.OVERSEER_ENABLED, "false");
+      }
+
+      // Now create a ZkController - it should respect the cluster property and have overseer
+      // disabled
+      CoreContainer cc = getCoreContainer();
+      try {
+        CloudConfig cloudConfig = new CloudConfig.CloudConfigBuilder("127.0.0.1", 8983).build();
+        try (ZkController zkController =
+            new ZkController(cc, server.getZkAddress(), TIMEOUT, cloudConfig)) {
+
+          // The overseer should be disabled due to the cluster property, meaning distributed state
+          // updates should be enabled
+          // Since overseerEnabled=false in cluster properties, !overseerEnabled should be true, so
+          // isDistributedStateUpdate() should return true
+          assertTrue(
+              "Overseer should be disabled when overseerEnabled cluster property is false, meaning distributed state updates should be enabled",
+              zkController.getDistributedClusterStateUpdater().isDistributedStateUpdate());
+        }
+      } finally {
+        cc.shutdown();
+      }
+    } finally {
+      server.shutdown();
+    }
+  }
+
+  @Test
+  public void testOverseerEnabledClusterPropertyTrue() throws Exception {
+    // Do not use MiniSolrCloudCluster
+    Path zkDir = createTempDir("testOverseerEnabledClusterPropertyTrue");
+    ZkTestServer server = new ZkTestServer(zkDir);
+    try {
+      server.run();
+
+      // Create cluster ZK nodes and set the overseerEnabled cluster property to true BEFORE
+      // ZkController starts
+      try (SolrZkClient client =
+          new SolrZkClient.Builder()
+              .withUrl(server.getZkAddress())
+              .withTimeout(TIMEOUT, TimeUnit.MILLISECONDS)
+              .build()) {
+
+        ZkController.createClusterZkNodes(client);
+        ClusterProperties cp = new ClusterProperties(client);
+        cp.setClusterProperty(ZkStateReader.OVERSEER_ENABLED, "true");
+      }
+
+      // Now create a ZkController - it should respect the cluster property and have overseer
+      // enabled
+      CoreContainer cc = getCoreContainer();
+      try {
+        CloudConfig cloudConfig = new CloudConfig.CloudConfigBuilder("127.0.0.1", 8983).build();
+        try (ZkController zkController =
+            new ZkController(cc, server.getZkAddress(), TIMEOUT, cloudConfig)) {
+
+          // The overseer should be enabled due to the cluster property, meaning distributed state
+          // updates should be disabled
+          // Since overseerEnabled=true in cluster properties, !overseerEnabled should be false, so
+          // isDistributedStateUpdate() should return false
+          assertFalse(
+              "Overseer should be enabled when overseerEnabled cluster property is true, meaning distributed state updates should be disabled",
+              zkController.getDistributedClusterStateUpdater().isDistributedStateUpdate());
+        }
+      } finally {
+        cc.shutdown();
+      }
+    } finally {
+      server.shutdown();
+    }
+  }
+
   private CoreContainer getCoreContainer() {
     return new MockCoreContainer();
   }

solr/core/src/test/org/apache/solr/cloud/ZkControllerTest.java

@@ -58,6 +58,7 @@ public static void setupCluster() throws Exception {
         .addConfig("config", TEST_PATH().resolve("collection1").resolve("conf"))
         .withSolrXml(TEST_PATH().resolve("solr.xml"))
         .withTraceIdGenerationDisabled()
+        .withOverseer(true) // some assertions assume overseer
         .configure();
 
     assertNotEquals(

solr/modules/opentelemetry/src/test/org/apache/solr/opentelemetry/TestDistributedTracing.java

@@ -1108,20 +1108,6 @@ public Builder addConfig(String configName, Path configPath) {
       return this;
     }
 
-    /**
-     * This method makes the MiniSolrCloudCluster use the "other" Collection API execution strategy
-     * than it normally would. When some test classes call this method (and some don't) we make sure
-     * that a run of multiple tests with a single seed will exercise both code lines (distributed
-     * and Overseer based Collection API) so regressions can be spotted faster.
-     *
-     * <p>The real need is for a few tests covering reasonable use cases to call this method. If
-     * you're adding a new test, you don't have to call it (but it's ok if you do).
-     */
-    public Builder flipOverseerEnablement() {
-      overseerEnabled = !overseerEnabled;
-      return this;
-    }
-
     /**
      * Force the cluster Collection and config state API execution as well as the cluster state
      * update strategy to be either Overseer based or distributed. <b>This method can be useful when
@@ -1181,7 +1167,7 @@ public MiniSolrCloudCluster configure() throws Exception {
      * @throws Exception if an error occurs on startup
      */
     public MiniSolrCloudCluster build() throws Exception {
-      this.clusterProperties.put(ZkStateReader.OVERSEER_ENABLED, Boolean.toString(overseerEnabled));
+      System.setProperty("solr.cloud.overseer.enabled", Boolean.toString(overseerEnabled));
 
       // eager init to prevent OTEL init races caused by test setup
       if (!disableTraceIdGeneration && TracerConfigurator.TRACE_ID_GEN_ENABLED) {
@@ -1204,6 +1190,7 @@ public MiniSolrCloudCluster build() throws Exception {
         cluster.uploadConfigSet(config.path, config.name);
       }
 
+      // TODO process BEFORE nodes start up!  Some props are only read on node start.
       if (clusterProperties.size() > 0) {
         ClusterProperties props = new ClusterProperties(cluster.getZkClient());
         for (Map.Entry<String, Object> entry : clusterProperties.entrySet()) {