SOLR-16975: Remove loading of solr.xml from zookeeper (main branch on…

…ly) (#1920) --------- Co-authored-by: Jan Høydahl <[email protected]>
apache · Nov 11, 2023 · 1e68438 · 1e68438
1 parent e740123
commit 1e68438
Show file tree

Hide file tree

Showing 15 changed files with 68 additions and 314 deletions.
diff --git a/dev-tools/scripts/cloud.sh b/dev-tools/scripts/cloud.sh
@@ -315,7 +315,6 @@ start(){
     # Need a fresh root in zookeeper...
     "${SOLR}/server/scripts/cloud-scripts/zkcli.sh" -zkhost localhost:${ZK_PORT} -cmd makepath "/solr_${SAFE_DEST}";
     "${SOLR}/server/scripts/cloud-scripts/zkcli.sh" -zkhost localhost:${ZK_PORT} -cmd put "/solr_${SAFE_DEST}" "created by cloud.sh"; # so we can test for existence next time
-    "${SOLR}/server/scripts/cloud-scripts/zkcli.sh" -zkhost localhost:${ZK_PORT} -cmd putfile "/solr_${SAFE_DEST}/solr.xml" "${SOLR}/server/solr/solr.xml";
   fi
 
   ACTUAL_NUM_NODES=$(ls -1 -d ${CLUSTER_WD}/n* | wc -l )

diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
@@ -43,6 +43,8 @@ Deprecation Removals
 
 * SOLR-16893: Remove bin/solr create_core and create_collection commands in favour of bin/solr create command.  (Eric Pugh)
 
+* SOLR-15959: Remove deprecated feature to load solr.xml from ZK (janhoy)
+
 * SOLR-17042: Remove deprecated `V2RequestSupport` and associated `SolrRequest` methods `setUseV2` and `setUseBinaryV2`. (Jason Gerlowski)
 
 Dependency Upgrades

diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkCLI.java b/solr/core/src/java/org/apache/solr/cloud/ZkCLI.java
@@ -225,10 +225,12 @@ public static void main(String[] args)
         stdout.println(
             "zkcli.sh -zkhost localhost:9983 -cmd "
                 + PUT_FILE
-                + " /solr.xml /User/myuser/solr/solr.xml");
-        stdout.println("zkcli.sh -zkhost localhost:9983 -cmd " + GET + " /solr.xml");
+                + " /clusterprops.json /User/myuser/solr/clusterprops.json");
+        stdout.println("zkcli.sh -zkhost localhost:9983 -cmd " + GET + " /clusterprops.json");
         stdout.println(
-            "zkcli.sh -zkhost localhost:9983 -cmd " + GET_FILE + " /solr.xml solr.xml.file");
+            "zkcli.sh -zkhost localhost:9983 -cmd "
+                + GET_FILE
+                + " /clusterprops.json clusterprops.json");
         stdout.println("zkcli.sh -zkhost localhost:9983 -cmd " + CLEAR + " /solr");
         stdout.println("zkcli.sh -zkhost localhost:9983 -cmd " + LIST);
         stdout.println("zkcli.sh -zkhost localhost:9983 -cmd " + LS + " /solr/live_nodes");

diff --git a/solr/core/src/java/org/apache/solr/cloud/ZkController.java b/solr/core/src/java/org/apache/solr/cloud/ZkController.java
@@ -24,6 +24,7 @@
 import static org.apache.solr.common.cloud.ZkStateReader.NODE_NAME_PROP;
 import static org.apache.solr.common.cloud.ZkStateReader.REJOIN_AT_HEAD_PROP;
 import static org.apache.solr.common.cloud.ZkStateReader.SHARD_ID_PROP;
+import static org.apache.solr.common.cloud.ZkStateReader.UNSUPPORTED_SOLR_XML;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDROLE;
 import static org.apache.zookeeper.ZooDefs.Ids.OPEN_ACL_UNSAFE;
 
