Newer downgrade a production deployment

4 files changed, 72 insertions, 19 deletions

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 3339e59d581..f974fbf0faf 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
@@ -319,11 +319,16 @@ public class ApplicationController {
                 store(application, lock); // store missing information even if we fail deployment below
             }
 
-            // Ensure that the deploying change is tested
-            if (! canDeployDirectlyTo(zone, options) &&
-                ! application.deploymentJobs().isDeployableTo(zone.environment(), application.deploying()))
-                throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
-                                                   " as " + application.deploying().get() + " is not tested");
+            if ( ! canDeployDirectlyTo(zone, options)) { // validate automated deployment
+                if (!application.deploymentJobs().isDeployableTo(zone.environment(), application.deploying()))
+                    throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
+                                                       " as " + application.deploying().get() + " is not tested");
+                Deployment existingDeployment = application.deployments().get(zone);
+                if (existingDeployment != null && existingDeployment.version().isAfter(version))
+                    throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
+                                                       " as the requested version " + version + " is older than" +
+                                                       " the current version " + existingDeployment.version());
+            }    
 
             // Carry out deployment
             DeploymentId deploymentId = new DeploymentId(applicationId, zone);
@@ -338,7 +343,8 @@ public class ApplicationController {
             // Use info from previous deployments is available
             Deployment previousDeployment = application.deployments().getOrDefault(zone, new Deployment(zone, revision, version, clock.instant()));
             Deployment newDeployment = new Deployment(zone, revision, version, clock.instant(),
-                        previousDeployment.clusterUtils(), previousDeployment.clusterInfo(), previousDeployment.metrics());
+                                                      previousDeployment.clusterUtils(), 
+                                                      previousDeployment.clusterInfo(), previousDeployment.metrics());
 
             application = application.with(newDeployment);
             store(application, lock);
