Prefer the best fulfilment over lowest cost

4 files changed, 55 insertions, 22 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 f553a4c76a4..1c3ea55163a 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
@@ -26,26 +26,32 @@ public class AllocatableClusterResources {
 
     private final ClusterSpec.Type clusterType;
 
+    private final double fulfilment;
+
     public AllocatableClusterResources(List<Node> nodes, HostResourcesCalculator calculator) {
         this.advertisedResources = nodes.get(0).flavor().resources();
         this.realResources = calculator.realResourcesOf(nodes.get(0));
         this.nodes = nodes.size();
         this.groups = (int)nodes.stream().map(node -> node.allocation().get().membership().cluster().group()).distinct().count();
         this.clusterType = nodes.get(0).allocation().get().membership().cluster().type();
+        this.fulfilment = 1;
     }
 
     public AllocatableClusterResources(ClusterResources realResources,
                                        NodeResources advertisedResources,
+                                       NodeResources idealResources,
                                        ClusterSpec.Type clusterType) {
         this.realResources = realResources.nodeResources();
         this.advertisedResources = advertisedResources;
         this.nodes = realResources.nodes();
         this.groups = realResources.groups();
         this.clusterType = clusterType;
+        this.fulfilment = fulfilment(realResources.nodeResources(), idealResources);
     }
 
     public AllocatableClusterResources(ClusterResources realResources,
                                        Flavor flavor,
+                                       NodeResources idealResources,
                                        ClusterSpec.Type clusterType,
                                        HostResourcesCalculator calculator) {
         this.realResources = realResources.nodeResources();
@@ -53,6 +59,7 @@ public class AllocatableClusterResources {
         this.nodes = realResources.nodes();
         this.groups = realResources.groups();
         this.clusterType = clusterType;
+        this.fulfilment = fulfilment(realResources.nodeResources(), idealResources);
     }
 
     /**
@@ -67,19 +74,38 @@ public class AllocatableClusterResources {
      */
     public NodeResources advertisedResources() { return advertisedResources; }
 
-    public double cost() { return nodes * Autoscaler.costOf(advertisedResources); }
+    public ClusterResources toAdvertisedClusterResources() {
+        return new ClusterResources(nodes, groups, advertisedResources);
+    }
 
     public int nodes() { return nodes; }
     public int groups() { return groups; }
     public ClusterSpec.Type clusterType() { return clusterType; }
 
-    public ClusterResources toAdvertisedClusterResources() {
-        return new ClusterResources(nodes, groups, advertisedResources);
+    public double cost() { return nodes * Autoscaler.costOf(advertisedResources); }
+
+    /**
+     * Returns the fraction measuring how well the real resources fulfils the ideal: 1 means completely fulfiled,
+     * 0 means we have zero real resources.
+     * The real may be short of the ideal due to resource limits imposed by the system or application.
+     */
+    public double fulfilment() { return fulfilment; }
+
+    private static double fulfilment(NodeResources realResources, NodeResources idealResources) {
+        double vcpuFulfilment     = Math.min(1, realResources.vcpu()     / idealResources.vcpu());
+        double memoryGbFulfilment = Math.min(1, realResources.memoryGb() / idealResources.memoryGb());
+        double diskGbFulfilment   = Math.min(1, realResources.diskGb()   / idealResources.diskGb());
+        return (vcpuFulfilment + memoryGbFulfilment + diskGbFulfilment) / 3;
+    }
+
+    public boolean preferableTo(AllocatableClusterResources other) {
+        if (this.fulfilment > other.fulfilment) return true; // we always want to fulfil as much as possible
+        return this.cost() < other.cost(); // otherwise, prefer lower cost
     }
 
     @Override
     public String toString() {
-        return "$" + cost() + ": " + realResources();
+        return "$" + cost() + " (fulfilment " + fulfilment + "): " + realResources();
     }
 
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java
index dc873ce4e69..bfaf6874b51 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Autoscaler.java
@@ -102,11 +102,11 @@ public class Autoscaler {
                                                                      Cluster cluster) {
         Optional<AllocatableClusterResources> bestAllocation = Optional.empty();
         for (ResourceIterator i = new ResourceIterator(cpuLoad, memoryLoad, diskLoad, currentAllocation, cluster); i.hasNext(); ) {
-            ClusterResources allocation = i.next();
-            Optional<AllocatableClusterResources> allocatableResources = toAllocatableResources(allocation,
-                                                                                                currentAllocation.clusterType());
+            Optional<AllocatableClusterResources> allocatableResources = toAllocatableResources(i.next(),
+                                                                                                currentAllocation.clusterType(),
+                                                                                                cluster);
             if (allocatableResources.isEmpty()) continue;
-            if (bestAllocation.isEmpty() || allocatableResources.get().cost() < bestAllocation.get().cost())
+            if (bestAllocation.isEmpty() || allocatableResources.get().preferableTo(bestAllocation.get()))
                 bestAllocation = allocatableResources;
         }
         return bestAllocation;
@@ -129,16 +129,26 @@ public class Autoscaler {
      * or empty if none available.
      */
     private Optional<AllocatableClusterResources> toAllocatableResources(ClusterResources resources,
-                                                                         ClusterSpec.Type clusterType) {
-        NodeResources nodeResources = nodeResourceLimits.enlargeToLegal(resources.nodeResources(), clusterType);
+                                                                         ClusterSpec.Type clusterType,
+                                                                         Cluster cluster) {
+        System.out.println("Candidate: " + resources);
+        NodeResources nodeResources = resources.nodeResources();
+        if ( ! cluster.minResources().equals(cluster.maxResources())) // enforce application limits unless suggest mode
+            nodeResources = cluster.capAtLimits(nodeResources);
+        nodeResources = nodeResourceLimits.enlargeToLegal(nodeResources, clusterType); // enforce system limits
+
         if (allowsHostSharing(nodeRepository.zone().cloud())) {
             // return the requested resources, or empty if they cannot fit on existing hosts
             for (Flavor flavor : nodeRepository.getAvailableFlavors().getFlavors()) {
                 if (flavor.resources().satisfies(nodeResources))
                     return Optional.of(new AllocatableClusterResources(resources.with(nodeResources),
                                                                        nodeResources,
+                                                                       resources.nodeResources(),
                                                                        clusterType));
+                else
+                    System.out.println("  " + flavor + " with " + flavor.resources() + " does not satisfy " + nodeResources);
             }
+            System.out.println("  ... returning empty");
             return Optional.empty();
         }
         else {
@@ -151,6 +161,7 @@ public class Autoscaler {
                     flavor = flavor.with(FlavorOverrides.ofDisk(nodeResources.diskGb()));
                 var candidate = new AllocatableClusterResources(resources.with(flavor.resources()),
                                                                 flavor,
+                                                                resources.nodeResources(),
                                                                 clusterType,
                                                                 resourcesCalculator);
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceIterator.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceIterator.java
index 19909e40441..bc14ca1779c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceIterator.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceIterator.java
@@ -72,7 +72,6 @@ public class ResourceIterator {
     public ClusterResources next() {
         ClusterResources next = resourcesWith(currentNodes);
         currentNodes += nodeIncrement;
-        System.out.println("Candidate: " + next);
         return next;
     }
 
@@ -135,10 +134,7 @@ public class ResourceIterator {
             }
         }
 
-        NodeResources resources = allocation.realResources().withVcpu(cpu).withMemoryGb(memory).withDiskGb(disk);
-        if ( ! suggestMode())
-            resources = cluster.capAtLimits(resources);
-        return resources;
+        return allocation.realResources().withVcpu(cpu).withMemoryGb(memory).withDiskGb(disk);
     }
 
     private double clusterUsage(Resource resource, double load) {
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 0e6d2365490..3bb0676da4f 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
@@ -120,7 +120,7 @@ public class AutoscalingTest {
     @Test
     public void testAutoscalingRespectsLowerLimit() {
         NodeResources resources = new NodeResources(3, 100, 100, 1);
-        ClusterResources min = new ClusterResources( 3, 1, new NodeResources(1.8, 7.4, 8.5, 1));
+        ClusterResources min = new ClusterResources( 4, 1, new NodeResources(1.8, 7.4, 8.5, 1));
         ClusterResources max = new ClusterResources( 6, 1, new NodeResources(2.4, 78, 79, 1));
         AutoscalingTester tester = new AutoscalingTester(resources);
 
@@ -133,25 +133,25 @@ public class AutoscalingTest {
         tester.addMeasurements(Resource.memory, 0.05f, 120, application1);
         tester.addMeasurements(Resource.disk,   0.05f, 120, application1);
         tester.assertResources("Scaling down to limit since resource usage is low",
-                               3, 1, 1.8,  7.4, 8.5,
+                               4, 1, 1.8,  7.4, 8.5,
                                tester.autoscale(application1, cluster1.id(), min, max));
     }
 
     @Test
     public void testAutoscalingRespectsGroupLimit() {
-        NodeResources resources = new NodeResources(3, 100, 100, 1);
+        NodeResources hostResources = new NodeResources(30.0, 100, 100, 1);
         ClusterResources min = new ClusterResources( 2, 2, new NodeResources(1, 1, 1, 1));
         ClusterResources max = new ClusterResources(18, 6, new NodeResources(100, 1000, 1000, 1));
-        AutoscalingTester tester = new AutoscalingTester(resources);
+        AutoscalingTester tester = new AutoscalingTester(hostResources);
 
         ApplicationId application1 = tester.applicationId("application1");
         ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.container, "cluster1");
 
         // deploy
-        tester.deploy(application1, cluster1, 5, 5, resources);
-        tester.addMeasurements(Resource.cpu,  0.25f, 1f, 120, application1);
+        tester.deploy(application1, cluster1, 5, 5, new NodeResources(3.0, 10, 10, 1));
+        tester.addMeasurements(Resource.cpu,  0.3f, 1f, 240, application1);
         tester.assertResources("Scaling up since resource usage is too high",
-                               6, 6, 2.5,  80.0, 80.0,
+                               6, 6, 3.6,  8.0, 8.0,
                                tester.autoscale(application1, cluster1.id(), min, max));
     }