Revert "Merge pull request #17653 from vespa-engine/bratseth/slobroks-on-clustercontrollers-only"

This reverts commit 237c30c205c0748e8e9b3580c29a76cc88ca201a, reversing
changes made to ae01f049bf23e67269fe1f1f222fe8eaf2bdee62.

9 files changed, 30 insertions, 91 deletions

diff --git a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
index 2784c111019..602e0c80d4b 100644
--- a/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
+++ b/config-model/src/main/java/com/yahoo/config/model/producer/AbstractConfigProducer.java
@@ -43,7 +43,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce
     private final String subId;
     private String configId = null;
 
-    private final List<Service> descendantServices = new ArrayList<>();
+    private List<Service> descendantServices = new ArrayList<>();
 
     private AbstractConfigProducer parent = null;
 
@@ -59,8 +59,8 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce
      * Creates a new AbstractConfigProducer with the given parent and subId.
      * This constructor will add the resulting producer to the children of parent.
      *
-     * @param parent the parent of this ConfigProducer
-     * @param subId  the fragment of the config id for the producer
+     * @param parent  The parent of this ConfigProducer
+     * @param subId   The fragment of the config id for the producer
      */
     public AbstractConfigProducer(AbstractConfigProducer parent, String subId) {
         this(subId);
@@ -69,13 +69,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce
         }
     }
 
