Don't move nodes between groups

Moving nodes between groups will not lose data but will cause
immediate reduced coverage until data is migrated.
To avoid this, this PR keeps surplus nodes in their
assigned group and provisions new replacement nodes.

8 files changed, 19 insertions, 25 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
index e69a67d425c..a1d8ffb03d3 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
@@ -118,8 +118,9 @@ public class GroupPreparer {
                 // Carry out and return allocation
                 nodeRepository.reserve(allocation.reservableNodes());
                 nodeRepository.addDockerNodes(new LockedNodeList(allocation.newNodes(), allocationLock));
-                surplusActiveNodes.removeAll(allocation.surplusNodes());
-                return allocation.finalNodes(surplusActiveNodes);
+                List<Node> acceptedNodes = allocation.finalNodes();
+                surplusActiveNodes.removeAll(acceptedNodes);
+                return acceptedNodes;
             }
         }
     }
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 83c68c91fc5..47d1b30a8e7 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
@@ -316,10 +316,9 @@ class NodeAllocation {
      * Prefer to retire nodes of the wrong flavor.
      * Make as few changes to the retired set as possible.
      *
-     * @param surplusNodes this will add nodes not any longer needed by this group to this list
      * @return the final list of nodes
      */
-    List<Node> finalNodes(List<Node> surplusNodes) {
+    List<Node> finalNodes() {
         int currentRetiredCount = (int) nodes.stream().filter(node -> node.node.allocation().get().membership().retired()).count();
         int deltaRetiredCount = requestedNodes.idealRetiredCount(nodes.size(), currentRetiredCount) - currentRetiredCount;
 
@@ -327,7 +326,6 @@ class NodeAllocation {
             for (PrioritizableNode node : byDecreasingIndex(nodes)) {
                 if ( ! node.node.allocation().get().membership().retired() && node.node.state() == Node.State.active) {
                     node.node = node.node.retire(Agent.application, nodeRepository.clock().instant());
-                    surplusNodes.add(node.node); // offer this node to other groups
                     if (--deltaRetiredCount == 0) break;
                 }
             }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
index d64188976ff..8a15c058ff4 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
@@ -198,9 +198,8 @@ public class NodePrioritizer {
      * parameters to the priority sorting procedure.
      */
     private PrioritizableNode toPrioritizable(Node node, boolean isSurplusNode, boolean isNewNode) {
-        PrioritizableNode.Builder builder = new PrioritizableNode.Builder(node)
-                .surplusNode(isSurplusNode)
-                .newNode(isNewNode);
+        PrioritizableNode.Builder builder = new PrioritizableNode.Builder(node).surplusNode(isSurplusNode)
+                                                                               .newNode(isNewNode);
 
         allNodes.parentOf(node).ifPresent(parent -> {
             NodeResources parentCapacity = capacity.freeCapacityOf(parent, false);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
index 913357b16ca..bd92357ea79 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
@@ -74,7 +74,6 @@ public class NodeRepositoryProvisioner implements Provisioner {
         this.preparer = new Preparer(nodeRepository,
                                      zone.environment() == Environment.prod ? SPARE_CAPACITY_PROD : SPARE_CAPACITY_NONPROD,
                                      provisionServiceProvider.getHostProvisioner(),
-                                     provisionServiceProvider.getHostResourcesCalculator(),
                                      flagSource,
                                      loadBalancerProvisioner);
         this.activator = new Activator(nodeRepository, loadBalancerProvisioner);
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 163cdc57d9a..4452fc21c52 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
@@ -29,7 +29,7 @@ class Preparer {
     private final int spareCount;
 
     public Preparer(NodeRepository nodeRepository, int spareCount, Optional<HostProvisioner> hostProvisioner,
-                    HostResourcesCalculator hostResourcesCalculator, FlagSource flagSource,
+                    FlagSource flagSource,
                     Optional<LoadBalancerProvisioner> loadBalancerProvisioner) {
         this.nodeRepository = nodeRepository;
         this.spareCount = spareCount;
@@ -46,7 +46,7 @@ class Preparer {
         }
         catch (OutOfCapacityException e) {
             throw new OutOfCapacityException("Could not satisfy " + requestedNodes +
-                                             ( wantedGroups > 1 ? " ( in " + wantedGroups + " groups)" : "") +
+                                             ( wantedGroups > 1 ? " (in " + wantedGroups + " groups)" : "") +
                                              " in " + application + " " + cluster +
                                              ": " + e.getMessage());
         }
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 8cfcbcb3797..a60dee6bb0c 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
@@ -33,7 +33,7 @@ class PrioritizableNode implements Comparable<PrioritizableNode> {
     /** True if the node is allocated to a host that should be dedicated as a spare */
     final boolean violatesSpares;
 
-    /** True if this is a node that has been retired earlier in the allocation process */
+    /** True if this node belongs to a group which will not be needed after this deployment */
     final boolean isSurplusNode;
 
     /** This node does not exist in the node repository yet */
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 050f3b7e865..3b03e4e9d91 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
@@ -40,9 +40,9 @@ public class MultigroupProvisioningTest {
     public void test_provisioning_of_multiple_groups() {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
 
-        ApplicationId application1 = tester.makeApplicationId();
+        ApplicationId application1 = tester.makeApplicationId("app1");
 
-        tester.makeReadyNodes(21, small);
+        tester.makeReadyNodes(31, small);
 
         deploy(application1, 6, 1, small, tester);
         deploy(application1, 6, 2, small, tester);
@@ -86,10 +86,10 @@ public class MultigroupProvisioningTest {
     public void test_provisioning_of_multiple_groups_after_flavor_migration() {
         ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east"))).build();
 
-        ApplicationId application1 = tester.makeApplicationId();
+        ApplicationId application1 = tester.makeApplicationId("app1");
 
         tester.makeReadyNodes(10, small);
-        tester.makeReadyNodes(10, large);
+        tester.makeReadyNodes(16, large);
 
         deploy(application1, 8, 1, small, tester);
         deploy(application1, 8, 1, large, tester);
@@ -125,10 +125,10 @@ public class MultigroupProvisioningTest {
     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();
 
-        ApplicationId application1 = tester.makeApplicationId();
+        ApplicationId application1 = tester.makeApplicationId("app1");
 
         tester.makeReadyNodes(10, small);
-        tester.makeReadyNodes(10, large);
+        tester.makeReadyNodes(16, large);
 
         deploy(application1, 8, 1, small, tester);
         deploy(application1, 8, 1, large, tester);
@@ -164,13 +164,8 @@ public class MultigroupProvisioningTest {
         int nodeCount = capacity.minResources().nodes();
         NodeResources nodeResources = capacity.minResources().nodeResources();
 
-        int previousActiveNodeCount = tester.getNodes(application, Node.State.active).resources(nodeResources).size();
-
         tester.activate(application, prepare(application, capacity, tester));
-        assertEquals("Superfluous nodes are retired, but no others - went from " + previousActiveNodeCount + " to " + nodeCount + " nodes",
-                     Math.max(0, previousActiveNodeCount - capacity.minResources().nodes()),
-                     tester.getNodes(application, Node.State.active).retired().resources(nodeResources).size());
-        assertEquals("Other flavors are retired",
+        assertEquals("Nodes of wrong size are retired",
                      0, tester.getNodes(application, Node.State.active).not().retired().not().resources(nodeResources).size());
 
         // Check invariants for all nodes
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 060aeb80d52..44688c61b34 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
@@ -497,18 +497,20 @@ 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()).deployZoneApp();
+        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());