Merge pull request #17980 from vespa-engine/hmusum/cleanup-10

ZooKeeperClient cleanup

5 files changed, 61 insertions, 102 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/InfraDeployerProvider.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/InfraDeployerProvider.java
index bc15121e190..1631774112b 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/InfraDeployerProvider.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/InfraDeployerProvider.java
@@ -1,4 +1,4 @@
-// Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.deploy;
 
 import com.yahoo.cloud.config.ConfigserverConfig;
@@ -50,10 +50,7 @@ public class InfraDeployerProvider {
 
     /** Creates either an empty provider or a provider having the given infrastructure deployer */
     public static InfraDeployerProvider from(Optional<InfraDeployer> infraDeployer) {
-        if (infraDeployer.isPresent())
-            return withInfraDeployer(infraDeployer.get());
-        else
-            return empty();
+        return infraDeployer.map(InfraDeployerProvider::withInfraDeployer).orElseGet(InfraDeployerProvider::empty);
     }
 
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClient.java
index 45384f78d4c..acb5e665c5f 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClient.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClient.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.deploy;
 
 import com.yahoo.component.Version;
@@ -17,6 +17,7 @@ import com.yahoo.path.Path;
 import com.yahoo.vespa.config.ConfigDefinitionKey;
 import com.yahoo.vespa.config.server.zookeeper.ConfigCurator;
 import com.yahoo.vespa.config.server.zookeeper.ZKApplicationPackage;
+import com.yahoo.yolean.Exceptions;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -27,6 +28,12 @@ import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 
+import static com.yahoo.config.application.api.ApplicationPackage.*;
+import static com.yahoo.vespa.config.server.zookeeper.ConfigCurator.DEFCONFIGS_ZK_SUBPATH;
+import static com.yahoo.vespa.config.server.zookeeper.ConfigCurator.META_ZK_PATH;
+import static com.yahoo.vespa.config.server.zookeeper.ConfigCurator.USERAPP_ZK_SUBPATH;
+import static com.yahoo.vespa.config.server.zookeeper.ConfigCurator.USER_DEFCONFIGS_ZK_SUBPATH;
+
 /**
  * A class used for reading and writing application data to zookeeper.
  *
@@ -36,59 +43,27 @@ public class ZooKeeperClient {
 
     private final ConfigCurator configCurator;
     private final DeployLogger logger;
-    /* This is the generation that will be used for reading and writing application data. (1 more than last deployed application) */
-    private final Path rootPath;
+    private final Path sessionPath; // session id
 
     private static final ApplicationFile.PathFilter xmlFilter = path -> path.getName().endsWith(".xml");
 
-    public ZooKeeperClient(ConfigCurator configCurator, DeployLogger logger, Path rootPath) {
+    public ZooKeeperClient(ConfigCurator configCurator, DeployLogger logger, Path sessionPath) {
         this.configCurator = configCurator;
         this.logger = logger;
-        this.rootPath = rootPath;
+        this.sessionPath = sessionPath;
     }
 
     /**
      * Sets up basic node structure in ZooKeeper and purges old data.
-     * This is the first operation on ZK during deploy-application.
-     *
-     * We have retries in this method because there have been cases of stray connection loss to ZK,
-     * even though the user has started the config server.
-     *
+     * This is the first operation on ZK during deploy.
      */
-    void setupZooKeeper() {
-        int retries = 5;
-        try {
-            while (retries > 0) {
-                try {
-                    createZooKeeperNodes();
-                    break;
-                } catch (RuntimeException e) {
-                    logger.log(Level.FINE, "ZK init failed, retrying: " + e);
-                    retries--;
-                    if (retries == 0) {
-                        throw e;
-                    }
-                    Thread.sleep(100);
-                    // Not reconnecting, ZK is supposed to handle that automatically
-                    // as long as the session doesn't expire. We'll see.
-                }
-            }
-        } catch (Exception e) {
-            throw new IllegalStateException("Unable to initialize vespa model writing to config server(s) " +
-                                            System.getProperty("configsources") + "\n" +
-                                            "Please ensure that cloudconfig_server is started on the config server node(s), " +
-                                            "and check the vespa log for configserver errors. ", e);
-        }
-    }
-
-    /** Sets the app id and attempts to set up zookeeper. The app id must be ordered for purge to work OK. */
-    private void createZooKeeperNodes() {
-        if ( ! configCurator.exists(rootPath.getAbsolute()))
-            configCurator.createNode(rootPath.getAbsolute());
+    void initialize() {
+        if ( ! configCurator.exists(sessionPath.getAbsolute()))
+            configCurator.createNode(sessionPath.getAbsolute());
 
-        for (String subPath : Arrays.asList(ConfigCurator.DEFCONFIGS_ZK_SUBPATH,
-                                            ConfigCurator.USER_DEFCONFIGS_ZK_SUBPATH,
-                                            ConfigCurator.USERAPP_ZK_SUBPATH,
+        for (String subPath : Arrays.asList(DEFCONFIGS_ZK_SUBPATH,
+                                            USER_DEFCONFIGS_ZK_SUBPATH,
+                                            USERAPP_ZK_SUBPATH,
                                             ZKApplicationPackage.fileRegistryNode)) {
             // TODO: The replaceFirst below is hackish.
             configCurator.createNode(getZooKeeperAppPath(null).getAbsolute(),
@@ -110,7 +85,7 @@ public class ZooKeeperClient {
             write(app.getMetaData());
         } catch (Exception e) {
             throw new IllegalStateException("Unable to write vespa model to config server(s) " + System.getProperty("configsources") + "\n" +
-                                            "Please ensure that cloudconfig_server is started on the config server node(s), " +
+                                            "Please ensure that config server is started " +
                                             "and check the vespa log for configserver errors. ", e);
         }
     }
@@ -121,7 +96,7 @@ public class ZooKeeperClient {
 
         // TODO: Change to SCHEMAS_DIR after March 2020
         // TODO: When it does also check RankExpressionFile.sendTo
-        Path zkPath = getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.SEARCH_DEFINITIONS_DIR);
+        Path zkPath = getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(ApplicationPackage.SEARCH_DEFINITIONS_DIR);
         configCurator.createNode(zkPath.getAbsolute());
         // Ensures that ranking expressions and other files are also written
         writeDir(app.getFile(ApplicationPackage.SEARCH_DEFINITIONS_DIR), zkPath, false);
@@ -140,38 +115,34 @@ public class ZooKeeperClient {
      */
     private void writeSomeOf(ApplicationPackage app) throws IOException {
         // TODO: We should have a way of doing this which doesn't require repeating all the content
-        writeFile(app.getFile(Path.fromString(ApplicationPackage.SERVICES)),
-                  getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH));
-        writeFile(app.getFile(Path.fromString(ApplicationPackage.HOSTS)),
-                  getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH));
-        writeFile(app.getFile(Path.fromString(ApplicationPackage.DEPLOYMENT_FILE.getName())),
-                  getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH));
-        writeFile(app.getFile(Path.fromString(ApplicationPackage.VALIDATION_OVERRIDES.getName())),
-                  getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH));
-        writeDir(app.getFile(ApplicationPackage.RULES_DIR),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.RULES_DIR),
+        writeFile(app.getFile(Path.fromString(SERVICES)), getZooKeeperAppPath(USERAPP_ZK_SUBPATH));
+        writeFile(app.getFile(Path.fromString(HOSTS)), getZooKeeperAppPath(USERAPP_ZK_SUBPATH));
+        writeFile(app.getFile(Path.fromString(DEPLOYMENT_FILE.getName())), getZooKeeperAppPath(USERAPP_ZK_SUBPATH));
+        writeFile(app.getFile(Path.fromString(VALIDATION_OVERRIDES.getName())), getZooKeeperAppPath(USERAPP_ZK_SUBPATH));
+        writeDir(app.getFile(RULES_DIR),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(RULES_DIR),
                  (path) -> path.getName().endsWith(ApplicationPackage.RULES_NAME_SUFFIX),
                  true);
-        writeDir(app.getFile(ApplicationPackage.QUERY_PROFILES_DIR),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.QUERY_PROFILES_DIR),
+        writeDir(app.getFile(QUERY_PROFILES_DIR),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(QUERY_PROFILES_DIR),
                  xmlFilter, true);
-        writeDir(app.getFile(ApplicationPackage.PAGE_TEMPLATES_DIR),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.PAGE_TEMPLATES_DIR),
+        writeDir(app.getFile(PAGE_TEMPLATES_DIR),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(PAGE_TEMPLATES_DIR),
                  xmlFilter, true);
-        writeDir(app.getFile(Path.fromString(ApplicationPackage.SEARCHCHAINS_DIR)),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.SEARCHCHAINS_DIR),
+        writeDir(app.getFile(Path.fromString(SEARCHCHAINS_DIR)),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(SEARCHCHAINS_DIR),
                  xmlFilter, true);
-        writeDir(app.getFile(Path.fromString(ApplicationPackage.DOCPROCCHAINS_DIR)),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.DOCPROCCHAINS_DIR),
+        writeDir(app.getFile(Path.fromString(DOCPROCCHAINS_DIR)),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(DOCPROCCHAINS_DIR),
                  xmlFilter, true);
-        writeDir(app.getFile(Path.fromString(ApplicationPackage.ROUTINGTABLES_DIR)),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.ROUTINGTABLES_DIR),
+        writeDir(app.getFile(Path.fromString(ROUTINGTABLES_DIR)),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(ROUTINGTABLES_DIR),
                  xmlFilter, true);
-        writeDir(app.getFile(ApplicationPackage.MODELS_GENERATED_REPLICATED_DIR),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.MODELS_GENERATED_REPLICATED_DIR),
+        writeDir(app.getFile(MODELS_GENERATED_REPLICATED_DIR),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(MODELS_GENERATED_REPLICATED_DIR),
                  true);
-        writeDir(app.getFile(ApplicationPackage.SECURITY_DIR),
-                 getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH).append(ApplicationPackage.SECURITY_DIR),
+        writeDir(app.getFile(SECURITY_DIR),
+                 getZooKeeperAppPath(USERAPP_ZK_SUBPATH).append(SECURITY_DIR),
                  true);
     }
 
@@ -234,10 +205,10 @@ public class ZooKeeperClient {
             ApplicationFile dir = applicationPackage.getFile(Path.fromString(userInclude));
             final List<ApplicationFile> files = dir.listFiles();
             if (files == null || files.isEmpty()) {
-                configCurator.createNode(getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH + "/" + userInclude).getAbsolute());
+                configCurator.createNode(getZooKeeperAppPath(USERAPP_ZK_SUBPATH + "/" + userInclude).getAbsolute());
             }
             writeDir(dir,
-                     getZooKeeperAppPath(ConfigCurator.USERAPP_ZK_SUBPATH + "/" + userInclude),
+                     getZooKeeperAppPath(USERAPP_ZK_SUBPATH + "/" + userInclude),
                      xmlFilter, true);
         }
     }
@@ -251,8 +222,8 @@ public class ZooKeeperClient {
         for (Map.Entry<ConfigDefinitionKey, UnparsedConfigDefinition> entry : configDefs.entrySet()) {
             ConfigDefinitionKey key = entry.getKey();
             String contents = entry.getValue().getUnparsedContent();
-            writeConfigDefinition(key.getName(), key.getNamespace(), getZooKeeperAppPath(ConfigCurator.USER_DEFCONFIGS_ZK_SUBPATH).getAbsolute(), contents);
-            writeConfigDefinition(key.getName(), key.getNamespace(), getZooKeeperAppPath(ConfigCurator.DEFCONFIGS_ZK_SUBPATH).getAbsolute(), contents);
+            writeConfigDefinition(key.getName(), key.getNamespace(), getZooKeeperAppPath(USER_DEFCONFIGS_ZK_SUBPATH).getAbsolute(), contents);
+            writeConfigDefinition(key.getName(), key.getNamespace(), getZooKeeperAppPath(DEFCONFIGS_ZK_SUBPATH).getAbsolute(), contents);
         }
         logger.log(Level.FINE, configDefs.size() + " user config definitions");
     }
@@ -276,19 +247,15 @@ public class ZooKeeperClient {
      * @param metaData The application metadata.
      */
     private void write(ApplicationMetaData metaData) {
-        configCurator.putData(getZooKeeperAppPath(ConfigCurator.META_ZK_PATH).getAbsolute(), metaData.asJsonBytes());
+        configCurator.putData(getZooKeeperAppPath(META_ZK_PATH).getAbsolute(), metaData.asJsonBytes());
     }
 
     void cleanupZooKeeper() {
         try {
-            for (String subPath : Arrays.asList(
-                    ConfigCurator.DEFCONFIGS_ZK_SUBPATH,
-                    ConfigCurator.USER_DEFCONFIGS_ZK_SUBPATH,
-                    ConfigCurator.USERAPP_ZK_SUBPATH)) {
-                configCurator.deleteRecurse(getZooKeeperAppPath(null).append(subPath).getAbsolute());
-            }
+            List.of(DEFCONFIGS_ZK_SUBPATH, USER_DEFCONFIGS_ZK_SUBPATH, USERAPP_ZK_SUBPATH)
+                .forEach(path -> configCurator.deleteRecurse(getZooKeeperAppPath(path).getAbsolute()));
         } catch (Exception e) {
-            logger.log(Level.WARNING, "Could not clean up in zookeeper");
+            logger.log(Level.WARNING, "Could not clean up in zookeeper: " + Exceptions.toMessageString(e));
             //Might be called in an exception handler before re-throw, so do not throw here.
         }
     }
@@ -301,15 +268,13 @@ public class ZooKeeperClient {
      * @return a String with the full ZK application path including trailing path, if set
      */
     private Path getZooKeeperAppPath(String trailingPath) {
-        if (trailingPath != null) {
-            return rootPath.append(trailingPath);
-        } else {
-            return rootPath;
-        }
+        if (trailingPath == null) return sessionPath;
+
+        return sessionPath.append(trailingPath);
     }
 
     public void write(AllocatedHosts hosts) throws IOException {
-        configCurator.putData(rootPath.append(ZKApplicationPackage.allocatedHostsNode).getAbsolute(),
+        configCurator.putData(sessionPath.append(ZKApplicationPackage.allocatedHostsNode).getAbsolute(),
                               AllocatedHostsSerializer.toJson(hosts));
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperDeployer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperDeployer.java
index e24d24f0c3f..12aa5b7cc35 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperDeployer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/ZooKeeperDeployer.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.deploy;
 
 import com.yahoo.config.application.api.ApplicationPackage;
@@ -11,7 +11,6 @@ import java.util.Map;
 
 /**
  * Interface for initializing zookeeper and deploying an application package to zookeeper.
- * Initialize must be called before each deploy.
  *
  * @author Ulf Lilleengen
  */
@@ -24,16 +23,16 @@ public class ZooKeeperDeployer {
     }
 
     /**
-     * Deploys an application package to zookeeper. initialize() must be called before calling this method.
+     * Deploys an application package to zookeeper
      *
      * @param applicationPackage the application package to persist.
      * @param fileRegistryMap the file registries to persist.
-     * @param allocatedHosts the provisioning info to persist.
+     * @param allocatedHosts the allocated hosts to persist.
      * @throws IOException if deploying fails
      */
     public void deploy(ApplicationPackage applicationPackage, Map<Version, FileRegistry> fileRegistryMap, 
                        AllocatedHosts allocatedHosts) throws IOException {
-        zooKeeperClient.setupZooKeeper();
+        zooKeeperClient.initialize();
         zooKeeperClient.write(applicationPackage);
         zooKeeperClient.write(fileRegistryMap);
         zooKeeperClient.write(allocatedHosts);
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java
index 30cdc0f6e8a..50b9ac55bda 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionPreparer.java
@@ -27,7 +27,6 @@ import com.yahoo.container.jdisc.secretstore.SecretStore;
 import com.yahoo.lang.SettableOptional;
 import com.yahoo.path.Path;
 import com.yahoo.vespa.config.server.TimeoutBudget;
-import com.yahoo.vespa.config.server.application.ApplicationCuratorDatabase;
 import com.yahoo.vespa.config.server.application.ApplicationSet;
 import com.yahoo.vespa.config.server.application.PermanentApplicationPackage;
 import com.yahoo.vespa.config.server.configchange.ConfigChangeActions;
@@ -44,7 +43,6 @@ import com.yahoo.vespa.config.server.tenant.ApplicationRolesStore;
 import com.yahoo.vespa.config.server.tenant.ContainerEndpointsCache;
 import com.yahoo.vespa.config.server.tenant.EndpointCertificateMetadataStore;
 import com.yahoo.vespa.config.server.tenant.EndpointCertificateRetriever;
-import com.yahoo.vespa.config.server.tenant.SecretStoreExternalIdRetriever;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.flags.FlagSource;
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClientTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClientTest.java
index deff0aba376..7d14b1996b0 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClientTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/ZooKeeperClientTest.java
@@ -63,7 +63,7 @@ public class ZooKeeperClientTest {
                                                                                                2L));
         Map<Version, FileRegistry> fileRegistries = createFileRegistries();
         app.writeMetaData();
-        zkc.setupZooKeeper();
+        zkc.initialize();
         zkc.write(app);
         zkc.write(fileRegistries);
     }
@@ -85,7 +85,7 @@ public class ZooKeeperClientTest {
         BaseDeployLogger logger = new BaseDeployLogger();
         long generation = 1L;
         ZooKeeperClient zooKeeperClient = new ZooKeeperClient(zk, logger, Path.fromString("/1"));
-        zooKeeperClient.setupZooKeeper();
+        zooKeeperClient.initialize();
         String appPath = "/";
         assertThat(zk.getChildren(appPath).size(), is(1));
         assertTrue(zk.exists("/" + generation));
@@ -157,7 +157,7 @@ public class ZooKeeperClientTest {
         BaseDeployLogger logger = new BaseDeployLogger();
         Path app = Path.fromString("/1");
         ZooKeeperClient zooKeeperClient = new ZooKeeperClient(zk, logger, app);
-        zooKeeperClient.setupZooKeeper();
+        zooKeeperClient.initialize();
         HostSpec host1 = new HostSpec("host1.yahoo.com", Collections.emptyList(), Optional.empty());
         HostSpec host2 = new HostSpec("host2.yahoo.com", Collections.emptyList(), Optional.empty());
         ImmutableSet<HostSpec> hosts = ImmutableSet.of(host1, host2);