Apply limits to forecasts

7 files changed, 120 insertions, 80 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 30aaf938139..e6cbddf96f2 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
@@ -132,41 +132,30 @@ public class AllocatableClusterResources {
                                                 .withBandwidthGbps(sum.bandwidthGbps() / nodes.size());
     }
 
-    /**
-     * Returns the best matching allocatable node resources given ideal node resources,
-     * or empty if none available within the limits.
-     *
-     * @param resources the real resources that should ideally be allocated
-     * @param exclusive whether resources should be allocated on entire hosts
-     *        (in which case the allocated resources will be all the real resources of the host
-     *         and limits are required to encompass the full resources of candidate host flavors)
-     */
-    public static Optional<AllocatableClusterResources> from(ClusterResources resources,
+    public static Optional<AllocatableClusterResources> from(ClusterResources wantedResources,
                                                              boolean exclusive,
                                                              ClusterSpec.Type clusterType,
                                                              Limits applicationLimits,
                                                              NodeRepository nodeRepository) {
         var systemLimits = new NodeResourceLimits(nodeRepository);
-        NodeResources cappedNodeResources = applicationLimits.cap(resources.nodeResources());
-        cappedNodeResources = systemLimits.enlargeToLegal(cappedNodeResources, clusterType);
-
         if ( !exclusive && nodeRepository.zone().getCloud().allowHostSharing()) { // Check if any flavor can fit these hosts
             // We decide resources: Add overhead to what we'll request (advertised) to make sure real becomes (at least) cappedNodeResources
-            NodeResources realResources = cappedNodeResources;
-            NodeResources advertisedResources = nodeRepository.resourcesCalculator().realToRequest(realResources);
-
+            NodeResources advertisedResources = nodeRepository.resourcesCalculator().realToRequest(wantedResources.nodeResources());
+            advertisedResources = systemLimits.enlargeToLegal(advertisedResources, clusterType); // Attempt to ask for something legal
+            advertisedResources = applicationLimits.cap(advertisedResources); // Overrides other conditions, even if it will then fail
+            NodeResources realResources = nodeRepository.resourcesCalculator().requestToReal(advertisedResources); // ... thus, what we really get may change
             if ( ! systemLimits.isWithinRealLimits(realResources, clusterType)) return Optional.empty();
-
             for (Flavor flavor : nodeRepository.flavors().getFlavors()) {
                 if (flavor.resources().satisfies(advertisedResources))
-                    return Optional.of(new AllocatableClusterResources(resources.with(realResources),
+                    return Optional.of(new AllocatableClusterResources(wantedResources.with(realResources),
                                                                        advertisedResources,
-                                                                       resources.nodeResources(),
+                                                                       wantedResources.nodeResources(),
                                                                        clusterType));
             }
             return Optional.empty();
         }
         else { // Return the cheapest flavor satisfying the requested resources, if any
+            NodeResources cappedWantedResources = applicationLimits.cap(wantedResources.nodeResources());
             Optional<AllocatableClusterResources> best = Optional.empty();
             for (Flavor flavor : nodeRepository.flavors().getFlavors()) {
                 // Flavor decide resources: Real resources are the worst case real resources we'll get if we ask for these advertised resources
@@ -175,19 +164,19 @@ public class AllocatableClusterResources {
 
                 // Adjust where we don't need exact match to the flavor
                 if (flavor.resources().storageType() == NodeResources.StorageType.remote) {
-                    advertisedResources = advertisedResources.withDiskGb(cappedNodeResources.diskGb());
-                    realResources = realResources.withDiskGb(cappedNodeResources.diskGb());
+                    advertisedResources = advertisedResources.withDiskGb(cappedWantedResources.diskGb());
+                    realResources = realResources.withDiskGb(cappedWantedResources.diskGb());
                 }
-                if (flavor.resources().bandwidthGbps() >= cappedNodeResources.bandwidthGbps()) {
-                    advertisedResources = advertisedResources.withBandwidthGbps(cappedNodeResources.bandwidthGbps());
-                    realResources = realResources.withBandwidthGbps(cappedNodeResources.bandwidthGbps());
+                if (flavor.resources().bandwidthGbps() >= advertisedResources.bandwidthGbps()) {
+                    advertisedResources = advertisedResources.withBandwidthGbps(cappedWantedResources.bandwidthGbps());
+                    realResources = realResources.withBandwidthGbps(cappedWantedResources.bandwidthGbps());
                 }
 
                 if ( ! between(applicationLimits.min().nodeResources(), applicationLimits.max().nodeResources(), advertisedResources)) continue;
                 if ( ! systemLimits.isWithinRealLimits(realResources, clusterType)) continue;
-                var candidate = new AllocatableClusterResources(resources.with(realResources),
+                var candidate = new AllocatableClusterResources(wantedResources.with(realResources),
                                                                 advertisedResources,
-                                                                resources.nodeResources(),
+                                                                wantedResources.nodeResources(),
                                                                 clusterType);
                 if (best.isEmpty() || candidate.preferableTo(best.get()))
                     best = Optional.of(candidate);
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 864cfad2f41..cc5c6851a92 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
@@ -277,16 +277,16 @@ public class AutoscalingTest {
 
     @Test
     public void autoscaling_avoids_illegal_configurations() {
-        NodeResources resources = new NodeResources(3, 100, 100, 1);
+        NodeResources hostResources = new NodeResources(3, 100, 100, 1);
         ClusterResources min = new ClusterResources( 2, 1, new NodeResources(1, 1, 1, 1));
         ClusterResources max = new ClusterResources(20, 1, 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.content, "cluster1");
 
         // deploy
-        tester.deploy(application1, cluster1, 6, 1, resources);
+        tester.deploy(application1, cluster1, 6, 1, hostResources);
         tester.addMeasurements(Resource.memory,  0.02f, 0.95f, 120, application1);
         tester.assertResources("Scaling down",
                                6, 1, 2.8, 4.0, 95.0,
@@ -325,7 +325,7 @@ public class AutoscalingTest {
             tester.addMeasurements(Resource.memory, 1.0f, 1000, application1);
             tester.addMeasurements(Resource.disk, 0.7f, 1000, application1);
             tester.assertResources("Scaling up",
-                                   4, 1, 7.0, 35, 200,
+                                   4, 1, 7.0, 34, 200,
                                    tester.autoscale(application1, cluster1.id(), min, max));
         }
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
index 963ca0d4341..b0e394c93d3 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/autoscale/AutoscalingTester.java
@@ -178,11 +178,12 @@ class AutoscalingTester {
                                             Optional<ClusterResources> resources) {
         double delta = 0.0000000001;
         assertTrue(message, resources.isPresent());
-        assertEquals("Node count:  " + message, nodeCount, resources.get().nodes());
-        assertEquals("Group count: " + message, groupCount, resources.get().groups());
-        assertEquals("Cpu: "    + message, approxCpu, Math.round(resources.get().nodeResources().vcpu() * 10) / 10.0, delta);
-        assertEquals("Memory: " + message, approxMemory, Math.round(resources.get().nodeResources().memoryGb() * 10) / 10.0, delta);
-        assertEquals("Disk: "   + message, approxDisk, Math.round(resources.get().nodeResources().diskGb() * 10) / 10.0, delta);
+        NodeResources nodeResources = resources.get().nodeResources();
+        assertEquals("Node count in " + resources.get() + ": " + message, nodeCount, resources.get().nodes());
+        assertEquals("Group count in " + resources.get() + ": " + message, groupCount, resources.get().groups());
+        assertEquals("Cpu in " + resources.get() + ": " + message, approxCpu, Math.round(nodeResources.vcpu() * 10) / 10.0, delta);
+        assertEquals("Memory in " + resources.get() + ": " + message, approxMemory, Math.round(nodeResources.memoryGb() * 10) / 10.0, delta);
+        assertEquals("Disk in: " + resources.get() + ": "  + message, approxDisk, Math.round(nodeResources.diskGb() * 10) / 10.0, delta);
         return resources.get();
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
index 3c1cace3dac..3ffb0dc34f0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningTest.java
@@ -8,6 +8,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.InstanceName;
 import com.yahoo.config.provision.NodeResources;
@@ -301,6 +302,50 @@ public class DockerProvisioningTest {
         }
     }
 
+    @Test
+    public void initial_allocation_is_within_limits() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                    .resourcesCalculator(3, 0)
+                                                                    .flavors(List.of(hostFlavor))
+                                                                    .build();
+        tester.makeReadyHosts(2, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        var resources = new NodeResources(1, 8, 10, 1);
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, resources),
+                                                      new ClusterResources(4, 1, resources)));
+        tester.assertNodes("Initial allocation at min with default resources",
+                           2, 1, 1, 8, 10, 1.0,
+                           app1, cluster1);
+    }
+
+    @Test
+    public void too_few_real_resources_causes_failure() {
+        try {
+            Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
+            ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                        .resourcesCalculator(3, 0)
+                                                                        .flavors(List.of(hostFlavor))
+                                                                        .build();
+            tester.makeReadyHosts(2, hostFlavor.resources()).deployZoneApp();
+
+            ApplicationId app1 = tester.makeApplicationId("app1");
+            ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+            // 5 Gb requested memory becomes 5-3=2 Gb real memory, which is an illegally small amount
+            var resources = new NodeResources(1, 5, 10, 1);
+            tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, resources),
+                                                          new ClusterResources(4, 1, resources)));
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("No allocation possible within limits: from 2 nodes with [vcpu: 1.0, memory: 5.0 Gb, disk 10.0 Gb, bandwidth: 1.0 Gbps] to 4 nodes with [vcpu: 1.0, memory: 5.0 Gb, disk 10.0 Gb, bandwidth: 1.0 Gbps]",
+                         e.getMessage());
+        }
+    }
+
     private Set<String> hostsOf(NodeList nodes) {
         return nodes.asList().stream().map(Node::parentHostname).map(Optional::get).collect(Collectors.toSet());
     }
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 25387a2746c..cf885941116 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
@@ -19,7 +19,6 @@ import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.hosted.provision.Node;
-import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.hosted.provision.node.IP;
 import com.yahoo.vespa.hosted.provision.testutils.MockNameResolver;
@@ -168,7 +167,7 @@ public class DynamicDockerProvisionTest {
                                                                     .flavors(flavors)
                                                                     .hostProvisioner(new MockHostProvisioner(flavors, memoryTax))
                                                                     .nameResolver(nameResolver)
-                                                                    .resourcesCalculator(new MockResourcesCalculator(memoryTax, 0))
+                                                                    .resourcesCalculator(new ProvisioningTester.MockResourcesCalculator(memoryTax, 0))
                                                                     .build();
 
         tester.deployZoneApp();
@@ -214,7 +213,7 @@ public class DynamicDockerProvisionTest {
                                                                     .flavors(flavors)
                                                                     .hostProvisioner(new MockHostProvisioner(flavors, memoryTax))
                                                                     .nameResolver(nameResolver)
-                                                                    .resourcesCalculator(new MockResourcesCalculator(memoryTax, 0))
+                                                                    .resourcesCalculator(new ProvisioningTester.MockResourcesCalculator(memoryTax, 0))
                                                                     .build();
 
         tester.deployZoneApp();
@@ -289,7 +288,7 @@ public class DynamicDockerProvisionTest {
                                                                     .flavors(flavors)
                                                                     .hostProvisioner(new MockHostProvisioner(flavors, memoryTax))
                                                                     .nameResolver(nameResolver)
-                                                                    .resourcesCalculator(new MockResourcesCalculator(memoryTax, 0))
+                                                                    .resourcesCalculator(new ProvisioningTester.MockResourcesCalculator(memoryTax, 0))
                                                                     .build();
 
         tester.deployZoneApp();
@@ -324,7 +323,7 @@ public class DynamicDockerProvisionTest {
                                                                     .flavors(flavors)
                                                                     .hostProvisioner(new MockHostProvisioner(flavors, memoryTax))
                                                                     .nameResolver(nameResolver)
-                                                                    .resourcesCalculator(new MockResourcesCalculator(memoryTax, localDiskTax))
+                                                                    .resourcesCalculator(new ProvisioningTester.MockResourcesCalculator(memoryTax, localDiskTax))
                                                                     .build();
 
         tester.deployZoneApp();
@@ -364,45 +363,6 @@ public class DynamicDockerProvisionTest {
         }).when(hostProvisioner).provisionHosts(any(), any(), any(), any());
     }
 
-    private static class MockResourcesCalculator implements HostResourcesCalculator {
-
-        private final int memoryTaxGb;
-        private final int localDiskTax;
-
-        public MockResourcesCalculator(int memoryTaxGb, int localDiskTax) {
-            this.memoryTaxGb = memoryTaxGb;
-            this.localDiskTax = localDiskTax;
-        }
-
-        @Override
-        public NodeResources realResourcesOf(Node node, NodeRepository nodeRepository) {
-            NodeResources resources = node.flavor().resources();
-            if (node.type() == NodeType.host) return resources;
-            return resources.withMemoryGb(resources.memoryGb() - memoryTaxGb)
-                            .withDiskGb(resources.diskGb() - ( resources.storageType() == local ? localDiskTax : 0));
-        }
-
-        @Override
-        public NodeResources advertisedResourcesOf(Flavor flavor) {
-            NodeResources resources = flavor.resources();
-            if ( ! flavor.isConfigured()) return resources;
-            return resources.withMemoryGb(resources.memoryGb() + memoryTaxGb);
-        }
-
-        @Override
-        public NodeResources requestToReal(NodeResources resources) {
-            return resources.withMemoryGb(resources.memoryGb() - memoryTaxGb)
-                            .withDiskGb(resources.diskGb() - ( resources.storageType() == local ? localDiskTax : 0) );
-        }
-
-        @Override
-        public NodeResources realToRequest(NodeResources resources) {
-            return resources.withMemoryGb(resources.memoryGb() + memoryTaxGb)
-                            .withDiskGb(resources.diskGb() + ( resources.storageType() == local ? localDiskTax : 0) );
-        }
-
-    }
-
     private static class MockHostProvisioner implements HostProvisioner {
 
         private final List<Flavor> hostFlavors;
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 b35f9dc88ae..7a7f8a7d891 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(4, hostFlavor.resources()).deployZoneApp();
 
         ApplicationId app1 = tester.makeApplicationId("app1");
         ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").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 9a02bc17b4b..2427c0303c6 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
@@ -56,6 +56,7 @@ import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
 
+import static com.yahoo.config.provision.NodeResources.StorageType.local;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
@@ -562,6 +563,11 @@ public class ProvisioningTester {
             return this;
         }
 
+        public Builder resourcesCalculator(int memoryTax, int diskTax) {
+            this.resourcesCalculator = new MockResourcesCalculator(memoryTax, diskTax);
+            return this;
+        }
+
         public Builder zone(Zone zone) {
             this.zone = zone;
             return this;
@@ -640,4 +646,43 @@ public class ProvisioningTester {
         @Override public void log(Level level, String message) { }
     }
 
+    static class MockResourcesCalculator implements HostResourcesCalculator {
+
+        private final int memoryTaxGb;
+        private final int localDiskTax;
+
+        public MockResourcesCalculator(int memoryTaxGb, int localDiskTax) {
+            this.memoryTaxGb = memoryTaxGb;
+            this.localDiskTax = localDiskTax;
+        }
+
+        @Override
+        public NodeResources realResourcesOf(Node node, NodeRepository nodeRepository) {
+            NodeResources resources = node.flavor().resources();
+            if (node.type() == NodeType.host) return resources;
+            return resources.withMemoryGb(resources.memoryGb() - memoryTaxGb)
+                            .withDiskGb(resources.diskGb() - ( resources.storageType() == local ? localDiskTax : 0));
+        }
+
+        @Override
+        public NodeResources advertisedResourcesOf(Flavor flavor) {
+            NodeResources resources = flavor.resources();
+            if ( ! flavor.isConfigured()) return resources;
+            return resources.withMemoryGb(resources.memoryGb() + memoryTaxGb);
+        }
+
+        @Override
+        public NodeResources requestToReal(NodeResources resources) {
+            return resources.withMemoryGb(resources.memoryGb() - memoryTaxGb)
+                            .withDiskGb(resources.diskGb() - ( resources.storageType() == local ? localDiskTax : 0) );
+        }
+
+        @Override
+        public NodeResources realToRequest(NodeResources resources) {
+            return resources.withMemoryGb(resources.memoryGb() + memoryTaxGb)
+                            .withDiskGb(resources.diskGb() + ( resources.storageType() == local ? localDiskTax : 0) );
+        }
+
+    }
+
 }