Merge pull request #12823 from vespa-engine/bratseth/preserve-disk-settings

Bratseth/preserve disk settings

10 files changed, 95 insertions, 13 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
index 6a52ff4f051..ad6eebe1ca5 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
@@ -60,6 +60,18 @@ public class NodesSpecification {
                                boolean required, boolean canFail, boolean exclusive,
                                Optional<String> dockerImageRepo,
                                Optional<String> combinedId) {
+        if (max.smallerThan(min))
+            throw new IllegalArgumentException("Min resources must be larger or equal to max resources, but " +
+                                               max + " is smaller than " + min);
+
+        // Non-scaled resources must be equal
+        if ( ! min.nodeResources().justNonNumbers().equals(max.nodeResources().justNonNumbers()))
+            throw new IllegalArgumentException("Min and max resources must have the same non-numeric settings, but " +
+                                               "min is " + min + " and max " + max);
+        if (min.nodeResources().bandwidthGbps() != max.nodeResources().bandwidthGbps())
+            throw new IllegalArgumentException("Min and max resources must have the same bandwith, but " +
+                                               "min is " + min + " and max " + max);
+
         this.min = min;
         this.max = max;
         this.dedicated = dedicated;
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterResources.java b/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterResources.java
index a4ed22c5266..87b5133d4eb 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterResources.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/ClusterResources.java
@@ -44,10 +44,12 @@ public class ClusterResources {
         return false;
     }
 
-    /** Returns true if this is within the given limits (inclusive) */
+    /** Returns true if this is within the given limits (inclusive) and is compatible with them */
     public boolean isWithin(ClusterResources min, ClusterResources max) {
         if (this.smallerThan(min)) return false;
         if (max.smallerThan(this)) return false;
+        if ( ! this.nodeResources.justNonNumbers().compatibleWith(min.nodeResources.justNonNumbers())) return false;
+        if ( ! this.nodeResources.justNonNumbers().compatibleWith(max.nodeResources.justNonNumbers())) return false;
         return true;
     }
 
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/NodeResources.java b/config-provisioning/src/main/java/com/yahoo/config/provision/NodeResources.java
index 5fc05a87a7d..05b604b263f 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/NodeResources.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/NodeResources.java
@@ -148,6 +148,11 @@ public class NodeResources {
         return with(NodeResources.DiskSpeed.any).with(StorageType.any);
     }
 
+    /** Returns this with all numbers set to 0 */
+    public NodeResources justNonNumbers() {
+        return withVcpu(0).withMemoryGb(0).withDiskGb(0).withBandwidthGbps(0);
+    }
+
     public NodeResources subtract(NodeResources other) {
         if ( ! this.isInterchangeableWith(other))
             throw new IllegalArgumentException(this + " and " + other + " are not interchangeable");
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
index bdae658f76e..93e5b160524 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java
@@ -50,6 +50,11 @@ public class NodeList implements Iterable<Node> {
         return filter(node -> node.allocation().get().membership().retired());
     }
 
+    /** Returns the subset of nodes which are removable */
+    public NodeList removable() {
+        return filter(node -> node.allocation().get().isRemovable());
+    }
+
     /** Returns the subset of nodes having exactly the given resources */
     public NodeList resources(NodeResources resources) { return filter(node -> node.flavor().resources().equals(resources)); }
 
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 bc14ca1779c..b7d5995884e 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
@@ -134,7 +134,12 @@ public class ResourceIterator {
             }
         }
 
-        return allocation.realResources().withVcpu(cpu).withMemoryGb(memory).withDiskGb(disk);
+        // Combine the scaled resource values computed here
+        // and the currently combined values of non-scaled resources
+        return new NodeResources(cpu, memory, disk,
+                                 cluster.minResources().nodeResources().bandwidthGbps(),
+                                 cluster.minResources().nodeResources().diskSpeed(),
+                                 cluster.minResources().nodeResources().storageType());
     }
 
     private double clusterUsage(Resource resource, double load) {
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 c92f7889496..d96282f1722 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
@@ -100,7 +100,6 @@ class NodeAllocation {
         List<Node> accepted = new ArrayList<>();
         for (PrioritizableNode node : nodesPrioritized) {
             Node offered = node.node;
-
             if (offered.allocation().isPresent()) {
                 ClusterMembership membership = offered.allocation().get().membership();
                 if ( ! offered.allocation().get().owner().equals(application)) continue; // wrong application
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 d03aa0cac91..37d0e9bbfb8 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
@@ -161,12 +161,22 @@ public class NodeRepositoryProvisioner implements Provisioner {
                                                         ClusterSpec.Id clusterId,
                                                         Capacity requested) {
         List<Node> nodes = NodeList.copyOf(nodeRepository.getNodes(applicationId, Node.State.active))
-                                   .cluster(clusterId).not().retired().asList();
-        if (nodes.size() < 1) return Optional.empty();
+                                   .cluster(clusterId)
+                                   .not().retired()
+                                   .not().removable()
+                                   .asList();
+        if (nodes.isEmpty()) return Optional.empty();
         long groups = nodes.stream().map(node -> node.allocation().get().membership().cluster().group()).distinct().count();
-        var resources = new ClusterResources(nodes.size(), (int)groups, nodes.get(0).flavor().resources());
-        if ( ! resources.isWithin(requested.minResources(), requested.maxResources())) return Optional.empty();
-        return Optional.of(resources);
+
+        // To allow non-numeric settings to be updated without resetting to the min target, we need to use
+        // the non-numeric settings of the current min limit with the current numeric settings
+        NodeResources nodeResources = nodes.get(0).allocation().get().requestedResources()
+                                           .with(requested.minResources().nodeResources().diskSpeed())
+                                           .with(requested.maxResources().nodeResources().storageType());
+        var currentResources = new ClusterResources(nodes.size(), (int)groups, nodeResources);
+        if ( ! currentResources.isWithin(requested.minResources(), requested.maxResources())) return Optional.empty();
+
+        return Optional.of(currentResources);
     }
 
     private void logIfDownscaled(int targetNodes, int actualNodes, ClusterSpec cluster, ProvisionLogger logger) {
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 8bfb17c0bd4..497a2a31ce5 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
@@ -31,16 +31,18 @@ public class AutoscalingTest {
 
     @Test
     public void testAutoscalingSingleContentGroup() {
-        NodeResources resources = 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);
+        NodeResources hostResources = new NodeResources(3, 100, 100, 1);
+        ClusterResources min = new ClusterResources( 2, 1,
+                                                     new NodeResources(1, 1, 1, 1, NodeResources.DiskSpeed.any));
+        ClusterResources max = new ClusterResources(20, 1,
+                                                    new NodeResources(100, 1000, 1000, 1, NodeResources.DiskSpeed.any));
+        AutoscalingTester tester = new AutoscalingTester(hostResources);
 
         ApplicationId application1 = tester.applicationId("application1");
         ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.content, "cluster1");
 
         // deploy
-        tester.deploy(application1, cluster1, 5, 1, resources);
+        tester.deploy(application1, cluster1, 5, 1, hostResources);
 
         assertTrue("No measurements -> No change", tester.autoscale(application1, cluster1.id(), min, max).isEmpty());
 
@@ -69,6 +71,35 @@ public class AutoscalingTest {
                                tester.autoscale(application1, cluster1.id(), min, max));
     }
 
+    @Test
+    public void testAutoscalingHandlesDiskSettingChanges() {
+        NodeResources hostResources = new NodeResources(3, 100, 100, 1, NodeResources.DiskSpeed.slow);
+        AutoscalingTester tester = new AutoscalingTester(hostResources);
+
+        ApplicationId application1 = tester.applicationId("application1");
+        ClusterSpec cluster1 = tester.clusterSpec(ClusterSpec.Type.content, "cluster1");
+
+        // deploy with slow
+        tester.deploy(application1, cluster1, 5, 1, hostResources);
+        tester.nodeRepository().getNodes(application1).stream()
+              .allMatch(n -> n.allocation().get().requestedResources().diskSpeed() == NodeResources.DiskSpeed.slow);
+
+        tester.addMeasurements(Resource.cpu, 0.25f, 1f, 120, application1);
+        // Changing min and max from slow to any
+        ClusterResources min = new ClusterResources( 2, 1,
+                                                     new NodeResources(1, 1, 1, 1, NodeResources.DiskSpeed.any));
+        ClusterResources max = new ClusterResources(20, 1,
+                                                    new NodeResources(100, 1000, 1000, 1, NodeResources.DiskSpeed.any));
+        AllocatableClusterResources scaledResources = tester.assertResources("Scaling up since resource usage is too high",
+                                                                             15, 1, 1.3,  28.6, 28.6,
+                                                                             tester.autoscale(application1, cluster1.id(), min, max));
+        assertEquals("Disk speed from min/max is used",
+                     NodeResources.DiskSpeed.any, scaledResources.realResources().diskSpeed());
+        tester.deploy(application1, cluster1, scaledResources);
+        tester.nodeRepository().getNodes(application1).stream()
+              .allMatch(n -> n.allocation().get().requestedResources().diskSpeed() == NodeResources.DiskSpeed.any);
+    }
+
     /** We prefer fewer nodes for container clusters as (we assume) they all use the same disk and memory */
     @Test
     public void testAutoscalingSingleContainerGroup() {
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 ebc4d158ded..aed262b6c96 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
@@ -206,6 +206,8 @@ class AutoscalingTester {
         flavor.minMainMemoryAvailableGb(resources.memoryGb());
         flavor.minDiskAvailableGb(resources.diskGb());
         flavor.bandwidth(resources.bandwidthGbps() * 1000);
+        flavor.fastDisk(resources.diskSpeed().compatibleWith(NodeResources.DiskSpeed.fast));
+        flavor.remoteStorage(resources.storageType().compatibleWith(NodeResources.StorageType.remote));
         return flavor;
     }
 
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 bd8be5063fd..8fc73d68785 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
@@ -354,6 +354,17 @@ public class ProvisioningTest {
             assertTrue(state.allHosts.stream().allMatch(host -> host.requestedResources().get().diskSpeed() == NodeResources.DiskSpeed.fast));
             assertTrue(tester.nodeRepository().getNodes(application).stream().allMatch(node -> node.allocation().get().requestedResources().diskSpeed() == NodeResources.DiskSpeed.fast));
         }
+
+        {
+            // Go back to any
+            SystemState state = prepare(application, 0, 0, 5, 3,
+                                        defaultResources.justNumbers(),
+                                        tester);
+            assertEquals(8, state.allHosts.size());
+            tester.activate(application, state.allHosts);
+            assertTrue(state.allHosts.stream().allMatch(host -> host.requestedResources().get().diskSpeed() == NodeResources.DiskSpeed.any));
+            assertTrue(tester.nodeRepository().getNodes(application).stream().allMatch(node -> node.allocation().get().requestedResources().diskSpeed() == NodeResources.DiskSpeed.any));
+        }
     }
 
     @Test