Merge pull request #21452 from vespa-engine/jonmv/deployment-orch-adjustments

Jonmv/deployment orch adjustments

6 files changed, 94 insertions, 68 deletions

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 3f12cce9aae..f36f5be7778 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
@@ -33,6 +33,7 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -139,10 +140,10 @@ public class DeploymentStatus {
         return dependents.contains(current);
     }
 
-    /** Whether any job is failing on anything older than version, with errors other than lack of capacity in a test zone.. */
-    public boolean hasFailures(ApplicationVersion version) {
+    /** Whether any job is failing on versions selected by the given filter, with errors other than lack of capacity in a test zone.. */
+    public boolean hasFailures(Predicate<ApplicationVersion> versionFilter) {
         return ! allJobs.failingHard()
-                        .matching(job -> job.lastTriggered().get().versions().targetApplication().compareTo(version) < 0)
+                        .matching(job -> versionFilter.test(job.lastTriggered().get().versions().targetApplication()))
                         .isEmpty();
     }
 
@@ -210,7 +211,7 @@ public class DeploymentStatus {
 
             Versions versions = Versions.from(change, application, firstProductionJobWithDeployment.flatMap(this::deploymentFor), systemVersion);
             if (step.completedAt(change, firstProductionJobWithDeployment).isEmpty())
-                jobs.merge(job, List.of(new Job(versions, step.readyAt(change), change)), DeploymentStatus::union);
+                jobs.merge(job, List.of(new Job(job.type(), versions, step.readyAt(change), change)), DeploymentStatus::union);
         });
         return Collections.unmodifiableMap(jobs);
     }
