Merge pull request #21584 from vespa-engine/jonmv/deployment-playground

Jonmv/deployment playground

10 files changed, 64 insertions, 51 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
index 4ab7901cd62..30f416747e0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
@@ -215,6 +215,7 @@ public class Application {
     public Optional<ApplicationVersion> oldestDeployedApplication() {
         return productionDeployments().values().stream().flatMap(List::stream)
                                       .map(Deployment::applicationVersion)
+                                      .filter(version -> ! version.isUnknown() && ! version.isDeployedDirectly())
                                       .min(Comparator.naturalOrder());
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 88df49ad3ab..5c3c43461dc 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -446,21 +446,17 @@ public class ApplicationController {
                 controller.notificationsDb().removeNotifications(notification.source());
         }
 
-        var oldestDeployedVersion = application.get().productionDeployments().values().stream()
-                                               .flatMap(List::stream)
-                                               .map(Deployment::applicationVersion)
-                                               .filter(version -> ! version.isDeployedDirectly())
-                                               .min(naturalOrder())
-                                               .orElse(ApplicationVersion.unknown);
-
-        var olderVersions = application.get().versions().stream()
-                .filter(version -> version.compareTo(oldestDeployedVersion) < 0)
-                .sorted()
-                .collect(Collectors.toList());
+        ApplicationVersion oldestDeployedVersion = application.get().oldestDeployedApplication()
+                                                              .orElse(ApplicationVersion.unknown);
+
+        List<ApplicationVersion> olderVersions = application.get().versions().stream()
+                                                            .filter(version -> version.compareTo(oldestDeployedVersion) < 0)
+                                                            .sorted()
+                                                            .collect(Collectors.toList());
 
         // Remove any version not deployed anywhere - but keep one
-        for (int i = 0; i < olderVersions.size() - 1; i++) {
-            application = application.withoutVersion(olderVersions.get(i));
+        for (ApplicationVersion version : olderVersions) {
+            application = application.withoutVersion(version);
         }
 
         store(application);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
index 8c933f98277..09fd9ca86bc 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
@@ -43,7 +43,6 @@ import static com.yahoo.config.provision.Environment.staging;
 import static com.yahoo.config.provision.Environment.test;
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.stagingTest;
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.systemTest;
-import static java.util.Collections.reverseOrder;
 import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
 import static java.util.Objects.requireNonNull;
@@ -201,7 +200,7 @@ public class DeploymentStatus {
         jobs.putAll(productionJobs);
         // Add runs for idle, declared test jobs if they have no successes on their instance's change's versions.
         jobSteps.forEach((job, step) -> {
-            if ( ! step.isDeclared() || step.type() != StepType.test || jobs.containsKey(job))
+            if ( ! step.isDeclared() || job.type().isProduction() || jobs.containsKey(job))
                 return;
 
             Change change = changes.get(job.application().instance());
@@ -212,9 +211,8 @@ public class DeploymentStatus {
                                                                        .filter(jobId -> jobId.type().isProduction() && jobId.type().isDeployment())
                                                                        .filter(jobId -> deploymentFor(jobId).isPresent())
                                                                        .findFirst();
-
             Versions versions = Versions.from(change, application, firstProductionJobWithDeployment.flatMap(this::deploymentFor), systemVersion);
-            if (step.completedAt(change, firstProductionJobWithDeployment).isEmpty())
+            if (step.completedAt(change, Optional.empty()).isEmpty())
                 jobs.merge(job, List.of(new Job(job.type(), versions, step.readyAt(change), change)), DeploymentStatus::union);
         });
         return Collections.unmodifiableMap(jobs);
@@ -266,16 +264,18 @@ public class DeploymentStatus {
      * For the "exclusive" revision upgrade policy it is the oldest such revision; otherwise, it is the latest.
      */
     public Change outstandingChange(InstanceName instance) {
-        return Optional.ofNullable(instanceSteps().get(instance))
-                       .flatMap(instanceStatus -> application.deployableVersions(application.deploymentSpec().requireInstance(instance).revisionTarget() == next).stream()
-                                                             .filter(version -> instanceStatus.dependenciesCompletedAt(Change.of(version), Optional.empty()).map(at -> ! at.isAfter(now)).orElse(false))
-                                                             .filter(version -> application.productionDeployments().getOrDefault(instance, List.of()).stream()
-                                                                                           .noneMatch(deployment -> deployment.applicationVersion().compareTo(version) > 0))
-                                                             .map(Change::of)
-                                                             .filter(change -> application.require(instance).change().application().map(change::upgrades).orElse(true))
-                                                             .filter(change -> ! hasCompleted(instance, change))
-                                                             .findFirst())
-                       .orElse(Change.empty());
+        StepStatus status = instanceSteps().get(instance);
+        if (status == null) return Change.empty();
+        for (ApplicationVersion version : application.deployableVersions(application.deploymentSpec().requireInstance(instance).revisionTarget() == next)) {
+            if (status.dependenciesCompletedAt(Change.of(version), Optional.empty()).map(now::isBefore).orElse(true)) continue;
+            Change change = Change.of(version);
+            if (application.productionDeployments().getOrDefault(instance, List.of()).stream()
+                           .anyMatch(deployment -> change.downgrades(deployment.applicationVersion()))) continue;
+            if ( ! application.require(instance).change().application().map(change::upgrades).orElse(true)) continue;
+            if (hasCompleted(instance, change)) continue;
+            return change;
+        }
+        return Change.empty();
     }
 
     /** Earliest instant when job was triggered with given versions, or both system and staging tests were successful. */
@@ -805,7 +805,7 @@ public class DeploymentStatus {
                                           .orElse(false))
                         return job.lastCompleted().flatMap(Run::end);
 
-                    return (dependent.equals(job()) ? job.lastSuccess().stream()
+                    return (dependent.equals(job()) ? job.lastTriggered().filter(run -> run.status() == RunStatus.success).stream()
                                                     : RunList.from(job).status(RunStatus.success).asList().stream())
                             .filter(run ->    change.platform().map(run.versions().targetPlatform()::equals).orElse(true)
                                            && change.application().map(run.versions().targetApplication()::equals).orElse(true))
@@ -857,10 +857,13 @@ public class DeploymentStatus {
                 @Override
                 Optional<Instant> completedAt(Change change, Optional<JobId> dependent) {
                     return RunList.from(job)
-                                  .matching(run -> run.versions().targetsMatch(Versions.from(change,
-                                                                                             status.application,
-                                                                                             dependent.flatMap(status::deploymentFor),
-                                                                                             status.systemVersion)))
+                                  .matching(run -> dependent.flatMap(status::deploymentFor)
+                                                            .map(deployment -> run.versions().targetsMatch(Versions.from(change,
+                                                                                                                         status.application,
+                                                                                                                         Optional.of(deployment),
+                                                                                                                         status.systemVersion)))
+                                                            .orElseGet(() ->    (change.platform().isEmpty()    || change.platform().get().equals(run.versions().targetPlatform()))
+                                                                             && (change.application().isEmpty() || change.application().get().equals(run.versions().targetApplication()))))
                                   .status(RunStatus.success)
                                   .asList().stream()
                                   .map(run -> run.end().get())
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index 66a30050f7a..647b52f7d51 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -607,11 +607,7 @@ public class JobController {
 
     private void prunePackages(TenantAndApplicationId id) {
         controller.applications().lockApplicationIfPresent(id, application -> {
-            application.get().productionDeployments().values().stream()
-                       .flatMap(List::stream)
-                       .map(Deployment::applicationVersion)
-                       .filter(version -> ! version.isUnknown() && ! version.isDeployedDirectly())
-                       .min(naturalOrder())
+            application.get().oldestDeployedApplication()
                        .ifPresent(oldestDeployed -> {
                            controller.applications().applicationStore().prune(id.tenant(), id.application(), oldestDeployed);
                            controller.applications().applicationStore().pruneTesters(id.tenant(), id.application(), oldestDeployed);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
index 21cc69369d8..989a7c31821 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentContext.java
@@ -236,8 +236,13 @@ public class DeploymentContext {
     /** Flush count pending DNS updates */
     public DeploymentContext flushDnsUpdates(int count) {
         var dispatcher = new NameServiceDispatcher(tester.controller(), Duration.ofSeconds(count));
-        dispatcher.run();
-        return this;
+        try {
+            dispatcher.run();
+            return this;
+        }
+        finally {
+            dispatcher.shutdown();
+        }
     }
 
     /** Add a routing policy for this in given zone, with status set to inactive */
@@ -255,6 +260,11 @@ public class DeploymentContext {
     }
 
     /** Submit given application package for deployment */
+    public DeploymentContext resubmit(ApplicationPackage applicationPackage) {
+        return submit(applicationPackage, Optional.of(defaultSourceRevision), salt.get());
+    }
+
+    /** Submit given application package for deployment */
     public DeploymentContext submit(ApplicationPackage applicationPackage) {
         return submit(applicationPackage, Optional.of(defaultSourceRevision));
     }
@@ -266,7 +276,7 @@ public class DeploymentContext {
 
     /** Submit given application package for deployment */
     public DeploymentContext submit(ApplicationPackage applicationPackage, Optional<SourceRevision> sourceRevision) {
-        return submit(applicationPackage, sourceRevision, salt.getAndIncrement());
+        return submit(applicationPackage, sourceRevision, salt.incrementAndGet());
     }
 
     /** Submit given application package for deployment */
@@ -597,8 +607,9 @@ public class DeploymentContext {
         runner.advance(currentRun(job));
         assertTrue(jobs.run(id).get().hasEnded());
         assertFalse(jobs.run(id).get().hasFailed());
-        assertEquals(job.type().isProduction(), instance().deployments().containsKey(zone));
-        assertTrue(configServer().nodeRepository().list(zone, NodeFilter.all().applications(TesterId.of(id.application()).id())).isEmpty());
+        Instance instance = tester.application(TenantAndApplicationId.from(instanceId)).require(id.application().instance());
+        assertEquals(job.type().isProduction(), instance.deployments().containsKey(zone));
+        assertTrue(configServer().nodeRepository().list(zone, NodeFilter.all().applications(TesterId.of(instance.id()).id())).isEmpty());
     }
 
     private JobId jobId(JobType type) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
index b2847a29654..c8126207a73 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTriggerTest.java
@@ -1873,7 +1873,7 @@ public class DeploymentTriggerTest {
         // Deploy manually again, then submit new package.
         app.runJob(productionCdUsEast1, cdPackage);
         app.submit(cdPackage);
-        app.runJob(systemTest);
+        app.triggerJobs().jobAborted(systemTest).runJob(systemTest);
         // Staging test requires unknown initial version, and is broken.
         tester.controller().applications().deploymentTrigger().forceTrigger(app.instanceId(), productionCdUsEast1, "user", false, true, true);
         app.runJob(productionCdUsEast1)
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
index 265dedec8d0..78341682f75 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/UpgraderTest.java
@@ -945,9 +945,9 @@ public class UpgraderTest {
 
         // Application downgrades to pinned version.
         tester.abortAll();
-        context.runJob(stagingTest).runJob(productionUsCentral1);
+        context.runJob(stagingTest).runJob(productionUsCentral1).runJob(productionUsWest1);
         assertTrue(context.instance().change().hasTargets());
-        context.runJob(productionUsWest1); // us-east-3 never upgraded, so no downgrade is needed.
+        context.runJob(productionUsEast3);
         assertFalse(context.instance().change().hasTargets());
     }
 
@@ -1011,7 +1011,7 @@ public class UpgraderTest {
         tester.controllerTester().upgradeSystem(v2);
         tester.upgrader().maintain();
         assertEquals(Change.of(v2), application.instance().change());
-        application.runJob(systemTest).runJob(stagingTest).runJob(productionUsCentral1);
+        application.runJob(systemTest).runJob(stagingTest).runJob(productionUsCentral1).timeOutConvergence(productionUsWest1);
         tester.triggerJobs();
 
         // While second deployment completes upgrade, version confidence becomes broken and upgrade is cancelled
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepoStats.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepoStats.java
index ba8aa49d87c..338187c5270 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepoStats.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepoStats.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.autoscale.NodeTimeseries;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -122,6 +123,10 @@ public class NodeRepoStats {
 
     public static class ApplicationStats implements Comparable<ApplicationStats> {
 
+        private static final Comparator<ApplicationStats> comparator = Comparator.comparingDouble(ApplicationStats::unutilizedCost).reversed()
+                                                                                 .thenComparingDouble(ApplicationStats::cost)
+                                                                                 .thenComparing(ApplicationStats::id);
+
         private final ApplicationId id;
         private final Load load;
         private final double cost;
@@ -148,7 +153,7 @@ public class NodeRepoStats {
 
         @Override
         public int compareTo(NodeRepoStats.ApplicationStats other) {
-            return -Double.compare(this.unutilizedCost(), other.unutilizedCost());
+            return comparator.compare(this, other);
         }
 
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/stats.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/stats.json
index 8867520fef6..8a46f8115be 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/stats.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/responses/stats.json
@@ -11,7 +11,7 @@
   },
   "applications": [
     {
-      "id": "tenant3.application3.instance3",
+      "id": "tenant1.application1.instance1",
       "load": {
         "cpu": 0.0,
         "memory": 0.0,
@@ -31,7 +31,7 @@
       "unutilizedCost": 0.0
     },
     {
-      "id": "tenant4.application4.instance4",
+      "id": "tenant3.application3.instance3",
       "load": {
         "cpu": 0.0,
         "memory": 0.0,
@@ -41,7 +41,7 @@
       "unutilizedCost": 0.0
     },
     {
-      "id": "tenant1.application1.instance1",
+      "id": "tenant4.application4.instance4",
       "load": {
         "cpu": 0.0,
         "memory": 0.0,
diff --git a/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java b/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
index 2c93312eb6d..f035e2c6f00 100644
--- a/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
+++ b/vespajlib/src/main/java/com/yahoo/collections/AbstractFilteringList.java
@@ -8,6 +8,7 @@ import java.util.Comparator;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -99,7 +100,7 @@ public abstract class AbstractFilteringList<Type, ListType extends AbstractFilte
     /** Returns the items grouped by the given classifier. */
     public final <OtherType> Map<OtherType, ListType> groupingBy(Function<Type, OtherType> classifier) {
         return items.stream().collect(Collectors.groupingBy(classifier,
-                                                            HashMap::new,
+                                                            LinkedHashMap::new,
                                                             Collectors.collectingAndThen(toUnmodifiableList(),
                                                                                          (list) -> constructor.apply(list, false))));
     }