Merge pull request #18414 from vespa-engine/bratseth/enlarge-remote-disks

Enlarge remote disks to smallest lergal size if smaller is wanted

4 files changed, 72 insertions, 4 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 4fe56e3a6af..b465917e9c9 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
@@ -162,8 +162,9 @@ public class AllocatableClusterResources {
 
                 // Adjust where we don't need exact match to the flavor
                 if (flavor.resources().storageType() == NodeResources.StorageType.remote) {
-                    advertisedResources = advertisedResources.withDiskGb(cappedWantedResources.diskGb());
-                    realResources = realResources.withDiskGb(cappedWantedResources.diskGb());
+                    double diskGb = systemLimits.enlargeToLegal(cappedWantedResources, clusterSpec.type(), exclusive).diskGb();
+                    advertisedResources = advertisedResources.withDiskGb(diskGb);
+                    realResources = realResources.withDiskGb(diskGb);
                 }
                 if (flavor.resources().bandwidthGbps() >= advertisedResources.bandwidthGbps()) {
                     advertisedResources = advertisedResources.withBandwidthGbps(cappedWantedResources.bandwidthGbps());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorConfigBuilder.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorConfigBuilder.java
index 54530297baa..19f932407d3 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorConfigBuilder.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/FlavorConfigBuilder.java
@@ -13,7 +13,7 @@ import com.yahoo.config.provisioning.FlavorsConfig;
  */
 public class FlavorConfigBuilder {
 
-    private FlavorsConfig.Builder builder = new FlavorsConfig.Builder();
+    private final FlavorsConfig.Builder builder = new FlavorsConfig.Builder();
 
     public FlavorsConfig build() {
         return new FlavorsConfig(builder);
@@ -65,4 +65,5 @@ public class FlavorConfigBuilder {
         }
         return new NodeFlavors(flavorConfigBuilder.build());
     }
+
 }
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 240422df8b6..d07ef8e435f 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
@@ -4,11 +4,14 @@ package com.yahoo.vespa.hosted.provision.autoscale;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.Cloud;
+import com.yahoo.config.provision.CloudName;
 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.NodeResources;
+import com.yahoo.config.provision.NodeResources.DiskSpeed;
+import com.yahoo.config.provision.NodeResources.StorageType;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
@@ -283,6 +286,38 @@ public class AutoscalingTest {
     }
 
     @Test
+    public void prefers_remote_disk_when_no_local_match() {
+        NodeResources resources = new NodeResources(3, 100, 100, 1);
+        ClusterResources min = new ClusterResources( 2, 1, new NodeResources(3, 100, 50, 1));
+        ClusterResources max = min;
+        // AutoscalingTester hardcodes 3Gb memory overhead:
+        Flavor localFlavor  = new Flavor("local",  new NodeResources(3, 97,  75, 1, DiskSpeed.fast, StorageType.local));
+        Flavor remoteFlavor = new Flavor("remote", new NodeResources(3, 97,  50, 1, DiskSpeed.fast, StorageType.remote));
+
+        var tester = new AutoscalingTester(new Zone(new Cloud.Builder().dynamicProvisioning(true).build(),
+                                                    SystemName.defaultSystem(), Environment.prod, RegionName.defaultName()),
+                                           List.of(localFlavor, remoteFlavor));
+        tester.provisioning().makeReadyNodes(5, localFlavor.name(),  NodeType.host, 8);
+        tester.provisioning().makeReadyNodes(5, remoteFlavor.name(), NodeType.host, 8);
+        tester.provisioning().activateTenantHosts();
+
+        ApplicationId application1 = tester.applicationId("application1");
+        ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.container, "cluster1");
+
+        // deploy
+        tester.deploy(application1, cluster1, 3, 1, min.nodeResources());
+        tester.addDiskMeasurements(0.01f, 1f, 120, application1);
+        tester.clock().advance(Duration.ofMinutes(-10 * 5));
+        tester.addQueryRateMeasurements(application1, cluster1.id(), 10, t -> 10.0); // Query traffic only
+        Optional<ClusterResources> suggestion = tester.suggest(application1, cluster1.id(), min, max).target();
+        tester.assertResources("Choosing the remote disk flavor as it has less disk",
+                               6, 1, 3.0,  100.0, 10.0,
+                               suggestion);
+        assertEquals("Choosing the remote disk flavor as it has less disk",
+                     StorageType.remote, suggestion.get().nodeResources().storageType());
+    }
+
+    @Test
     public void suggestions_ignores_limits() {
         NodeResources resources = new NodeResources(3, 100, 100, 1);
         ClusterResources min = new ClusterResources( 2, 1, new NodeResources(1, 1, 1, 1));
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 7a1c6152d03..1902d762fb4 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
@@ -162,6 +162,37 @@ class AutoscalingTester {
      * @param count the number of measurements
      * @param applicationId the application we're adding measurements for all nodes of
      */
+    public void addDiskMeasurements(float value, float otherResourcesLoad,
+                                    int count, ApplicationId applicationId) {
+        NodeList nodes = nodeRepository().nodes().list(Node.State.active).owner(applicationId);
+        float oneExtraNodeFactor = (float)(nodes.size() - 1.0) / (nodes.size());
+        for (int i = 0; i < count; i++) {
+            clock().advance(Duration.ofMinutes(5));
+            for (Node node : nodes) {
+                Load load = new Load(ClusterModel.idealQueryCpuLoad * otherResourcesLoad,
+                                     ClusterModel.idealDiskLoad * otherResourcesLoad,
+                                     value).multiply(oneExtraNodeFactor);
+                nodeMetricsDb().addNodeMetrics(List.of(new Pair<>(node.hostname(),
+                                                                  new NodeMetricSnapshot(clock().instant(),
+                                                                                         load,
+                                                                                         0,
+                                                                                         true,
+                                                                                         true,
+                                                                                         0.0))));
+            }
+        }
+    }
+
+    /**
+     * Adds measurements with the given resource value and ideal values for the other resources,
+     * scaled to take one node redundancy into account.
+     * (I.e we adjust to measure a bit lower load than "naively" wanted to offset for the autoscaler
+     * wanting to see the ideal load with one node missing.)
+     *
+     * @param otherResourcesLoad the load factor relative to ideal to use for other resources
+     * @param count the number of measurements
+     * @param applicationId the application we're adding measurements for all nodes of
+     */
     public void addMemMeasurements(float value, float otherResourcesLoad,
                                    int count, ApplicationId applicationId) {
         NodeList nodes = nodeRepository().nodes().list(Node.State.active).owner(applicationId);
@@ -302,7 +333,7 @@ class AutoscalingTester {
                                             double approxCpu, double approxMemory, double approxDisk,
                                             Optional<ClusterResources> resources) {
         double delta = 0.0000000001;
-        assertTrue(message, resources.isPresent());
+        assertTrue("Resources are present: " + message, resources.isPresent());
         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());