Only roll "next" revision when current is failing

3 files changed, 36 insertions, 12 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..7e810037804 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();
     }
 
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..55b9dea7869 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;
@@ -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/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..070782e5888 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,37 @@ 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();
 
-        tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(revision1.get()));
+        app.submit(appPackage);
+        Optional<ApplicationVersion> revision5 = app.lastSubmission();
+
+        // 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().jobAborted(systemTest).jobAborted(stagingTest).timeOutConvergence(systemTest);
+        tester.outstandingChangeDeployer().run();
+        tester.outstandingChangeDeployer().run();
+        assertEquals(revision4, app.instance().change().application());
+        assertEquals(revision5, app.deploymentStatus().outstandingChange(InstanceName.defaultName()).application());
     }
 
     @Test