Clear outdated target and cleanup

9 files changed, 60 insertions, 79 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Application.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Application.java
index 05aeae45ad9..3a3d3e4ec2e 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Application.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/applications/Application.java
@@ -62,16 +62,6 @@ public class Application {
         return with(cluster);
     }
 
-    /**
-     * Returns an application with the given target for the given cluster,
-     * if it exists and the target is within the bounds
-     */
-    public Application withClusterTarget(ClusterSpec.Id id, ClusterResources target) {
-        Cluster cluster = clusters.get(id);
-        if (cluster == null) return this;
-        return with(cluster.withTarget(target));
-    }
-
     @Override
     public int hashCode() {
         return id.hashCode();
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 9e0a9f368a8..15a5545bc2c 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
@@ -64,20 +64,12 @@ public class Cluster {
         return new Cluster(id, min, max, suggested, target);
     }
 
-    public Cluster withSuggested(ClusterResources suggested) {
-        return new Cluster(id, min, max, Optional.of(suggested), target);
-    }
-
-    public Cluster withoutSuggested() {
-        return new Cluster(id, min, max, Optional.empty(), target);
-    }
-
-    public Cluster withTarget(ClusterResources target) {
-        return new Cluster(id, min, max, suggested, Optional.of(target));
+    public Cluster withSuggested(Optional<ClusterResources> suggested) {
+        return new Cluster(id, min, max, suggested, target);
     }
 
-    public Cluster withoutTarget() {
-        return new Cluster(id, min, max, suggested, Optional.empty());
+    public Cluster withTarget(Optional<ClusterResources> target) {
+        return new Cluster(id, min, max, suggested, target);
     }
 
     public NodeResources capAtLimits(NodeResources resources) {
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 14d1dca3e81..6dca9c9a796 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
@@ -62,8 +62,10 @@ public class Autoscaler {
      * @param clusterNodes the list of all the active nodes in a cluster
      * @return a new suggested allocation for this cluster, or empty if it should not be rescaled at this time
      */
-    public Optional<AllocatableClusterResources> suggest(Cluster cluster, List<Node> clusterNodes) {
-        return autoscale(cluster, clusterNodes, false);
+    public Optional<ClusterResources> suggest(Cluster cluster, List<Node> clusterNodes) {
+        return autoscale(cluster, clusterNodes, false)
+                       .map(AllocatableClusterResources::toAdvertisedClusterResources);
+
     }
 
     /**
@@ -72,9 +74,10 @@ public class Autoscaler {
      * @param clusterNodes the list of all the active nodes in a cluster
      * @return a new suggested allocation for this cluster, or empty if it should not be rescaled at this time
      */
-    public Optional<AllocatableClusterResources> autoscale(Cluster cluster, List<Node> clusterNodes) {
+    public Optional<ClusterResources> autoscale(Cluster cluster, List<Node> clusterNodes) {
         if (cluster.minResources().equals(cluster.maxResources())) return Optional.empty(); // Shortcut
-        return autoscale(cluster, clusterNodes, true);
+        return autoscale(cluster, clusterNodes, true)
+                       .map(AllocatableClusterResources::toAdvertisedClusterResources);
     }
 
     private Optional<AllocatableClusterResources> autoscale(Cluster cluster, List<Node> clusterNodes, boolean respectLimits) {
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 3b97195ad2d..94487547fab 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
@@ -3,13 +3,16 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.collections.Pair;
 import com.yahoo.config.provision.ApplicationId;
+import com.yahoo.config.provision.ClusterResources;
 import com.yahoo.config.provision.ClusterSpec;
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.jdisc.Metric;
+import com.yahoo.transaction.Mutex;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.applications.Application;
+import com.yahoo.vespa.hosted.provision.applications.Applications;
 import com.yahoo.vespa.hosted.provision.applications.Cluster;
 import com.yahoo.vespa.hosted.provision.autoscale.AllocatableClusterResources;
 import com.yahoo.vespa.hosted.provision.autoscale.Autoscaler;
@@ -66,26 +69,22 @@ public class AutoscalingMaintainer extends Maintainer {
                            List<Node> clusterNodes,
                            MaintenanceDeployment deployment) {
         Application application = nodeRepository().applications().get(applicationId).orElse(new Application(applicationId));
-        Cluster cluster = application.clusters().get(clusterId);
-        if (cluster == null) return;
-        if (cluster.minResources().equals(cluster.maxResources())) return;
-        Optional<AllocatableClusterResources> target = autoscaler.autoscale(cluster, clusterNodes);
-        if (target.isEmpty()) return; // current resources are fine
-
-        logAutoscaling(target.get(), applicationId, clusterId, clusterNodes);
-        autoscaleTo(target.get(), clusterId, application, deployment);
+        Optional<Cluster> cluster = application.cluster(clusterId);
+        if (cluster.isEmpty()) return;
+        Optional<ClusterResources> target = autoscaler.autoscale(cluster.get(), clusterNodes);
+        if ( ! cluster.get().targetResources().equals(target))
+            applications().put(application.with(cluster.get().withTarget(target)), deployment.applicationLock().get());
+        if (target.isPresent()) {
+            logAutoscaling(target.get(), applicationId, clusterId, clusterNodes);
+            deployment.activate();
+        }
     }
 
-    private void autoscaleTo(AllocatableClusterResources target,
-                             ClusterSpec.Id clusterId,
-                             Application application,
-                             MaintenanceDeployment deployment) {
-        nodeRepository().applications().put(application.withClusterTarget(clusterId, target.toAdvertisedClusterResources()),
-                                            deployment.applicationLock().get());
-        deployment.activate();
+    private Applications applications() {
+        return nodeRepository().applications();
     }
 
-    private void logAutoscaling(AllocatableClusterResources target,
+    private void logAutoscaling(ClusterResources target,
                                 ApplicationId application,
                                 ClusterSpec.Id clusterId,
                                 List<Node> clusterNodes) {
@@ -93,7 +92,7 @@ public class AutoscalingMaintainer extends Maintainer {
         ClusterSpec.Type clusterType = clusterNodes.get(0).allocation().get().membership().cluster().type();
         log.info("Autoscaling " + application + " " + clusterType + " " + clusterId + ":" +
                  "\nfrom " + toString(clusterNodes.size(), currentGroups, clusterNodes.get(0).flavor().resources()) +
-                 "\nto   " + toString(target.nodes(), target.groups(), target.advertisedResources()));
+                 "\nto   " + toString(target.nodes(), target.groups(), target.nodeResources()));
     }
 
     private String toString(int nodes, int groups, NodeResources resources) {
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 985c5c7d1a7..6296543aea4 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
@@ -60,22 +60,19 @@ public class ScalingSuggestionsMaintainer extends Maintainer {
         Application application = applications().get(applicationId).orElse(new Application(applicationId));
         Optional<Cluster> cluster = application.cluster(clusterId);
         if (cluster.isEmpty()) return;
-        Optional<AllocatableClusterResources> suggestion = autoscaler.suggest(cluster.get(), clusterNodes);
+        Optional<ClusterResources> suggestion = autoscaler.suggest(cluster.get(), clusterNodes);
         try (Mutex lock = nodeRepository().lock(applicationId)) {
             applications().get(applicationId).ifPresent(a -> storeSuggestion(suggestion, clusterId, a, lock));
         }
     }
 
-    private void storeSuggestion(Optional<AllocatableClusterResources> suggestion,
+    private void storeSuggestion(Optional<ClusterResources> suggestion,
                                  ClusterSpec.Id clusterId,
                                  Application application,
                                  Mutex lock) {
         Optional<Cluster> cluster = application.cluster(clusterId);
         if (cluster.isEmpty()) return;
-        if (suggestion.isPresent())
-            applications().put(application.with(cluster.get().withSuggested(suggestion.get().toAdvertisedClusterResources())), lock);
-        else
-            applications().put(application.with(cluster.get().withoutSuggested()), lock);
+        applications().put(application.with(cluster.get().withSuggested(suggestion)), lock);
     }
 
     private Map<ClusterSpec.Id, List<Node>> nodesByCluster(List<Node> applicationNodes) {
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 1d0ed788b41..e3b7c62cc0d 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
@@ -129,7 +129,7 @@ public class NodeRepositoryProvisioner implements Provisioner {
 
     /**
      * Returns the target cluster resources, a value between the min and max in the requested capacity,
-     * and updates the application store with the received min and max,
+     * and updates the application store with the received min and max.
      */
     private ClusterResources decideTargetResources(ApplicationId applicationId, ClusterSpec.Id clusterId, Capacity requested) {
         try (Mutex lock = nodeRepository.lock(applicationId)) {
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 0cafe35ee52..6fafd496174 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
@@ -154,10 +154,10 @@ public class MockNodeRepository extends NodeRepository {
                             null);
         Application app1 = applications().get(app1Id).get();
         Cluster cluster1 = app1.cluster(cluster1Id.id()).get();
-        cluster1 = cluster1.withSuggested(new ClusterResources(6, 2,
-                                                               new NodeResources(3, 20, 100, 1)));
-        cluster1 = cluster1.withTarget(new ClusterResources(4, 1,
-                                                            new NodeResources(3, 16, 100, 1)));
+        cluster1 = cluster1.withSuggested(Optional.of(new ClusterResources(6, 2,
+                                                                           new NodeResources(3, 20, 100, 1))));
+        cluster1 = cluster1.withTarget(Optional.of(new ClusterResources(4, 1,
+                                                                        new NodeResources(3, 16, 100, 1))));
         try (Mutex lock = lock(app1Id)) {
             applications().put(app1.with(cluster1), lock);
         }
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 4dcbcd10111..f98d55511ec 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
@@ -50,9 +50,9 @@ public class AutoscalingTest {
         assertTrue("Too few measurements -> No change", tester.autoscale(application1, cluster1.id(), min, max).isEmpty());
 
         tester.addMeasurements(Resource.cpu, 0.25f, 1f, 60, application1);
-        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));
+        ClusterResources 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));
 
         tester.deploy(application1, cluster1, scaledResources);
         assertTrue("Cluster in flux -> No further change", tester.autoscale(application1, cluster1.id(), min, max).isEmpty());
@@ -90,11 +90,11 @@ public class AutoscalingTest {
                                                      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));
+        ClusterResources 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());
+                     NodeResources.DiskSpeed.any, scaledResources.nodeResources().diskSpeed());
         tester.deploy(application1, cluster1, scaledResources);
         tester.nodeRepository().getNodes(application1).stream()
               .allMatch(n -> n.allocation().get().requestedResources().diskSpeed() == NodeResources.DiskSpeed.any);
@@ -115,9 +115,9 @@ public class AutoscalingTest {
         tester.deploy(application1, cluster1, 5, 1, resources);
 
         tester.addMeasurements(Resource.cpu, 0.25f, 1f, 120, application1);
-        AllocatableClusterResources scaledResources = tester.assertResources("Scaling up since cpu usage is too high",
-                                                                             7, 1, 2.6,  80.0, 80.0,
-                                                                             tester.autoscale(application1, cluster1.id(), min, max));
+        ClusterResources scaledResources = tester.assertResources("Scaling up since cpu usage is too high",
+                                                                  7, 1, 2.6,  80.0, 80.0,
+                                                                  tester.autoscale(application1, cluster1.id(), min, max));
 
         tester.deploy(application1, cluster1, scaledResources);
         tester.deactivateRetired(application1, cluster1, scaledResources);
@@ -295,7 +295,7 @@ public class AutoscalingTest {
         tester.deploy(application1, cluster1, 5, 1, new NodeResources(3, 103, 100, 1));
 
         tester.addMeasurements(Resource.memory, 0.9f, 0.6f, 120, application1);
-        AllocatableClusterResources scaledResources = tester.assertResources("Scaling up since resource usage is too high.",
+        ClusterResources scaledResources = tester.assertResources("Scaling up since resource usage is too high.",
                                                                   8, 1, 3,  83, 34.3,
                                                                   tester.autoscale(application1, cluster1.id(), min, max));
 
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 6eb3aaea66e..49bb51c1d79 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
@@ -79,8 +79,8 @@ class AutoscalingTester {
         return ClusterSpec.request(type, ClusterSpec.Id.from(clusterId)).vespaVersion("7").build();
     }
 
-    public void deploy(ApplicationId application, ClusterSpec cluster, AllocatableClusterResources resources) {
-        deploy(application, cluster, resources.nodes(), resources.groups(), resources.advertisedResources());
+    public void deploy(ApplicationId application, ClusterSpec cluster, ClusterResources resources) {
+        deploy(application, cluster, resources.nodes(), resources.groups(), resources.nodeResources());
     }
 
     public List<HostSpec> deploy(ApplicationId application, ClusterSpec cluster, int nodes, int groups, NodeResources resources) {
@@ -101,7 +101,7 @@ class AutoscalingTester {
             nodeRepository().setReady(List.of(host), Agent.system, getClass().getSimpleName());
     }
 
-    public void deactivateRetired(ApplicationId application, ClusterSpec cluster, AllocatableClusterResources resources) {
+    public void deactivateRetired(ApplicationId application, ClusterSpec cluster, ClusterResources resources) {
         try (Mutex lock = nodeRepository().lock(application)){
             for (Node node : nodeRepository().getNodes(application, Node.State.active)) {
                 if (node.allocation().get().membership().retired())
@@ -154,7 +154,7 @@ class AutoscalingTester {
         }
     }
 
-    public Optional<AllocatableClusterResources> autoscale(ApplicationId applicationId, ClusterSpec.Id clusterId,
+    public Optional<ClusterResources> autoscale(ApplicationId applicationId, ClusterSpec.Id clusterId,
                                                            ClusterResources min, ClusterResources max) {
         Application application = nodeRepository().applications().get(applicationId).orElse(new Application(applicationId))
                                                   .withClusterLimits(clusterId, min, max);
@@ -163,7 +163,7 @@ class AutoscalingTester {
                                     nodeRepository().getNodes(applicationId, Node.State.active));
     }
 
-    public Optional<AllocatableClusterResources> suggest(ApplicationId applicationId, ClusterSpec.Id clusterId,
+    public Optional<ClusterResources> suggest(ApplicationId applicationId, ClusterSpec.Id clusterId,
                                                            ClusterResources min, ClusterResources max) {
         Application application = nodeRepository().applications().get(applicationId).orElse(new Application(applicationId))
                                                   .withClusterLimits(clusterId, min, max);
@@ -172,18 +172,18 @@ class AutoscalingTester {
                                   nodeRepository().getNodes(applicationId, Node.State.active));
     }
 
-    public AllocatableClusterResources assertResources(String message,
+    public ClusterResources assertResources(String message,
                                             int nodeCount, int groupCount,
                                             double approxCpu, double approxMemory, double approxDisk,
-                                            Optional<AllocatableClusterResources> actualResources) {
+                                            Optional<ClusterResources> resources) {
         double delta = 0.0000000001;
-        assertTrue(message, actualResources.isPresent());
-        assertEquals("Node count:  " + message, nodeCount, actualResources.get().nodes());
-        assertEquals("Group count: " + message, groupCount, actualResources.get().groups());
-        assertEquals("Cpu: "    + message, approxCpu, Math.round(actualResources.get().advertisedResources().vcpu() * 10) / 10.0, delta);
-        assertEquals("Memory: " + message, approxMemory, Math.round(actualResources.get().advertisedResources().memoryGb() * 10) / 10.0, delta);
-        assertEquals("Disk: "   + message, approxDisk, Math.round(actualResources.get().advertisedResources().diskGb() * 10) / 10.0, delta);
-        return actualResources.get();
+        assertTrue(message, resources.isPresent());
+        assertEquals("Node count:  " + message, nodeCount, resources.get().nodes());
+        assertEquals("Group count: " + message, groupCount, resources.get().groups());
+        assertEquals("Cpu: "    + message, approxCpu, Math.round(resources.get().nodeResources().vcpu() * 10) / 10.0, delta);
+        assertEquals("Memory: " + message, approxMemory, Math.round(resources.get().nodeResources().memoryGb() * 10) / 10.0, delta);
+        assertEquals("Disk: "   + message, approxDisk, Math.round(resources.get().nodeResources().diskGb() * 10) / 10.0, delta);
+        return resources.get();
     }
 
     public ManualClock clock() {