Suggest only when useful

- Closes a loophole where the suggestion made will be lower than the current allocation
  if the current allocation is the max need observed over the last week.
- Since we now store the suggestion even if it is current we check at read time
  whether to suggest, and then also refrain from making suggestions inside
  the autoscaling interval.

11 files changed, 82 insertions, 47 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Cluster.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Cluster.java
index 6477b9b1cd0..b16859fa6fb 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Cluster.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Cluster.java
@@ -3,6 +3,7 @@ package com.yahoo.vespa.hosted.provision.applications;
 
 import com.yahoo.config.provision.ClusterResources;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.vespa.hosted.provision.autoscale.Autoscaler;
 
 import java.time.Instant;
 import java.util.ArrayList;
@@ -73,10 +74,18 @@ public class Cluster {
 
     /**
      * The suggested size of this cluster, which may or may not be within the min and max limits,
-     * or empty if there is currently no suggestion.
+     * or empty if there is currently no recorded suggestion.
      */
     public Optional<Suggestion> suggestedResources() { return suggested; }
 
+    /** Returns true if there is a current suggestion and we should actually make this suggestion to users. */
+    public boolean shouldSuggestResources(ClusterResources currentResources) {
+        if (suggested.isEmpty()) return false;
+        if (suggested.get().resources().isWithin(min, max)) return false;
+        if (Autoscaler.similar(suggested.get().resources(), currentResources)) return false;
+        return true;
+    }
+
     /** Returns the recent scaling events in this cluster */
     public List<ScalingEvent> scalingEvents() { return scalingEvents; }
 
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 8f28e307c5d..ab8b954610a 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
@@ -12,7 +12,6 @@ import com.yahoo.vespa.hosted.provision.provisioning.NodeResourceLimits;
 
 import java.util.List;
 import java.util.Optional;
-import java.util.stream.Collectors;
 
 /**
  * @author bratseth
@@ -69,15 +68,15 @@ public class AllocatableClusterResources {
      * Returns the resources which will actually be available per node in this cluster with this allocation.
      * These should be used for reasoning about allocation to meet measured demand.
      */
-    public NodeResources realResources() { return realResources; }
+    public ClusterResources realResources() {
+        return new ClusterResources(nodes, groups, realResources);
+    }
 
     /**
      * Returns the resources advertised by the cloud provider, which are the basis for charging
      * and which must be used in resource allocation requests
      */
-    public NodeResources advertisedResources() { return advertisedResources; }
-
-    public ClusterResources toAdvertisedClusterResources() {
+    public ClusterResources advertisedResources() {
         return new ClusterResources(nodes, groups, advertisedResources);
     }
 
@@ -115,9 +114,7 @@ public class AllocatableClusterResources {
 
     @Override
     public String toString() {
-        return nodes + " nodes " +
-               ( groups > 1 ? "(in " + groups + " groups) " : "" ) +
-               "with " + advertisedResources() +
+        return advertisedResources() +
                " at cost $" + cost() +
                (fulfilment < 1.0 ? " (fulfilment " + fulfilment + ")" : "");
     }
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 1ec2b841790..bfb3bfeb480 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
@@ -98,7 +98,7 @@ public class AllocationOptimizer {
         // Combine the scaled resource values computed here
         // with the currently configured non-scaled values, given in the limits, if any
         NodeResources nonScaled = limits.isEmpty() || limits.min().nodeResources().isUnspecified()
-                                  ? current.toAdvertisedClusterResources().nodeResources()
+                                  ? current.advertisedResources().nodeResources()
                                   : limits.min().nodeResources(); // min=max for non-scaled
         return nonScaled.withVcpu(cpu).withMemoryGb(memory).withDiskGb(disk);
     }
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 445f3f7746c..c4ff86a5390 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
@@ -94,34 +94,31 @@ public class Autoscaler {
         if (bestAllocation.isEmpty())
             return Advice.dontScale("No allocation changes are possible within configured limits");
 
-        if (similar(bestAllocation.get(), currentAllocation))
+        if (similar(bestAllocation.get().realResources(), currentAllocation.realResources()))
             return Advice.dontScale("Cluster is ideally scaled within configured limits");
 
         if (isDownscaling(bestAllocation.get(), currentAllocation) && scaledIn(scalingWindow.multipliedBy(3), cluster))
             return Advice.dontScale("Waiting " + scalingWindow.multipliedBy(3) + " since last rescaling before reducing resources");
 
-        return Advice.scaleTo(bestAllocation.get().toAdvertisedClusterResources());
+        return Advice.scaleTo(bestAllocation.get().advertisedResources());
     }
 
     /** Returns true if both total real resources and total cost are similar */
-    private boolean similar(AllocatableClusterResources a, AllocatableClusterResources b) {
+    public static boolean similar(ClusterResources a, ClusterResources b) {
         return similar(a.cost(), b.cost(), costDifferenceWorthReallocation) &&
-               similar(a.realResources().vcpu() * a.nodes(),
-                       b.realResources().vcpu() * b.nodes(), resourceDifferenceWorthReallocation) &&
-               similar(a.realResources().memoryGb() * a.nodes(),
-                       b.realResources().memoryGb() * b.nodes(), resourceDifferenceWorthReallocation) &&
-               similar(a.realResources().diskGb() * a.nodes(),
-                       b.realResources().diskGb() * b.nodes(), resourceDifferenceWorthReallocation);
+               similar(a.totalResources().vcpu(), b.totalResources().vcpu(), resourceDifferenceWorthReallocation) &&
+               similar(a.totalResources().memoryGb(), b.totalResources().memoryGb(), resourceDifferenceWorthReallocation) &&
+               similar(a.totalResources().diskGb(), b.totalResources().diskGb(), resourceDifferenceWorthReallocation);
     }
 
-    private boolean similar(double r1, double r2, double threshold) {
+    private static boolean similar(double r1, double r2, double threshold) {
         return Math.abs(r1 - r2) / (( r1 + r2) / 2) < threshold;
     }
 
     /** Returns true if this reduces total resources in any dimension */
     private boolean isDownscaling(AllocatableClusterResources target, AllocatableClusterResources current) {
-        NodeResources targetTotal = target.toAdvertisedClusterResources().totalResources();
-        NodeResources currentTotal = current.toAdvertisedClusterResources().totalResources();
+        NodeResources targetTotal = target.advertisedResources().totalResources();
+        NodeResources currentTotal = current.advertisedResources().totalResources();
         return ! targetTotal.justNumbers().satisfies(currentTotal.justNumbers());
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Resource.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Resource.java
index b6d75165340..ddfb4c48e84 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Resource.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/autoscale/Resource.java
@@ -12,24 +12,24 @@ public enum Resource {
 
     /** Cpu utilization ratio */
     cpu {
-        double idealAverageLoad() { return 0.2; }
+        public double idealAverageLoad() { return 0.2; }
         double valueFrom(NodeResources resources) { return resources.vcpu(); }
     },
 
     /** Memory utilization ratio */
     memory {
-        double idealAverageLoad() { return 0.7; }
+        public double idealAverageLoad() { return 0.7; }
         double valueFrom(NodeResources resources) { return resources.memoryGb(); }
     },
 
     /** Disk utilization ratio */
     disk {
-        double idealAverageLoad() { return 0.6; }
+        public double idealAverageLoad() { return 0.6; }
         double valueFrom(NodeResources resources) { return resources.diskGb(); }
     };
 
     /** The load we should have of this resource on average, when one node in the cluster is down */
-    abstract double idealAverageLoad();
+    public abstract double idealAverageLoad();
 
     abstract double valueFrom(NodeResources resources);
 
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 bf5e53d823b..b00323818d5 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
@@ -41,7 +41,7 @@ public class ResourceTarget {
     }
 
     private static double nodeUsage(Resource resource, double load, AllocatableClusterResources current) {
-        return load * resource.valueFrom(current.realResources());
+        return load * resource.valueFrom(current.realResources().nodeResources());
     }
 
     /** Create a target of achieving ideal load given a current load */
@@ -55,9 +55,9 @@ public class ResourceTarget {
 
     /** Crete a target of preserving a current allocation */
     public static ResourceTarget preserve(AllocatableClusterResources current) {
-        return new ResourceTarget(current.realResources().vcpu(),
-                                  current.realResources().memoryGb(),
-                                  current.realResources().diskGb(),
+        return new ResourceTarget(current.realResources().nodeResources().vcpu(),
+                                  current.realResources().nodeResources().memoryGb(),
+                                  current.realResources().nodeResources().diskGb(),
                                   false);
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
index c4744f6cb6a..8eb92217356 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
@@ -18,7 +18,6 @@ import com.yahoo.vespa.hosted.provision.autoscale.MetricSnapshot;
 import com.yahoo.vespa.hosted.provision.autoscale.MetricsDb;
 import com.yahoo.vespa.hosted.provision.autoscale.NodeTimeseries;
 import com.yahoo.vespa.hosted.provision.node.History;
-import com.yahoo.vespa.orchestrator.status.ApplicationLock;
 
 import java.time.Duration;
 import java.time.Instant;
@@ -126,7 +125,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer {
                                 ApplicationId application,
                                 Cluster cluster,
                                 NodeList clusterNodes) {
-        ClusterResources current = new AllocatableClusterResources(clusterNodes.asList(), nodeRepository(), cluster.exclusive()).toAdvertisedClusterResources();
+        ClusterResources current = new AllocatableClusterResources(clusterNodes.asList(), nodeRepository(), cluster.exclusive()).advertisedResources();
         log.info("Autoscaling " + application + " " + clusterNodes.clusterSpec() + ":" +
                  "\nfrom " + toString(current) + "\nto   " + toString(target));
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java
index be8966068c9..3bbebd7798d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainer.java
@@ -70,7 +70,9 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         if (suggestion.isEmpty()) return false;
         // Wait only a short time for the lock to avoid interfering with change deployments
         try (Mutex lock = nodeRepository().lock(applicationId, Duration.ofSeconds(1))) {
-            applications().get(applicationId).ifPresent(a -> updateSuggestion(suggestion.target(), clusterId, a, lock));
+            // empty suggested resources == keep the current allocation, so we record that
+            var suggestedResources = suggestion.target().orElse(clusterNodes.not().retired().toResources());
+            applications().get(applicationId).ifPresent(a -> updateSuggestion(suggestedResources, clusterId, a, lock));
             return true;
         }
         catch (ApplicationLockException e) {
@@ -78,7 +80,7 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         }
     }
 
-    private void updateSuggestion(Optional<ClusterResources> suggestion,
+    private void updateSuggestion(ClusterResources suggestion,
                                   ClusterSpec.Id clusterId,
                                   Application application,
                                   Mutex lock) {
@@ -88,8 +90,8 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         var currentSuggestion = cluster.get().suggestedResources();
         if (currentSuggestion.isEmpty()
             || currentSuggestion.get().at().isBefore(at.minus(Duration.ofDays(7)))
-            || suggestion.isPresent() && isHigher(suggestion.get(), currentSuggestion.get().resources()))
-            applications().put(application.with(cluster.get().withSuggested(suggestion.map(s -> new Cluster.Suggestion(s,  at)))), lock);
+            || isHigher(suggestion, currentSuggestion.get().resources()))
+            applications().put(application.with(cluster.get().withSuggested(Optional.of(new Cluster.Suggestion(suggestion,  at)))), lock);
     }
 
     private boolean isHigher(ClusterResources r1, ClusterResources r2) {
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 33f011f65e2..96bce8b71d4 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
@@ -179,13 +179,13 @@ public class NodeRepositoryProvisioner implements Provisioner {
         if (limits.min().equals(limits.max())) return limits.min();
 
         // Don't change current deployments that are still legal
-        var currentAsAdvertised = current.toAdvertisedClusterResources();
+        var currentAsAdvertised = current.advertisedResources();
         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)
                                   .orElseThrow(() -> new IllegalArgumentException("No allocation possible within " + limits))
-                                  .toAdvertisedClusterResources();
+                                  .advertisedResources();
     }
 
     private void logIfDownscaled(int targetNodes, int actualNodes, ClusterSpec cluster, ProvisionLogger logger) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationSerializer.java
index 0530e0cc9b6..f2be6308fd3 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/ApplicationSerializer.java
@@ -40,17 +40,16 @@ public class ApplicationSerializer {
     }
 
     private static void toSlime(Cluster cluster, List<Node> applicationNodes, Cursor clustersObject) {
-        List<Node> nodes = NodeList.copyOf(applicationNodes).not().retired().cluster(cluster.id()).asList();
+        NodeList nodes = NodeList.copyOf(applicationNodes).not().retired().cluster(cluster.id());
         if (nodes.isEmpty()) return;
-
-        int groups = (int)nodes.stream().map(node -> node.allocation().get().membership().cluster().group()).distinct().count();
-        ClusterResources currentResources = new ClusterResources(nodes.size(), groups, nodes.get(0).resources());
+        ClusterResources currentResources = nodes.toResources();
 
         Cursor clusterObject = clustersObject.setObject(cluster.id().value());
         toSlime(cluster.minResources(), clusterObject.setObject("min"));
         toSlime(cluster.maxResources(), clusterObject.setObject("max"));
         toSlime(currentResources, clusterObject.setObject("current"));
-        cluster.suggestedResources().ifPresent(suggested -> toSlime(suggested.resources(), clusterObject.setObject("suggested")));
+        if (cluster.shouldSuggestResources(currentResources))
+            cluster.suggestedResources().ifPresent(suggested -> toSlime(suggested.resources(), clusterObject.setObject("suggested")));
         cluster.targetResources().ifPresent(target -> toSlime(target, clusterObject.setObject("target")));
         scalingEventsToSlime(cluster.scalingEvents(), clusterObject.setArray("scalingEvents"));
         clusterObject.setString("autoscalingStatus", cluster.autoscalingStatus());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainerTest.java
index 5ec29cb9043..aac9122f8ab 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ScalingSuggestionsMaintainerTest.java
@@ -15,16 +15,21 @@ import com.yahoo.config.provision.Zone;
 import com.yahoo.config.provisioning.FlavorsConfig;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
+import com.yahoo.vespa.hosted.provision.applications.Cluster;
 import com.yahoo.vespa.hosted.provision.autoscale.MetricSnapshot;
 import com.yahoo.vespa.hosted.provision.autoscale.MetricsDb;
+import com.yahoo.vespa.hosted.provision.autoscale.Resource;
 import com.yahoo.vespa.hosted.provision.provisioning.FlavorConfigBuilder;
 import com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester;
 import org.junit.Test;
 
 import java.time.Duration;
 import java.util.List;
+import java.util.Optional;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 /**
  * Tests the scaling suggestions maintainer integration.
@@ -67,9 +72,9 @@ public class ScalingSuggestionsMaintainerTest {
         maintainer.maintain();
 
         assertEquals("14 nodes with [vcpu: 6.9, memory: 5.1 Gb, disk 15.0 Gb, bandwidth: 0.1 Gbps, storage type: remote]",
-                     tester.nodeRepository().applications().get(app1).get().cluster(cluster1.id()).get().suggestedResources().get().resources().toString());
+                     suggestionOf(app1, cluster1, tester).get().resources().toString());
         assertEquals("8 nodes with [vcpu: 14.7, memory: 4.0 Gb, disk 11.8 Gb, bandwidth: 0.1 Gbps, storage type: remote]",
-                     tester.nodeRepository().applications().get(app2).get().cluster(cluster2.id()).get().suggestedResources().get().resources().toString());
+                     suggestionOf(app2, cluster2, tester).get().resources().toString());
 
         // Utilization goes way down
         tester.clock().advance(Duration.ofHours(13));
@@ -77,14 +82,41 @@ public class ScalingSuggestionsMaintainerTest {
         maintainer.maintain();
         assertEquals("Suggestion stays at the peak value observed",
                      "14 nodes with [vcpu: 6.9, memory: 5.1 Gb, disk 15.0 Gb, bandwidth: 0.1 Gbps, storage type: remote]",
-                     tester.nodeRepository().applications().get(app1).get().cluster(cluster1.id()).get().suggestedResources().get().resources().toString());
+                     suggestionOf(app1, cluster1, tester).get().resources().toString());
         // Utilization is still way down and a week has passed
         tester.clock().advance(Duration.ofDays(7));
         addMeasurements(0.10f, 0.10f, 0.10f, 0, 500, app1, tester.nodeRepository(), metricsDb);
         maintainer.maintain();
         assertEquals("Peak suggestion has been  outdated",
                      "6 nodes with [vcpu: 2.0, memory: 4.0 Gb, disk 10.0 Gb, bandwidth: 0.1 Gbps, storage type: remote]",
-                     tester.nodeRepository().applications().get(app1).get().cluster(cluster1.id()).get().suggestedResources().get().resources().toString());
+                     suggestionOf(app1, cluster1, tester).get().resources().toString());
+        assertTrue(shouldSuggest(app1, cluster1, tester));
+
+        tester.clock().advance(Duration.ofDays(3));
+        addMeasurements(0.7f, 0.7f, 0.7f, 0, 500, app1, tester.nodeRepository(), metricsDb);
+        maintainer.maintain();
+        var suggested = tester.nodeRepository().applications().get(app1).get().cluster(cluster1.id()).get().suggestedResources().get().resources();
+        tester.deploy(app1, cluster1, Capacity.from(suggested, suggested, false, true));
+        tester.clock().advance(Duration.ofDays(2));
+        addMeasurements((float)Resource.cpu.idealAverageLoad(),
+                        (float)Resource.memory.idealAverageLoad(),
+                        (float)Resource.disk.idealAverageLoad(),
+                        0, 500, app1, tester.nodeRepository(), metricsDb);
+        maintainer.maintain();
+        assertEquals("Suggestion is to keep the current allocation",
+                     suggested,
+                     suggestionOf(app1, cluster1, tester).get().resources());
+        assertFalse("Suggestion is not made as it matches what we have", shouldSuggest(app1, cluster1, tester));
+    }
+
+    private Optional<Cluster.Suggestion> suggestionOf(ApplicationId app, ClusterSpec cluster, ProvisioningTester tester) {
+        return tester.nodeRepository().applications().get(app).get().cluster(cluster.id()).get().suggestedResources();
+    }
+
+    private boolean shouldSuggest(ApplicationId app, ClusterSpec cluster, ProvisioningTester tester) {
+        var currentResources = tester.nodeRepository().list(app).cluster(cluster.id()).not().retired().toResources();
+        return tester.nodeRepository().applications().get(app).get().cluster(cluster.id()).get()
+                     .shouldSuggestResources(currentResources);
     }
 
     public void addMeasurements(float cpu, float memory, float disk, int generation, int count, ApplicationId applicationId,