Only allow activate/remove legal infra applications in duper model

2 files changed, 54 insertions, 102 deletions

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