Allow cancellation of all change from web API and fix bug

5 files changed, 30 insertions, 18 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 ba444d274b3..0f27131aaad 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
@@ -36,6 +36,7 @@ import java.util.logging.Logger;
  *
  * @author bratseth
  * @author mpolden
+ * @author jvenstad
  */
 public class DeploymentTrigger {
 
@@ -99,6 +100,7 @@ public class DeploymentTrigger {
             }
             else if (report.jobType().isProduction() && deploymentComplete(application)) {
                 // change completed
+                // TODO jvenstad: Check for and remove individual parts of Change.
                 application = application.withChange(Change.empty());
             }
 
@@ -242,12 +244,14 @@ public class DeploymentTrigger {
      *
      * @param applicationId the application to trigger
      */
-    public void cancelChange(ApplicationId applicationId) {
+    public void cancelChange(ApplicationId applicationId, boolean keepApplicationChange) {
         applications().lockOrThrow(applicationId, application -> {
-            deploymentQueue.removeJobs(application.id());
             applications().store(application.withChange(application.change().application()
-                                                                .map(Change::of)
-                                                                .orElse(Change.empty())));
+                                                                   .map(Change::of)
+                                                                   .filter(change -> keepApplicationChange)
+                                                                   .orElse(Change.empty())));
+            if ( ! applications().require(applicationId).change().isPresent())
+                deploymentQueue.removeJobs(application.id());
         });
     }
 
@@ -365,8 +369,11 @@ public class DeploymentTrigger {
     }
 
     /**
-     * Returns whether the currently deployed application in the zone for the given production job already
-     * has the current changes, in which case we should avoid an unsupported downgrade, or an unnecessary redeploy.
+     * Returns whether the given application should skip deployment of its current change to the given production job zone.
+     *
+     * If the currently deployed application has a newer platform or application version than the application's
+     * current change, the method returns {@code true}, to avoid a downgrade.
+     * Otherwise, it returns whether the current change is redundant, i.e., all its components are already deployed.
      */
     private boolean changeDeployed(Application application, JobType job) {
         if ( ! job.isProduction())
@@ -376,13 +383,18 @@ public class DeploymentTrigger {
         if (deployment == null)
             return false;
 
-        if (application.change().platform().filter(version -> version.isAfter(deployment.version())).isPresent())
-            return false;
+        int applicationComparison = application.change().application()
+                                               .map(version -> version.compareTo(deployment.applicationVersion()))
+                                               .orElse(0);
 
-        if (application.change().application().filter(version -> version.compareTo(deployment.applicationVersion()) > 0).isPresent())
-            return false;
+        int platformComparion = application.change().platform()
+                                           .map(version -> version.compareTo(deployment.version()))
+                                           .orElse(0);
 
-        return true;
+        if (applicationComparison == -1 || platformComparion == -1)
+            return true; // Avoid downgrades!
+
+        return applicationComparison == 0 && platformComparion == 0;
     }
 
     private boolean acceptNewApplicationVersionNow(LockedApplication application) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index 8c661e7db9d..535e5a71fb6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -109,7 +109,7 @@ public class Upgrader extends Maintainer {
         if (applications.isEmpty()) return;
         log.info("Cancelling upgrading of " + applications.asList().size() + " applications: " + reason);
         for (Application application : applications.asList())
-            controller().applications().deploymentTrigger().cancelChange(application.id());
+            controller().applications().deploymentTrigger().cancelChange(application.id(), true);
     }
 
     /** Returns the number of applications to upgrade in this run */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index 337ad934d41..e85157a57c2 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -712,7 +712,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
             return new MessageResponse("No deployment in progress for " + application + " at this time");
 
         controller.applications().lockOrThrow(id, lockedApplication ->
-            controller.applications().deploymentTrigger().cancelChange(id));
+            controller.applications().deploymentTrigger().cancelChange(id, false));
 
         return new MessageResponse("Cancelled " + change + " for " + 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 33fb1b39bb7..19a21eb8f09 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
@@ -447,7 +447,7 @@ public class DeploymentTriggerTest {
         assertEquals(appVersion1, app.get().deployments().get(ZoneId.from("prod.us-central-1")).applicationVersion());
 
         // Verify the application change is not removed when change is cancelled.
-        tester.deploymentTrigger().cancelChange(application.id());
+        tester.deploymentTrigger().cancelChange(application.id(), true);
         assertEquals(Change.of(appVersion1), app.get().change());
 
         // Now cancel the change -- this should not normally happen.
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 4481c1201d8..088895408fc 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
@@ -438,10 +438,10 @@ public class UpgraderTest {
 
         // We "manually" cancel upgrades to V1 so that we can use the applications to make V2 fail instead
         // But we keep one (default4) to avoid V1 being garbage collected
-        tester.deploymentTrigger().cancelChange(default0.id());
-        tester.deploymentTrigger().cancelChange(default1.id());
-        tester.deploymentTrigger().cancelChange(default2.id());
-        tester.deploymentTrigger().cancelChange(default3.id());
+        tester.deploymentTrigger().cancelChange(default0.id(), false);
+        tester.deploymentTrigger().cancelChange(default1.id(), false);
+        tester.deploymentTrigger().cancelChange(default2.id(), false);
+        tester.deploymentTrigger().cancelChange(default3.id(), false);
         tester.clock().advance(Duration.ofHours(13)); // Currently we don't cancel running jobs, so this is necessary to allow a new triggering below
 
         // Canaries upgrade and raise confidence of V2