Retire on group size reduction

5 files changed, 52 insertions, 8 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
index 8c52f389daf..cdf3b9233d5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
@@ -53,8 +53,10 @@ class NodeAllocation {
     /** The requested nodes of this list */
     private final NodeSpec requested;
 
+    private final int extraNodesNeeded;
+
     /** The node candidates this has accepted so far, keyed on hostname */
-    private final Map<String, NodeCandidate> nodes = new LinkedHashMap<>();
+    public final Map<String, NodeCandidate> nodes = new LinkedHashMap<>();
 
     /** The number of already allocated nodes of compatible size */
     private int acceptedAndCompatible = 0;
@@ -86,11 +88,12 @@ class NodeAllocation {
     private final Optional<String> requiredHostFlavor;
 
     NodeAllocation(NodeList allNodes, ApplicationId application, ClusterSpec cluster, NodeSpec requested,
-                   Supplier<Integer> nextIndex, NodeRepository nodeRepository) {
+                   int extraNodesNeeded, Supplier<Integer> nextIndex, NodeRepository nodeRepository) {
         this.allNodes = allNodes;
         this.application = application;
         this.cluster = cluster;
         this.requested = requested;
+        this.extraNodesNeeded = extraNodesNeeded;
         this.nextIndex = nextIndex;
         this.nodeRepository = nodeRepository;
         this.requiredHostFlavor = Optional.of(PermanentFlags.HOST_FLAVOR.bindTo(nodeRepository.flagSource())
@@ -306,7 +309,7 @@ class NodeAllocation {
 
     /** Returns true if no more nodes are needed in this list */
     public boolean saturated() {
-        return requested.saturatedBy(acceptedAndCompatible);
+        return requested.saturatedBy(acceptedAndCompatible - extraNodesNeeded);
     }
 
     /** Returns true if the content of this list is sufficient to meet the request */
@@ -420,7 +423,7 @@ class NodeAllocation {
 
     /** Returns the number of nodes accepted this far */
     private int acceptedAndCompatibleOrResizable() {
-        if (nodeType() == NodeType.tenant) return acceptedAndCompatibleOrResizable;
+        if (nodeType() == NodeType.tenant) return acceptedAndCompatibleOrResizable - extraNodesNeeded; // xxx
         // Infrastructure nodes are always allocated by type. Count all nodes as accepted so that we never exceed
         // the wanted number of nodes for the type.
         return allNodes.nodeType(nodeType()).size();
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
index d4c4e86f0a3..d8565b81e41 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
@@ -55,6 +55,7 @@ public abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidat
     final boolean exclusiveSwitch;
 
     /** True if this node belongs to a group which will not be needed after this deployment */
+    // TODO: Always false
     final boolean isSurplus;
 
     /** This node does not exist in the node repository yet */
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 0206c3a4a26..f1c84be324e 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
@@ -188,7 +188,8 @@ public class Preparer {
     private NodeAllocation prepareAllocation(ApplicationId application, ClusterSpec cluster, NodeSpec requested,
                                              Supplier<Integer> nextIndex, LockedNodeList allNodes) {
         validateAccount(requested.cloudAccount(), application, allNodes);
-        NodeAllocation allocation = new NodeAllocation(allNodes, application, cluster, requested, nextIndex, nodeRepository);
+        int extraNodesNeeded = extraNodesNeeded(application, cluster, requested, allNodes);
+        NodeAllocation allocation = new NodeAllocation(allNodes, application, cluster, requested, extraNodesNeeded, nextIndex, nodeRepository);
         IP.Allocation.Context allocationContext = IP.Allocation.Context.from(nodeRepository.zone().cloud().name(),
                                                                              requested.cloudAccount().isExclave(nodeRepository.zone()),
                                                                              nodeRepository.nameResolver());
@@ -213,6 +214,17 @@ public class Preparer {
         return allocation;
     }
 
+    private int extraNodesNeeded(ApplicationId application, ClusterSpec cluster, NodeSpec requested, LockedNodeList allNodes) {
+        if ( ! cluster.isStateful()) return 0;
+        if ( ! requested.count().isPresent()) return 0;
+        int currentGroupSize = allNodes.owner(application).cluster(cluster.id()).state(Node.State.active).not().retired().group(0).size();
+        long currentGroupCount = allNodes.owner(application).cluster(cluster.id()).state(Node.State.active).not().retired().stream()
+                                         .map(node -> node.allocation().get().membership().cluster().group().get()).distinct().count();
+        int newGroupSize = requested.groupSize();
+        int surplusPerGroup = Math.max(0, currentGroupSize - newGroupSize);
+        return surplusPerGroup * (int) currentGroupCount;
+    }
+
     private void validateAccount(CloudAccount requestedAccount, ApplicationId application, LockedNodeList allNodes) {
         CloudAccount effectiveAccount = requestedAccount.isUnspecified() ? nodeRepository.zone().cloud().account() : requestedAccount;
         List<Node> nodesInOtherAccount = allNodes.owner(application).nodeType(NodeType.tenant).stream()
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
index abcef421b4c..78a34326949 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
@@ -553,6 +553,34 @@ public class DynamicProvisioningTest {
                        2, 1, resources);
     }
 
+    @Test
+    public void split_into_two_groups() {
+        List<Flavor> flavors = List.of(new Flavor("2x",  new NodeResources(2, 20, 200, 0.1, fast, local)));
+
+        ProvisioningTester tester = new ProvisioningTester.Builder().dynamicProvisioning(true, false)
+                                                                    .flavors(flavors)
+                                                                    .hostProvisioner(new MockHostProvisioner(flavors))
+                                                                    .nameResolver(nameResolver)
+                                                                    .build();
+
+        tester.activateTenantHosts();
+
+        ApplicationId app1 = applicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(content, new ClusterSpec.Id("cluster1")).vespaVersion("8").build();
+
+        System.out.println("Initial deployment ----------------------");
+        tester.activate(app1, cluster1, Capacity.from(resources(6, 1, 2, 20, 200, fast, StorageType.any)));
+        tester.assertNodes("Initial deployment: 1 group",
+                           6, 1, 2, 20, 200, fast, remote, app1, cluster1);
+
+        System.out.println("Split into 2 groups ---------------------");
+        tester.activate(app1, cluster1, Capacity.from(resources(6, 2, 2, 20, 200, fast, StorageType.any)));
+        tester.assertNodes("Change to 2 groups: Gets 6 active non-retired nodes",
+                           6, 2, 2, 20, 200, fast, remote, app1, cluster1);
+        List<Node> retired = tester.nodeRepository().nodes().list().owner(app1).cluster(cluster1.id()).state(Node.State.active).retired().asList();
+        assertEquals("... and in addition 3 retired nodes", 3, retired.size());
+    }
+
     private ProvisioningTester tester(boolean sharing) {
         var hostProvisioner = new MockHostProvisioner(new NodeFlavors(ProvisioningTester.createConfig()).getFlavors(), nameResolver, 0);
         return new ProvisioningTester.Builder().dynamicProvisioning(true, sharing).hostProvisioner(hostProvisioner).nameResolver(nameResolver).build();
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 5f2790e886a..b29ee4e03f1 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
@@ -288,13 +288,13 @@ public class ProvisioningTest {
         assertEquals("Superfluous container nodes are also dirtyed",
                      4-2 + 5-2 + 1 + 4-2, tester.nodeRepository().nodes().list(Node.State.dirty).size());
         assertEquals("Superfluous content nodes are retired",
-                     5-3 + 6-3 - 1, tester.getNodes(application1, Node.State.active).retired().size());
+                     5-3 + 6-3, tester.getNodes(application1, Node.State.active).retired().size());
 
         // increase content slightly
         SystemState state6 = prepare(application1, 2, 2, 4, 3, defaultResources, tester);
         tester.activate(application1, state6.allHosts);
         assertEquals("One content node is unretired",
-                     5-4 + 6-3 - 1, tester.getNodes(application1, Node.State.active).retired().size());
+                     5-4 + 6-3, tester.getNodes(application1, Node.State.active).retired().size());
 
         // Then reserve more
         SystemState state7 = prepare(application1, 8, 2, 2, 2, defaultResources, tester);
@@ -505,7 +505,7 @@ public class ProvisioningTest {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
                                                                     .flavors(List.of(hostFlavor))
                                                                     .build();
-        tester.makeReadyHosts(31, hostFlavor.resources()).activateTenantHosts();
+        tester.makeReadyHosts(32, hostFlavor.resources()).activateTenantHosts();
 
         ApplicationId app1 = ProvisioningTester.applicationId("app1");
         ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();