@@ -303,20 +304,29 @@ public class DeploymentStatus {
     private Map<JobId, List<Job>> productionJobs(InstanceName instance, Change change, boolean assumeUpgradesSucceed) {
         Map<JobId, List<Job>> jobs = new LinkedHashMap<>();
         jobSteps.forEach((job, step) -> {
-            // When computing eager test jobs for outstanding changes, assume current upgrade completes successfully.
-            Optional<Deployment> deployment = deploymentFor(job)
-                    .map(existing -> assumeUpgradesSucceed ? withChange(existing, change.withoutApplication()) : existing);
+            // When computing eager test jobs for outstanding changes, assume current change completes successfully.
+            Optional<Deployment> deployment = deploymentFor(job);
+            Optional<Version> existingPlatform = deployment.map(Deployment::version);
+            Optional<ApplicationVersion> existingApplication = deployment.map(Deployment::applicationVersion);
+            if (assumeUpgradesSucceed) {
+                Change currentChange = application.require(instance).change();
+                Versions target = Versions.from(currentChange, application, deployment, systemVersion);
+                existingPlatform = Optional.of(target.targetPlatform());
+                existingApplication = Optional.of(target.targetApplication());
+            }
             if (job.application().instance().equals(instance) && job.type().isProduction()) {
-
                 List<Job> toRun = new ArrayList<>();
                 List<Change> changes = changes(job, step, change);
                 if (changes.isEmpty()) return;
                 for (Change partial : changes) {
-                    toRun.add(new Job(Versions.from(partial, application, deployment, systemVersion),
-                                      step.readyAt(partial, Optional.of(job)),
-                                      partial));
+                    Job jobToRun = new Job(job.type(),
+                                           Versions.from(partial, application, existingPlatform, existingApplication, systemVersion),
+                                           step.readyAt(partial, Optional.of(job)),
+                                           partial);
+                    toRun.add(jobToRun);
                     // Assume first partial change is applied before the second.
-                    deployment = deployment.map(existing -> withChange(existing, partial));
+                    existingPlatform = Optional.of(jobToRun.versions.targetPlatform());
+                    existingApplication = Optional.of(jobToRun.versions.targetApplication());
                 }
                 jobs.put(job, toRun);
             }
@@ -324,17 +334,6 @@ public class DeploymentStatus {
         return jobs;
     }
 
-    private static Deployment withChange(Deployment deployment, Change change) {
-        return new Deployment(deployment.zone(),
-                              change.application().orElse(deployment.applicationVersion()),
-                              change.platform().orElse(deployment.version()),
-                              deployment.at(),
-                              deployment.metrics(),
-                              deployment.activity(),
-                              deployment.quota(),
-                              deployment.cost());
-    }
-
     /** Changes to deploy with the given job, possibly split in two steps. */
     private List<Change> changes(JobId job, StepStatus step, Change change) {
         // Signal strict completion criterion by depending on job itself.
@@ -432,7 +431,8 @@ public class DeploymentStatus {
                     declaredTest(job.application(), testType).ifPresent(testJob -> {
                         for (Job productionJob : versionsList)
                             if (allJobs.successOn(productionJob.versions()).get(testJob).isEmpty())
-                                testJobs.merge(testJob, List.of(new Job(productionJob.versions(),
+                                testJobs.merge(testJob, List.of(new Job(testJob.type(),
+                                                                        productionJob.versions(),
                                                                         jobSteps().get(testJob).readyAt(productionJob.change),
                                                                         productionJob.change)),
                                                DeploymentStatus::union);
@@ -448,7 +448,8 @@ public class DeploymentStatus {
                                                       && testJobs.get(test).stream().anyMatch(testJob -> testJob.versions().equals(productionJob.versions())))) {
                         JobId testJob = firstDeclaredOrElseImplicitTest(testType);
                         testJobs.merge(testJob,
-                                       List.of(new Job(productionJob.versions(),
+                                       List.of(new Job(testJob.type(),
+                                                       productionJob.versions(),
                                                        jobSteps.get(testJob).readyAt(productionJob.change),
                                                        productionJob.change)),
                                        DeploymentStatus::union);
@@ -872,8 +873,8 @@ public class DeploymentStatus {
         private final Optional<Instant> readyAt;
         private final Change change;
 
-        public Job(Versions versions, Optional<Instant> readyAt, Change change) {
-            this.versions = versions;
+        public Job(JobType type, Versions versions, Optional<Instant> readyAt, Change change) {
+            this.versions = type == systemTest ? versions.withoutSources() : versions;
             this.readyAt = readyAt;
             this.change = change;
         }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index bb8180e9f14..20fa539c820 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.deployment;
 
 import com.yahoo.config.application.api.DeploymentInstanceSpec;
+import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.InstanceName;
 import com.yahoo.text.Text;
@@ -30,6 +31,7 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.OptionalLong;
+import java.util.function.Predicate;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -197,7 +199,7 @@ public class DeploymentTrigger {
 
         DeploymentStatus status = jobs.deploymentStatus(application);
         Versions versions = Versions.from(instance.change(), application, status.deploymentFor(job), controller.readSystemVersion());
-        DeploymentStatus.Job toTrigger = new DeploymentStatus.Job(versions, Optional.of(controller.clock().instant()), instance.change());
+        DeploymentStatus.Job toTrigger = new DeploymentStatus.Job(job.type(), versions, Optional.of(controller.clock().instant()), instance.change());
         Map<JobId, List<DeploymentStatus.Job>> jobs = status.testJobs(Map.of(job, List.of(toTrigger)));
         if (jobs.isEmpty() || ! requireTests)
             jobs = Map.of(job, List.of(toTrigger));
@@ -388,9 +390,13 @@ public class DeploymentTrigger {
     private boolean acceptNewApplicationVersion(DeploymentStatus status, InstanceName instance, ApplicationVersion version) {
         if (status.application().deploymentSpec().instance(instance).isEmpty()) return false; // Unknown instance.
         boolean isChangingRevision = status.application().require(instance).change().application().isPresent();
-        switch (status.application().deploymentSpec().requireInstance(instance).revisionChange()) {
+        DeploymentInstanceSpec spec = status.application().deploymentSpec().requireInstance(instance);
+        Predicate<ApplicationVersion> versionFilter = spec.revisionTarget() == DeploymentSpec.RevisionTarget.next
+                                                      ? failing -> status.application().require(instance).change().application().get().compareTo(failing) == 0
+                                                      : failing -> version.compareTo(failing) > 0;
+        switch (spec.revisionChange()) {
             case whenClear:   return ! isChangingRevision;
-            case whenFailing: return ! isChangingRevision || status.hasFailures(version);
+            case whenFailing: return ! isChangingRevision || status.hasFailures(versionFilter);
             case always:      return true;
             default:          throw new IllegalStateException("Unknown revision upgrade policy");
         }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Versions.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Versions.java
index 1e183d44377..0ea18d9cfa2 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Versions.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/Versions.java
@@ -37,6 +37,11 @@ public class Versions {
         this.sourceApplication = requireNonNull(sourceApplication);
     }
 
+    /** A copy of this, without source versions. */
+    public Versions withoutSources() {
+        return new Versions(targetPlatform, targetApplication, Optional.empty(), Optional.empty());
+    }
+
     /** Target platform version for this */
     public Version targetPlatform() {
         return targetPlatform;
@@ -98,36 +103,36 @@ public class Versions {
     }
 
     /** Create versions using given change and application */
+    public static Versions from(Change change, Application application, Optional<Version> existingPlatform,
+                                Optional<ApplicationVersion> existingApplication, Version defaultPlatformVersion) {
+        return new Versions(targetPlatform(application, change, existingPlatform, defaultPlatformVersion),
+                            targetApplication(application, change, existingApplication),
+                            existingPlatform,
+                            existingApplication);
+    }
+
+    /** Create versions using given change and application */
     public static Versions from(Change change, Application application, Optional<Deployment> deployment,
                                 Version defaultPlatformVersion) {
-        return new Versions(targetPlatform(application, change, deployment, defaultPlatformVersion),
-                            targetApplication(application, change, deployment),
+        return new Versions(targetPlatform(application, change, deployment.map(Deployment::version), defaultPlatformVersion),
+                            targetApplication(application, change, deployment.map(Deployment::applicationVersion)),
                             deployment.map(Deployment::version),
                             deployment.map(Deployment::applicationVersion));
     }
 
-    public static Versions from(Change change, Deployment deployment) {
-        return new Versions(change.platform().filter(version -> change.isPinned() || deployment.version().isBefore(version))
-                                  .orElse(deployment.version()),
-                            change.application().filter(version -> deployment.applicationVersion().compareTo(version) < 0)
-                                  .orElse(deployment.applicationVersion()),
-                            Optional.of(deployment.version()),
-                            Optional.of(deployment.applicationVersion()));
-    }
-
-    private static Version targetPlatform(Application application, Change change, Optional<Deployment> deployment,
+    private static Version targetPlatform(Application application, Change change, Optional<Version> existing,
                                           Version defaultVersion) {
         if (change.isPinned() && change.platform().isPresent())
             return change.platform().get();
 
-        return max(change.platform(), deployment.map(Deployment::version))
+        return max(change.platform(), existing)
                 .orElseGet(() -> application.oldestDeployedPlatform().orElse(defaultVersion));
     }
 
     private static ApplicationVersion targetApplication(Application application, Change change,
-                                                        Optional<Deployment> deployment) {
+                                                        Optional<ApplicationVersion> existing) {
         return change.application()
-                     .or(() -> deployment.map(Deployment::applicationVersion))
+                     .or(() -> existing)
                      .orElseGet(() -> defaultApplicationVersion(application));
     }
 
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 bfedd325ae7..805d727d355 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
@@ -251,9 +251,6 @@ public class DeploymentTriggerTest {
                                                                        .build();
         DeploymentContext app = tester.newDeploymentContext()
                                       .submit(appPackage);
-        Optional<ApplicationVersion> revision0 = app.lastSubmission();
-
-        app.submit(appPackage);
         Optional<ApplicationVersion> revision1 = app.lastSubmission();
 
         app.submit(appPackage);
@@ -262,17 +259,47 @@ public class DeploymentTriggerTest {
         app.submit(appPackage);
         Optional<ApplicationVersion> revision3 = app.lastSubmission();
 
-        assertEquals(revision0, app.instance().change().application());
-        assertEquals(revision1, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
+        app.submit(appPackage);
+        Optional<ApplicationVersion> revision4 = app.lastSubmission();
+
+        app.submit(appPackage);
+        Optional<ApplicationVersion> revision5 = app.lastSubmission();
 
-        tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(revision1.get()));
+        // 5 revisions submitted; the first is rolling out, and the others are queued.
+        tester.outstandingChangeDeployer().run();
         assertEquals(revision1, app.instance().change().application());
         assertEquals(revision2, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
 
-        app.deploy();
+        // The second revision is set as the target by user interaction.
+        tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(revision2.get()));
         tester.outstandingChangeDeployer().run();
         assertEquals(revision2, app.instance().change().application());
         assertEquals(revision3, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
+
+        // The second revision deploys completely, and the third starts rolling out.
+        app.runJob(systemTest).runJob(stagingTest)
+           .runJob(productionUsEast3);
+        tester.outstandingChangeDeployer().run();
+        tester.outstandingChangeDeployer().run();
+        assertEquals(revision3, app.instance().change().application());
+        assertEquals(revision4, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
+
+        // The third revision fails, and the fourth is chosen to replace it.
+        app.triggerJobs().timeOutConvergence(systemTest);
+        tester.outstandingChangeDeployer().run();
+        tester.outstandingChangeDeployer().run();
+        assertEquals(revision4, app.instance().change().application());
+        assertEquals(revision5, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
+
+        // Tests for outstanding change are relevant when current revision completes.
+        app.runJob(systemTest).runJob(systemTest)
+           .jobAborted(stagingTest).runJob(stagingTest).runJob(stagingTest)
+           .runJob(productionUsEast3);
+        tester.outstandingChangeDeployer().run();
+        tester.outstandingChangeDeployer().run();
+        assertEquals(revision5, app.instance().change().application());
+        assertEquals(Change.empty(), app.deploymentStatus().outstandingChange(InstanceName.defaultName()));
+        app.runJob(productionUsEast3);
     }
 
     @Test
@@ -1810,7 +1837,8 @@ public class DeploymentTriggerTest {
         // System and staging tests both require unknown versions, and are broken.
         tester.controller().applications().deploymentTrigger().forceTrigger(app.instanceId(), productionCdUsEast1, "user", false);
         app.runJob(productionCdUsEast1)
-           .abortJob(systemTest)
+           .triggerJobs()
+           .jobAborted(systemTest)
            .jobAborted(stagingTest)
            .runJob(systemTest)  // Run test for aws zone again.
            .runJob(stagingTest) // Run test for aws zone again.
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-overview-2.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-overview-2.json
index b05be36a24a..ed82039d923 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-overview-2.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/deployment-overview-2.json
@@ -144,13 +144,6 @@
               "compileVersion": "6.1.0",
               "sourceUrl": "repository1/tree/commit1",
               "commit": "commit1"
-            },
-            "sourcePlatform": "6.1.0",
-            "sourceApplication": {
-              "build": 2,
-              "compileVersion": "6.1.0",
-              "sourceUrl": "repository1/tree/commit1",
-              "commit": "commit1"
             }
           },
           "steps": [
@@ -209,13 +202,6 @@
               "compileVersion": "6.1.0",
               "sourceUrl": "repository1/tree/commit1",
               "commit": "commit1"
-            },
-            "sourcePlatform": "6.1.0",
-            "sourceApplication": {
-              "build": 1,
-              "compileVersion": "6.1.0",
-              "sourceUrl": "repository1/tree/commit1",
-              "commit": "commit1"
             }
           },
           "steps": [
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/responses/root.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/responses/root.json
index c5286d4a04b..211aa57d8ed 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/responses/root.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/responses/root.json
@@ -194,7 +194,7 @@
             {
               "name": "system-test",
               "coolingDownUntil": "(ignore)",
-              "pending": "platform"
+              "pending": "application"
             },
             {
               "name": "staging-test",