Add revision upgrade policy "exclusive" which _never_ joins commits

3 files changed, 45 insertions, 23 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 f03d32aa838..5742e0a8816 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
@@ -36,11 +36,13 @@ import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static com.yahoo.config.application.api.DeploymentSpec.UpgradeRevision.exclusive;
 import static com.yahoo.config.provision.Environment.prod;
 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;
@@ -247,23 +249,24 @@ public class DeploymentStatus {
     }
 
     /**
-     * The change of this application's latest submission, if this upgrades any of its production deployments,
-     * and has not yet started rolling out, due to some other change or a block window being present at the time of submission.
+     * The change to a revision which all dependencies of the given instance has completed,
+     * which does not downgrade any deployments in the instance,
+     * which is not already rolling out to the instance, and
+     * which causes at least one job to run if deployed to the instance.
+     * For the "exclusive" revision upgrade policy it is the oldest such revision; otherwise, it is the latest.
      */
     public Change outstandingChange(InstanceName instance) {
-        return nextVersion(instance).map(Change::of)
-                          .filter(change -> application.require(instance).change().application().map(change::upgrades).orElse(true))
-                          .filter(change -> ! jobsToRun(Map.of(instance, change)).isEmpty())
-                          .orElse(Change.empty());
-    }
-
-    /** The next application version to roll out to instance. */
-    private Optional<ApplicationVersion> nextVersion(InstanceName instance) {
-        return Optional.ofNullable(instanceSteps().get(instance)).stream()
-                       .flatMap(this::allDependencies)
-                       .flatMap(step -> step.instance.latestDeployed().stream())
-                       .min(naturalOrder())
-                       .or(application::latestVersion);
+        return Optional.ofNullable(instanceSteps().get(instance))
+                       .flatMap(instanceStatus -> application.versions().stream()
+                                                             .sorted(application.deploymentSpec().requireInstance(instance).upgradeRevision() == exclusive ? naturalOrder() : reverseOrder())
+                                                             .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 -> ! jobsToRun(Map.of(instance, change)).isEmpty())
+                                                             .findFirst())
+                       .orElse(Change.empty());
     }
 
     private Stream<InstanceStatus> allDependencies(StepStatus step) {
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 3be21aec608..ade3c9cbaf5 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
@@ -388,10 +388,13 @@ public class DeploymentTrigger {
 
     private boolean acceptNewApplicationVersion(DeploymentStatus status, InstanceName instance, ApplicationVersion version) {
         if (status.application().deploymentSpec().instance(instance).isEmpty()) return false; // Unknown instance.
-        if (status.hasFailures(version)) return true; // Allow changes to fix upgrade or previous revision problems.
-        DeploymentInstanceSpec spec = status.application().deploymentSpec().requireInstance(instance);
-        Change change = status.application().require(instance).change();
-        return change.application().isEmpty() || spec.upgradeRevision() != DeploymentSpec.UpgradeRevision.separate;
+        boolean isChangingRevision = status.application().require(instance).change().application().isPresent();
+        switch (status.application().deploymentSpec().requireInstance(instance).upgradeRevision()) {
+            case exclusive: return ! isChangingRevision;
+            case separate:  return ! isChangingRevision || status.hasFailures(version);
+            case latest:    return true;
+            default:        throw new IllegalStateException("Unknown revision upgrade policy");
+        }
     }
 
     private Instance withRemainingChange(Instance instance, Change change, DeploymentStatus status) {
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 67fa2f87794..925aabf76e3 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
@@ -1439,7 +1439,7 @@ public class DeploymentTriggerTest {
                 "        </prod>\n" +
                 "    </instance>\n" +
                 "    <instance id='gamma'>\n" +
-                "        <upgrade revision='separate' />\n" + // TODO: change to new, even stricter policy.
+                "        <upgrade revision='exclusive' />\n" +
                 "        <prod>\n" +
                 "            <region>us-east-3</region>\n" +
                 "            <test>us-east-3</test>\n" +
@@ -1506,14 +1506,30 @@ public class DeploymentTriggerTest {
         assertEquals(Optional.empty(), alpha.instance().change().application());
         assertEquals(revision6, beta.instance().change().application());
 
-        // revision6 rolls through beta, but revision3 is the next target for the strictest revision policy, in gamma
+        // revision6 rolls through beta, but revision3 is the next target for gamma with "exclusive" revision upgrades
         alpha.jobAborted(stagingTest).runJob(stagingTest);
         beta.runJob(productionUsEast3).runJob(testUsEast3);
-        gamma.runJob(productionUsEast3).runJob(testUsEast3);
+
+        // revision 2 fails, but this does not bring on revision 3
+        gamma.failDeployment(productionUsEast3);
+        tester.outstandingChangeDeployer().run();
+        assertEquals(Optional.empty(), beta.instance().change().application());
+        assertEquals(revision2, gamma.instance().change().application());
+
+        // revision 2 completes
+        gamma.runJob(productionUsEast3)
+             .runJob(testUsEast3);
         tester.outstandingChangeDeployer().run();
         assertEquals(Optional.empty(), alpha.instance().change().application());
         assertEquals(Optional.empty(), beta.instance().change().application());
-        // TODO: assertEquals(revision3, gamma.instance().change().application());
+        assertEquals(revision3, gamma.instance().change().application());
+
+        // revision 6 is next, once 3 is done
+        // revision 3 completes
+        gamma.runJob(productionUsEast3)
+             .runJob(testUsEast3);
+        tester.outstandingChangeDeployer().run();
+        assertEquals(revision6, gamma.instance().change().application());
     }
 
     @Test