Merge pull request #13324 from vespa-engine/bratseth/remove-groups-immediately

Bratseth/remove groups immediately

10 files changed, 117 insertions, 93 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
index 6097688df72..1b2f73a2f5f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
@@ -161,4 +161,9 @@ public class NodeList extends AbstractFilteringList<Node, NodeList> {
         return new NodeList(nodes, false);
     }
 
+    @Override
+    public String toString() {
+        return asList().toString();
+    }
+
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
index 4452fc21c52..ad3dbc7eaa5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java
@@ -73,7 +73,7 @@ class Preparer {
             replace(acceptedNodes, accepted);
         }
         moveToActiveGroup(surplusNodes, wantedGroups, cluster.group());
-        replace(acceptedNodes, retire(surplusNodes));
+        acceptedNodes.removeAll(surplusNodes);
         return acceptedNodes;
     }
 
@@ -140,13 +140,4 @@ class Preparer {
         return highestIndex;
     }
 
-    /** Returns retired copies of the given nodes, unless they are removable */
-    private List<Node> retire(List<Node> nodes) {
-        List<Node> retired = new ArrayList<>(nodes.size());
-        for (Node node : nodes) {
-            if ( ! node.allocation().get().isRemovable())
-                retired.add(node.retire(Agent.application, nodeRepository.clock().instant()));
-        }
-        return retired;
-    }
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
index a60dee6bb0c..3fc60c1192d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
@@ -74,14 +74,14 @@ class PrioritizableNode implements Comparable<PrioritizableNode> {
         if (!this.isSurplusNode && other.isSurplusNode) return -1;
         if (!other.isSurplusNode && this.isSurplusNode) return 1;
 
-        // Choose inactive nodes
-        if (this.node.state() == Node.State.inactive && other.node.state() != Node.State.inactive) return -1;
-        if (other.node.state() == Node.State.inactive && this.node.state() != Node.State.inactive) return 1;
-
         // Choose reserved nodes from a previous allocation attempt (the exist in node repo)
         if (this.isInNodeRepoAndReserved() && ! other.isInNodeRepoAndReserved()) return -1;
         if (other.isInNodeRepoAndReserved() && ! this.isInNodeRepoAndReserved()) return 1;
 
+        // Choose inactive nodes
+        if (this.node.state() == Node.State.inactive && other.node.state() != Node.State.inactive) return -1;
+        if (other.node.state() == Node.State.inactive && this.node.state() != Node.State.inactive) return 1;
+
         // Choose ready nodes
         if (this.node.state() == Node.State.ready && other.node.state() != Node.State.ready) return -1;
         if (other.node.state() == Node.State.ready && this.node.state() != Node.State.ready) return 1;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java
index 781ef1f6114..08e4237fe00 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTest.java
@@ -248,7 +248,6 @@ public class AutoscalingTest {
         // deploy
         tester.deploy(application1, cluster1, 6, 2, resources);
         tester.addMeasurements(Resource.cpu,  0.25f, 1f, 120, application1);
-        System.out.println("Autoscaling ... ");
         tester.assertResources("Scaling up since resource usage is too high, changing to 1 group is cheaper",
                                8, 1, 2.7,  83.3, 83.3,
                                tester.autoscale(application1, cluster1.id(), min, max));
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
index e57bae09280..addbf717811 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirerTest.java
@@ -119,37 +119,6 @@ public class RetiredExpirerTest {
     }
 
     @Test
-    public void ensure_retired_groups_time_out() {
-        createReadyNodes(8, nodeResources, nodeRepository);
-        createHostNodes(4, nodeRepository, nodeFlavors);
-
-        ApplicationId applicationId = ApplicationId.from(TenantName.from("foo"), ApplicationName.from("bar"), InstanceName.from("fuz"));
-
-        ClusterSpec cluster = ClusterSpec.request(ClusterSpec.Type.content, ClusterSpec.Id.from("test")).vespaVersion("6.42").build();
-        activate(applicationId, cluster, 8, 8, provisioner);
-        activate(applicationId, cluster, 2, 2, provisioner);
-        assertEquals(8, nodeRepository.getNodes(applicationId, Node.State.active).size());
-        assertEquals(0, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
-
-        // Cause inactivation of retired nodes
-        clock.advance(Duration.ofHours(30)); // Retire period spent
-        MockDeployer deployer =
-            new MockDeployer(provisioner,
-                             clock,
-                             Collections.singletonMap(applicationId, new MockDeployer.ApplicationContext(applicationId,
-                                                                                                         cluster,
-                                                                                                         Capacity.from(new ClusterResources(2, 1, nodeResources)))));
-        createRetiredExpirer(deployer).run();
-        assertEquals(2, nodeRepository.getNodes(applicationId, Node.State.active).size());
-        assertEquals(6, nodeRepository.getNodes(applicationId, Node.State.inactive).size());
-        assertEquals(1, deployer.redeployments);
-
-        // inactivated nodes are not retired
-        for (Node node : nodeRepository.getNodes(applicationId, Node.State.inactive))
-            assertFalse(node.allocation().get().membership().retired());
-    }
-
-    @Test
     public void ensure_early_inactivation() throws OrchestrationException {
         createReadyNodes(7, nodeResources, nodeRepository);
         createHostNodes(4, nodeRepository, nodeFlavors);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
index 5654b4000c7..26039c29ae8 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
@@ -228,7 +228,7 @@ public class LoadBalancerProvisionerTest {
         }
         Set<HostSpec> allNodes = new LinkedHashSet<>();
         for (ClusterSpec spec : specs) {
-            allNodes.addAll(tester.prepare(application, spec, capacity, false));
+            allNodes.addAll(tester.prepare(application, spec, capacity));
         }
         return allNodes;
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
index 3b03e4e9d91..6e609d13d3b 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterResources;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.RegionName;
@@ -21,6 +22,7 @@ import java.time.Duration;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
@@ -121,6 +123,99 @@ public class MultigroupProvisioningTest {
         deploy(application1, Capacity.from(new ClusterResources(2, 2, large), true, true), tester);
     }
 
+    /**
+     * When increasing the number of groups without changing node count, we need to provison new nodes for
+     * the new groups since although we can remove nodes from existing groups without losing data we
+     * cannot do so without losing coverage.
+     */
+    @Test
+    public void test_split_to_groups() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                    .flavors(List.of(hostFlavor))
+                                                                    .build();
+        tester.makeReadyHosts(6, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        // Deploy with 1 group
+        tester.activate(app1, cluster1, Capacity.from(resources(4, 1, 10, 30,  10)));
+        assertEquals(4, tester.getNodes(app1, Node.State.active).size());
+        assertEquals(4, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals(0, tester.getNodes(app1, Node.State.active).group(0).retired().size());
+
+        // Split into 2 groups
+        tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 10, 30,  10)));
+        assertEquals(6, tester.getNodes(app1, Node.State.active).size());
+        assertEquals(4, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(0).retired().size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(1).size());
+        assertEquals(0, tester.getNodes(app1, Node.State.active).group(1).retired().size());
+    }
+
+    @Test
+    public void test_remove_group() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                    .flavors(List.of(hostFlavor))
+                                                                    .build();
+        tester.makeReadyHosts(6, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        // Deploy with 3 groups
+        tester.activate(app1, cluster1, Capacity.from(resources(6, 3, 10, 30,  10)));
+        assertEquals(6, tester.getNodes(app1, Node.State.active).size());
+        assertEquals(0, tester.getNodes(app1, Node.State.active).retired().size());
+        assertEquals(0, tester.getNodes(app1, Node.State.inactive).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(1).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(2).size());
+
+        // Remove a group
+        tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 10, 30,  10)));
+        assertEquals(4, tester.getNodes(app1, Node.State.active).size());
+        assertEquals(0, tester.getNodes(app1, Node.State.active).retired().size());
+        assertEquals(2, tester.getNodes(app1, Node.State.inactive).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals(2, tester.getNodes(app1, Node.State.active).group(1).size());
+        assertEquals(0, tester.getNodes(app1, Node.State.active).group(2).size());
+    }
+
+    @Test
+    public void test_layout_change_to_fewer_groups() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                    .flavors(List.of(hostFlavor))
+                                                                    .build();
+        tester.makeReadyHosts(12, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        // Deploy with 3 groups
+        tester.activate(app1, cluster1, Capacity.from(resources(12, 4, 10, 30,  10)));
+        assertEquals(12, tester.getNodes(app1, Node.State.active).size());
+        assertEquals( 0, tester.getNodes(app1, Node.State.active).retired().size());
+        assertEquals( 0, tester.getNodes(app1, Node.State.inactive).size());
+        assertEquals( 3, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals( 3, tester.getNodes(app1, Node.State.active).group(1).size());
+        assertEquals( 3, tester.getNodes(app1, Node.State.active).group(2).size());
+        assertEquals( 3, tester.getNodes(app1, Node.State.active).group(3).size());
+
+        // Remove a group
+        tester.activate(app1, cluster1, Capacity.from(resources(12, 3, 10, 30,  10)));
+        assertEquals(12, tester.getNodes(app1, Node.State.active).size());
+        assertEquals( 0, tester.getNodes(app1, Node.State.active).retired().size());
+        assertEquals( 0, tester.getNodes(app1, Node.State.inactive).size());
+        assertEquals( 4, tester.getNodes(app1, Node.State.active).group(0).size());
+        assertEquals( 4, tester.getNodes(app1, Node.State.active).group(1).size());
+        assertEquals( 4, tester.getNodes(app1, Node.State.active).group(2).size());
+        assertEquals( 0, tester.getNodes(app1, Node.State.active).group(3).size());
+    }
+
     @Test
     public void test_provisioning_of_multiple_groups_after_flavor_migration_and_exiration() {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
@@ -211,4 +306,8 @@ public class MultigroupProvisioningTest {
         return new HashSet<>(tester.prepare(application, cluster(), capacity));
     }
 
+    private ClusterResources resources(int nodes, int groups, double vcpu, double memory, double disk) {
+        return new ClusterResources(nodes, groups, new NodeResources(vcpu, memory, disk, 0.1));
+    }
+
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java
index 1d9c135b4b5..3865baa51c1 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java
@@ -29,8 +29,8 @@ public class PrioritizableNodeTest {
         List<PrioritizableNode> expected = List.of(
                 new PrioritizableNode(node("01", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.empty(), false, true, false, false),
                 new PrioritizableNode(node("02", Node.State.active), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false, false),
-                new PrioritizableNode(node("03", Node.State.inactive), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false, false),
                 new PrioritizableNode(node("04", Node.State.reserved), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false, false),
+                new PrioritizableNode(node("03", Node.State.inactive), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false, false),
                 new PrioritizableNode(node("05", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.active)), true, false, true, false),
                 new PrioritizableNode(node("06", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.ready)), true, false, true, false),
                 new PrioritizableNode(node("07", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.provisioned)), true, false, true, false),
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
index 44688c61b34..03c07515cd5 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
@@ -486,40 +486,6 @@ public class ProvisioningTest {
                            app1, cluster1);
     }
 
-    /**
-     * When increasing the number of groups without changing node count, we need to provison new nodes for
-     * the new groups since although we can remove nodes from existing groups without losing data we
-     * cannot do so without losing coverage.
-     */
-    @Test
-    public void test_change_group_layout() {
-        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
-        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
-                                                                    .flavors(List.of(hostFlavor))
-                                                                    .build();
-        tester.makeReadyHosts(6, hostFlavor.resources()).deployZoneApp();
-
-        ApplicationId app1 = tester.makeApplicationId("app1");
-        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
-
-        // Deploy with 1 group
-        System.out.println("--- Deploying 1 group");
-        tester.activate(app1, cluster1, Capacity.from(resources(4, 1, 10, 30,  10)));
-        assertEquals(4, tester.getNodes(app1, Node.State.active).size());
-        assertEquals(4, tester.getNodes(app1, Node.State.active).group(0).size());
-        assertEquals(0, tester.getNodes(app1, Node.State.active).group(0).retired().size());
-
-        // Split into 2 groups
-        System.out.println("--- Deploying 2 groups");
-        tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 10, 30,  10)));
-        assertEquals(6, tester.getNodes(app1, Node.State.active).size());
-        assertEquals(4, tester.getNodes(app1, Node.State.active).group(0).size());
-        assertEquals(2, tester.getNodes(app1, Node.State.active).group(0).retired().size());
-        assertEquals(2, tester.getNodes(app1, Node.State.active).group(1).size());
-        assertEquals(0, tester.getNodes(app1, Node.State.active).group(1).retired().size());
-    }
-
-
     @Test(expected = IllegalArgumentException.class)
     public void prod_deployment_requires_redundancy() {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
index c9d7e6bfa70..326af2fc60a 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
@@ -145,17 +145,12 @@ public class ProvisioningTester {
     }
 
     public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, Capacity capacity) {
-        return prepare(application, cluster, capacity, true);
-    }
-
-    public List<HostSpec> prepare(ApplicationId application, ClusterSpec cluster, Capacity capacity, boolean idempotentPrepare) {
         Set<String> reservedBefore = toHostNames(nodeRepository.getNodes(application, Node.State.reserved));
         Set<String> inactiveBefore = toHostNames(nodeRepository.getNodes(application, Node.State.inactive));
         List<HostSpec> hosts1 = provisioner.prepare(application, cluster, capacity, provisionLogger);
-        if (idempotentPrepare) { // prepare twice to ensure idempotence
-            List<HostSpec> hosts2 = provisioner.prepare(application, cluster, capacity, provisionLogger);
-            assertEquals(hosts1, hosts2);
-        }
+        // prepare twice to ensure idempotence
+        List<HostSpec> hosts2 = provisioner.prepare(application, cluster, capacity, provisionLogger);
+        assertEquals(hosts1, hosts2);
         Set<String> newlyActivated = toHostNames(nodeRepository.getNodes(application, Node.State.reserved));
         newlyActivated.removeAll(reservedBefore);
         newlyActivated.removeAll(inactiveBefore);
@@ -163,7 +158,7 @@ public class ProvisioningTester {
     }
 
     public Collection<HostSpec> activate(ApplicationId application, ClusterSpec cluster, Capacity capacity) {
-        List<HostSpec> preparedNodes = prepare(application, cluster, capacity, true);
+        List<HostSpec> preparedNodes = prepare(application, cluster, capacity);
 
         // Add ip addresses and activate parent host if necessary
         for (HostSpec prepared : preparedNodes) {
@@ -197,7 +192,7 @@ public class ProvisioningTester {
     public void prepareAndActivateInfraApplication(ApplicationId application, NodeType nodeType, Version version) {
         ClusterSpec cluster = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from(nodeType.toString())).vespaVersion(version).build();
         Capacity capacity = Capacity.fromRequiredNodeType(nodeType);
-        List<HostSpec> hostSpecs = prepare(application, cluster, capacity, true);
+        List<HostSpec> hostSpecs = prepare(application, cluster, capacity);
         activate(application, hostSpecs);
     }
 
@@ -256,7 +251,7 @@ public class ProvisioningTester {
                             double vcpu, double memory, double disk, double bandwidth,
                             DiskSpeed diskSpeed, StorageType storageType,
                             ApplicationId app, ClusterSpec cluster) {
-        List<Node> nodeList = nodeRepository.list().owner(app).cluster(cluster.id()).not().retired().asList();
+        List<Node> nodeList = nodeRepository.list().owner(app).cluster(cluster.id()).state(Node.State.active).not().retired().asList();
         assertEquals(explanation + ": Node count",
                      nodes,
                      nodeList.size());