Merge pull request #5169 from vespa-engine/hakonhall/avoid-slobrok-in-node-admin-cluster

Avoid Slobrok in node-admin cluster

3 files changed, 122 insertions, 32 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java
index cc9957144f3..44de53991f4 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/DomAdminV4Builder.java
@@ -2,22 +2,23 @@
 package com.yahoo.vespa.model.builder.xml.dom;
 
 import com.yahoo.component.Version;
+import com.yahoo.config.model.ConfigModelContext;
 import com.yahoo.config.model.api.ConfigServerSpec;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.vespa.model.HostResource;
 import com.yahoo.vespa.model.HostSystem;
-import com.yahoo.vespa.model.admin.*;
-import com.yahoo.config.model.ConfigModelContext;
 import com.yahoo.vespa.model.admin.Admin;
+import com.yahoo.vespa.model.admin.Logserver;
+import com.yahoo.vespa.model.admin.Slobrok;
 import com.yahoo.vespa.model.container.Container;
 import com.yahoo.vespa.model.container.ContainerModel;
-
 import org.w3c.dom.Element;
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.stream.Collectors;
 
@@ -27,6 +28,7 @@ import java.util.stream.Collectors;
  * @author bratseth
  */
 public class DomAdminV4Builder extends DomAdminBuilderBase {
+    private ApplicationId ZONE_APPLICATION_ID = ApplicationId.from("hosted-vespa", "routing", "default");
 
     private final Collection<ContainerModel> containerModels;
     private final ConfigModelContext context;
@@ -63,7 +65,7 @@ public class DomAdminV4Builder extends DomAdminBuilderBase {
             createSlobroks(admin, allocateHosts(admin.getHostSystem(), "slobroks", nodesSpecification));
         }
         else {
-            createSlobroks(admin, pickContainerHosts(nodesSpecification.count(), 2));
+            createSlobroks(admin, pickContainerHostsForSlobrok(nodesSpecification.count(), 2));
         }
     }
 
