More flexibly change change

7 files changed, 77 insertions, 61 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 aeb9e6fa453..6dbdefb0913 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
@@ -222,35 +222,46 @@ public class DeploymentTrigger {
                 .map(Job::jobType).collect(toList());
     }
 
-    /**
-     * Triggers a change of this application
-     *
-     * @param applicationId the application to trigger
-     * @throws IllegalArgumentException if this application already has an ongoing change
-     */
+    /** Triggers a change of this application, unless it already has a change. */
     public void triggerChange(ApplicationId applicationId, Change change) {
         applications().lockOrThrow(applicationId, application -> {
-            if (application.get().change().isPresent() && ! application.get().deploymentJobs().hasFailures())
-                throw new IllegalArgumentException("Could not start " + change + " on " + application + ": " +
-                                                   application.get().change() + " is already in progress");
-            application = application.withChange(change);
-            if (change.application().isPresent())
-                application = application.withOutstandingChange(Change.empty());
+            if ( ! application.get().change().isPresent()) {
+                if (change.application().isPresent())
+                    application = application.withOutstandingChange(Change.empty());
 
-            applications().store(application);
+                applications().store(application.withChange(change));
+            }
+        });
+    }
+
+    /** Overrides the given application's platform and application changes with any contained in the given change. */
+    public void forceChange(ApplicationId applicationId, Change change) {
+        applications().lockOrThrow(applicationId, application -> {
+            Change current = application.get().change();
+            if (change.platform().isPresent())
+                current = current.with(change.platform().get());
+            if (change.application().isPresent())
+                current = current.with(change.application().get());
+            applications().store(application.withChange(current));
         });
     }
 
     /** Cancels a platform upgrade of the given application, and an application upgrade as well if {@code keepApplicationChange}. */
