Merge pull request #20792 from vespa-engine/jonmv/allow-older-application-deployments

Jonmv/allow older application deployments

6 files changed, 33 insertions, 27 deletions

diff --git a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
index ab638a1da7d..db86df88fc5 100644
--- a/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
+++ b/clustercontroller-core/src/test/java/com/yahoo/vespa/clustercontroller/core/MasterElectionTest.java
@@ -283,7 +283,7 @@ public class MasterElectionTest extends FleetControllerTest {
         waitForCompleteCycles();
         timer.advanceTime(options.zooKeeperSessionTimeout);
         waitForZookeeperDisconnected();
-        // Noone can be master if server is unavailable
+        // No one can be master if server is unavailable
         log.log(Level.INFO, "Checking master status");
         for (int i=0; i<fleetControllers.size(); ++i) {
             assertFalse("Index " + i, fleetControllers.get(i).isMaster());
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/ApplicationVersion.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/ApplicationVersion.java
index a0dee6c059f..3a863f21ca0 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/ApplicationVersion.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/deployment/ApplicationVersion.java
@@ -175,7 +175,7 @@ public class ApplicationVersion implements Comparable<ApplicationVersion> {
             return Boolean.compare(buildNumber().isPresent(), o.buildNumber.isPresent()); // Unknown version sorts first
 
         if (deployedDirectly || o.deployedDirectly)
-            return Boolean.compare(deployedDirectly, o.deployedDirectly); // Directly deployed versions sort first
+            return Boolean.compare( ! deployedDirectly, ! o.deployedDirectly); // Directly deployed versions sort first
 
         return Long.compare(buildNumber().getAsLong(), o.buildNumber().getAsLong());
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index a23cb40dcb1..d710a8a2948 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -370,7 +370,6 @@ public class ApplicationController {
             try (Lock lock = lock(applicationId)) {
                 LockedApplication application = new LockedApplication(requireApplication(applicationId), lock);
                 Instance instance = application.get().require(job.application().instance());
-                rejectOldChange(instance, platform, revision, job, zone);
 
                 if (   ! applicationPackage.trustedCertificates().isEmpty()
                     &&   run.testerCertificate().isPresent())
@@ -801,20 +800,6 @@ public class ApplicationController {
         }
     }
 
-    private void rejectOldChange(Instance instance, Version platform, ApplicationVersion revision, JobId job, ZoneId zone) {
-        Deployment deployment = instance.deployments().get(zone);
-        if (deployment == null) return;
-        if (!zone.environment().isProduction()) return;
-
-        boolean platformIsOlder = platform.compareTo(deployment.version()) < 0 && !instance.change().isPinned();
-        boolean revisionIsOlder = revision.compareTo(deployment.applicationVersion()) < 0 &&
-                                  !(revision.isUnknown() && controller.system().isCd());
-        if (platformIsOlder || revisionIsOlder)
-            throw new IllegalArgumentException(Text.format("Rejecting deployment of application %s to %s, as the requested versions (platform: %s, application: %s)" +
-                                                             " are older than the currently deployed (platform: %s, application: %s).",
-                                                             job.application(), zone, platform, revision, deployment.version(), deployment.applicationVersion()));
-    }
-
     private TenantAndApplicationId dashToUnderscore(TenantAndApplicationId id) {
         return TenantAndApplicationId.from(id.tenant().value(), id.application().value().replaceAll("-", "_"));
     }
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 ce03e84f2b9..e7521b37dbf 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
@@ -26,6 +26,7 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -612,6 +613,9 @@ public class DeploymentStatus {
                         && ! existingDeployment.map(Deployment::version).equals(change.platform()))
                         return Optional.empty();
 
+                    if (change.application().isPresent() && ! existingDeployment.map(Deployment::applicationVersion).equals(change.application()))
+                        return Optional.empty();
+
                     Change fullChange = status.application().require(instance).change();
                     if (existingDeployment.map(deployment ->    ! (change.upgrades(deployment.version()) || change.upgrades(deployment.applicationVersion()))
                                                              &&   (fullChange.downgrades(deployment.version()) || fullChange.downgrades(deployment.applicationVersion())))
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 779ce6fa7fe..1e183d44377 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
@@ -126,8 +126,9 @@ public class Versions {
 
     private static ApplicationVersion targetApplication(Application application, Change change,
                                                         Optional<Deployment> deployment) {
-        return max(change.application(), deployment.map(Deployment::applicationVersion))
-                .orElseGet(() -> defaultApplicationVersion(application));
+        return change.application()
+                     .or(() -> deployment.map(Deployment::applicationVersion))
+                     .orElseGet(() -> defaultApplicationVersion(application));
     }
 
     private static ApplicationVersion defaultApplicationVersion(Application 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 102dfde16ec..fd7ba8693e2 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
@@ -465,6 +465,23 @@ public class DeploymentTriggerTest {
     }
 
     @Test
+    public void downgradingApplicationVersionWorks() {
+        var app = tester.newDeploymentContext().submit().deploy();
+        ApplicationVersion appVersion0 = app.lastSubmission().get();
+        app.submit().deploy();
+
+        // Downgrading application version.
+        tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(appVersion0));
+        assertEquals(Change.of(appVersion0), app.instance().change());
+        app.runJob(stagingTest)
+           .runJob(productionUsCentral1)
+           .runJob(productionUsEast3)
+           .runJob(productionUsWest1);
+        assertEquals(Change.empty(), app.instance().change());
+        assertEquals(appVersion0, app.instance().deployments().get(productionUsEast3.zone(tester.controller().system())).applicationVersion());
+    }
+
+    @Test
     public void settingANoOpChangeIsANoOp() {
         var app = tester.newDeploymentContext().submit().deploy();
         ApplicationVersion appVersion0 = app.lastSubmission().get();
@@ -473,8 +490,6 @@ public class DeploymentTriggerTest {
 
         // Triggering a roll-out of an already deployed application is a no-op.
         assertEquals(Change.empty(), app.instance().change());
-        tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(appVersion0));
-        assertEquals(Change.empty(), app.instance().change());
         tester.deploymentTrigger().forceChange(app.instanceId(), Change.of(appVersion1));
         assertEquals(Change.empty(), app.instance().change());
     }
@@ -1114,9 +1129,10 @@ public class DeploymentTriggerTest {
         tester.controller().applications().deploymentTrigger().forceTrigger(app.instanceId(), productionCdUsEast1, "user", false);
         app.runJob(productionCdUsEast1)
            .abortJob(stagingTest) // Complete failing run.
-           .runJob(stagingTest)
+           .runJob(stagingTest)   // Run staging-test for production zone with no prior deployment.
            .runJob(productionCdAwsUsEast1a);
 
+        // Manually deploy to east again, then upgrade the system.
         app.runJob(productionCdUsEast1, cdPackage);
         var version = new Version("7.1");
         tester.controllerTester().upgradeSystem(version);
@@ -1124,16 +1140,16 @@ 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)
-           .jobAborted(systemTest)
+           .abortJob(systemTest)
            .jobAborted(stagingTest)
-           .runJob(systemTest)
-           .runJob(stagingTest)
+           .runJob(systemTest)  // Run test for aws zone again.
+           .runJob(stagingTest) // Run test for aws zone again.
            .runJob(productionCdAwsUsEast1a);
 
+        // Deploy manually again, then submit new package.
         app.runJob(productionCdUsEast1, cdPackage);
         app.submit(cdPackage);
-        app.jobAborted(systemTest)
-           .runJob(systemTest);
+        app.runJob(systemTest);
         // Staging test requires unknown initial version, and is broken.
         tester.controller().applications().deploymentTrigger().forceTrigger(app.instanceId(), productionCdUsEast1, "user", false);
         app.runJob(productionCdUsEast1)