@@ -94,17 +96,36 @@ public class DomAdminV4Builder extends DomAdminBuilderBase {
      *        on topology changes, and less nodes may be returned if fewer are available
      * @param minHostsPerContainerCluster the desired number of hosts per cluster
      */
-    private List<HostResource> pickContainerHosts(int count, int minHostsPerContainerCluster) {
+    private List<HostResource> pickContainerHostsForSlobrok(int count, int minHostsPerContainerCluster) {
+        Collection<ContainerModel> containerModelsWithSlobrok = containerModels.stream()
+                .filter(this::shouldHaveSlobrok)
+                .collect(Collectors.toList());
+        int hostsPerCluster = (int) Math.max(
+                minHostsPerContainerCluster,
+                Math.ceil((double) count / containerModelsWithSlobrok.size()));
+
         // Pick from all container clusters to make sure we don't lose all nodes at once if some clusters are removed.
         // This will overshoot the desired size (due to ceil and picking at least one node per cluster).
         List<HostResource> picked = new ArrayList<>();
-        for (ContainerModel containerModel : containerModels)
-            picked.addAll(pickContainerHostsFrom(containerModel,
-                                                 (int) Math.max(minHostsPerContainerCluster,
-                                                                Math.ceil((double) count / containerModels.size()))));
+        for (ContainerModel containerModel : containerModelsWithSlobrok)
+            picked.addAll(pickContainerHostsFrom(containerModel, hostsPerCluster));
         return picked;
     }
 
+    private boolean shouldHaveSlobrok(ContainerModel containerModel) {
+        // Avoid Slobroks on node-admin container cluster, as node-admin is migrating
+        // TODO: Remove this hack once node-admin has migrated out the zone app
+
+        ApplicationId applicationId = context.getDeployState().getProperties().applicationId();
+        if (!applicationId.equals(ZONE_APPLICATION_ID)) {
+            return true;
+        }
+
+        // aka clustername, aka application-model's ClusterId
+        String clustername = containerModel.getCluster().getName();
+        return !Objects.equals(clustername, "node-admin");
+    }
+
     private List<HostResource> pickContainerHostsFrom(ContainerModel model, int count) {
         boolean retired = true;
         List<HostResource> picked = sortedContainerHostsFrom(model, count, !retired);
diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
index 91be617d0ea..d4ce445d50c 100644
--- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
+++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
@@ -1,46 +1,57 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.config.model.provision;
 
-import static com.yahoo.config.model.test.TestUtil.joinLines;
-import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.*;
-
-import java.io.StringReader;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
-import java.util.stream.Collectors;
-
 import com.yahoo.config.application.api.ApplicationPackage;
+import com.yahoo.config.model.api.HostInfo;
 import com.yahoo.config.model.deploy.DeployProperties;
+import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.provision.ClusterMembership;
 import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provisioning.FlavorsConfig;
 import com.yahoo.search.config.QrStartConfig;
+import com.yahoo.searchdefinition.parser.ParseException;
 import com.yahoo.vespa.config.search.core.ProtonConfig;
-import static com.yahoo.vespa.defaults.Defaults.getDefaults;
+import com.yahoo.vespa.model.AbstractService;
 import com.yahoo.vespa.model.HostResource;
 import com.yahoo.vespa.model.HostSystem;
+import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.admin.Admin;
 import com.yahoo.vespa.model.admin.Slobrok;
 import com.yahoo.vespa.model.container.Container;
 import com.yahoo.vespa.model.container.ContainerCluster;
 import com.yahoo.vespa.model.content.ContentSearchCluster;
+import com.yahoo.vespa.model.content.StorageNode;
+import com.yahoo.vespa.model.content.cluster.ContentCluster;
 import com.yahoo.vespa.model.search.Dispatch;
 import com.yahoo.vespa.model.search.SearchNode;
 import com.yahoo.vespa.model.test.VespaModelTester;
+import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
+import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithMockPkg;
 import org.junit.Ignore;
 import org.junit.Test;
 
-import com.yahoo.config.model.deploy.DeployState;
-import com.yahoo.searchdefinition.parser.ParseException;
-import com.yahoo.vespa.model.VespaModel;
-import com.yahoo.vespa.model.content.StorageNode;
-import com.yahoo.vespa.model.content.cluster.ContentCluster;
-import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
-import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithMockPkg;
+import java.io.StringReader;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static com.yahoo.config.model.test.TestUtil.joinLines;
+import static com.yahoo.vespa.defaults.Defaults.getDefaults;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.collection.IsIn.isIn;
+import static org.hamcrest.core.Every.everyItem;
+import static org.hamcrest.core.IsNot.not;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * Test cases for provisioning nodes to entire vespamodels
@@ -736,6 +747,52 @@ public class ModelProvisioningTest {
     }
 
     @Test
+    public void testSlobroksAreSpreadOverAllContainerClustersExceptNodeAdmin() {
+        String services =
+                "<?xml version='1.0' encoding='utf-8' ?>\n" +
+                        "<services>" +
+                        "  <admin version='4.0'/>" +
+                        "  <container version='1.0' id='routing'>" +
+                        "     <nodes count='10'/>" +
+                        "  </container>" +
+                        "  <container version='1.0' id='node-admin'>" +
+                        "     <nodes count='3'/>" +
+                        "  </container>" +
+                        "</services>";
+
+        int numberOfHosts = 13;
+        VespaModelTester tester = new VespaModelTester();
+        tester.addHosts(numberOfHosts);
+        tester.setApplicationId("hosted-vespa", "routing", "default");
+        VespaModel model = tester.createModel(services, true);
+        assertThat(model.getRoot().getHostSystem().getHosts().size(), is(numberOfHosts));
+
+        Set<String> routingHosts = getClusterHostnames(model, "routing");
+        assertEquals(10, routingHosts.size());
+
+        Set<String> nodeAdminHosts = getClusterHostnames(model, "node-admin");
+        assertEquals(3, nodeAdminHosts.size());
+
+        Set<String> slobrokHosts = model.getAdmin().getSlobroks().stream()
+                .map(AbstractService::getHostName)
+                .collect(Collectors.toSet());
+        assertEquals(3, slobrokHosts.size());
+
+        assertThat(slobrokHosts, everyItem(isIn(routingHosts)));
+        assertThat(slobrokHosts, everyItem(not(isIn(nodeAdminHosts))));
+    }
+
+    private Set<String> getClusterHostnames(VespaModel model, String clusterId) {
+        return model.getHosts().stream()
+                .filter(host -> host.getServices().stream()
+                        .anyMatch(serviceInfo -> Objects.equals(
+                                serviceInfo.getProperty("clustername"),
+                                Optional.of(clusterId))))
+                .map(HostInfo::getHostname)
+                .collect(Collectors.toSet());
+    }
+
+    @Test
     public void test2ContentNodesProduces1ClusterController() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
index 37ddf458121..715b84c7093 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
@@ -12,6 +12,7 @@ import com.yahoo.config.model.provision.Host;
 import com.yahoo.config.model.provision.Hosts;
 import com.yahoo.config.model.provision.InMemoryProvisioner;
 import com.yahoo.config.model.provision.SingleNodeProvisioner;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Flavor;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
@@ -43,6 +44,7 @@ public class VespaModelTester {
 
     private boolean hosted = true;
     private Map<String, Collection<Host>> hostsByFlavor = new HashMap<>();
+    private ApplicationId applicationId = ApplicationId.defaultId();
 
     public VespaModelTester() {
         this(new NullConfigModelRegistry());
@@ -84,6 +86,11 @@ public class VespaModelTester {
     /** Sets whether this sets up a model for a hosted system. Default: true */
     public void setHosted(boolean hosted) { this.hosted = hosted; }
 
+    /** Sets the tenant, application name, and instance name of the model being built. */
+    public void setApplicationId(String tenant, String applicationName, String instanceName) {
+        applicationId = ApplicationId.from(tenant, applicationName, instanceName);
+    }
+
     /** Creates a model which uses 0 as start index and fails on out of capacity */
     public VespaModel createModel(String services, String ... retiredHostNames) {
         return createModel(services, true, retiredHostNames);
@@ -108,11 +115,16 @@ public class VespaModelTester {
                                       new InMemoryProvisioner(hostsByFlavor, failOnOutOfCapacity, startIndexForClusters, retiredHostNames) :
                                       new SingleNodeProvisioner();
 
+        DeployProperties properties = new DeployProperties.Builder()
+                .hostedVespa(hosted)
+                .applicationId(applicationId)
+                .build();
+
         DeployState deployState = new DeployState.Builder()
                 .applicationPackage(appPkg)
                 .modelHostProvisioner(provisioner)
-                .properties((new DeployProperties.Builder()).hostedVespa(hosted).build()).build(true);
+                .properties(properties)
+                .build(true);
         return modelCreatorWithMockPkg.create(false, deployState, configModelRegistry);
     }
-
 }