Merge pull request #25347 from vespa-engine/hmusum/start-config-server-in-self-hosted-with-non-reconfigurable-zookeeper-server

Make zookeeper server non-reconfigurable in self-hosted [run-systemtest]

11 files changed, 100 insertions, 41 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
index 5d60cec0679..69accef2fe4 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/clustercontroller/ClusterControllerContainer.java
@@ -65,7 +65,8 @@ public class ClusterControllerContainer extends Container implements
                    "com.yahoo.vespa.clustercontroller.apps.clustercontroller.StateRestApiV2Handler",
                    "/cluster/v2/*",
                    CLUSTERCONTROLLER_BUNDLE);
-        addComponent(new AccessLogComponent(containerCluster().orElse(null), AccessLogComponent.AccessLogType.jsonAccessLog,
+        addComponent(new AccessLogComponent(containerCluster().orElse(null),
+                                            AccessLogComponent.AccessLogType.jsonAccessLog,
                                             deployState.featureFlags().logFileCompressionAlgorithm("zstd"),
                                             Optional.of("controller"),
                                             deployState.isHosted()));
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
index ec1776730b8..c0182c2f5ac 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ApplicationContainerCluster.java
@@ -31,7 +31,6 @@ import com.yahoo.vespa.config.search.core.OnnxModelsConfig;
 import com.yahoo.vespa.config.search.core.RankingConstantsConfig;
 import com.yahoo.vespa.config.search.core.RankingExpressionsConfig;
 import com.yahoo.vespa.model.AbstractService;
-import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.admin.metricsproxy.MetricsProxyContainer;
 import com.yahoo.vespa.model.container.component.BindingPattern;
 import com.yahoo.vespa.model.container.component.Component;
@@ -39,7 +38,6 @@ import com.yahoo.vespa.model.container.component.Handler;
 import com.yahoo.vespa.model.container.component.SystemBindingPattern;
 import com.yahoo.vespa.model.container.configserver.ConfigserverCluster;
 import com.yahoo.vespa.model.utils.FileSender;
-
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -316,8 +314,8 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
                                    ! previousHosts.contains(container.getHostName()))
                          .retired(container.isRetired());
             builder.server(serverBuilder);
-            builder.dynamicReconfiguration(true);
         }
+        builder.dynamicReconfiguration(true);
     }
 
     @Override
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
index a165b4862b5..8f257110a04 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/configserver/ConfigserverCluster.java
@@ -84,6 +84,8 @@ public class ConfigserverCluster extends AbstractConfigProducer
         if (options.hostedVespa().orElse(false)) {
             builder.vespaTlsConfigFile(Defaults.getDefaults().underVespaHome("var/zookeeper/conf/tls.conf.json"));
         }
+
+        builder.dynamicReconfiguration(options.hostedVespa().orElse(false));
     }
 
     @Override
diff --git a/configserver/src/main/resources/configserver-app/services.xml b/configserver/src/main/resources/configserver-app/services.xml
index 713b897f23a..bba7d9627dd 100644
--- a/configserver/src/main/resources/configserver-app/services.xml
+++ b/configserver/src/main/resources/configserver-app/services.xml
@@ -11,9 +11,7 @@
 
     <component id="com.yahoo.vespa.config.server.ConfigServerBootstrap" bundle="configserver" />
     <component id="com.yahoo.vespa.config.server.monitoring.Metrics" bundle="configserver" />
-    <component id="com.yahoo.vespa.zookeeper.ReconfigurableVespaZooKeeperServer" bundle="zookeeper-server" />
-    <component id="com.yahoo.vespa.zookeeper.Reconfigurer" bundle="zookeeper-server" />
-    <component id="com.yahoo.vespa.zookeeper.VespaZooKeeperAdminImpl" bundle="zookeeper-server" />
+    <component id="com.yahoo.vespa.zookeeper.ConfigServerZooKeeperServer" bundle="zookeeper-server" />
     <component id="com.yahoo.vespa.config.server.rpc.RpcServer" bundle="configserver" />
     <component id="com.yahoo.vespa.config.server.ConfigServerDB" bundle="configserver" />
     <component id="com.yahoo.vespa.config.server.filedistribution.FileDistributionFactory" bundle="configserver" />
diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
index 0ad4a4b8294..af42e30422b 100644
--- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
+++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Configurator.java
@@ -98,7 +98,7 @@ public class Configurator {
         sb.append("serverCnxnFactory=org.apache.zookeeper.server.VespaNettyServerCnxnFactory").append("\n");
         sb.append("quorumListenOnAllIPs=true").append("\n");
         sb.append("standaloneEnabled=false").append("\n");
-        sb.append("reconfigEnabled=true").append("\n");
+        sb.append("reconfigEnabled=").append(config.dynamicReconfiguration()).append("\n");
         sb.append("skipACL=yes").append("\n");
         ensureThisServerIsRepresented(config.myid(), config.server());
         config.server().forEach(server -> sb.append(serverSpec(server, server.joining())).append("\n"));
diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
index 9f1ed3967f4..6719a68e1cd 100644
--- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
+++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
@@ -54,7 +54,8 @@ public class Reconfigurer extends AbstractComponent {
         shutdown();
     }
 
-    QuorumPeer startOrReconfigure(ZookeeperServerConfig newConfig, VespaZooKeeperServer server,
+    QuorumPeer startOrReconfigure(ZookeeperServerConfig newConfig,
+                                  VespaZooKeeperServer server,
                                   Supplier<QuorumPeer> quorumPeerCreator) {
         if (zooKeeperRunner == null) {
             peer = quorumPeerCreator.get(); // Obtain the peer from the server. This will be shared with later servers.
diff --git a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
index 6dcdc76a593..5d0031d5b55 100644
--- a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
+++ b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ConfiguratorTest.java
@@ -54,26 +54,21 @@ public class ConfiguratorTest {
     }
 
     @Test
-    public void config_is_written_correctly_when_one_server() {
+    public void config_is_written_correctly_with_one_server() {
         ZookeeperServerConfig.Builder builder = createConfigBuilderForSingleHost(cfgFile, idFile);
         new Configurator(builder.build()).writeConfigToDisk(VespaTlsConfig.tlsDisabled());
-        validateConfigFileSingleHost(cfgFile);
+        validateConfigFileSingleHost(cfgFile, false);
         validateIdFile(idFile, "0\n");
     }
 
     @Test
-    public void config_is_written_correctly_when_multiple_servers() {
-        ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
-        builder.zooKeeperConfigFile(cfgFile.getAbsolutePath());
-        builder.server(newServer(0, "foo", 123, 321, false));
-        builder.server(newServer(1, "bar", 234, 432, false));
-        builder.server(newServer(2, "baz", 345, 543, true));
-        builder.myidFile(idFile.getAbsolutePath());
-        builder.myid(1);
-        builder.tickTime(1234);
-        new Configurator(builder.build()).writeConfigToDisk(VespaTlsConfig.tlsDisabled());
-        validateConfigFileMultipleHosts(cfgFile);
-        validateIdFile(idFile, "1\n");
+    public void config_is_written_correctly_with_multiple_servers() {
+        three_config_servers(false);
+    }
+
+    @Test
+    public void config_is_written_correctly_with_multiple_servers_on_hosted_vespa() {
+        three_config_servers(true);
     }
 
     @Test
@@ -81,7 +76,7 @@ public class ConfiguratorTest {
         ZookeeperServerConfig.Builder builder = createConfigBuilderForSingleHost(cfgFile, idFile);
         TlsContext tlsContext = createTlsContext();
         new Configurator(builder.build()).writeConfigToDisk(new VespaTlsConfig(tlsContext, MixedMode.TLS_CLIENT_MIXED_SERVER));
-        validateConfigFileTlsWithMixedMode(cfgFile);
+        validateConfigFileTlsWithMixedMode(cfgFile, false);
     }
 
     @Test
@@ -89,7 +84,7 @@ public class ConfiguratorTest {
         ZookeeperServerConfig.Builder builder = createConfigBuilderForSingleHost(cfgFile, idFile);
         TlsContext tlsContext = createTlsContext();
         new Configurator(builder.build()).writeConfigToDisk(new VespaTlsConfig(tlsContext, MixedMode.DISABLED));
-        validateConfigFileTlsWithoutMixedMode(cfgFile);
+        validateConfigFileTlsWithoutMixedMode(cfgFile, false);
     }
 
     @Test(expected = RuntimeException.class)
@@ -122,6 +117,21 @@ public class ConfiguratorTest {
         assertEquals("" + max_buffer, System.getProperty(ZOOKEEPER_JUTE_MAX_BUFFER));
     }
 
+    private void three_config_servers(boolean hosted) {
+        ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
+        builder.zooKeeperConfigFile(cfgFile.getAbsolutePath());
+        builder.server(newServer(0, "foo", 123, 321, false));
+        builder.server(newServer(1, "bar", 234, 432, false));
+        builder.server(newServer(2, "baz", 345, 543, true));
+        builder.myidFile(idFile.getAbsolutePath());
+        builder.myid(1);
+        builder.tickTime(1234);
+        builder.dynamicReconfiguration(hosted);
+        new Configurator(builder.build()).writeConfigToDisk(VespaTlsConfig.tlsDisabled());
+        validateConfigFileMultipleHosts(cfgFile, hosted);
+        validateIdFile(idFile, "1\n");
+    }
+
     private ZookeeperServerConfig.Builder createConfigBuilderForSingleHost(File cfgFile, File idFile) {
         ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
         builder.zooKeeperConfigFile(cfgFile.getAbsolutePath());
@@ -147,7 +157,7 @@ public class ConfiguratorTest {
         assertEquals(expected, actual);
     }
 
-    private String commonConfig() {
+    private String commonConfig(boolean hosted) {
         return "tickTime=1234\n" +
                "initLimit=20\n" +
                "syncLimit=15\n" +
@@ -161,13 +171,13 @@ public class ConfiguratorTest {
                "serverCnxnFactory=org.apache.zookeeper.server.VespaNettyServerCnxnFactory\n" +
                "quorumListenOnAllIPs=true\n" +
                "standaloneEnabled=false\n" +
-               "reconfigEnabled=true\n" +
+               "reconfigEnabled=" + hosted + "\n" +
                "skipACL=yes\n";
     }
 
-    private void validateConfigFileSingleHost(File cfgFile) {
+    private void validateConfigFileSingleHost(File cfgFile, boolean hosted) {
         String expected =
-                commonConfig() +
+                commonConfig(hosted) +
                 "server.0=foo:321:123;2181\n" +
                 "sslQuorum=false\n" +
                 "portUnification=false\n" +
@@ -191,9 +201,9 @@ public class ConfiguratorTest {
                 "ssl.clientAuth=NEED\n";
     }
 
-    private void validateConfigFileMultipleHosts(File cfgFile) {
+    private void validateConfigFileMultipleHosts(File cfgFile, boolean hosted) {
         String expected =
-                commonConfig() +
+                commonConfig(hosted) +
                 "server.0=foo:321:123;2181\n" +
                 "server.1=bar:432:234;2181\n" +
                 "server.2=baz:543:345:observer;2181\n" +
@@ -204,9 +214,9 @@ public class ConfiguratorTest {
     }
 
 
-    private void validateConfigFileTlsWithMixedMode(File cfgFile) {
+    private void validateConfigFileTlsWithMixedMode(File cfgFile, boolean hosted) {
         String expected =
-                commonConfig() +
+                commonConfig(hosted) +
                 "server.0=foo:321:123;2181\n" +
                 "sslQuorum=true\n" +
                 "portUnification=true\n" +
@@ -216,9 +226,9 @@ public class ConfiguratorTest {
         validateConfigFile(cfgFile, expected);
     }
 
-    private void validateConfigFileTlsWithoutMixedMode(File cfgFile) {
+    private void validateConfigFileTlsWithoutMixedMode(File cfgFile, boolean hosted) {
         String expected =
-                commonConfig() +
+                commonConfig(hosted) +
                 "server.0=foo:321:123;2181\n" +
                 "sslQuorum=true\n" +
                 "portUnification=false\n" +
diff --git a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ConfigServerZooKeeperServer.java b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ConfigServerZooKeeperServer.java
new file mode 100644
index 00000000000..17179aa5e69
--- /dev/null
+++ b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ConfigServerZooKeeperServer.java
@@ -0,0 +1,43 @@
+// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.zookeeper;
+
+import com.yahoo.cloud.config.ZookeeperServerConfig;
+import com.yahoo.component.AbstractComponent;
+import com.yahoo.component.annotation.Inject;
+import java.nio.file.Path;
+
+/**
+ *
+ * Server used for starting config server, needed to be able to have different behavior for hosted and
+ * self-hosted Vespa (controlled by zookeeperServerConfig.dynamicReconfiguration).
+ *
+ * @author Harald Musum
+ */
+public class ConfigServerZooKeeperServer extends AbstractComponent implements VespaZooKeeperServer {
+
+    private final VespaZooKeeperServer zooKeeperServer;
+
+    @Inject
+    public ConfigServerZooKeeperServer(ZookeeperServerConfig zookeeperServerConfig) {
+        this.zooKeeperServer = zookeeperServerConfig.dynamicReconfiguration()
+                ? new ReconfigurableVespaZooKeeperServer(new Reconfigurer(new VespaZooKeeperAdminImpl()), zookeeperServerConfig)
+                : new VespaZooKeeperServerImpl(zookeeperServerConfig);
+    }
+
+    @Override
+    public void deconstruct() { zooKeeperServer.shutdown(); }
+
+    @Override
+    public void shutdown() {
+        zooKeeperServer.shutdown();
+    }
+
+    @Override
+    public void start(Path configFilePath) {
+        zooKeeperServer.start(configFilePath);
+    }
+
+    @Override
+    public boolean reconfigurable() { return zooKeeperServer.reconfigurable(); }
+
+}
diff --git a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ReconfigurableVespaZooKeeperServer.java b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ReconfigurableVespaZooKeeperServer.java
index e94110af2fb..8161e4bba4b 100644
--- a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ReconfigurableVespaZooKeeperServer.java
+++ b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/ReconfigurableVespaZooKeeperServer.java
@@ -1,13 +1,12 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.zookeeper;
 
-import com.yahoo.component.annotation.Inject;
+import ai.vespa.validation.Validation;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.component.AbstractComponent;
-
+import com.yahoo.component.annotation.Inject;
 import java.nio.file.Path;
 import java.time.Duration;
-import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * Starts or reconfigures zookeeper cluster.
@@ -22,6 +21,9 @@ public class ReconfigurableVespaZooKeeperServer extends AbstractComponent implem
 
     @Inject
     public ReconfigurableVespaZooKeeperServer(Reconfigurer reconfigurer, ZookeeperServerConfig zookeeperServerConfig) {
+        Validation.require(zookeeperServerConfig.dynamicReconfiguration(),
+                           zookeeperServerConfig.dynamicReconfiguration(),
+                           "dynamicReconfiguration must be true");
         peer = reconfigurer.startOrReconfigure(zookeeperServerConfig, this, () -> peer = new VespaQuorumPeer());
     }
 
diff --git a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaQuorumPeer.java b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaQuorumPeer.java
index 47ec03367c1..e5c35a185b5 100644
--- a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaQuorumPeer.java
+++ b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaQuorumPeer.java
@@ -13,8 +13,8 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
- * Starts/stops a ZooKeeper server. Extends QuorumPeerMain to be able to call initializeAndRun() and wraps
- * exceptions so it can be used by code that does not depend on ZooKeeper.
+ * Starts or stops a ZooKeeper server. Extends QuorumPeerMain to be able to call initializeAndRun() and wraps
+ * exceptions so that it can be used by code that does not depend on ZooKeeper.
  *
  * @author hmusum
  */
@@ -41,7 +41,7 @@ class VespaQuorumPeer extends QuorumPeerMain implements QuorumPeer {
                 //
                 // When a VespaZooKeeperServer component receives a new config, the container will try to start a new
                 // server with the new config, this will fail until the old server is deconstructed. If the old server
-                // fails to deconstruct/shut down, the new one will never start and if that happens forcing a restart is
+                // fails to deconstruct/shutdown, the new one will never start and if that happens forcing a restart is
                 // the better option.
                 Process.logAndDie("Failed to shut down ZooKeeper server properly, forcing shutdown", e);
             }
diff --git a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaZooKeeperServerImpl.java b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaZooKeeperServerImpl.java
index 48f95d28910..e5bfa60f47e 100644
--- a/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaZooKeeperServerImpl.java
+++ b/zookeeper-server/zookeeper-server/src/main/java/com/yahoo/vespa/zookeeper/VespaZooKeeperServerImpl.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.zookeeper;
 
+import ai.vespa.validation.Validation;
 import com.yahoo.component.annotation.Inject;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.component.AbstractComponent;
@@ -19,6 +20,9 @@ public class VespaZooKeeperServerImpl extends AbstractComponent implements Vespa
 
     @Inject
     public VespaZooKeeperServerImpl(ZookeeperServerConfig zookeeperServerConfig) {
+        Validation.require(! zookeeperServerConfig.dynamicReconfiguration(),
+                           ! zookeeperServerConfig.dynamicReconfiguration(),
+                           "dynamicReconfiguration must be false");
         this.peer = new VespaQuorumPeer();
         this.runner = new ZooKeeperRunner(zookeeperServerConfig, this);
     }