Consider all group sizes

5 files changed, 67 insertions, 62 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocatableClusterResources.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocatableClusterResources.java
index def3b18d14c..1637d99a07a 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocatableClusterResources.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocatableClusterResources.java
@@ -118,13 +118,16 @@ public class AllocatableClusterResources {
     }
 
     public boolean preferableTo(AllocatableClusterResources other) {
-        if (this.fulfilment > other.fulfilment) return true; // we always want to fulfil as much as possible
+        if (this.fulfilment < 1 || other.fulfilment < 1)
+            return this.fulfilment > other.fulfilment;  // we always want to fulfil as much as possible
         return this.cost() < other.cost(); // otherwise, prefer lower cost
     }
 
     @Override
     public String toString() {
-        return nodes + " nodes with " + realResources() +
+        return nodes + " nodes " +
+               ( groups > 1 ? "(in " + groups + " groups) " : "" ) +
+               "with " + realResources() +
                " at cost $" + cost() +
                (fulfilment < 1.0 ? " (fulfilment " + fulfilment + ")" : "");
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocationOptimizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocationOptimizer.java
index d1a5a91e985..d0969e13952 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocationOptimizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/AllocationOptimizer.java
@@ -39,36 +39,26 @@ public class AllocationOptimizer {
     public Optional<AllocatableClusterResources> findBestAllocation(ResourceTarget target,
                                                                     AllocatableClusterResources current,
                                                                     Limits limits) {
-        // What number of nodes is it effective to add or remove at the time from this cluster?
-        // This is the group size, since we (for now) assume the group size is decided by someone wiser than us
-        // and we decide the number of groups.
-        // The exception is when we only have one group, where we can add and remove single nodes in it.
-
         Optional<AllocatableClusterResources> bestAllocation = Optional.empty();
-        for (int nodes = minNodes(limits); nodes <= maxNodes(limits); nodes++) {
-            boolean singleGroupMode = current.groups() == 1 && ( limits.isEmpty() || limits.min().groups() == 1 );
-            int groups = singleGroupMode ? 1 : nodes / current.groupSize();
-            if ( ! limits.isEmpty() && ( groups < limits.min().groups() || groups > limits.max().groups())) continue;
-
-            if (nodes % groups != 0) continue;
-            int groupSize = nodes / groups;
-            if (groups != 1 && groupSize != current.groupSize()) continue; // TODO: Remove this line
-
-
-            // Adjust for redundancy: Node in group if groups = 1, an extra group if multiple groups
-            // TODO: Make the best choice based on size and redundancy setting instead
-            int nodesAdjustedForRedundancy = target.adjustForRedundancy() ? ( groups == 1 ? nodes - 1 : nodes - groupSize ) : nodes;
-            if (nodesAdjustedForRedundancy < 1) continue;
-            int groupsAdjustedForRedundancy = target.adjustForRedundancy() ? ( groups == 1 ? 1 : groups - 1 ) : groups;
-
-            ClusterResources next = new ClusterResources(nodes,
-                                                         groups,
-                                                         nodeResourcesWith(nodesAdjustedForRedundancy, groupsAdjustedForRedundancy, limits, current, target));
-
-            var allocatableResources = AllocatableClusterResources.from(next, current.clusterType(), limits, nodeRepository);
-            if (allocatableResources.isEmpty()) continue;
-            if (bestAllocation.isEmpty() || allocatableResources.get().preferableTo(bestAllocation.get()))
-                bestAllocation = allocatableResources;
+        for (int groups = minGroups(limits); groups <= maxGroups(limits); groups++) {
+            for (int nodes = minNodes(limits); nodes <= maxNodes(limits); nodes++) {
+                if (nodes % groups != 0) continue;
+                int groupSize = nodes / groups;
+
+                // Adjust for redundancy: Node in group if groups = 1, an extra group if multiple groups
+                // TODO: Make the best choice based on size and redundancy setting instead
+                int nodesAdjustedForRedundancy = target.adjustForRedundancy() ? (groups == 1 ? nodes - 1 : nodes - groupSize) : nodes;
+                int groupsAdjustedForRedundancy = target.adjustForRedundancy() ? (groups == 1 ? 1 : groups - 1) : groups;
+
+                ClusterResources next = new ClusterResources(nodes,
+                                                             groups,
+                                                             nodeResourcesWith(nodesAdjustedForRedundancy, groupsAdjustedForRedundancy, limits, current, target));
+
+                var allocatableResources = AllocatableClusterResources.from(next, current.clusterType(), limits, nodeRepository);
+                if (allocatableResources.isEmpty()) continue;
+                if (bestAllocation.isEmpty() || allocatableResources.get().preferableTo(bestAllocation.get()))
+                    bestAllocation = allocatableResources;
+            }
         }
 
         return bestAllocation;
@@ -84,6 +74,16 @@ public class AllocationOptimizer {
         return limits.max().nodes();
     }
 
+    private int minGroups(Limits limits) {
+        if (limits.isEmpty()) return 1;
+        return limits.min().groups();
+    }
+
+    private int maxGroups(Limits limits) {
+        if (limits.isEmpty()) return maximumNodes;
+        return limits.max().groups();
+    }
+
     /**
      * For the observed load this instance is initialized with, returns the resources needed per node to be at
      * ideal load given a target node count
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 8721eabf5e4..2e065280614 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
@@ -236,7 +236,7 @@ public class AutoscalingTest {
     }
 
     @Test
-    public void testAutoscalingGroupSize3() {
+    public void testAutoscalingGroupSizeByCpu() {
         NodeResources resources = new NodeResources(3, 100, 100, 1);
         ClusterResources min = new ClusterResources( 3, 1, new NodeResources(1, 1, 1, 1));
         ClusterResources max = new ClusterResources(21, 7, new NodeResources(100, 1000, 1000, 1));
@@ -247,27 +247,28 @@ public class AutoscalingTest {
 
         // deploy
         tester.deploy(application1, cluster1, 6, 2, resources);
-        tester.addMeasurements(Resource.cpu,  0.22f, 1f, 120, application1);
-        tester.assertResources("Scaling up since resource usage is too high",
-                               9, 3, 2.7,  83.3, 83.3,
+        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));
     }
 
     @Test
     public void testAutoscalingGroupSize() {
-        NodeResources resources = new NodeResources(3, 100, 100, 1);
+        NodeResources hostResources = new NodeResources(100, 1000, 1000, 100);
         ClusterResources min = new ClusterResources( 3, 2, new NodeResources(1, 1, 1, 1));
-        ClusterResources max = new ClusterResources(30, 10, new NodeResources(100, 1000, 1000, 1));
-        AutoscalingTester tester = new AutoscalingTester(resources);
+        ClusterResources max = new ClusterResources(30, 30, new NodeResources(100, 100, 1000, 1));
+        AutoscalingTester tester = new AutoscalingTester(hostResources);
 
         ApplicationId application1 = tester.applicationId("application1");
-        ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.container, "cluster1");
+        ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.content, "cluster1");
 
         // deploy
-        tester.deploy(application1, cluster1, 6, 2, resources);
-        tester.addMeasurements(Resource.memory,  0.9f, 1f, 120, application1);
-        tester.assertResources("Should increase cluster size to reduce memory usage",
-                               9, 3, 2.7,  83.3, 83.3,
+        tester.deploy(application1, cluster1, 6, 2, new NodeResources(10, 100, 100, 1));
+        tester.addMeasurements(Resource.memory,  1.0f, 1f, 1000, application1);
+        tester.assertResources("Increase group size to reduce memory load",
+                               8, 2, 12.9,  89.3, 62.5,
                                tester.autoscale(application1, cluster1.id(), min, max));
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
index 73e337dd47c..88e67a961c8 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisionTest.java
@@ -251,28 +251,29 @@ public class DynamicDockerProvisionTest {
         tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 2, 10, 20),
                                                       resources(6, 3, 3, 25, 25)));
         tester.assertNodes("New allocation at new max",
-                           6, 3, 2, 20, 25,
+                           6, 2, 2, 20, 25,
                            app1, cluster1);
 
-        // Widening window lets us find a cheaper alternative
+        // Widening window does not change allocation
         tester.activate(app1, cluster1, Capacity.from(resources(2, 1, 1, 5, 15),
                                                       resources(8, 4, 4, 20, 30)));
-        tester.assertNodes("Cheaper allocation",
-                           8, 4, 1, 10, 25,
+        tester.assertNodes("No change",
+                           6, 2, 2, 20, 25,
                            app1, cluster1);
 
-        // Changing group size
+        // Force 1 more groups: Reducing to 2 nodes per group to preserve node count is rejected
+        //                      since it will reduce total group memory from 60 to 40.
         tester.activate(app1, cluster1, Capacity.from(resources(6, 3, 0.5,  5,  10),
                                                       resources(9, 3,   5, 20, 15)));
-        tester.assertNodes("Groups changed",
-                           6, 3, 1, 10, 15,
+        tester.assertNodes("Group size is preserved",
+                           9, 3, 2, 20, 15,
                            app1, cluster1);
 
         // Stop specifying node resources
         tester.activate(app1, cluster1, Capacity.from(new ClusterResources(6, 3, NodeResources.unspecified),
                                                       new ClusterResources(9, 3, NodeResources.unspecified)));
-        tester.assertNodes("Minimal allocation",
-                           6, 3, 1, 10, 15,
+        tester.assertNodes("Existing allocation is preserved",
+                           9, 3, 2, 20, 15,
                            app1, cluster1);
     }
 
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 694c1f2d9fc..b3069d4baea 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
@@ -413,7 +413,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(30, hostFlavor.resources()).deployZoneApp();
+        tester.makeReadyHosts(31, hostFlavor.resources()).deployZoneApp();
 
         ApplicationId app1 = tester.makeApplicationId("app1");
         ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
@@ -426,45 +426,45 @@ public class ProvisioningTest {
                            app1, cluster1);
 
         // Move window above current allocation
-        tester.activate(app1, cluster1, Capacity.from(resources(8, 4, 4, 20, 40),
+        tester.activate(app1, cluster1, Capacity.from(resources(8, 4, 4, 21, 40),
                                                       resources(10, 5, 5, 25, 50)));
         tester.assertNodes("New allocation at new min",
-                           8, 4, 4, 20, 40,
+                           8, 4, 4, 21, 40,
                            app1, cluster1);
 
         // Move window below current allocation
         tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 2, 10, 20),
                                                       resources(6, 3, 3, 15, 25)));
-        tester.assertNodes("New allocation at new max",
-                           6, 3, 3, 15, 25,
+        tester.assertNodes("Allocation preserving resources within new limits",
+                           6, 2, 3, 8.0/4*21 / (6.0/2), 25,
                            app1, cluster1);
 
         // Widening window does not change allocation
         tester.activate(app1, cluster1, Capacity.from(resources(4, 2, 1, 5, 15),
-                                                      resources(8, 4, 4, 20, 30)));
+                                                      resources(8, 4, 4, 21, 30)));
         tester.assertNodes("Same allocation",
-                           6, 3, 3, 15, 25,
+                           6, 2, 3, 8.0/4*21 / (6.0/2), 25,
                            app1, cluster1);
 
         // Changing limits in opposite directions cause a mixture of min and max
         tester.activate(app1, cluster1, Capacity.from(resources(2, 1, 10, 30,  10),
                                                       resources(4, 2, 14, 40, 13)));
         tester.assertNodes("A mix of min and max",
-                           3, 1, 10, 30, 13.0,
+                           4, 1, 10, 30, 13,
                            app1, cluster1);
 
         // Changing group size
         tester.activate(app1, cluster1, Capacity.from(resources(6, 3, 8, 25,  10),
                                                       resources(9, 3, 12, 35, 15)));
         tester.assertNodes("Groups changed",
-                           9, 3, 8, 30, 13,
+                           9, 3, 8, 35, 15,
                            app1, cluster1);
 
         // Stop specifying node resources
         tester.activate(app1, cluster1, Capacity.from(new ClusterResources(6, 3, NodeResources.unspecified),
                                                       new ClusterResources(9, 3, NodeResources.unspecified)));
         tester.assertNodes("No change",
-                           9, 3, 8, 30, 13,
+                           9, 3, 8, 35, 15,
                            app1, cluster1);
     }