@@ -376,7 +377,7 @@ public ZkController(
             .withClosedCheck(cc::isShutDown)
             .withCompressor(compressor)
             .build();
-    // Refuse to start if ZK has a non empty /clusterstate.json
+    // Refuse to start if ZK has a non empty /clusterstate.json or a /solr.xml file
     checkNoOldClusterstate(zkClient);
 
     this.overseerRunningMap = Overseer.getRunningMap(zkClient);
@@ -529,12 +530,20 @@ private void onReconnect(Supplier<List<CoreDescriptor>> descriptorsSupplier)
 
   /**
    * Verifies if /clusterstate.json exists in Zookeepeer, and if it does and is not empty, refuses
-   * to start and outputs a helpful message regarding collection migration.
+   * to start and outputs a helpful message regarding collection migration. Also aborts if /solr.xml
+   * exists in zookeeper.
    *
    * <p>If /clusterstate.json exists and is empty, it is removed.
    */
   private void checkNoOldClusterstate(final SolrZkClient zkClient) throws InterruptedException {
     try {
+      if (zkClient.exists(UNSUPPORTED_SOLR_XML, true)) {
+        String message =
+            "solr.xml found in ZooKeeper. Loading solr.xml from ZooKeeper is no longer supported since Solr 10. "
+                + "Cannot start Solr. The file can be removed with command bin/solr zk rm /solr.xml -z host:port";
+        log.error(message);
+        throw new SolrException(ErrorCode.INVALID_STATE, message);
+      }
       if (!zkClient.exists(ZkStateReader.UNSUPPORTED_CLUSTER_STATE, true)) {
         return;
       }

diff --git a/solr/core/src/java/org/apache/solr/core/NodeConfig.java b/solr/core/src/java/org/apache/solr/core/NodeConfig.java
@@ -16,7 +16,6 @@
  */
 package org.apache.solr.core;
 
-import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.net.URL;
@@ -35,11 +34,11 @@
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import org.apache.lucene.search.IndexSearcher;
+import org.apache.solr.client.solrj.impl.SolrZkClientTimeout;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.common.util.StrUtils;
-import org.apache.solr.logging.DeprecationLog;
 import org.apache.solr.logging.LogWatcherConfig;
 import org.apache.solr.search.CacheConfig;
 import org.apache.solr.servlet.SolrDispatchFilter;
@@ -117,10 +116,6 @@ public class NodeConfig {
 
   private final PluginInfo tracerConfig;
 
-  // Track if this config was loaded from zookeeper so that we can skip validating the zookeeper
-  // connection later. If it becomes necessary to track multiple potential sources in the future,
-  // replace this with an Enum
-  private final boolean fromZookeeper;
   private final String defaultZkHost;
 
   private NodeConfig(
@@ -153,7 +148,6 @@ private NodeConfig(
       MetricsConfig metricsConfig,
       Map<String, CacheConfig> cachesConfig,
       PluginInfo tracerConfig,
-      boolean fromZookeeper,
       String defaultZkHost,
       Set<Path> allowPaths,
       List<String> allowUrls,
@@ -191,7 +185,6 @@ private NodeConfig(
     this.metricsConfig = metricsConfig;
     this.cachesConfig = cachesConfig == null ? Collections.emptyMap() : cachesConfig;
     this.tracerConfig = tracerConfig;
-    this.fromZookeeper = fromZookeeper;
     this.defaultZkHost = defaultZkHost;
     this.allowPaths = allowPaths;
     this.allowUrls = allowUrls;
@@ -218,44 +211,36 @@ private NodeConfig(
   }
 
   /**
-   * Get the NodeConfig whether stored on disk, in ZooKeeper, etc. This may also be used by custom
-   * filters to load relevant configuration.
+   * Get the NodeConfig. This may also be used by custom filters to load relevant configuration.
    *
    * @return the NodeConfig
    */
   public static NodeConfig loadNodeConfig(Path solrHome, Properties nodeProperties) {
-    if (StrUtils.isNotNullOrEmpty(System.getProperty("solr.solrxml.location"))) {
-      log.warn(
-          "Solr property solr.solrxml.location is no longer supported. Will automatically load solr.xml from ZooKeeper if it exists");
-    }
     final SolrResourceLoader loader = new SolrResourceLoader(solrHome);
     initModules(loader, null);
     nodeProperties = SolrXmlConfig.wrapAndSetZkHostFromSysPropIfNeeded(nodeProperties);
+
+    // TODO: Only job of this block is to
+    //  delay starting a solr core to satisfy
+    //  ZkFailoverTest test case...
     String zkHost = nodeProperties.getProperty(SolrXmlConfig.ZK_HOST);
     if (StrUtils.isNotNullOrEmpty(zkHost)) {
-      int startUpZkTimeOut = Integer.getInteger("waitForZk", 30);
-      startUpZkTimeOut *= 1000;
+      int startUpZkTimeOut = 1000 * Integer.getInteger("waitForZk", 0);
+      if (startUpZkTimeOut == 0) {
+        startUpZkTimeOut = SolrZkClientTimeout.DEFAULT_ZK_CLIENT_TIMEOUT;
+      }
       try (SolrZkClient zkClient =
           new SolrZkClient.Builder()
               .withUrl(zkHost)
               .withTimeout(startUpZkTimeOut, TimeUnit.MILLISECONDS)
               .withConnTimeOut(startUpZkTimeOut, TimeUnit.MILLISECONDS)
               .withSolrClassLoader(loader)
               .build()) {
-        if (zkClient.exists("/solr.xml", true)) {
-          log.info("solr.xml found in ZooKeeper. Loading...");
-          DeprecationLog.log(
-              "solrxml-zookeeper",
-              "Loading solr.xml from zookeeper is deprecated. See reference guide for details.");
-          byte[] data = zkClient.getData("/solr.xml", null, null, true);
-          return SolrXmlConfig.fromInputStream(
-              solrHome, new ByteArrayInputStream(data), nodeProperties, true);
-        }
+        zkClient.exists("/configs", true);
       } catch (Exception e) {
         throw new SolrException(
-            ErrorCode.SERVER_ERROR, "Error occurred while loading solr.xml from zookeeper", e);
+            ErrorCode.SERVER_ERROR, "Error occurred while testing zookeeper connection", e);
       }
-      log.info("Loading solr.xml from SolrHome (not found in ZooKeeper)");
     }
 
     return SolrXmlConfig.fromSolrHome(solrHome, nodeProperties);
@@ -419,37 +404,6 @@ public PluginInfo getTracerConfiguratorPluginInfo() {
     return tracerConfig;
   }
 
-  /**
-   * True if this node config was loaded from zookeeper
-   *
-   * @see #getDefaultZkHost
-   */
-  public boolean isFromZookeeper() {
-    return fromZookeeper;
-  }
-
-  /**
-   * This method returns the default "zkHost" value for this node -- either read from the system
-   * properties, or from the "extra" properties configured explicitly on the SolrDispatchFilter; or
-   * null if not specified.
-   *
-   * <p>This is the value that would have been used when attempting to locate the solr.xml in
-   * ZooKeeper (regardless of whether the file was actually loaded from ZK or from local disk)
-   *
-   * <p>(This value should only be used for "accounting" purposes to track where the node config
-   * came from if it <em>was</em> loaded from zk -- ie: to check if the chroot has already been
-   * applied. It may be different from the "zkHost" <em>configured</em> in the "cloud" section of
-   * the solr.xml, which should be used for all zk connections made by this node to participate in
-   * the cluster)
-   *
-   * @see #isFromZookeeper
-   * @see #getCloudConfig()
-   * @see CloudConfig#getZkHost()
-   */
-  public String getDefaultZkHost() {
-    return defaultZkHost;
-  }
-
   /**
    * Extra file paths that will be allowed for core creation, in addition to SOLR_HOME,
    * SOLR_DATA_HOME and coreRootDir
@@ -498,11 +452,6 @@ public String getModules() {
     return modules;
   }
 
-  /** Returns the list of hidden system properties. The list values are regex expressions */
-  public Set<String> getHiddenSysProps() {
-    return hiddenSysProps;
-  }
-
   /** Returns whether a given system property is hidden */
   public boolean isSysPropHidden(String sysPropName) {
     return hiddenSysPropPattern.test(sysPropName);
@@ -627,7 +576,6 @@ public static class NodeConfigBuilder {
     private MetricsConfig metricsConfig;
     private Map<String, CacheConfig> cachesConfig;
     private PluginInfo tracerConfig;
-    private boolean fromZookeeper = false;
     private String defaultZkHost;
     private Set<Path> allowPaths = Collections.emptySet();
     private List<String> allowUrls = Collections.emptyList();
@@ -814,11 +762,6 @@ public NodeConfigBuilder setTracerConfig(PluginInfo tracerConfig) {
       return this;
     }
 
-    public NodeConfigBuilder setFromZookeeper(boolean fromZookeeper) {
-      this.fromZookeeper = fromZookeeper;
-      return this;
-    }
-
     public NodeConfigBuilder setDefaultZkHost(String defaultZkHost) {
       this.defaultZkHost = defaultZkHost;
       return this;
@@ -929,7 +872,6 @@ public NodeConfig build() {
           metricsConfig,
           cachesConfig,
           tracerConfig,
-          fromZookeeper,
           defaultZkHost,
           allowPaths,
           allowUrls,

diff --git a/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java b/solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
@@ -95,22 +95,12 @@ public static Properties wrapAndSetZkHostFromSysPropIfNeeded(final Properties pr
   }
 
   public static NodeConfig fromConfig(
-      Path solrHome,
-      Properties substituteProperties,
-      boolean fromZookeeper,
-      ConfigNode root,
-      SolrResourceLoader loader) {
+      Path solrHome, Properties substituteProperties, ConfigNode root, SolrResourceLoader loader) {
 
     checkForIllegalConfig(root);
 
-    // sanity check: if our config came from zookeeper, then there *MUST* be Node Properties that
-    // tell us what zkHost was used to read it (either via webapp context attribute, or that
-    // SolrDispatchFilter filled in for us from system properties)
-    assert ((!fromZookeeper)
-        || (null != substituteProperties && null != substituteProperties.getProperty(ZK_HOST)));
-
-    // Regardless of where/how we this XmlConfigFile was loaded from, if it contains a zkHost
-    // property, we're going to use that as our "default" and only *directly* check the system
+    // If solr.xml contains a zkHost property, we're going to use that as our "default" and only
+    // *directly* check the system
     // property if it's not specified.
     //
     // (checking the sys prop here is really just for tests that by-pass SolrDispatchFilter. In
@@ -179,7 +169,6 @@ public static NodeConfig fromConfig(
     configBuilder.setHiddenSysProps(getHiddenSysProps(root.get("metrics")));
     configBuilder.setMetricsConfig(getMetricsConfig(root.get("metrics")));
     configBuilder.setCachesConfig(getCachesConfig(loader, root.get("caches")));
-    configBuilder.setFromZookeeper(fromZookeeper);
     configBuilder.setDefaultZkHost(defaultZkHost);
     configBuilder.setCoreAdminHandlerActions(coreAdminHandlerActions);
     return fillSolrSection(configBuilder, root);
@@ -222,11 +211,6 @@ public static NodeConfig fromString(Path solrHome, String xml) {
 
   public static NodeConfig fromInputStream(
       Path solrHome, InputStream is, Properties substituteProps) {
-    return fromInputStream(solrHome, is, substituteProps, false);
-  }
-
-  public static NodeConfig fromInputStream(
-      Path solrHome, InputStream is, Properties substituteProps, boolean fromZookeeper) {
     SolrResourceLoader loader = new SolrResourceLoader(solrHome);
     if (substituteProps == null) {
       substituteProps = new Properties();
@@ -239,7 +223,6 @@ public static NodeConfig fromInputStream(
         return fromConfig(
             solrHome,
             substituteProps,
-            fromZookeeper,
             new DataConfigNode(new DOMConfigNode(config.getDocument().getDocumentElement())),
             loader);
       }

diff --git a/solr/core/src/java/org/apache/solr/core/ZkContainer.java b/solr/core/src/java/org/apache/solr/core/ZkContainer.java
@@ -127,13 +127,7 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig config) {
         }
         boolean createRoot = Boolean.getBoolean("createZkChroot");
 
-        // We may have already loaded NodeConfig from zookeeper with same connect string, so no need
-        // to recheck chroot
-        boolean alreadyUsedChroot =
-            (cc.getConfig().isFromZookeeper()
-                && zookeeperHost.equals(cc.getConfig().getDefaultZkHost()));
-        if (!alreadyUsedChroot
-            && !ZkController.checkChrootPath(zookeeperHost, zkRunOnly || createRoot)) {
+        if (!ZkController.checkChrootPath(zookeeperHost, zkRunOnly || createRoot)) {
           throw new ZooKeeperException(
               SolrException.ErrorCode.SERVER_ERROR,
               "A chroot was specified in ZkHost but the znode doesn't exist. " + zookeeperHost);