Merge pull request #24451 from vespa-engine/jonmv/configurable-zk-session-timeout

Jonmv/configurable zk session timeout

6 files changed, 31 insertions, 10 deletions

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 5703de4cb18..9ebaa4f0fd6 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
@@ -89,6 +89,7 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
 
     private MbusParams mbusParams;
     private boolean messageBusEnabled = true;
+    private int zookeeperSessionTimeoutSeconds = 30;
     private final int transport_events_before_wakeup;
     private final int transport_connections_per_target;
     private final int heapSizePercentageOfTotalNodeMemory;
@@ -312,11 +313,11 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
 
     @Override
     public void getConfig(CuratorConfig.Builder builder) {
-        if (getParent() instanceof ConfigserverCluster) return; // Produces its own config
         super.getConfig(builder);
+        if (getParent() instanceof ConfigserverCluster) return; // Produces its own config
 
-        // 12s is 2x the current ZookeeperServerConfig.tickTime() of 6s, and the default minimum the server will accept.
-        builder.zookeeperSessionTimeoutSeconds(12); // TODO jonmv: make configurable
+        // Will be bounded by 2x and 20x ZookeeperServerConfig.tickTime(), which is currently 6s.
+        builder.zookeeperSessionTimeoutSeconds(zookeeperSessionTimeoutSeconds);
     }
 
     public Optional<String> getTlsClientAuthority() {
@@ -329,6 +330,10 @@ public final class ApplicationContainerCluster extends ContainerCluster<Applicat
 
     public void setMessageBusEnabled(boolean messageBusEnabled) { this.messageBusEnabled = messageBusEnabled; }
 
+    public void setZookeeperSessionTimeoutSeconds(int timeoutSeconds) {
+        this.zookeeperSessionTimeoutSeconds = timeoutSeconds;
+    }
+
     protected boolean messageBusEnabled() { return messageBusEnabled; }
 
     public void addMbusServer(ComponentId chainId) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index 6fbe334ba30..0219f8a5275 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -233,7 +233,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     }
 
     private void addZooKeeper(ApplicationContainerCluster cluster, Element spec) {
-        if ( ! hasZooKeeper(spec)) return;
+        Element zooKeeper = getZooKeeper(spec);
+        if (zooKeeper == null) return;
+
         Element nodesElement = XML.getChild(spec, "nodes");
         boolean isCombined = nodesElement != null && nodesElement.hasAttribute("of");
         if (isCombined) {
@@ -247,6 +249,17 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         }
         cluster.addSimpleComponent("com.yahoo.vespa.curator.Curator", null, "zkfacade");
         cluster.addSimpleComponent("com.yahoo.vespa.curator.CuratorWrapper", null, "zkfacade");
+        String sessionTimeoutSeconds = zooKeeper.getAttribute("session-timeout-seconds");
+        if ( ! sessionTimeoutSeconds.isBlank()) {
+            try {
+                int timeoutSeconds = Integer.parseInt(sessionTimeoutSeconds);
+                if (timeoutSeconds <= 0) throw new IllegalArgumentException("must be a positive value");
+                cluster.setZookeeperSessionTimeoutSeconds(timeoutSeconds);
+            }
+            catch (RuntimeException e) {
+                throw new IllegalArgumentException("invalid zookeeper session-timeout-seconds '" + sessionTimeoutSeconds + "'", e);
+            }
+        }
 
         // These need to be setup so that they will use the container's config id, since each container
         // have different config (id of zookeeper server)
@@ -806,7 +819,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                                                                                   ClusterSpec.Type.container,
                                                                                   ClusterSpec.Id.from(cluster.getName()), 
                                                                                   log,
-                                                                                  hasZooKeeper(containerElement));
+                                                                                  getZooKeeper(containerElement) != null);
         return createNodesFromHosts(hosts, cluster, context.getDeployState());
     }
 
@@ -1036,8 +1049,8 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                         ));
     }
 
-    private static boolean hasZooKeeper(Element spec) {
-        return XML.getChild(spec, "zookeeper") != null;
+    private static Element getZooKeeper(Element spec) {
+        return XML.getChild(spec, "zookeeper");
     }
 
     /** Disallow renderers named "XmlRenderer" or "JsonRenderer" */
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
index e258790cd42..9a2f8a3f0ec 100755
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/ContainerClusterTest.java
@@ -347,7 +347,7 @@ public class ContainerClusterTest {
         assertEquals(List.of("host-c1", "host-c2"),
                 config.server().stream().map(CuratorConfig.Server::hostname).collect(Collectors.toList()));
         assertTrue(config.zookeeperLocalhostAffinity());
-        assertEquals(12, config.zookeeperSessionTimeoutSeconds());
+        assertEquals(30, config.zookeeperSessionTimeoutSeconds());
     }
 
     @Test
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/configserver/ConfigserverClusterTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/configserver/ConfigserverClusterTest.java
index c71f3946937..72b7e7649fd 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/configserver/ConfigserverClusterTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/configserver/ConfigserverClusterTest.java
@@ -121,6 +121,7 @@ public class ConfigserverClusterTest {
         assertEquals(1, config.server().size());
         assertEquals("localhost", config.server().get(0).hostname());
         assertEquals(2181, config.server().get(0).port());
+        assertEquals(120, config.zookeeperSessionTimeoutSeconds());
         assertTrue(config.zookeeperLocalhostAffinity());
     }
 
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
index 562b1c25b6c..e172e68c0ee 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilderTest.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.model.container.xml;
 
+import com.yahoo.cloud.config.CuratorConfig;
 import com.yahoo.cloud.config.ZookeeperServerConfig;
 import com.yahoo.component.ComponentId;
 import com.yahoo.config.application.api.ApplicationPackage;
@@ -561,7 +562,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
     void cluster_with_zookeeper() {
         Function<Integer, String> servicesXml = (nodeCount) -> "<container version='1.0' id='default'>" +
                 "<nodes count='" + nodeCount + "'/>" +
-                "<zookeeper/>" +
+                "<zookeeper session-timeout-seconds='30'/>" +
                 "</container>";
         VespaModelTester tester = new VespaModelTester();
         tester.addHosts(3);
@@ -571,6 +572,7 @@ public class ContainerModelBuilderTest extends ContainerModelBuilderTestBase {
             assertNotNull(cluster);
             assertComponentConfigured(cluster, "com.yahoo.vespa.curator.Curator");
             assertComponentConfigured(cluster, "com.yahoo.vespa.curator.CuratorWrapper");
+            assertEquals(30, model.getConfig(CuratorConfig.class, cluster.getConfigId()).zookeeperSessionTimeoutSeconds());
             cluster.getContainers().forEach(container -> {
                 assertComponentConfigured(container, "com.yahoo.vespa.zookeeper.ReconfigurableVespaZooKeeperServer");
                 assertComponentConfigured(container, "com.yahoo.vespa.zookeeper.Reconfigurer");
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
index 8bf93e838d3..7a3acd18cd6 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
@@ -131,7 +131,7 @@ public class Nodes {
     /** Adds a list of newly created reserved nodes to the node repository */
     public List<Node> addReservedNodes(LockedNodeList nodes) {
         for (Node node : nodes) {
-            if ( node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER)
+            if (node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER)
                 illegal("Cannot add " + node + ": This is not a child node");
             if (node.allocation().isEmpty())
                 illegal("Cannot add " + node + ": Child nodes need to be allocated");