Get rid of Advice.present

3 files changed, 22 insertions, 27 deletions

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 c816abc060c..0d643dc070b 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
@@ -124,13 +124,11 @@ public class Autoscaler {
 
     public static class Advice {
 
-        private final boolean present;
         private final Optional<ClusterResources> target;
         private final AutoscalingStatus reason;
 
-        private Advice(Optional<ClusterResources> target, boolean present, AutoscalingStatus reason) {
+        private Advice(Optional<ClusterResources> target, AutoscalingStatus reason) {
             this.target = target;
-            this.present = present;
             this.reason = Objects.requireNonNull(reason);
         }
 
@@ -140,33 +138,26 @@ public class Autoscaler {
          */
         public Optional<ClusterResources> target() { return target; }
 
-        /** True if this does not provide any advice */
-        public boolean isEmpty() { return ! present; }
-
-        /** True if this provides advice (which may be to keep the current allocation) */
-        public boolean isPresent() { return present; }
-
         /** The reason for this advice */
         public AutoscalingStatus reason() { return reason; }
 
         private static Advice none(Status status, String description) {
-            return new Advice(Optional.empty(), false, new AutoscalingStatus(status, description));
+            return new Advice(Optional.empty(), new AutoscalingStatus(status, description));
         }
 
         private static Advice dontScale(Status status, String description) {
-            return new Advice(Optional.empty(), true, new AutoscalingStatus(status, description));
+            return new Advice(Optional.empty(), new AutoscalingStatus(status, description));
         }
 
         private static Advice scaleTo(ClusterResources target) {
-            return new Advice(Optional.of(target), true,
+            return new Advice(Optional.of(target),
                               new AutoscalingStatus(AutoscalingStatus.Status.rescaling,
                                                     "Rescaling initiated due to load changes"));
         }
 
         @Override
         public String toString() {
-            return "autoscaling advice: " +
-                   (present ? (target.isPresent() ? "Scale to " + target.get() : "Don't scale") : "None");
+            return "autoscaling advice: " + (target.isPresent() ? "Scale to " + target.get() : "Don't scale");
         }
 
     }
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 65f6eecf790..7eedad98697 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
@@ -87,7 +87,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer {
             applications().put(application.get().with(updatedCluster), lock);
 
             var current = new AllocatableClusterResources(clusterNodes, nodeRepository()).advertisedResources();
-            if (advice.isPresent() && advice.target().isPresent() && !current.equals(advice.target().get())) {
+            if (advice.target().isPresent() && !current.equals(advice.target().get())) {
                 // 2. Also autoscale
                 try (MaintenanceDeployment deployment = new MaintenanceDeployment(applicationId, deployer, metric, nodeRepository())) {
                     if (deployment.isValid()) {
@@ -101,7 +101,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer {
 
     private boolean anyChanges(Autoscaler.Advice advice, Cluster cluster, Cluster updatedCluster, NodeList clusterNodes) {
         if (updatedCluster != cluster) return true;
-        if (advice.isPresent() && !cluster.target().resources().equals(advice.target())) return true;
+        if (!cluster.target().resources().equals(advice.target())) return true;
         if ( ! advice.reason().equals(cluster.target().status())) return true;
         if (advice.target().isPresent() &&
             !advice.target().get().equals(new AllocatableClusterResources(clusterNodes, nodeRepository()).advertisedResources())) 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 0201ffae103..248329966cc 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
@@ -64,11 +64,14 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         Optional<Cluster> cluster = application.cluster(clusterId);
         if (cluster.isEmpty()) return true;
         var suggestion = autoscaler.suggest(application, cluster.get(), clusterNodes);
-        if (suggestion.isEmpty()) return true;
+        if (suggestion.reason().status() == AutoscalingStatus.Status.waiting) return true;
+
+        // empty suggested resources == keep the current allocation
+        var suggestedResources = suggestion.target().orElse(clusterNodes.not().retired().toResources());
+        if ( ! shouldUpdateSuggestion(cluster.get().suggested(), suggestedResources)) return true;
+
         // Wait only a short time for the lock to avoid interfering with change deployments
         try (Mutex lock = nodeRepository().applications().lock(applicationId, Duration.ofSeconds(1))) {
-            // 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;
         }
@@ -77,20 +80,21 @@ public class ScalingSuggestionsMaintainer extends NodeRepositoryMaintainer {
         }
     }
 
+    private boolean shouldUpdateSuggestion(Autoscaling currentSuggestion, ClusterResources suggestedResources) {
+        return currentSuggestion.resources().isEmpty()
+               || currentSuggestion.at().isBefore(nodeRepository().clock().instant().minus(Duration.ofDays(7)))
+               || isHigher(suggestedResources, currentSuggestion.resources().get());
+    }
+
     private void updateSuggestion(ClusterResources suggestion,
                                   ClusterSpec.Id clusterId,
                                   Application application,
                                   Mutex lock) {
         Optional<Cluster> cluster = application.cluster(clusterId);
         if (cluster.isEmpty()) return;
-        var at = nodeRepository().clock().instant();
-        var currentSuggestion = cluster.get().suggested();
-        if (currentSuggestion.resources().isEmpty()
-            || currentSuggestion.at().isBefore(at.minus(Duration.ofDays(7)))
-            || isHigher(suggestion, currentSuggestion.resources().get()))
-            applications().put(application.with(cluster.get().withSuggested(new Autoscaling(suggestion,
-                                                                                            AutoscalingStatus.empty(),
-                                                                                            at))), lock);
+        applications().put(application.with(cluster.get().withSuggested(new Autoscaling(suggestion,
+                                                                                        AutoscalingStatus.empty(),
+                                                                                        nodeRepository().clock().instant()))), lock);
     }
 
     private boolean isHigher(ClusterResources r1, ClusterResources r2) {