Allow application change during failing upgrade

Due to the change in how we handle application version, we could end up ignoring
application changes that arrive while upgrade is failing.

3 files changed, 59 insertions, 2 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
index 603f16a9ef6..0b50ec36a32 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/Change.java
@@ -98,4 +98,8 @@ public final class Change {
         return new Change(Optional.of(platformChange), Optional.empty());
     }
 
+    public static Change of(Version platformChange, ApplicationVersion applicationVersion) {
+        return new Change(Optional.of(platformChange), Optional.of(applicationVersion));
+    }
+
 }
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 8c93d53ea9e..70c1702d0b5 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
@@ -86,8 +86,12 @@ public class DeploymentTrigger {
             if (report.success()) {
                 if (report.jobType() == JobType.component) {
                     if (acceptNewApplicationVersionNow(application)) {
-                        // Set this as the change we are doing, unless we are already pushing a platform change
-                        if ( ! ( application.change().platform().isPresent())) {
+                        // If there's an ongoing upgrade, we allow the upgrade and new application package to be
+                        // deployed together
+                        if (application.change().platform().isPresent()) {
+                            application = application.withChange(Change.of(application.change().platform().get(),
+                                                                           applicationVersion));
+                        } else {
                             application = application.withChange(Change.of(applicationVersion));
                         }
                     }
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 7c4dd58fa33..e13f4fca97c 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
@@ -837,4 +837,53 @@ public class UpgraderTest {
         assertTrue("All jobs consumed", tester.buildSystem().jobs().isEmpty());
     }
 
+    @Test
+    public void testAllowApplicationChangeDuringFailingUpgrade() {
+        DeploymentTester tester = new DeploymentTester();
+        Version version = Version.fromString("5.0");
+        tester.updateVersionStatus(version);
+
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .environment(Environment.prod)
+                .region("us-west-1")
+                .build();
+
+        Application app = tester.createAndDeploy("app1", 1, applicationPackage);
+
+        // New version is released
+        version = Version.fromString("5.1");
+        tester.updateVersionStatus(version);
+        tester.upgrader().maintain();
+
+        tester.deployAndNotify(app, applicationPackage, true, systemTest);
+        tester.deployAndNotify(app, applicationPackage, true, stagingTest);
+
+        // Production job fails and exhausts retries, new application changes are now accepted
+        tester.deployAndNotify(app, applicationPackage, false, productionUsWest1);
+        tester.clock().advance(Duration.ofHours(1));
+        tester.deployAndNotify(app, applicationPackage, false, productionUsWest1);
+
+        // New application change
+        tester.jobCompletion(component).application(app).buildNumber(43).uploadArtifact(applicationPackage).submit();
+        String applicationVersion = "1.0.43-commit1";
+
+        // Application change recorded together with ongoing upgrade
+        app = tester.application(app.id());
+        assertTrue("Change contains both upgrade and application change",
+                   app.change().platform().get().equals(version) &&
+                   app.change().application().get().id().equals(applicationVersion));
+
+        // Deployment completes
+        tester.deployAndNotify(app, applicationPackage, true, systemTest);
+        tester.deployAndNotify(app, applicationPackage, true, stagingTest);
+        tester.deployAndNotify(app, applicationPackage, true, productionUsWest1);
+        assertTrue("All jobs consumed", tester.buildSystem().jobs().isEmpty());
+
+        app = tester.application(app.id());
+        for (Deployment deployment : app.deployments().values()) {
+            assertEquals(version, deployment.version());
+            assertEquals(applicationVersion, deployment.applicationVersion().id());
+        }
+    }
+
 }