Add config option throwIfBootstrappingTenantRepoFails

If option is true and bootstrapping one or more tenants fails an
exception will be thrown and config server will not start. Default
value is false for now.

2 files changed, 96 insertions, 38 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantRepository.java
index 779571b737e..bd5b694d657 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/tenant/TenantRepository.java
@@ -23,18 +23,22 @@ import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 /**
  * This component will monitor the set of tenants in the config server by watching in ZooKeeper.
@@ -70,9 +74,10 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
 
     private final MetricUpdater metricUpdater;
     private final ExecutorService pathChildrenExecutor = Executors.newFixedThreadPool(1, ThreadFactoryFactory.getThreadFactory(TenantRepository.class.getName()));
+    private final ExecutorService bootstrapExecutor;
     private final ScheduledExecutorService checkForRemovedApplicationsService = new ScheduledThreadPoolExecutor(1);
     private final Optional<Curator.DirectoryCache> directoryCache;
-
+    private final boolean throwExceptionIfBootstrappingFails;
 
     /**
      * Creates a new tenant repository
@@ -93,13 +98,16 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
      */
     public TenantRepository(GlobalComponentRegistry globalComponentRegistry, boolean useZooKeeperWatchForTenantChanges) {
         this.globalComponentRegistry = globalComponentRegistry;
+        ConfigserverConfig configserverConfig = globalComponentRegistry.getConfigserverConfig();
+        this.bootstrapExecutor = Executors.newFixedThreadPool(configserverConfig.numParallelTenantLoaders());
+        this.throwExceptionIfBootstrappingFails = configserverConfig.throwIfBootstrappingTenantRepoFails();
         this.curator = globalComponentRegistry.getCurator();
         metricUpdater = globalComponentRegistry.getMetrics().getOrCreateMetricUpdater(Collections.emptyMap());
         this.tenantListeners.add(globalComponentRegistry.getTenantListener());
         curator.framework().getConnectionStateListenable().addListener(this);
 
         curator.create(tenantsPath);
-        createSystemTenants(globalComponentRegistry.getConfigserverConfig());
+        createSystemTenants(configserverConfig);
         curator.create(vespaPath);
 
         if (useZooKeeperWatchForTenantChanges) {
@@ -110,7 +118,7 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
             this.directoryCache = Optional.empty();
         }
         log.log(LogLevel.DEBUG, "Creating all tenants");
-        createTenants();
+        bootstrapTenants();
         notifyTenantsLoaded();
         log.log(LogLevel.DEBUG, "All tenants created");
         checkForRemovedApplicationsService.scheduleWithFixedDelay(this::removeUnusedApplications,
@@ -129,30 +137,20 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
         addTenant(TenantBuilder.create(globalComponentRegistry, tenantName));
     }
 
-    // For testing
     public synchronized void addTenant(TenantBuilder builder) {
         writeTenantPath(builder.getTenantName());
         createTenant(builder);
     }
 
-    /**
-     * Reads the set of tenants in patch cache.
-     *
-     * @return a set of tenant names
-     */
-    private Set<TenantName> readTenantsFromZooKeeper() {
-        Set<TenantName> tenants = new LinkedHashSet<>();
-        for (String tenant : curator.getChildren(tenantsPath)) {
-            tenants.add(TenantName.from(tenant));
-        }
-        return tenants;
+     private static Set<TenantName> readTenantsFromZooKeeper(Curator curator) {
+        return curator.getChildren(tenantsPath).stream().map(TenantName::from).collect(Collectors.toSet());
     }
 
-    synchronized void createTenants() {
-        Set<TenantName> allTenants = readTenantsFromZooKeeper();
+    private synchronized void updateTenants() {
+        Set<TenantName> allTenants = readTenantsFromZooKeeper(curator);
         log.log(LogLevel.DEBUG, "Create tenants, tenants found in zookeeper: " + allTenants);
         checkForRemovedTenants(allTenants);
-        checkForAddedTenants(allTenants);
+        allTenants.stream().filter(tenantName -> ! tenants.containsKey(tenantName)).forEach(this::createTenant);
         metricUpdater.setTenants(tenants.size());
     }
 
@@ -164,16 +162,32 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
         }
     }
 
-    private void checkForAddedTenants(Set<TenantName> newTenants) {
-        // TODO: Creating an executor here for every invocation does not seem optimal
-        ExecutorService executor = Executors.newFixedThreadPool(globalComponentRegistry.getConfigserverConfig().numParallelTenantLoaders());
-        for (TenantName tenantName : newTenants) {
-            // Note: the http handler will check if the tenant exists, and throw accordingly
-            executor.execute(() -> createTenant(tenantName));
+    private void bootstrapTenants() {
+        // Keep track of tenants created
+        Map<TenantName, Future<?>> futures = new HashMap<>();
+        readTenantsFromZooKeeper(curator).forEach(t -> futures.put(t, bootstrapExecutor.submit(() -> createTenant(t))));
+
+        // Wait for all tenants to be created
+        Set<TenantName> failed = new HashSet<>();
+        for (Map.Entry<TenantName, Future<?>> f : futures.entrySet()) {
+            TenantName tenantName = f.getKey();
+            try {
+                f.getValue().get();
+            } catch (ExecutionException e) {
+                log.log(LogLevel.WARNING, "Failed to create tenant " + tenantName);
+                failed.add(tenantName);
+            } catch (InterruptedException e) {
+                log.log(LogLevel.WARNING, "Interrupted while creating tenant '" + tenantName + "'", e);
+            }
         }
-        executor.shutdown();
+
+        if (failed.size() > 0 && throwExceptionIfBootstrappingFails)
+            throw new RuntimeException("Could not create all tenants when bootstrapping, failed to create: " + failed);
+
+        metricUpdater.setTenants(tenants.size());
+        bootstrapExecutor.shutdown();
         try {
-            executor.awaitTermination(365, TimeUnit.DAYS); // Timeout should never happen
+            bootstrapExecutor.awaitTermination(365, TimeUnit.DAYS); // Timeout should never happen
         } catch (InterruptedException e) {
             throw new RuntimeException("Executor for creating tenants did not terminate within timeout");
         }
@@ -183,19 +197,15 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
         createTenant(TenantBuilder.create(globalComponentRegistry, tenantName));
     }
 
-    // TODO: Fix exception handling and make method return tenant
-    private void createTenant(TenantBuilder builder) {
+    // Creates tenant and all its dependencies. This also includes loading active applications
+    protected void createTenant(TenantBuilder builder) {
         TenantName tenantName = builder.getTenantName();
         if (tenants.containsKey(tenantName)) return;
 
-        try {
-            log.log(LogLevel.INFO, "Creating tenant '" + tenantName + "'");
-            Tenant tenant = builder.build();
-            notifyNewTenant(tenant);
-            tenants.putIfAbsent(tenantName, tenant);
-        } catch (Exception e) {
-            log.log(LogLevel.WARNING, "Error loading tenant '" + tenantName + "', skipping.", e);
-        }
+        log.log(LogLevel.INFO, "Creating tenant '" + tenantName + "'");
+        Tenant tenant = builder.build();
+        notifyNewTenant(tenant);
+        tenants.putIfAbsent(tenantName, tenant);
     }
 
     /**
@@ -334,7 +344,7 @@ public class TenantRepository implements ConnectionStateListener, PathChildrenCa
         switch (event.getType()) {
             case CHILD_ADDED:
             case CHILD_REMOVED:
-                createTenants();
+                updateTenants();
                 break;
         }
     }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
index 27ed72da090..d7f209a917e 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/tenant/TenantRepositoryTest.java
@@ -1,10 +1,12 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.tenant;
 
+import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.config.model.test.MockApplicationPackage;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Version;
+import com.yahoo.vespa.config.server.GlobalComponentRegistry;
 import com.yahoo.vespa.config.server.application.ApplicationSet;
 import com.yahoo.vespa.config.server.ServerCache;
 import com.yahoo.vespa.config.server.TestComponentRegistry;
@@ -15,7 +17,10 @@ import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.model.VespaModel;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
 import org.xml.sax.SAXException;
 
 import java.io.IOException;
@@ -40,6 +45,12 @@ public class TenantRepositoryTest {
     private MockTenantListener tenantListener;
     private Curator curator;
 
+    @Rule
+    public ExpectedException expectedException = ExpectedException.none();
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
     @Before
     public void setupSessions() {
         curator = new MockCurator();
@@ -147,6 +158,21 @@ public class TenantRepositoryTest {
         }
     }
 
+    @Test
+    public void testFailingBootstrap() throws IOException {
+        tenantRepository.close(); // stop using the one setup in Before method
+
+        // No exception if config is false
+        boolean throwIfBootstrappingTenantRepoFails = false;
+        new FailingDuringBootstrapTenantRepository(createComponentRegistry(throwIfBootstrappingTenantRepoFails));
+
+        // Should get exception if config is true
+        throwIfBootstrappingTenantRepoFails = true;
+        expectedException.expect(RuntimeException.class);
+        expectedException.expectMessage("Could not create all tenants when bootstrapping, failed to create: [default]");
+        new FailingDuringBootstrapTenantRepository(createComponentRegistry(throwIfBootstrappingTenantRepoFails));
+    }
+
     private List<String> readZKChildren(String path) throws Exception {
         return curator.framework().getChildren().forPath(path);
     }
@@ -155,4 +181,26 @@ public class TenantRepositoryTest {
         assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenantRepository.tenantZkPath(tenantName)));
     }
 
+    private GlobalComponentRegistry createComponentRegistry(boolean throwIfBootstrappingTenantRepoFails) throws IOException {
+        return new TestComponentRegistry.Builder()
+                .curator(new MockCurator())
+                .configServerConfig(new ConfigserverConfig(new ConfigserverConfig.Builder()
+                                                                   .throwIfBootstrappingTenantRepoFails(throwIfBootstrappingTenantRepoFails)
+                                                                   .configDefinitionsDir(temporaryFolder.newFolder("configdefs" + throwIfBootstrappingTenantRepoFails).getAbsolutePath())
+                                                                   .configServerDBDir(temporaryFolder.newFolder("configserverdb" + throwIfBootstrappingTenantRepoFails).getAbsolutePath())))
+                .build();
+    }
+
+    private static class FailingDuringBootstrapTenantRepository extends TenantRepository {
+
+        public FailingDuringBootstrapTenantRepository(GlobalComponentRegistry globalComponentRegistry) {
+            super(globalComponentRegistry, false);
+        }
+
+        @Override
+        protected void createTenant(TenantBuilder builder) {
+            throw new RuntimeException("Failed to create: " + builder.getTenantName());
+        }
+    }
+
 }