Merge pull request #16081 from vespa-engine/bratseth/suggest-on-human-scale

Bratseth/suggest on human scale

7 files changed, 100 insertions, 19 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 92bc62229ed..6477b9b1cd0 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
@@ -24,7 +24,7 @@ public class Cluster {
     private final ClusterSpec.Id id;
     private final boolean exclusive;
     private final ClusterResources min, max;
-    private final Optional<ClusterResources> suggested;
+    private final Optional<Suggestion> suggested;
     private final Optional<ClusterResources> target;
 
     /** The maxScalingEvents last scaling events of this, sorted by increasing time (newest last) */
@@ -35,7 +35,7 @@ public class Cluster {
                    boolean exclusive,
                    ClusterResources minResources,
                    ClusterResources maxResources,
-                   Optional<ClusterResources> suggestedResources,
+                   Optional<Suggestion> suggestedResources,
                    Optional<ClusterResources> targetResources,
                    List<ScalingEvent> scalingEvents,
                    String autoscalingStatus) {
@@ -75,7 +75,7 @@ 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.
      */
-    public Optional<ClusterResources> suggestedResources() { return suggested; }
+    public Optional<Suggestion> suggestedResources() { return suggested; }
 
     /** Returns the recent scaling events in this cluster */
     public List<ScalingEvent> scalingEvents() { return scalingEvents; }
@@ -92,7 +92,7 @@ public class Cluster {
         return new Cluster(id, exclusive, min, max, suggested, target, scalingEvents, autoscalingStatus);
     }
 
-    public Cluster withSuggested(Optional<ClusterResources> suggested) {
+    public Cluster withSuggested(Optional<Suggestion> suggested) {
         return new Cluster(id, exclusive, min, max, suggested, target, scalingEvents, autoscalingStatus);
     }
 
@@ -146,4 +146,41 @@ public class Cluster {
         return -1;
     }
 
+    public static class Suggestion {
+
+        private final ClusterResources resources;
+        private final Instant at;
+
+        public Suggestion(ClusterResources resources, Instant at) {
+            this.resources = resources;
+            this.at = at;
+        }
+
+        /** Returns the suggested resources */
+        public ClusterResources resources() { return resources; }
+
+        /** Returns the instant this suggestion was made */
+        public Instant at() { return at; }
+
+        @Override
+        public String toString() {
+            return "suggestion made at " + at + ": " + resources;
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(resources, at);
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if ( ! (o instanceof Suggestion)) return false;
+            Suggestion other = (Suggestion)o;
+            if ( ! this.at.equals(other.at)) return false;
+            if ( ! this.resources.equals(other.resources)) return false;
+            return true;
+        }
+
+    }
+
 }
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 7cb0270636f..be8966068c9 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,7 @@ 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 -> storeSuggestion(suggestion.target(), clusterId, a, lock));
+            applications().get(applicationId).ifPresent(a -> updateSuggestion(suggestion.target(), clusterId, a, lock));
             return true;
         }
         catch (ApplicationLockException e) {
@@ -78,13 +78,23 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         }
     }
 
-    private void storeSuggestion(Optional<ClusterResources> suggestion,
-                                 ClusterSpec.Id clusterId,
-                                 Application application,
-                                 Mutex lock) {
+    private void updateSuggestion(Optional<ClusterResources> suggestion,
+                                  ClusterSpec.Id clusterId,
+                                  Application application,
+                                  Mutex lock) {
         Optional<Cluster> cluster = application.cluster(clusterId);
         if (cluster.isEmpty()) return;
-        applications().put(application.with(cluster.get().withSuggested(suggestion)), lock);
+        var at = nodeRepository().clock().instant();
+        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);
+    }
+
+    private boolean isHigher(ClusterResources r1, ClusterResources r2) {
+        // Use cost as a measure of "highness" over multiple dimensions
+        return r1.totalResources().cost() > r2.totalResources().cost();
     }
 
     private Map<ClusterSpec.Id, List<Node>> nodesByCluster(List<Node> applicationNodes) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializer.java