@@ -614,7 +620,7 @@ public class ApplicationController {
 
     /** Returns whether a direct deployment to given zone is allowed */
     private static boolean canDeployDirectlyTo(Zone zone, DeployOptions options) {
-        return !options.screwdriverBuildJob.isPresent() ||
+        return ! options.screwdriverBuildJob.isPresent() ||
                options.screwdriverBuildJob.get().screwdriverId == null ||
                zone.environment().isManuallyDeployed();
     }
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 11074e1a68e..9de35f77943 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
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.deployment;
 
+import com.yahoo.component.Version;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
@@ -12,6 +13,7 @@ import com.yahoo.vespa.hosted.controller.ApplicationController;
 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.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport;
@@ -155,22 +157,28 @@ public class DeploymentTrigger {
      * newer (different) than the one last completed successfully in next
      */
     private boolean changesAvailable(Application application, JobStatus previous, JobStatus next) {
-        if ( ! previous.lastSuccess().isPresent()) return false;
-
         if ( ! application.deploying().isPresent()) return false;
         Change change = application.deploying().get();
-        if (change instanceof Change.VersionChange && // the last completed is out of date - don't continue with it 
-            ! ((Change.VersionChange)change).version().equals(previous.lastSuccess().get().version()))
-            return false;
+
+        if ( ! previous.lastSuccess().isPresent() && 
+             ! isProductionJobPreviouslySucceedingOnCurrentlyDeployingVersion(previous, change)) return false;
+
+        if (change instanceof Change.VersionChange) {
+            Version targetVersion = ((Change.VersionChange)change).version();
+            if ( ! (targetVersion.equals(previous.lastSuccess().get().version())) )
+                return false; // version is outdated
+            if (isOnNewerVersionInProductionThan(targetVersion, application, next.type()))
+                return false; // Don't downgrade
+        }
 
         if (next == null) return true;
         if ( ! next.lastSuccess().isPresent()) return true;
-        
+
         JobStatus.JobRun previousSuccess = previous.lastSuccess().get();
         JobStatus.JobRun nextSuccess = next.lastSuccess().get();
         if (previousSuccess.revision().isPresent() &&  ! previousSuccess.revision().get().equals(nextSuccess.revision().get()))
             return true;
-        if (! previousSuccess.version().equals(nextSuccess.version()))
+        if ( ! previousSuccess.version().equals(nextSuccess.version()))
             return true;
         return false;
     }
@@ -396,6 +404,10 @@ public class DeploymentTrigger {
         if  ( ! deploysTo(application, jobType)) return false;
         // Ignore applications that are not associated with a project
         if ( ! application.deploymentJobs().projectId().isPresent()) return false;
+        if (application.deploying().isPresent() && application.deploying().get() instanceof Change.VersionChange) {
+            Version targetVersion = ((Change.VersionChange)application.deploying().get()).version();
+            if (isOnNewerVersionInProductionThan(targetVersion, application, jobType)) return false; // Don't downgrade
+        }
         
         return true;
     }
@@ -404,6 +416,40 @@ public class DeploymentTrigger {
         return application.deploymentJobs().jobStatus().entrySet().stream()
                 .anyMatch(entry -> entry.getKey().isProduction() && entry.getValue().isRunning(jobTimeoutLimit()));
     }
+
+    /**
+     * When upgrading it is ok to trigger the next job even if the previous failed if the previous has earlier succeeded
+     * on the version we are currently upgrading to
+     */
+    private boolean isProductionJobPreviouslySucceedingOnCurrentlyDeployingVersion(JobStatus jobStatus, Change change) {
+        if ( ! (change instanceof Change.VersionChange) ) return false;
+        if ( ! isProduction(jobStatus.type())) return false;
+        Optional<JobStatus.JobRun> lastSuccess = jobStatus.lastSuccess();
+        if ( ! lastSuccess.isPresent()) return false;
+        return lastSuccess.get().version().equals(((Change.VersionChange)change).version());
+    }
+
+    /** 
+     * Returns whether the current deployed version in the zone given by the job
+     * is never than the given version. This may be the case even if the production job
+     * in question failed, if the failure happens after deployment.
+     * In that case we should never deploy an earlier version as that may potentially
+     * downgrade production nodes which we are not guaranteed to support.
+     */
+    private boolean isOnNewerVersionInProductionThan(Version version, Application application, JobType job) {
+        if ( ! isProduction(job)) return false;
+        Optional<Zone> zone = job.zone(controller.system());
+        if ( ! zone.isPresent()) return false;
+        Deployment existingDeployment = application.deployments().get(zone.get());
+        if (existingDeployment == null) return false;
+        return existingDeployment.version().isAfter(version);
+    }
+    
+    private boolean isProduction(JobType job) {
+        Optional<Zone> zone = job.zone(controller.system());
+        if ( ! zone.isPresent()) return false; // arbitrary
+        return zone.get().environment() == Environment.prod;
+    }
     
     private boolean acceptNewRevisionNow(Application application) {
         if ( ! application.deploying().isPresent()) return true;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
index 78b4f7f895f..4886eba40b6 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/OutstandingChangeDeployerTest.java
@@ -24,13 +24,13 @@ public class OutstandingChangeDeployerTest {
     @Test
     public void testChangeDeployer() {
         DeploymentTester tester = new DeploymentTester();
+        tester.configServer().setDefaultVersion(new Version(6, 1));
         OutstandingChangeDeployer deployer = new OutstandingChangeDeployer(tester.controller(), Duration.ofMinutes(10), 
                                                                            new JobControl(new MockCuratorDb()));
-
         tester.createAndDeploy("app1", 11, "default");
         tester.createAndDeploy("app2", 22, "default");
 
-        Version version = new Version(5, 2);
+        Version version = new Version(6, 2);
         tester.deploymentTrigger().triggerChange(tester.application("app1").id(), new Change.VersionChange(version));
         
         assertEquals(new Change.VersionChange(version), tester.application("app1").deploying().get());
@@ -52,5 +52,5 @@ public class OutstandingChangeDeployerTest {
         assertEquals(DeploymentJobs.JobType.systemTest.id(), jobs.get(0).jobName());
         assertFalse(tester.application("app1").hasOutstandingChange());
     }
-    
+
 }
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 2f24d1deca5..0414cda3f55 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
@@ -147,6 +147,7 @@ public class UpgraderTest {
         tester.upgrader().maintain();
         assertEquals("Applications are on 5.3 - nothing to do", 0, tester.buildSystem().jobs().size());
         
+        // --- Starting upgrading to a new version which breaks, causing upgrades to commence on the previous version
         version = Version.fromString("5.4");
         Application default3 = tester.createAndDeploy("default3", 5, "default"); // need 4 to break a version
         Application default4 = tester.createAndDeploy("default4", 5, "default");
@@ -183,7 +184,7 @@ public class UpgraderTest {
         tester.completeUpgradeWithError(default0, version, "default", DeploymentJobs.JobType.stagingTest);
         tester.completeUpgradeWithError(default1, version, "default", DeploymentJobs.JobType.stagingTest);
         tester.completeUpgradeWithError(default2, version, "default", DeploymentJobs.JobType.stagingTest);
-        tester.completeUpgradeWithError(default3, version, "default", DeploymentJobs.JobType.stagingTest);
+        tester.completeUpgradeWithError(default3, version, "default", DeploymentJobs.JobType.productionUsWest1);
         tester.completeUpgrade(default4, version, "default");
         tester.updateVersionStatus(version);
         assertEquals(VespaVersion.Confidence.broken, tester.controller().versionStatus().systemVersion().get().confidence());
@@ -192,7 +193,7 @@ public class UpgraderTest {
                      3, tester.buildSystem().jobs().size());
         assertEquals("5.4", ((Change.VersionChange)tester.application(default1.id()).deploying().get()).version().toString());
         assertEquals("5.4", ((Change.VersionChange)tester.application(default2.id()).deploying().get()).version().toString());
-        assertEquals("5.4", ((Change.VersionChange)tester.application(default2.id()).deploying().get()).version().toString());
+        assertEquals("5.4", ((Change.VersionChange)tester.application(default3.id()).deploying().get()).version().toString());
     }
 
     @Test