-    public void cancelChange(ApplicationId applicationId, boolean keepApplicationChange) {
+    public void cancelChange(ApplicationId applicationId, ChangesToCancel cancellation) {
         applications().lockOrThrow(applicationId, application -> {
-            applications().store(application.withChange(application.get().change().application()
-                                                                   .filter(__ -> keepApplicationChange)
-                                                                   .map(Change::of)
-                                                                   .orElse(Change.empty())));
+            Change change;
+            switch (cancellation) {
+                case ALL: change = Change.empty(); break;
+                case PLATFORM: change = application.get().change().withoutPlatform(); break;
+                case APPLICATION: change = application.get().change().withoutApplication(); break;
+                default: throw new IllegalArgumentException("Unknown cancellation choice '" + cancellation + "'!");
+            }
+            applications().store(application.withChange(change));
         });
     }
 
+    public enum ChangesToCancel { ALL, PLATFORM, APPLICATION }
+
     // ---------- Conveniences ----------
 
     private ApplicationController applications() {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
index 0c267bd5f6a..2b317032c52 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployer.java
@@ -20,9 +20,8 @@ public class OutstandingChangeDeployer extends Maintainer {
     @Override
     protected void maintain() {
         for (Application application : controller().applications().asList()) {
-            if (   ! application.change().isPresent() // TODO jvenstad: Revisit this: should it check for only platform here?
-                &&   application.outstandingChange().isPresent()
-                &&   application.deploymentSpec().canChangeRevisionAt(controller().clock().instant())) {
+            if (   application.outstandingChange().isPresent()
+                && application.deploymentSpec().canChangeRevisionAt(controller().clock().instant())) {
                 controller().applications().deploymentTrigger().triggerChange(application.id(),
                                                                               application.outstandingChange());
             }
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 702df58ab98..66481e88a98 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
@@ -7,6 +7,8 @@ import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.application.ApplicationList;
+import com.yahoo.vespa.hosted.controller.application.Change;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion.Confidence;
@@ -22,6 +24,8 @@ import java.util.Optional;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
+import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.PLATFORM;
+
 /**
  * Maintenance job which schedules applications for Vespa version upgrade
  *
@@ -100,20 +104,15 @@ public class Upgrader extends Maintainer {
         applications = applications.canUpgradeAt(controller().clock().instant()); // wait with applications that are currently blocking upgrades
         applications = applications.byIncreasingDeployedVersion(); // start with lowest versions
         applications = applications.first(numberOfApplicationsToUpgrade()); // throttle upgrades
-        for (Application application : applications.asList()) {
-            try {
-                controller().applications().deploymentTrigger().triggerChange(application.id(), application.change().with(version));
-            } catch (IllegalArgumentException e) {
-                log.log(Level.INFO, "Could not trigger change: " + Exceptions.toMessageString(e));
-            }
-        }
+        for (Application application : applications.asList())
+            controller().applications().deploymentTrigger().triggerChange(application.id(), Change.of(version));
     }
 
     private void cancelUpgradesOf(ApplicationList applications, String reason) {
         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(), true);
+            controller().applications().deploymentTrigger().cancelChange(application.id(), PLATFORM);
     }
 
     /** 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 ff9aca9ee00..a0d3b8ff0e4 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
@@ -65,6 +65,7 @@ import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.RotationStatus;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.athenz.impl.ZmsClientFacade;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel;
 import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse;
 import com.yahoo.vespa.hosted.controller.restapi.MessageResponse;
 import com.yahoo.vespa.hosted.controller.restapi.ResourceResponse;
@@ -97,6 +98,8 @@ import java.util.Scanner;
 import java.util.logging.Level;
 import java.util.stream.Collectors;
 
+import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.ALL;
+
 /**
  * This implements the application/v4 API which is used to deploy and manage applications
  * on hosted Vespa.
@@ -738,22 +741,30 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
     /** Trigger deployment of the last built application package, on a given version */
     // TODO Consider move to API for maintenance related operations
     private HttpResponse deploy(String tenantName, String applicationName, HttpRequest request) {
-        Version version = decideDeployVersion(request);
-        if ( ! systemHasVersion(version))
-            throw new IllegalArgumentException("Cannot trigger deployment of version '" + version + "': " +
-                                                       "Version is not active in this system. " +
-                                                       "Active versions: " + controller.versionStatus().versions()
-                                                                                       .stream()
-                                                                                       .map(VespaVersion::versionNumber)
-                                                                                       .map(Version::toString)
-                                                                                       .collect(Collectors.joining(", ")));
-
         ApplicationId id = ApplicationId.from(tenantName, applicationName, "default");
+        String requestVersion = readToString(request.getData());
+        StringBuilder response = new StringBuilder();
         controller.applications().lockOrThrow(id, application -> {
-            controller.applications().deploymentTrigger().triggerChange(application.get().id(),
-                                                                        application.get().change().with(version));
+            Change change;
+            if ("commit".equals(requestVersion))
+                change = Change.of(application.get().outstandingChange().application()
+                                              .orElseThrow(() -> new IllegalArgumentException("No outstanding commit to deploy!")));
+            else {
+                Version version = requestVersion == null ? controller.systemVersion() : new Version(requestVersion);
+                if ( ! systemHasVersion(version))
+                    throw new IllegalArgumentException("Cannot trigger deployment of version '" + version + "': " +
+                                                               "Version is not active in this system. " +
+                                                               "Active versions: " + controller.versionStatus().versions()
+                                                                                               .stream()
+                                                                                               .map(VespaVersion::versionNumber)
+                                                                                               .map(Version::toString)
+                                                                                               .collect(Collectors.joining(", ")));
+                change = Change.of(version);
+            }
+            controller.applications().deploymentTrigger().forceChange(application.get().id(), change);
+            response.append("Triggered " + change + " for " + application);
         });
-        return new MessageResponse("Triggered deployment of application '" + id + "' on version " + version);
+        return new MessageResponse(response.toString());
     }
 
     /** Cancel any ongoing change for given application */
@@ -766,7 +777,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, false));
+            controller.applications().deploymentTrigger().cancelChange(id, ALL));
 
         return new MessageResponse("Cancelled " + change + " for " + application);
     }
@@ -1123,14 +1134,6 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
         return controller.versionStatus().versions().stream().anyMatch(v -> v.versionNumber().equals(version));
     }
 
-    private Version decideDeployVersion(HttpRequest request) {
-        String requestVersion = readToString(request.getData());
-        if (requestVersion != null)
-            return new Version(requestVersion);
-        else
-            return controller.systemVersion();
-    }
-
     public static void toSlime(DeploymentCost deploymentCost, Cursor object) {
         object.setLong("tco", (long)deploymentCost.getTco());
         object.setLong("waste", (long)deploymentCost.getWaste());
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 9dd4c25050f..53ab488949e 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
@@ -22,6 +22,7 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
+import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel;
 import com.yahoo.vespa.hosted.controller.maintenance.JobControl;
 import com.yahoo.vespa.hosted.controller.maintenance.ReadyJobsTrigger;
 import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
@@ -51,6 +52,8 @@ import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobTy
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.productionUsWest1;
 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 com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.ALL;
+import static com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.PLATFORM;
 import static java.time.temporal.ChronoUnit.MILLIS;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
@@ -133,7 +136,7 @@ public class DeploymentTriggerTest {
         tester.jobCompletion(component).application(app).nextBuildNumber().uploadArtifact(applicationPackage).submit();
         tester.assertRunning(productionUsCentral1, app.id());
 
-        tester.applications().deploymentTrigger().cancelChange(app.id(), false);
+        tester.applications().deploymentTrigger().cancelChange(app.id(), ALL);
         tester.deployAndNotify(app, false, systemTest);
         tester.deployAndNotify(app, false, stagingTest);
         tester.deployAndNotify(app, false, productionUsCentral1);
@@ -561,11 +564,11 @@ 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(), true);
+        tester.deploymentTrigger().cancelChange(application.id(), PLATFORM);
         assertEquals(Change.of(appVersion1), app.get().change());
 
         // Now cancel the change as is done through the web API.
-        tester.deploymentTrigger().cancelChange(application.id(), false);
+        tester.deploymentTrigger().cancelChange(application.id(), ALL);
         assertEquals(Change.empty(), app.get().change());
 
         // A new version is released, which should now deploy the currently deployed application version to avoid downgrades.
@@ -609,7 +612,7 @@ public class DeploymentTriggerTest {
         tester.upgradeSystem(version1);
         // Deploy application2 to keep this version present in the system
         tester.deployCompletely(application2, applicationPackage);
-        tester.applications().deploymentTrigger().cancelChange(application1.id(), false);
+        tester.applications().deploymentTrigger().cancelChange(application1.id(), ALL);
         tester.buildService().clear(); // Clear stale build jobs for cancelled change
 
         // version2 is released and application1 starts upgrading
@@ -688,7 +691,7 @@ public class DeploymentTriggerTest {
         tester.upgradeSystem(v2);
         tester.deployAndNotify(application, true, systemTest);
         tester.deployAndNotify(application, true, stagingTest);
-        tester.deploymentTrigger().cancelChange(application.id(), true);
+        tester.deploymentTrigger().cancelChange(application.id(), PLATFORM);
         tester.deploy(productionEuWest1, application, applicationPackage);
         assertEquals(v2, app.get().deployments().get(productionEuWest1.zone(main)).version());
         assertEquals(v1, app.get().deployments().get(productionUsEast3.zone(main)).version());
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 6d4f58531ae..587cbde991f 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
@@ -27,6 +27,7 @@ import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobTy
 import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.productionUsWest1;
 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 com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger.ChangesToCancel.ALL;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -487,10 +488,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(), false);
-        tester.deploymentTrigger().cancelChange(default1.id(), false);
-        tester.deploymentTrigger().cancelChange(default2.id(), false);
-        tester.deploymentTrigger().cancelChange(default3.id(), false);
+        tester.deploymentTrigger().cancelChange(default0.id(), ALL);
+        tester.deploymentTrigger().cancelChange(default1.id(), ALL);
+        tester.deploymentTrigger().cancelChange(default2.id(), ALL);
+        tester.deploymentTrigger().cancelChange(default3.id(), ALL);
         tester.buildService().clear();
 
         // Applications with default policy start upgrading to V2
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment.json
index f74b2290d81..e3c0bbe0679 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/application-deployment.json
@@ -1 +1 @@
-{"message":"Triggered deployment of application 'tenant1.application1' on version 6.1"}
\ No newline at end of file
+{"message":"Triggered upgrade to 6.1 for application 'tenant1.application1'"}
\ No newline at end of file