index 4b9b14656ca..dd1c9028afe 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializer.java
@@ -41,7 +41,8 @@ public class ApplicationSerializer {
     private static final String exclusiveKey = "exclusive";
     private static final String minResourcesKey = "min";
     private static final String maxResourcesKey = "max";
-    private static final String suggestedResourcesKey = "suggested";
+    private static final String suggestedKey = "suggested";
+    private static final String resourcesKey = "resources";
     private static final String targetResourcesKey = "target";
     private static final String nodesKey = "nodes";
     private static final String groupsKey = "groups";
@@ -94,7 +95,7 @@ public class ApplicationSerializer {
         clusterObject.setBool(exclusiveKey, cluster.exclusive());
         toSlime(cluster.minResources(), clusterObject.setObject(minResourcesKey));
         toSlime(cluster.maxResources(), clusterObject.setObject(maxResourcesKey));
-        cluster.suggestedResources().ifPresent(suggested -> toSlime(suggested, clusterObject.setObject(suggestedResourcesKey)));
+        cluster.suggestedResources().ifPresent(suggested -> toSlime(suggested, clusterObject.setObject(suggestedKey)));
         cluster.targetResources().ifPresent(target -> toSlime(target, clusterObject.setObject(targetResourcesKey)));
         scalingEventsToSlime(cluster.scalingEvents(), clusterObject.setArray(scalingEventsKey));
         clusterObject.setString(autoscalingStatusKey, cluster.autoscalingStatus());
@@ -105,12 +106,17 @@ public class ApplicationSerializer {
                            clusterObject.field(exclusiveKey).asBool(),
                            clusterResourcesFromSlime(clusterObject.field(minResourcesKey)),
                            clusterResourcesFromSlime(clusterObject.field(maxResourcesKey)),
-                           optionalClusterResourcesFromSlime(clusterObject.field(suggestedResourcesKey)),
+                           optionalSuggestionFromSlime(clusterObject.field(suggestedKey)),
                            optionalClusterResourcesFromSlime(clusterObject.field(targetResourcesKey)),
                            scalingEventsFromSlime(clusterObject.field(scalingEventsKey)),
                            clusterObject.field(autoscalingStatusKey).asString());
     }
 
+    private static void toSlime(Cluster.Suggestion suggestion, Cursor suggestionObject) {
+        toSlime(suggestion.resources(), suggestionObject.setObject(resourcesKey));
+        suggestionObject.setLong(atKey, suggestion.at().toEpochMilli());
+    }
+
     private static void toSlime(ClusterResources resources, Cursor clusterResourcesObject) {
         clusterResourcesObject.setLong(nodesKey, resources.nodes());
         clusterResourcesObject.setLong(groupsKey, resources.groups());
@@ -123,6 +129,16 @@ public class ApplicationSerializer {
                                     NodeResourcesSerializer.resourcesFromSlime(clusterResourcesObject.field(nodeResourcesKey)));
     }
 
+    private static Optional<Cluster.Suggestion> optionalSuggestionFromSlime(Inspector suggestionObject) {
+        if ( ! suggestionObject.valid()) return Optional.empty();
+
+        if (suggestionObject.field(nodesKey).valid()) // TODO: Remove this line and the next after January 2021
+            return Optional.of(new Cluster.Suggestion(clusterResourcesFromSlime(suggestionObject),  Instant.EPOCH));
+
+        return Optional.of(new Cluster.Suggestion(clusterResourcesFromSlime(suggestionObject.field(resourcesKey)),
+                                                  Instant.ofEpochMilli(suggestionObject.field(atKey).asLong())));
+    }
+
     private static Optional<ClusterResources> optionalClusterResourcesFromSlime(Inspector clusterResourcesObject) {
         return clusterResourcesObject.valid() ? Optional.of(clusterResourcesFromSlime(clusterResourcesObject))
                                               : Optional.empty();
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 91b54fa37e9..0530e0cc9b6 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
@@ -50,7 +50,7 @@ public class ApplicationSerializer {
         toSlime(cluster.minResources(), clusterObject.setObject("min"));
         toSlime(cluster.maxResources(), clusterObject.setObject("max"));
         toSlime(currentResources, clusterObject.setObject("current"));
-        cluster.suggestedResources().ifPresent(suggested -> toSlime(suggested, clusterObject.setObject("suggested")));
+        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/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
index b24d2417db5..1f8f3d32043 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
@@ -164,8 +164,9 @@ public class MockNodeRepository extends NodeRepository {
                                 null), app1Id, provisioner);
         Application app1 = applications().get(app1Id).get();
         Cluster cluster1 = app1.cluster(cluster1Id.id()).get();
-        cluster1 = cluster1.withSuggested(Optional.of(new ClusterResources(6, 2,
-                                                                           new NodeResources(3, 20, 100, 1))));
+        cluster1 = cluster1.withSuggested(Optional.of(new Cluster.Suggestion(new ClusterResources(6, 2,
+                                                                                                  new NodeResources(3, 20, 100, 1)),
+                                                                             clock().instant())));
         cluster1 = cluster1.withTarget(Optional.of(new ClusterResources(4, 1,
                                                                         new NodeResources(3, 16, 100, 1))));
         try (Mutex lock = lock(app1Id)) {
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 d1fc13c9796..5ec29cb9043 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
@@ -67,9 +67,24 @@ 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().toString());
+                     tester.nodeRepository().applications().get(app1).get().cluster(cluster1.id()).get().suggestedResources().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().toString());
+                     tester.nodeRepository().applications().get(app2).get().cluster(cluster2.id()).get().suggestedResources().get().resources().toString());
+
+        // Utilization goes way down
+        tester.clock().advance(Duration.ofHours(13));
+        addMeasurements(0.10f, 0.10f, 0.10f, 0, 500, app1, tester.nodeRepository(), metricsDb);
+        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());
+        // 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());
     }
 
     public void addMeasurements(float cpu, float memory, float disk, int generation, int count, ApplicationId applicationId,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializerTest.java
index 06473e60712..6881733324e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/ApplicationSerializerTest.java
@@ -40,7 +40,9 @@ public class ApplicationSerializerTest {
                                  true,
                                  new ClusterResources( 8, 4, minResources),
                                  new ClusterResources(14, 7, new NodeResources(3, 6, 21, 24)),
-                                 Optional.of(new ClusterResources(20, 10, new NodeResources(0.5, 4, 14, 16))),
+                                 Optional.of(new Cluster.Suggestion(new ClusterResources(20, 10,
+                                                                                         new NodeResources(0.5, 4, 14, 16)),
+                                                                    Instant.ofEpochMilli(1234L))),
                                  Optional.of(new ClusterResources(10, 5, new NodeResources(2, 4, 14, 16))),
                                  List.of(new ScalingEvent(new ClusterResources(10, 5, minResources),
                                                           new ClusterResources(12, 6, minResources),