Preserve allocation when possible

We want to avoid unnecessary small adjustments to capacity.
Since the worst case real resources we'll get is generally worse
than the actual allocation, we cannot guarantee that we compute
the same allocation to request as the one leading to the current
allocation. So instead of relying on that we explicitly reuse
the current (requested) resources when they are still legal
instead of recomputing each time.

6 files changed, 206 insertions, 10 deletions

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 d2589b15421..e57011b0e4a 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
@@ -30,7 +30,7 @@ public class AllocationOptimizer {
     }
 
     /**
-     * An AllocationSearcher searches the space of possible allocations given a target
+     * Searches the space of possible allocations given a target
      * and (optionally) cluster limits and returns the best alternative.
      *
      * @return the best allocation, if there are any possible legal allocations, fulfilling the target
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceTarget.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceTarget.java
index f7c2a51436a..bf5e53d823b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceTarget.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/ResourceTarget.java
@@ -11,7 +11,7 @@ public class ResourceTarget {
 
     private final boolean adjustForRedundancy;
 
-    /** The target resources per node, assuming the node assignment where this was decided */
+    /** The target real resources per node, assuming the node assignment where this was decided */
     private final double cpu, memory, disk;
 
     private ResourceTarget(double cpu, double memory, double disk, boolean adjustForRedundancy) {
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 227e0744314..b246793096a 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
@@ -158,19 +158,26 @@ public class NodeRepositoryProvisioner implements Provisioner {
                                    .not().retired()
                                    .not().removable()
                                    .asList();
+        boolean firstDeployment = nodes.isEmpty();
         AllocatableClusterResources currentResources =
-                nodes.isEmpty() ? new AllocatableClusterResources(requested.minResources(),
-                                                                  clusterSpec.type(),
-                                                                  nodeRepository) // new deployment: Use min
-                                : new AllocatableClusterResources(nodes, nodeRepository);
-        return within(Limits.of(requested), clusterSpec.isExclusive(), currentResources);
+                firstDeployment // start at min, preserve current resources otherwise
+                ? new AllocatableClusterResources(requested.minResources(), clusterSpec.type(), nodeRepository)
+                : new AllocatableClusterResources(nodes, nodeRepository);
+        return within(Limits.of(requested), clusterSpec.isExclusive(), currentResources, firstDeployment);
     }
 
     /** Make the minimal adjustments needed to the current resources to stay within the limits */
-    private ClusterResources within(Limits limits, boolean exclusive, AllocatableClusterResources current) {
-        if (limits.isEmpty()) return current.toAdvertisedClusterResources();
+    private ClusterResources within(Limits limits,
+                                    boolean exclusive,
+                                    AllocatableClusterResources current,
+                                    boolean firstDeployment) {
         if (limits.min().equals(limits.max())) return limits.min();
 
+        // Don't change current deployments that are still legal
+        var currentAsAdvertised = current.toAdvertisedClusterResources();
+        if (! firstDeployment && currentAsAdvertised.isWithin(limits.min(), limits.max())) return currentAsAdvertised;
+
+        // Otherwise, find an allocation that preserves the current resources as well as possible
         return allocationOptimizer.findBestAllocation(ResourceTarget.preserve(current), current, limits, exclusive)
                                   .orElseThrow(() -> new IllegalArgumentException("No allocation possible within " + limits))
                                   .toAdvertisedClusterResources();
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningCompleteHostCalculatorTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningCompleteHostCalculatorTest.java
new file mode 100644
index 00000000000..24cdc5c8fd0
--- /dev/null
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DockerProvisioningCompleteHostCalculatorTest.java
@@ -0,0 +1,149 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.provision.provisioning;
+
+import com.yahoo.config.provision.ApplicationId;
+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.NodeResources;
+import com.yahoo.config.provision.RegionName;
+import com.yahoo.config.provision.Zone;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeRepository;
+import org.junit.Test;
+
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * @author bratseth
+ */
+public class DockerProvisioningCompleteHostCalculatorTest {
+
+    @Test
+    public void changing_to_different_range_preserves_allocation() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 1000, 4));
+        ProvisioningTester tester = new ProvisioningTester.Builder().zone(new Zone(Environment.prod, RegionName.from("us-east")))
+                                                                    .resourcesCalculator(new CompleteResourcesCalculator(hostFlavor))
+                                                                    .flavors(List.of(hostFlavor))
+                                                                    .build();
+        tester.makeReadyHosts(9, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        var initialResources = new NodeResources(2, 16, 50, 1);
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, initialResources),
+                                                      new ClusterResources(2, 1, initialResources)));
+        tester.assertNodes("Initial allocation",
+                           2, 1, 2, 16, 50, 1.0,
+                           app1, cluster1);
+
+        var newMinResources = new NodeResources(0.5,  4, 11, 1);
+        var newMaxResources = new NodeResources(2.0, 10, 30, 1);
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(7, 1, newMinResources),
+                                                      new ClusterResources(7, 1, newMaxResources)));
+        tester.assertNodes("New allocation preserves total resources",
+                           7, 1, 0.7, 4.6, 14.3, 1.0,
+                           app1, cluster1);
+
+        System.out.println("---------------------redeploying the same---------------------");
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(7, 1, newMinResources),
+                                                      new ClusterResources(7, 1, newMaxResources)));
+        tester.assertNodes("Redeploying the same ranges does not cause changes",
+                           7, 1, 0.7, 4.6, 14.3, 1.0,
+                           app1, cluster1);
+    }
+
+    @Test
+    public void testResourcesCalculator() {
+        Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 1000, 4));
+        var calculator = new CompleteResourcesCalculator(hostFlavor);
+        var originalReal = new NodeResources(0.7, 6.0, 12.9, 1.0);
+        var realToRequest = calculator.realToRequest(originalReal);
+        var requestToReal = calculator.requestToReal(realToRequest);
+        var realResourcesOf = calculator.realResourcesOf(realToRequest);
+        assertEquals(originalReal, requestToReal);
+        assertEquals(originalReal, realResourcesOf);
+    }
+
+    private static class CompleteResourcesCalculator implements HostResourcesCalculator {
+
+        private final Flavor hostFlavor; // Has the real resources
+        private final double memoryOverhead = 1;
+        private final double diskOverhead = 100;
+
+        public CompleteResourcesCalculator(Flavor hostFlavor) {
+            this.hostFlavor = hostFlavor;
+        }
+
+        @Override
+        public NodeResources realResourcesOf(Node node, NodeRepository nodeRepository) {
+            if (node.parentHostname().isEmpty()) return node.flavor().resources(); // hosts use configured flavors
+            return realResourcesOf(node.resources());
+        }
+
+        NodeResources realResourcesOf(NodeResources advertisedResources) {
+            var r = advertisedResources.withMemoryGb(advertisedResources.memoryGb() -
+                                                     memoryOverhead(advertisedResourcesOf(hostFlavor).memoryGb(), advertisedResources, false))
+                                       .withDiskGb(advertisedResources.diskGb() -
+                                                   diskOverhead(advertisedResourcesOf(hostFlavor).diskGb(), advertisedResources, false));
+            System.out.println("        real given " + advertisedResources + ": " + r);
+            System.out.println("        adv. given those: " + realToRequest(r));
+            return advertisedResources.withMemoryGb(advertisedResources.memoryGb() -
+                                                    memoryOverhead(advertisedResourcesOf(hostFlavor).memoryGb(), advertisedResources, false))
+                                      .withDiskGb(advertisedResources.diskGb() -
+                                                  diskOverhead(advertisedResourcesOf(hostFlavor).diskGb(), advertisedResources, false));
+        }
+
+        @Override
+        public NodeResources requestToReal(NodeResources advertisedResources) {
+            double memoryOverhead = memoryOverhead(advertisedResourcesOf(hostFlavor).memoryGb(), advertisedResources, false);
+            double diskOverhead = diskOverhead(advertisedResourcesOf(hostFlavor).diskGb(), advertisedResources, false);
+            return advertisedResources.withMemoryGb(advertisedResources.memoryGb() - memoryOverhead)
+                                      .withDiskGb(advertisedResources.diskGb() - diskOverhead);
+        }
+
+        @Override
+        public NodeResources advertisedResourcesOf(Flavor flavor) {
+            if ( ! flavor.equals(hostFlavor)) return flavor.resources(); // Node 'flavors' just wrap the advertised resources
+            return hostFlavor.resources().withMemoryGb(hostFlavor.resources().memoryGb() + memoryOverhead)
+                             .withDiskGb(hostFlavor.resources().diskGb() + diskOverhead);
+        }
+
+        @Override
+        public NodeResources realToRequest(NodeResources realResources) {
+            double memoryOverhead = memoryOverhead(advertisedResourcesOf(hostFlavor).memoryGb(), realResources, true);
+            double diskOverhead = diskOverhead(advertisedResourcesOf(hostFlavor).diskGb(), realResources, true);
+            return realResources.withMemoryGb(realResources.memoryGb() + memoryOverhead)
+                                .withDiskGb(realResources.diskGb() + diskOverhead);
+        }
+
+        /**
+         * Returns the memory overhead resulting if the given advertised resources are placed on the given node
+         *
+         * @param real true if the given resources are in real values, false if they are in advertised
+         */
+        private double memoryOverhead(double hostAdvertisedMemoryGb, NodeResources resources, boolean real) {
+            double memoryShare = resources.memoryGb() /
+                                 ( hostAdvertisedMemoryGb - (real ? memoryOverhead : 0));
+            return memoryOverhead * memoryShare;
+        }
+
+        /**
+         * Returns the disk overhead resulting if the given advertised resources are placed on the given node
+         *
+         * @param real true if the resources are in real values, false if they are in advertised
+         */
+        private double diskOverhead(double hostAdvertisedDiskGb, NodeResources resources, boolean real) {
+            double diskShare = resources.diskGb() /
+                               ( hostAdvertisedDiskGb - (real ? diskOverhead : 0) );
+            return diskOverhead * diskShare;
+        }
+
+    }
+
+}
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 0c5a682c3c5..cddc1fcb253 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
@@ -323,6 +323,40 @@ public class DockerProvisioningTest {
     }
 
     @Test
