diff options
author | Valerij Fredriksen <valerij92@gmail.com> | 2019-06-01 16:22:22 +0200 |
---|---|---|
committer | Valerij Fredriksen <valerij92@gmail.com> | 2019-06-01 16:22:22 +0200 |
commit | ee6b26191c0855396971a5b23c2f7cbe6142367c (patch) | |
tree | 3d687cd8ff608789b90bcb8d7f8de018d3283396 | |
parent | 92100f39dea78a928b0fc86de35420050e7d690d (diff) |
Only allow activate/remove legal infra applications in duper model
3 files changed, 55 insertions, 102 deletions
diff --git a/configdefinitions/src/vespa/configserver.def b/configdefinitions/src/vespa/configserver.def index 296518a50c0..af1ced533ad 100644 --- a/configdefinitions/src/vespa/configserver.def +++ b/configdefinitions/src/vespa/configserver.def @@ -64,3 +64,4 @@ sleepTimeWhenRedeployingFails long default=30 buildMinimalSetOfConfigModels bool default=true throwIfBootstrappingTenantRepoFails bool default=true canReturnEmptySentinelConfig bool default=false +serverNodeType enum {config, controller} default=config diff --git a/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModelManager.java b/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModelManager.java index ce99126c481..2477c77bdad 100644 --- a/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModelManager.java +++ b/service-monitor/src/main/java/com/yahoo/vespa/service/duper/DuperModelManager.java @@ -9,7 +9,6 @@ import com.yahoo.config.model.api.SuperModelListener; import com.yahoo.config.model.api.SuperModelProvider; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.HostName; -import com.yahoo.log.LogLevel; import com.yahoo.vespa.flags.FlagSource; import com.yahoo.vespa.service.monitor.DuperModelInfraApi; import com.yahoo.vespa.service.monitor.InfraApplicationApi; @@ -20,7 +19,6 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Function; -import java.util.logging.Logger; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -28,40 +26,42 @@ import java.util.stream.Stream; * @author hakonhall */ public class DuperModelManager implements DuperModelInfraApi { - private static Logger logger = Logger.getLogger(DuperModelManager.class.getName()); // Infrastructure applications - private final ConfigServerHostApplication configServerHostApplication = new ConfigServerHostApplication(); - private final ProxyHostApplication proxyHostApplication = new ProxyHostApplication(); - private final ControllerApplication controllerApplication = new ControllerApplication(); - private final ControllerHostApplication controllerHostApplication = new ControllerHostApplication(); - private final ConfigServerApplication configServerApplication = new ConfigServerApplication(); - - private final Map<ApplicationId, InfraApplication> supportedInfraApplications = Stream.of( - configServerApplication, - configServerHostApplication, - proxyHostApplication, - controllerApplication, - controllerHostApplication) - .collect(Collectors.toMap(InfraApplication::getApplicationId, Function.identity())); - - private final boolean multitenant; + static final ControllerHostApplication controllerHostApplication = new ControllerHostApplication(); + static final ControllerApplication controllerApplication = new ControllerApplication(); + static final ConfigServerHostApplication configServerHostApplication = new ConfigServerHostApplication(); + static final ConfigServerApplication configServerApplication = new ConfigServerApplication(); + static final ProxyHostApplication proxyHostApplication = new ProxyHostApplication(); + + private final Map<ApplicationId, InfraApplication> supportedInfraApplications; private final Object monitor = new Object(); private final DuperModel duperModel; // The set of active infrastructure ApplicationInfo. Not all are necessarily in the DuperModel for historical reasons. - private final Set<ApplicationId> activeInfraInfos = new HashSet<>(2 * supportedInfraApplications.size()); + private final Set<ApplicationId> activeInfraInfos = new HashSet<>(10); @Inject public DuperModelManager(ConfigserverConfig configServerConfig, FlagSource flagSource, SuperModelProvider superModelProvider) { - this(configServerConfig.multitenant(), superModelProvider, new DuperModel()); + this(configServerConfig.multitenant(), + configServerConfig.serverNodeType() == ConfigserverConfig.ServerNodeType.Enum.controller, + superModelProvider, new DuperModel(), flagSource); } /** For testing */ - public DuperModelManager(boolean multitenant, SuperModelProvider superModelProvider, DuperModel duperModel) { - this.multitenant = multitenant; + DuperModelManager(boolean multitenant, boolean isController, SuperModelProvider superModelProvider, DuperModel duperModel, FlagSource flagSource) { this.duperModel = duperModel; + if (multitenant) { + supportedInfraApplications = + (isController ? + Stream.of(controllerHostApplication, controllerApplication) : + Stream.of(configServerHostApplication, configServerApplication, proxyHostApplication) + ).collect(Collectors.toUnmodifiableMap(InfraApplication::getApplicationId, Function.identity())); + } else { + supportedInfraApplications = Map.of(); + } + superModelProvider.registerListener(new SuperModelListener() { @Override public void applicationActivated(SuperModel superModel, ApplicationInfo application) { @@ -89,26 +89,6 @@ public class DuperModelManager implements DuperModelInfraApi { } } - public ConfigServerApplication getConfigServerApplication() { - return configServerApplication; - } - - public ConfigServerHostApplication getConfigServerHostApplication() { - return configServerHostApplication; - } - - public ProxyHostApplication getProxyHostApplication() { - return proxyHostApplication; - } - - public ControllerApplication getControllerApplication() { - return controllerApplication; - } - - public ControllerHostApplication getControllerHostApplication() { - return controllerHostApplication; - } - @Override public List<InfraApplicationApi> getSupportedInfraApplications() { return new ArrayList<>(supportedInfraApplications.values()); @@ -140,19 +120,19 @@ public class DuperModelManager implements DuperModelInfraApi { synchronized (monitor) { activeInfraInfos.add(applicationId); - if (infraApplicationBelongsInDuperModel(applicationId)) { - duperModel.add(application.makeApplicationInfo(hostnames)); - } + duperModel.add(application.makeApplicationInfo(hostnames)); } } @Override public void infraApplicationRemoved(ApplicationId applicationId) { + if (!supportedInfraApplications.containsKey(applicationId)) { + throw new IllegalArgumentException("There is no infrastructure application with ID '" + applicationId + "'"); + } + synchronized (monitor) { activeInfraInfos.remove(applicationId); - if (infraApplicationBelongsInDuperModel(applicationId)) { - duperModel.remove(applicationId); - } + duperModel.remove(applicationId); } } @@ -161,29 +141,4 @@ public class DuperModelManager implements DuperModelInfraApi { return duperModel.getApplicationInfos(); } } - - private boolean infraApplicationBelongsInDuperModel(ApplicationId applicationId) { - // At most 1 of the config server and controller applications can be in the duper model. - // The problem of allowing more than 1 is that orchestration will fail since hostname -> application lookup - // will not be unique. - if (applicationId.equals(controllerApplication.getApplicationId())) { - if (!multitenant) return false; - if (duperModel.contains(configServerApplication.getApplicationId())) { - logger.log(LogLevel.ERROR, "Refusing to add controller application to duper model " + - "since it already contains config server"); - return false; - } - return true; - } else if (applicationId.equals(configServerApplication.getApplicationId())) { - if (!multitenant) return false; - if (duperModel.contains(controllerApplication.getApplicationId())) { - logger.log(LogLevel.ERROR, "Refusing to add config server application to duper model " + - "since it already contains controller"); - return false; - } - return true; - } else { - return true; - } - } } diff --git a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java index 4ab97d7e81e..290d67d0642 100644 --- a/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java +++ b/service-monitor/src/test/java/com/yahoo/vespa/service/duper/DuperModelManagerTest.java @@ -7,6 +7,7 @@ import com.yahoo.config.model.api.SuperModelListener; import com.yahoo.config.model.api.SuperModelProvider; import com.yahoo.config.provision.ApplicationId; import com.yahoo.config.provision.HostName; +import com.yahoo.vespa.flags.InMemoryFlagSource; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -14,6 +15,10 @@ import java.util.List; import java.util.stream.Collectors; import java.util.stream.Stream; +import static com.yahoo.vespa.service.duper.DuperModelManager.configServerApplication; +import static com.yahoo.vespa.service.duper.DuperModelManager.controllerApplication; +import static com.yahoo.vespa.service.duper.DuperModelManager.proxyHostApplication; +import static org.junit.Assert.fail; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -27,12 +32,13 @@ public class DuperModelManagerTest { private final SuperModelProvider superModelProvider = mock(SuperModelProvider.class); private final SuperModel superModel = mock(SuperModel.class); private final DuperModel duperModel = mock(DuperModel.class); + private final InMemoryFlagSource flagSource = new InMemoryFlagSource(); private DuperModelManager manager; private SuperModelListener superModelListener; - private void makeManager() { - manager = new DuperModelManager(true, superModelProvider, duperModel); + private void makeManager(boolean isController) { + manager = new DuperModelManager(true, isController, superModelProvider, duperModel, flagSource); when(superModelProvider.getSuperModel()).thenReturn(superModel); verify(duperModel, times(0)).add(any()); @@ -44,7 +50,7 @@ public class DuperModelManagerTest { @Test public void testSuperModelAffectsDuperModel() { - makeManager(); + makeManager(false); verify(duperModel, times(0)).add(any()); superModelListener.applicationActivated(superModel, mock(ApplicationInfo.class)); @@ -57,9 +63,9 @@ public class DuperModelManagerTest { @Test public void testInfraApplicationsAffectsDuperModel() { - makeManager(); + makeManager(false); - ApplicationId id = manager.getProxyHostApplication().getApplicationId(); + ApplicationId id = proxyHostApplication.getApplicationId(); List<HostName> proxyHostHosts = Stream.of("proxyhost1", "proxyhost2").map(HostName::from).collect(Collectors.toList()); verify(duperModel, times(0)).add(any()); manager.infraApplicationActivated(id, proxyHostHosts); @@ -73,18 +79,18 @@ public class DuperModelManagerTest { @Test public void testEnabledConfigServerInfraApplications() { - makeManager(); + makeManager(false); testEnabledConfigServerLikeInfraApplication( - manager.getConfigServerApplication().getApplicationId(), - manager.getControllerApplication().getApplicationId()); + configServerApplication.getApplicationId(), + controllerApplication.getApplicationId()); } @Test public void testEnabledControllerInfraApplications() { - makeManager(); + makeManager(true); testEnabledConfigServerLikeInfraApplication( - manager.getControllerApplication().getApplicationId(), - manager.getConfigServerApplication().getApplicationId()); + controllerApplication.getApplicationId(), + configServerApplication.getApplicationId()); } private void testEnabledConfigServerLikeInfraApplication(ApplicationId firstId, ApplicationId secondId) { @@ -95,33 +101,24 @@ public class DuperModelManagerTest { // Adding the second config server like application will be ignored List<HostName> hostnames2 = Stream.of("node21", "node22").map(HostName::from).collect(Collectors.toList()); - manager.infraApplicationActivated(secondId, hostnames2); + assertThrows(IllegalArgumentException.class, () -> manager.infraApplicationActivated(secondId, hostnames2)); verify(duperModel, times(1)).add(any()); // Removing the second config server like application cannot be removed since it wasn't added verify(duperModel, times(0)).remove(any()); - manager.infraApplicationRemoved(secondId); + assertThrows(IllegalArgumentException.class, () -> manager.infraApplicationRemoved(secondId)); verify(duperModel, times(0)).remove(any()); - verify(duperModel, times(0)).remove(any()); manager.infraApplicationRemoved(firstId); verify(duperModel, times(1)).remove(any()); - when(duperModel.contains(firstId)).thenReturn(false); } - @Test - public void testSingleTenant() { - manager = new DuperModelManager(false, superModelProvider, duperModel); - - when(superModelProvider.getSuperModel()).thenReturn(superModel); - verify(duperModel, times(0)).add(any()); - - List<HostName> hostnames = Stream.of("node1", "node2").map(HostName::from).collect(Collectors.toList()); - manager.infraApplicationActivated(manager.getConfigServerApplication().getApplicationId(), hostnames); - verify(duperModel, times(0)).add(any()); - - verify(duperModel, times(0)).remove(any()); - manager.infraApplicationRemoved(manager.getConfigServerApplication().getApplicationId()); - verify(duperModel, times(0)).remove(any()); + private static void assertThrows(Class<? extends Throwable> clazz, Runnable runnable) { + try { + runnable.run(); + fail("Expected " + clazz); + } catch (Throwable e) { + if (!clazz.isInstance(e)) throw e; + } } }
\ No newline at end of file |