-    /** Removes this from the config model */
-    protected void remove() {
-        if (parent != null)
-            parent.removeChild(this);
-    }
-
-    protected final void setParent(AbstractConfigProducer<?> parent) {
+    protected final void setParent(AbstractConfigProducer parent) {
         this.parent = parent;
         computeConfigId();
     }
@@ -98,7 +92,7 @@ public abstract class AbstractConfigProducer<CHILD extends AbstractConfigProduce
     /**
      * Adds a child to this config producer.
      *
-     * @param child the child config producer to add
+     * @param child  The child config producer to add.
      */
     protected void addChild(CHILD child) {
         if (child == null) {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
index 6d4da3d995c..6e16b1a120e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/AbstractService.java
@@ -92,11 +92,10 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon
      * Preferred constructor when building from XML. Use this if you are building
      * in doBuild() in an AbstractConfigProducerBuilder.
      * build() will call initService() in that case, after setting hostalias and baseport.
-     *
-     * @param parent the parent config producer in the model tree
-     * @param name   the name of this service
+     * @param parent Parent config producer in the model tree.
+     * @param name   Name of this service.
      */
-    public AbstractService(AbstractConfigProducer<?> parent, String name) {
+    public AbstractService(AbstractConfigProducer parent, String name) {
         super(parent, name);
     }
 
@@ -108,13 +107,6 @@ public abstract class AbstractService extends AbstractConfigProducer<AbstractCon
         super(name);
     }
 
-    @Override
-    protected void remove() {
-        super.remove();
-        if (hostResource != null)
-            hostResource.deallocateService(this);
-    }
-
     /**
      * Distribute affinity on a collection of services. Services that are located on the same host
      * will be assigned a specific cpu socket on that host.
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java b/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java
index 38b668f121e..a80982fe75b 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/HostPorts.java
@@ -6,12 +6,9 @@ import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.provision.NetworkPorts;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
-import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
-import java.util.ListIterator;
 import java.util.Map;
 import java.util.Optional;
 import java.util.logging.Level;
@@ -146,13 +143,6 @@ public class HostPorts {
         return allocator.result();
     }
 
-    void deallocatePorts(NetworkPortRequestor service) {
-        if (networkPortsList.isPresent())
-            throw new IllegalStateException("Cannot deallocate ports after calling flushPortReservations()");
-        portDB.entrySet().removeIf(entry -> entry.getValue().getServiceName().equals(service.getServiceName()));
-        allocatedPorts--;
-    }
-
     public void flushPortReservations() {
         this.networkPortsList = Optional.of(new NetworkPorts(portFinder.allocations()));
     }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
index ef041d06978..3bc07db9507 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
@@ -79,11 +79,6 @@ public class HostResource implements Comparable<HostResource> {
         return ports;
     }
 
-    void deallocateService(AbstractService service) {
-        hostPorts.deallocatePorts(service);
-        services.remove(service.getServiceName());
-    }
-
     /**
      * Returns the service with the given "sentinel name" on this Host,
      * or null if the name does not match any service.
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java
index e080ce43730..d1b9bbb4e58 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Admin.java
@@ -148,23 +148,15 @@ public class Admin extends AbstractConfigProducer<Admin> implements Serializable
 
     public void setClusterControllers(ClusterControllerContainerCluster clusterControllers, DeployLogger deployLogger) {
         this.clusterControllers = clusterControllers;
-        if (isHostedVespa) {
-            // Prefer to put Slobroks on the admin cluster running cluster controllers to avoid unnecessary
-            // movement of the slobroks when there are changes to the content cluster nodes
-            removeSlobroks();
+        if (isHostedVespa)
             addSlobroks(createSlobroksOn(clusterControllers, deployLogger));
-        }
-    }
-
-    private void removeSlobroks() {
-        slobroks.forEach(Slobrok::remove);
-        slobroks.clear();
     }
 
     private List<Slobrok> createSlobroksOn(ClusterControllerContainerCluster clusterControllers, DeployLogger deployLogger) {
         List<Slobrok> slobroks = new ArrayList<>();
+        int index = this.slobroks.size();
         for (ClusterControllerContainer clusterController : clusterControllers.getContainers()) {
-            Slobrok slobrok = new Slobrok(this, clusterController.index());
+            Slobrok slobrok = new Slobrok(this, index++);
             slobrok.setHostResource(clusterController.getHostResource());
             slobroks.add(slobrok);
             slobrok.initService(deployLogger);
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java
index 00b6d872ddd..eed886b707f 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/Slobrok.java
@@ -9,10 +9,9 @@ import com.yahoo.vespa.model.PortAllocBridge;
 /**
  * Represents a Slobrok service.
  *
- * @author gjoranv
+ * @author  gjoranv
  */
 public class Slobrok extends AbstractService implements StateserverConfig.Producer {
-
     private static final long serialVersionUID = 1L;
 
     public final static int BASEPORT = 19099;
@@ -26,7 +25,7 @@ public class Slobrok extends AbstractService implements StateserverConfig.Produc
      * @param parent the parent ConfigProducer.
      * @param index  unique index for all slobroks
      */
-    public Slobrok(AbstractConfigProducer<?> parent, int index) {
+    public Slobrok(AbstractConfigProducer parent, int index) {
         super(parent, "slobrok." + index);
         portsMeta.on(0).tag("rpc").tag("admin").tag("status");
         portsMeta.on(1).tag("http").tag("state");
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 e27faa13000..a0673824907 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
@@ -67,7 +67,7 @@ public class DomAdminV4Builder extends DomAdminBuilderBase {
                            admin,
                            allocateHosts(admin.hostSystem(), "slobroks", nodesSpecification));
         }
-        else { // These will be removed later, if an admin cluster (for cluster controllers) is assigned
+        else { // TODO: Remove
             createSlobroks(deployLogger,
                            admin,
                            pickContainerHostsForSlobrok(nodesSpecification.minResources().nodes(), 2));
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 79e3e869b52..326bf577acc 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
@@ -494,8 +494,9 @@ public class ModelProvisioningTest {
         Set<HostResource> clusterControllerHosts = admin.getClusterControllers().getContainers()
                                                         .stream().map(cc -> cc.getHostResource()).collect(Collectors.toSet());
         Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet());
-        assertEquals(3, slobrokHosts.size());
-        assertTrue("Slobroks are assigned on cluster controller nodes", clusterControllerHosts.containsAll(slobrokHosts));
+        assertEquals(6, slobrokHosts.size());
+        assertTrue("Slobroks are assigned from container and cluster controller nodes",
+                   union(containerHosts, clusterControllerHosts).containsAll(slobrokHosts));
         assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost()));
         assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size());
         assertEquals("Dedicated admin cluster controllers when hosted", 3, admin.getClusterControllers().getContainers().size());
@@ -559,40 +560,6 @@ public class ModelProvisioningTest {
     }
 
     @Test
-    public void testSlobroksOnContainersIfNoContentClusters() {
-        String services =
-                "<?xml version='1.0' encoding='utf-8' ?>\n" +
-                "<services>" +
-                "  <admin version='4.0'/>" +
-                "  <container version='1.0' id='foo'>" +
-                "     <nodes count='10'/>" +
-                "  </container>" +
-                "</services>";
-
-        int numberOfHosts = 10;
-        VespaModelTester tester = new VespaModelTester();
-        tester.addHosts(numberOfHosts);
-        VespaModel model = tester.createModel(services, true);
-        assertEquals(numberOfHosts, model.getRoot().hostSystem().getHosts().size());
-
-        // Check container cluster
-        assertEquals(1, model.getContainerClusters().size());
-        Set<HostResource> containerHosts = model.getContainerClusters().get("foo").getContainers().stream()
-                                                .map(Container::getHost)
-                                                .collect(Collectors.toSet());
-        assertEquals(10, containerHosts.size());
-
-        // Check admin clusters
-        Admin admin = model.getAdmin();
-        Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet());
-        assertEquals(3, slobrokHosts.size());
-        assertTrue("Slobroks are assigned from container nodes",
-                   containerHosts.containsAll(slobrokHosts));
-        assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost()));
-        assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size());
-    }
-
-    @Test
     public void testUsingNodesAndGroupCountAttributesWithoutDedicatedClusterControllers() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