+    public void changing_to_different_range_preserves_allocation() {
+        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(9, hostFlavor.resources()).deployZoneApp();
+
+        ApplicationId app1 = tester.makeApplicationId("app1");
+        ClusterSpec cluster1 = ClusterSpec.request(ClusterSpec.Type.content, new ClusterSpec.Id("cluster1")).vespaVersion("7").build();
+
+        var initialResources = new NodeResources(2, 16, 50, 1);
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(2, 1, initialResources),
+                                                      new ClusterResources(2, 1, initialResources)));
+        tester.assertNodes("Initial allocation",
+                           2, 1, 2, 16, 50, 1.0,
+                           app1, cluster1);
+
+        var newMinResources = new NodeResources(0.5,  6, 11, 1);
+        var newMaxResources = new NodeResources(2.0, 10, 30, 1);
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(7, 1, newMinResources),
+                                                      new ClusterResources(7, 1, newMaxResources)));
+        tester.assertNodes("New allocation preserves total resources",
+                           7, 1, 0.7, 6.7, 14.3, 1.0,
+                           app1, cluster1);
+
+        tester.activate(app1, cluster1, Capacity.from(new ClusterResources(7, 1, newMinResources),
+                                                      new ClusterResources(7, 1, newMaxResources)));
+        tester.assertNodes("Redeploying does not cause changes",
+                           7, 1, 0.7, 6.7, 14.3, 1.0,
+                           app1, cluster1);
+    }
+
+    @Test
     public void too_few_real_resources_causes_failure() {
         try {
             Flavor hostFlavor = new Flavor(new NodeResources(20, 40, 100, 4));
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 b814bec683a..d56cae799b2 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
@@ -266,7 +266,13 @@ public class ProvisioningTester {
         for (Node node : nodeList) {
             var expected = new NodeResources(vcpu, memory, disk, bandwidth, diskSpeed, storageType);
             assertTrue(explanation + ": Resources: Expected " + expected + " but was " + node.resources(),
-                       expected.compatibleWith(node.resources()));
+                       expected.justNonNumbers().compatibleWith(node.resources().justNonNumbers()));
+            assertEquals(explanation + ": Vcpu: Expected " + expected.vcpu() + " but was " + node.resources().vcpu(),
+                         expected.vcpu(), node.resources().vcpu(), 0.05);
+            assertEquals(explanation + ": Memory: Expected " + expected.memoryGb() + " but was " + node.resources().memoryGb(),
+                         expected.memoryGb(), node.resources().memoryGb(), 0.05);
+            assertEquals(explanation + ": Disk: Expected " + expected.diskGb() + " but was " + node.resources().diskGb(),
+                         expected.diskGb(), node.resources().diskGb(), 0.05);
         }
     }