Let "separate" revision upgrade strategy wait for previous, unless they fail

3 files changed, 49 insertions, 6 deletions

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 d3f2d4e4501..9790ece421e 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
@@ -364,11 +364,13 @@ public class DeploymentTrigger {
     // ---------- Change management o_O ----------
 
     private boolean acceptNewApplicationVersion(DeploymentStatus status, InstanceName instance) {
-        if (status.application().require(instance).change().application().isPresent()) return true; // Replacing a previous application change is ok.
-        if (status.hasFailures()) return true; // Allow changes to fix upgrade problems.
-        if (status.application().deploymentSpec().instance(instance) // Leading upgrade allows app change to join in.
-                  .map(spec -> spec.upgradeRollout() == DeploymentSpec.UpgradeRollout.leading).orElse(false)) return true;
-        return status.application().require(instance).change().platform().isEmpty();
+        if (status.application().deploymentSpec().instance(instance).isEmpty()) return false; // Unknown instance.
+        if (status.hasFailures()) 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();
+        if (change.platform().isPresent() && spec.upgradeRollout() == DeploymentSpec.UpgradeRollout.separate) return false;
+        if (change.application().isPresent() && spec.upgradeRevision() == DeploymentSpec.UpgradeRevision.separate) return false;
+        return true;
     }
 
     private Instance withRemainingChange(Instance instance, Change change, DeploymentStatus status) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index a338efd856c..bc5a70b1fa0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -57,6 +57,7 @@ public class ApplicationPackageBuilder {
     private OptionalInt majorVersion = OptionalInt.empty();
     private String instances = "default";
     private String upgradePolicy = null;
+    private String upgradeRevision = "latest";
     private String upgradeRollout = null;
     private String globalServiceId = null;
     private String athenzIdentityAttributes = "athenz-domain='domain' athenz-service='service'";
@@ -80,6 +81,11 @@ public class ApplicationPackageBuilder {
         return this;
     }
 
+    public ApplicationPackageBuilder upgradeRevision(String upgradeRevision) {
+        this.upgradeRevision = upgradeRevision;
+        return this;
+    }
+
     public ApplicationPackageBuilder upgradeRollout(String upgradeRollout) {
         this.upgradeRollout = upgradeRollout;
         return this;
@@ -253,9 +259,10 @@ public class ApplicationPackageBuilder {
         }
         xml.append(">\n");
         xml.append("  <instance id='").append(instances).append("'>\n");
-        if (upgradePolicy != null || upgradeRollout != null) {
+        if (upgradePolicy != null || upgradeRevision != null || upgradeRollout != null) {
             xml.append("    <upgrade ");
             if (upgradePolicy != null) xml.append("policy='").append(upgradePolicy).append("' ");
+            if (upgradeRevision != null) xml.append("revision='").append(upgradeRevision).append("' ");
             if (upgradeRollout != null) xml.append("rollout='").append(upgradeRollout).append("' ");
             xml.append("/>\n");
         }
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 52521775ddd..cded075a7f3 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
@@ -107,6 +107,40 @@ public class DeploymentTriggerTest {
     }
 
     @Test
+    public void separateRevisionMakesApplicationChangeWaitForPreviousToComplete() {
+        DeploymentContext app = tester.newDeploymentContext();
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .upgradeRevision(null) // separate by default, but we override this in test builder
+                .region("us-east-3")
+                .test("us-east-3")
+                .build();
+
+        app.submit(applicationPackage).runJob(systemTest).runJob(stagingTest).runJob(productionUsEast3);
+        Optional<ApplicationVersion> v0 = app.lastSubmission();
+
+        app.submit(applicationPackage);
+        Optional<ApplicationVersion> v1 = app.lastSubmission();
+        assertEquals(v0, app.instance().change().application());
+
+        // Eager tests still run before new revision rolls out.
+        app.runJob(systemTest).runJob(stagingTest);
+
+        // v0 rolls out completely.
+        app.runJob(testUsEast3);
+        assertEquals(Optional.empty(), app.instance().change().application());
+
+        // v1 starts rolling when v0 is done.
+        tester.outstandingChangeDeployer().run();
+        assertEquals(v1, app.instance().change().application());
+
+        // v1 fails, so v2 starts immediately.
+        app.runJob(productionUsEast3).failDeployment(testUsEast3);
+        app.submit(applicationPackage);
+        Optional<ApplicationVersion> v2 = app.lastSubmission();
+        assertEquals(v2, app.instance().change().application());
+    }
+
+    @Test
     public void leadingUpgradeAllowsApplicationChangeWhileUpgrading() {
         var applicationPackage = new ApplicationPackageBuilder().region("us-east-3")
                                                                 .upgradeRollout("leading")