From 92504d7eb79f1553049d3b665ce50f3463ec3a8f Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sun, 28 Oct 2018 19:31:34 +0100 Subject: Cleanup tests a bit --- .../config/server/tenant/TenantRepositoryTest.java | 43 ++++++++++------------ 1 file changed, 20 insertions(+), 23 deletions(-) (limited to 'configserver/src') 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 046369edce0..27ed72da090 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 @@ -23,11 +23,9 @@ import java.util.Arrays; import java.util.List; import java.util.Set; -import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -48,7 +46,7 @@ public class TenantRepositoryTest { globalComponentRegistry = new TestComponentRegistry.Builder().curator(curator).build(); listener = (TenantRequestHandlerTest.MockReloadListener)globalComponentRegistry.getReloadListener(); tenantListener = (MockTenantListener)globalComponentRegistry.getTenantListener(); - tenantListener.tenantsLoaded = false; + assertFalse(tenantListener.tenantsLoaded); tenantRepository = new TenantRepository(globalComponentRegistry); assertTrue(tenantListener.tenantsLoaded); tenantRepository.addTenant(tenant1); @@ -76,30 +74,27 @@ public class TenantRepositoryTest { Version.fromIntValues(1, 2, 3), MetricUpdater.createTestUpdater(), ApplicationId.defaultId()))); - assertThat(listener.reloaded.get(), is(1)); - } - - private List readZKChildren(String path) throws Exception { - return curator.framework().getChildren().forPath(path); + assertEquals(1, listener.reloaded.get()); } @Test public void testTenantListenersNotified() throws Exception { tenantRepository.addTenant(tenant3); - assertThat("tenant3 not the last created tenant. Tenants: " + tenantRepository.getAllTenantNames() + - ", /config/v2/tenants: " + readZKChildren("/config/v2/tenants"), - tenantListener.tenantCreatedName, is(tenant3)); + assertEquals("tenant3 not the last created tenant. Tenants: " + tenantRepository.getAllTenantNames() + + ", /config/v2/tenants: " + readZKChildren("/config/v2/tenants"), + tenant3, tenantListener.tenantCreatedName); tenantRepository.deleteTenant(tenant2); assertFalse(tenantRepository.getAllTenantNames().contains(tenant2)); - assertThat(tenantListener.tenantDeletedName, is(tenant2)); + assertEquals(tenant2, tenantListener.tenantDeletedName); } @Test - public void testAddTenant() { + public void testAddTenant() throws Exception { Set allTenants = tenantRepository.getAllTenantNames(); assertTrue(allTenants.contains(tenant1)); assertTrue(allTenants.contains(tenant2)); tenantRepository.addTenant(tenant3); + assertZooKeeperTenantPathExists(tenant3); allTenants = tenantRepository.getAllTenantNames(); assertTrue(allTenants.contains(tenant1)); assertTrue(allTenants.contains(tenant2)); @@ -107,14 +102,8 @@ public class TenantRepositoryTest { } @Test - public void testPutAdd() throws Exception { - tenantRepository.addTenant(tenant3); - assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenantRepository.tenantZkPath(tenant3))); - } - - @Test - public void testDelete() throws Exception { - assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenantRepository.tenantZkPath(tenant1))); + public void testDeleteTenant() throws Exception { + assertZooKeeperTenantPathExists(tenant1); tenantRepository.deleteTenant(tenant1); assertFalse(tenantRepository.getAllTenantNames().contains(tenant1)); @@ -127,9 +116,9 @@ public class TenantRepositoryTest { } @Test(expected = IllegalArgumentException.class) - public void testDeleteOfDefaultTenant() { + public void testDeletingDefaultTenant() { try { - assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenantRepository.tenantZkPath(TenantName.defaultName()))); + assertZooKeeperTenantPathExists(TenantName.defaultName()); } catch (Exception e) { fail("default tenant does not exist"); } @@ -158,4 +147,12 @@ public class TenantRepositoryTest { } } + private List readZKChildren(String path) throws Exception { + return curator.framework().getChildren().forPath(path); + } + + private void assertZooKeeperTenantPathExists(TenantName tenantName) throws Exception { + assertNotNull(globalComponentRegistry.getCurator().framework().checkExists().forPath(tenantRepository.tenantZkPath(tenantName))); + } + } -- cgit v1.2.3 From bbbe7dcbbe361001b7247a1d911430cf1bd42f0d Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sun, 28 Oct 2018 19:33:46 +0100 Subject: 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. --- configdefinitions/src/vespa/configserver.def | 1 + .../config/server/tenant/TenantRepository.java | 86 ++++++++++++---------- .../config/server/tenant/TenantRepositoryTest.java | 48 ++++++++++++ 3 files changed, 97 insertions(+), 38 deletions(-) (limited to 'configserver/src') diff --git a/configdefinitions/src/vespa/configserver.def b/configdefinitions/src/vespa/configserver.def index 83d48ceab6c..4f7ff28b477 100644 --- a/configdefinitions/src/vespa/configserver.def +++ b/configdefinitions/src/vespa/configserver.def @@ -69,3 +69,4 @@ sleepTimeWhenRedeployingFails long default=30 deleteApplicationLegacy bool default=false buildMinimalSetOfConfigModels bool default=true useDedicatedNodeForLogserver bool default=true +throwIfBootstrappingTenantRepoFails bool default=false 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 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 readTenantsFromZooKeeper() { - Set tenants = new LinkedHashSet<>(); - for (String tenant : curator.getChildren(tenantsPath)) { - tenants.add(TenantName.from(tenant)); - } - return tenants; + private static Set readTenantsFromZooKeeper(Curator curator) { + return curator.getChildren(tenantsPath).stream().map(TenantName::from).collect(Collectors.toSet()); } - synchronized void createTenants() { - Set allTenants = readTenantsFromZooKeeper(); + private synchronized void updateTenants() { + Set 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 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> futures = new HashMap<>(); + readTenantsFromZooKeeper(curator).forEach(t -> futures.put(t, bootstrapExecutor.submit(() -> createTenant(t)))); + + // Wait for all tenants to be created + Set failed = new HashSet<>(); + for (Map.Entry> 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 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()); + } + } + } -- cgit v1.2.3