Support blocking application changes

9 files changed, 134 insertions, 32 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
index f68ce3ebfa5..d53c16d2ce6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Application.java
@@ -276,4 +276,16 @@ public class Application {
         return true;
     }
     
+    /** Returns true if there is no outstanding change to deploy - i.e deploying is empty or completedly deployed */
+    public boolean deployingCompleted() { 
+        if ( ! deploying.isPresent()) return true;
+        return deploymentJobs().isDeployed(deploying.get()); 
+    }
+
+    /** Returns true if there is a current change which is blocked from being deployed to production at this instant */
+    public boolean deployingBlocked(Instant instant) {
+        if ( ! deploying.isPresent()) return false;
+        return deploying.get().blockedBy(deploymentSpec, instant);
+    }
+
 }
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 596cbbebd45..bc82451580a 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
@@ -2,7 +2,9 @@
 package com.yahoo.vespa.hosted.controller.application;
 
 import com.yahoo.component.Version;
+import com.yahoo.config.application.api.DeploymentSpec;
 
+import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
 
@@ -13,6 +15,9 @@ import java.util.Optional;
  */
 public abstract class Change {
 
+    /** Returns true if this change is blocked by the given spec art the given instant */
+    public abstract boolean blockedBy(DeploymentSpec deploymentSpec, Instant instant);
+    
     /** A change to the application package revision of an application */
     public static class ApplicationChange extends Change {
         
@@ -27,6 +32,11 @@ public abstract class Change {
         public Optional<ApplicationRevision> revision() { return revision; }
 
         @Override
+        public boolean blockedBy(DeploymentSpec deploymentSpec, Instant instant) {
+            return  ! deploymentSpec.canChangeRevisionAt(instant);
+        }
+
+        @Override
         public int hashCode() { return revision.hashCode(); }
         
         @Override
@@ -71,6 +81,11 @@ public abstract class Change {
         public Version version() { return version; }
 
         @Override
+        public boolean blockedBy(DeploymentSpec deploymentSpec, Instant instant) {
+            return ! deploymentSpec.canUpgradeAt(instant);
+        }
+
+        @Override
         public int hashCode() { return version.hashCode(); }
 
         @Override
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
index 26bef06adcf..5036493603a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
@@ -118,14 +118,11 @@ public class DeploymentJobs {
         return true; // other environments do not have any preconditions
     }
 
-    /** Returns whether change has been deployed completely */
-    public boolean isDeployed(Optional<Change> change) {
-        if (!change.isPresent()) {
-            return true;
-        }
+    /** Returns whether the given change has been deployed completely */
+    public boolean isDeployed(Change change) {
         return status.values().stream()
                 .filter(status -> status.type().isProduction())
-                .allMatch(status -> isSuccessful(change.get(), status.type()));
+                .allMatch(status -> isSuccessful(change, status.type()));
     }
 
     /** Returns whether job has completed successfully */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
index 48ebbc9f972..0dac79964f7 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
@@ -94,6 +94,9 @@ public class DeploymentOrder {
         return job == JobType.component;
     }
 
+    /** Returns whether this is the last job before production */
+    public boolean isLastBeforeProduction(JobType jobType) { return jobType == JobType.stagingTest; }
+
     /** Returns whether the given job is last in a deployment */
     public boolean isLast(JobType job, Application application) {
         List<DeploymentSpec.Step> deploymentSteps = deploymentSteps(application);
@@ -105,7 +108,7 @@ public class DeploymentOrder {
         // Step may not exist for all jobs, e.g. component
         return step.map(s -> s.equals(lastStep)).orElse(false);
     }
-
+    
     /** Returns jobs for given deployment spec, in the order they are declared */
     public List<JobType> jobsFrom(DeploymentSpec deploymentSpec) {
         return deploymentSpec.steps().stream()
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 423d8c674df..5fe6edf84cc 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
@@ -65,17 +65,26 @@ public class DeploymentTrigger {
             Application application = applications().require(report.applicationId());
             application = application.withJobCompletion(report, clock.instant(), controller);
             
-            // Handle successful first and last job
-            if (order.isFirst(report.jobType()) && report.success()) { // the first job tells us that a change occurred
-                if (application.deploying().isPresent() && ! application.deploymentJobs().hasFailures()) { // postpone until the current deployment is done
-                    applications().store(application.withOutstandingChange(true), lock);
+            // Handle successful starting and ending
+            if (report.success()) {
+                if (order.isFirst(report.jobType())) {
+                    // the first job tells us that a change occurred
+                    if (application.deploying().isPresent() && !application.deploymentJobs().hasFailures()) { // postpone until the current deployment is done
+                        applications().store(application.withOutstandingChange(true), lock);
+                        return;
+                    } else { // start a new change deployment
+                        application = application.withDeploying(Optional.of(Change.ApplicationChange.unknown()));
+                    }
+                } 
+                else if (order.isLastBeforeProduction(report.jobType()) && application.deployingBlocked(clock.instant())) {
+                    // run tests to provide feedback on errors but stop before production
+                    applications().store(application.withDeploying(Optional.empty()), lock);
                     return;
                 }
-                else { // start a new change deployment
-                    application = application.withDeploying(Optional.of(Change.ApplicationChange.unknown()));
+                else if (order.isLast(report.jobType(), application) && application.deployingCompleted()) {
+                    // change completed
+                    application = application.withDeploying(Optional.empty());
                 }
-            } else if (order.isLast(report.jobType(), application) && report.success() && application.deploymentJobs().isDeployed(application.deploying())) {
-                application = application.withDeploying(Optional.empty());
             }
 
             // Trigger next
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index f193d4718c6..4394ae0498e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -29,6 +29,7 @@ public class ApplicationPackageBuilder {
     private final StringBuilder environmentBody = new StringBuilder();
     private final StringBuilder validationOverridesBody = new StringBuilder();
     private final StringBuilder blockUpgrade = new StringBuilder();
+    private String searchDefinition = "search test { }";
 
     public ApplicationPackageBuilder upgradePolicy(String upgradePolicy) {
         this.upgradePolicy = upgradePolicy;
@@ -61,14 +62,15 @@ public class ApplicationPackageBuilder {
         return this;
     }
 
-    public ApplicationPackageBuilder blockUpgrade(String daySpec, String hourSpec, String zoneSpec) {
-        blockUpgrade.append("  <block-upgrade days=\"");
-        blockUpgrade.append(daySpec);
-        blockUpgrade.append("\" hours=\"");
-        blockUpgrade.append(hourSpec);
-        blockUpgrade.append("\" time-zone=\"");
-        blockUpgrade.append(zoneSpec);
-        blockUpgrade.append("\"/>\n");
+    public ApplicationPackageBuilder blockChange(boolean revision, boolean version, 
+                                                 String daySpec, String hourSpec, String zoneSpec) {
+        blockUpgrade.append("  <block-change");
+        blockUpgrade.append(" revision='").append(revision).append("'");
+        blockUpgrade.append(" version='").append(version).append("'");
+        blockUpgrade.append(" days='").append(daySpec).append("'");
+        blockUpgrade.append(" hours='").append(hourSpec).append("'");
+        blockUpgrade.append(" time-zone='").append(zoneSpec).append("'");
+        blockUpgrade.append("/>\n");
         return this;
     }
 
@@ -80,7 +82,13 @@ public class ApplicationPackageBuilder {
         validationOverridesBody.append("</allow>\n");
         return this;
     }
-
+    
+    /** Sets the content of the search definition test.sd */
+    public ApplicationPackageBuilder searchDefinition(String testSearchDefinition) {
+        this.searchDefinition = testSearchDefinition;
+        return this;
+    }
+    
     private byte[] deploymentSpec() {
         StringBuilder xml = new StringBuilder("<deployment version='1.0'>\n");
         if (upgradePolicy != null) {
@@ -98,7 +106,7 @@ public class ApplicationPackageBuilder {
         xml.append(">\n</deployment>");
         return xml.toString().getBytes(StandardCharsets.UTF_8);
     }
-
+    
     private byte[] validationOverrides() {
         String xml = "<validation-overrides version='1.0'>\n" +
                 validationOverridesBody +
@@ -106,6 +114,10 @@ public class ApplicationPackageBuilder {
         return xml.getBytes(StandardCharsets.UTF_8);
     }
 
+    private byte[] searchDefinition() { 
+        return searchDefinition.getBytes(StandardCharsets.UTF_8);
+    }
+
     public ApplicationPackage build() {
         ByteArrayOutputStream zip = new ByteArrayOutputStream();
         ZipOutputStream out = new ZipOutputStream(zip);
@@ -116,6 +128,9 @@ public class ApplicationPackageBuilder {
             out.putNextEntry(new ZipEntry("validation-overrides.xml"));
             out.write(validationOverrides());
             out.closeEntry();
+            out.putNextEntry(new ZipEntry("search-definitions/test.sd"));
+            out.write(searchDefinition());
+            out.closeEntry();
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         } finally {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
index ad723677686..c4829e5594f 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java
@@ -22,8 +22,10 @@ import com.yahoo.vespa.hosted.controller.maintenance.Upgrader;
 import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
 
 import java.time.Duration;
+import java.util.List;
 import java.util.Optional;
 import java.util.UUID;
+import java.util.stream.Collectors;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -128,12 +130,23 @@ public class DeploymentTester {
     public void deployCompletely(Application application, ApplicationPackage applicationPackage) {
         notifyJobCompletion(JobType.component, application, true);
         assertTrue(applications().require(application.id()).deploying().isPresent());
-        completeDeployment(application, applicationPackage, Optional.empty());
+        completeDeployment(application, applicationPackage, Optional.empty(), true);
     }
 
-    private void completeDeployment(Application application, ApplicationPackage applicationPackage, Optional<JobType> failOnJob) {
+    /** Deploy application using the given application package, but expecting to stop after test phases */
+    public void deployTestOnly(Application application, ApplicationPackage applicationPackage) {
+        notifyJobCompletion(JobType.component, application, true);
+        assertTrue(applications().require(application.id()).deploying().isPresent());
+        completeDeployment(application, applicationPackage, Optional.empty(), false);
+    }
+
+    private void completeDeployment(Application application, ApplicationPackage applicationPackage, 
+                                    Optional<JobType> failOnJob, boolean includingProductionZones) {
         DeploymentOrder order = new DeploymentOrder(controller());
-        for (JobType job : order.jobsFrom(applicationPackage.deploymentSpec())) {
+        List<JobType> jobs = order.jobsFrom(applicationPackage.deploymentSpec());
+        if ( ! includingProductionZones)
+            jobs = jobs.stream().filter(job -> ! job.isProduction()).collect(Collectors.toList());
+        for (JobType job : jobs) {
             boolean failJob = failOnJob.map(j -> j.equals(job)).orElse(false);
             deployAndNotify(application, applicationPackage, !failJob, job);
             if (failJob) {
@@ -159,7 +172,7 @@ public class DeploymentTester {
     public void completeUpgrade(Application application, Version version, String upgradePolicy) {
         assertTrue(applications().require(application.id()).deploying().isPresent());
         assertEquals(new Change.VersionChange(version), applications().require(application.id()).deploying().get());
-        completeDeployment(application, applicationPackage(upgradePolicy), Optional.empty());
+        completeDeployment(application, applicationPackage(upgradePolicy), Optional.empty(), true);
     }
 
     public void completeUpgradeWithError(Application application, Version version, String upgradePolicy, JobType failOnJob) {
@@ -173,7 +186,7 @@ public class DeploymentTester {
     private void completeUpgradeWithError(Application application, Version version, ApplicationPackage applicationPackage, Optional<JobType> failOnJob) {
         assertTrue(applications().require(application.id()).deploying().isPresent());
         assertEquals(new Change.VersionChange(version), applications().require(application.id()).deploying().get());
-        completeDeployment(application, applicationPackage, failOnJob);
+        completeDeployment(application, applicationPackage, failOnJob, true);
     }
 
     public void deploy(JobType job, Application application, ApplicationPackage applicationPackage) {
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 5abadf28cfb..650a5865006 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
@@ -3,7 +3,9 @@ package com.yahoo.vespa.hosted.controller.deployment;
 
 import com.yahoo.component.Version;
 import com.yahoo.config.provision.Environment;
+import com.yahoo.test.ManualClock;
 import com.yahoo.vespa.hosted.controller.Application;
+import com.yahoo.vespa.hosted.controller.ControllerTester;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
@@ -11,6 +13,7 @@ import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
 import org.junit.Test;
 
 import java.time.Duration;
+import java.time.Instant;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -257,6 +260,41 @@ public class DeploymentTriggerTest {
     }
 
     @Test
+    public void testBlockRevisionChange() {
+        ManualClock clock = new ManualClock(Instant.parse("2017-09-26T17:30:00.00Z")); // Tuesday, 17:30
+        DeploymentTester tester = new DeploymentTester(new ControllerTester(clock));
+        Version version = Version.fromString("5.0");
+        tester.updateVersionStatus(version);
+
+        ApplicationPackageBuilder applicationPackageBuilder = new ApplicationPackageBuilder()
+                .upgradePolicy("canary")
+                // Block revision changes on tuesday in hours 18 and 19
+                .blockChange(true, false, "tue", "18-19", "UTC")
+                .region("us-west-1");
+
+        Application app = tester.createAndDeploy("app1", 1, applicationPackageBuilder.build());
+
+        tester.clock().advance(Duration.ofHours(1)); // Enter block window: 18:30
+        
+        String searchDefinition =
+                "search test {\n" +
+                "  document test {\n" +
+                "    field test type string {\n" +
+                "    }\n" +
+                "  }\n" +
+                "}\n";
+        ApplicationPackage changedApplication = applicationPackageBuilder.searchDefinition(searchDefinition).build();
+
+        tester.deployTestOnly(app, changedApplication);
+        assertFalse(tester.application("app1").deploying().isPresent());
+
+        tester.clock().advance(Duration.ofHours(2)); // Exit block window: 20:30
+
+        tester.deployCompletely(app, changedApplication);
+        assertFalse(tester.application("app1").deploying().isPresent());
+    }
+
+    @Test
     public void testHandleMultipleNotificationsFromLastJob() {
         DeploymentTester tester = new DeploymentTester();
         BuildSystem buildSystem = tester.buildSystem();
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 95b700f4dd9..08878dd0051 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
@@ -364,7 +364,7 @@ public class UpgraderTest {
     }
 
     @Test
-    public void testConsidersBlockUpgradeWindow() {
+    public void testBlockVersionChange() {
         ManualClock clock = new ManualClock(Instant.parse("2017-09-26T18:00:00.00Z")); // A tuesday
         DeploymentTester tester = new DeploymentTester(new ControllerTester(clock));
         Version version = Version.fromString("5.0");
@@ -373,7 +373,7 @@ public class UpgraderTest {
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .upgradePolicy("canary")
                 // Block upgrades on tuesday in hours 18 and 19
-                .blockUpgrade("tue", "18-19", "UTC")
+                .blockChange(false, true, "tue", "18-19", "UTC")
                 .region("us-west-1")
                 .build();