@@ -635,8 +602,9 @@ public class ModelProvisioningTest {
         Set<HostResource> clusterControllerHosts = admin.getClusterControllers().getContainers()
                                                         .stream().map(cc -> cc.getHostResource()).collect(Collectors.toSet());
         Set<HostResource> slobrokHosts = admin.getSlobroks().stream().map(Slobrok::getHost).collect(Collectors.toSet());
-        assertEquals(3, slobrokHosts.size());
-        assertTrue("Slobroks are assigned on cluster controller nodes", clusterControllerHosts.containsAll(slobrokHosts));
+        assertEquals(6, slobrokHosts.size());
+        assertTrue("Slobroks are assigned from container and cluster controller nodes",
+                   union(containerHosts, clusterControllerHosts).containsAll(slobrokHosts));
         assertTrue("Logserver is assigned from container nodes", containerHosts.contains(admin.getLogserver().getHost()));
         assertEquals("No in-cluster config servers in a hosted environment", 0, admin.getConfigservers().size());
         assertEquals(3, admin.getClusterControllers().getContainers().size());
@@ -1645,7 +1613,7 @@ public class ModelProvisioningTest {
         tester.addHosts(6);
         VespaModel model = tester.createModel(services, true);
         assertEquals(6, model.getRoot().hostSystem().getHosts().size());
-        assertEquals(3, model.getAdmin().getSlobroks().size());
+        assertEquals(5, model.getAdmin().getSlobroks().size());
         assertEquals(2, model.getContainerClusters().get("foo").getContainers().size());
         assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes(true));
     }
@@ -2170,4 +2138,11 @@ public class ModelProvisioningTest {
         assertProvisioned(nodeCount, id, null, type, model);
     }
 
+    private Set<HostResource> union(Set<HostResource> a, Set<HostResource> b) {
+        Set<HostResource> union = new HashSet<>();
+        union.addAll(a);
+        union.addAll(b);
+        return union;
+    }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
index a79d705963b..6bda6dc8a25 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
@@ -57,9 +57,11 @@ public class DeploymentMetricsMaintainer extends ControllerMaintainer {
                     for (Deployment deployment : instance.deployments().values()) {
                         attempts.incrementAndGet();
                         try {
+                            if (deployment.version().getMajor() < 7) continue;
                             DeploymentId deploymentId = new DeploymentId(instance.id(), deployment.zone());
                             List<ClusterMetrics> clusterMetrics = controller().serviceRegistry().configServer().getDeploymentMetrics(deploymentId);
                             Instant now = controller().clock().instant();
+
                             applications.lockApplicationIfPresent(application.id(), locked -> {
                                 Deployment existingDeployment = locked.get().require(instance.name()).deployments().get(deployment.zone());
                                 if (existingDeployment == null) return; // Deployment removed since we started collecting metrics