Suppress blocked parts of Change, with test

2 files changed, 90 insertions, 34 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 f9afc3400f8..b6d37bb9088 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
@@ -9,7 +9,6 @@ import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
 import com.yahoo.vespa.hosted.controller.Controller;
-import com.yahoo.vespa.hosted.controller.LockedApplication;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService.JobState;
 import com.yahoo.vespa.hosted.controller.application.ApplicationList;
@@ -294,7 +293,7 @@ public class DeploymentTrigger {
                                                     .filter(step -> step.deploysTo(prod) || step.zones().isEmpty())
                                                     .collect(toList());
 
-            Change change = application.change();
+            Change change = application.changeAt(clock.instant());
             @SuppressWarnings("cast") // Bad compiler!
             Optional<Instant> completedAt = max((Optional<Instant>) application.deploymentJobs().statusOf(systemTest)
                                                                                .flatMap(job -> job.lastSuccess().map(JobRun::at)),
@@ -303,37 +302,37 @@ public class DeploymentTrigger {
             String reason = "New change available";
             List<Job> testJobs = null;
 
-            for (Step step : productionSteps) {
-                Set<JobType> stepJobs = step.zones().stream().map(order::toJob).collect(toSet());
-                Map<Optional<Instant>, List<JobType>> jobsByCompletion = stepJobs.stream().collect(groupingBy(job -> completedAt(change, application, job)));
-                if (jobsByCompletion.containsKey(empty())) { // Step not complete, because some jobs remain -- trigger these if the previous step was done.
-                    for (JobType job : jobsByCompletion.get(empty())) {
-                        State target = targetFor(application, change, deploymentFor(application, job));
-                        if (isTested(application, target)) {
-                            if (completedAt.isPresent())
-                                jobs.add(deploymentJob(application, target, change, job, reason, completedAt.get(), stepJobs));
+            if (change.isPresent())
+                for (Step step : productionSteps) {
+                    Set<JobType> stepJobs = step.zones().stream().map(order::toJob).collect(toSet());
+                    Map<Optional<Instant>, List<JobType>> jobsByCompletion = stepJobs.stream().collect(groupingBy(job -> completedAt(change, application, job)));
+                    if (jobsByCompletion.containsKey(empty())) { // Step not complete, because some jobs remain -- trigger these if the previous step was done.
+                        for (JobType job : jobsByCompletion.get(empty())) {
+                            State target = targetFor(application, change, deploymentFor(application, job));
+                            if (isTested(application, target)) {
+                                if (completedAt.isPresent())
+                                    jobs.add(deploymentJob(application, target, change, job, reason, completedAt.get(), stepJobs));
+                            }
+                            else if (testJobs == null) {
+                                if ( ! alreadyTriggered(application, target)) // TODO jvenstad: This is always true now ...
+                                    testJobs = testJobsFor(application, target, "Testing deployment for " + job.jobName(), completedAt.orElse(clock.instant()));
+                                else
+                                    testJobs = emptyList();
+                            }
                         }
-                        else if (testJobs == null) {
-                            if ( ! alreadyTriggered(application, target))
-                                testJobs = testJobsFor(application, target, "Testing deployment for " + job.jobName(), completedAt.orElse(clock.instant()));
-                            else
-                                testJobs = emptyList();
-                        }
-                    }
-                }
-                else { // All jobs are complete -- find the time of completion of this step.
-                    if (stepJobs.isEmpty()) { // No jobs means this is delay step.
-                        Duration delay = ((DeploymentSpec.Delay) step).duration();
-                        completedAt = completedAt.map(at -> at.plus(delay)).filter(at -> ! at.isAfter(clock.instant()));
-                        reason += " after a delay of " + delay;
                     }
-                    else {
-                        completedAt = jobsByCompletion.keySet().stream().map(Optional::get).max(naturalOrder());
-                        reason = "Available change in " + stepJobs.stream().map(JobType::jobName).collect(joining(", "));
+                    else { // All jobs are complete -- find the time of completion of this step.
+                        if (stepJobs.isEmpty()) { // No jobs means this is delay step.
+                            Duration delay = ((DeploymentSpec.Delay) step).duration();
+                            completedAt = completedAt.map(at -> at.plus(delay)).filter(at -> ! at.isAfter(clock.instant()));
+                            reason += " after a delay of " + delay;
+                        }
+                        else {
+                            completedAt = jobsByCompletion.keySet().stream().map(Optional::get).max(naturalOrder());
+                            reason = "Available change in " + stepJobs.stream().map(JobType::jobName).collect(joining(", "));
+                        }
                     }
                 }
-            }
-
             if (testJobs == null)
                 testJobs = testJobsFor(application, targetFor(application, application.change(), empty()), "Testing last changes outside prod", clock.instant());
             jobs.addAll(testJobs);
@@ -466,9 +465,6 @@ public class DeploymentTrigger {
         if ( ! job.concurrentlyWith.containsAll(runningProductionJobsFor(application)))
             return false;
 
-        if ( ! job.change.effectiveAt(application.deploymentSpec(), clock.instant()).isPresent())
-            return false;
-
         return true;
     }
 
@@ -483,7 +479,7 @@ public class DeploymentTrigger {
         return controller.applications();
     }
 
-    private boolean acceptNewApplicationVersion(LockedApplication application) {
+    private boolean acceptNewApplicationVersion(Application application) {
         if (application.change().application().isPresent()) return true; // More application changes are ok.
         if (application.deploymentJobs().hasFailures()) return true; // Allow changes to fix upgrade problems.
         // Otherwise, allow an application change if not currently upgrading.
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 3dcc16ccb88..f27cb0ba171 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
@@ -331,11 +331,71 @@ public class DeploymentTriggerTest {
         assertEquals(0, tester.buildService().jobs().size());
 
         tester.clock().advance(Duration.ofHours(2)); // ---------------- Exit block window: 20:30
-        tester.deploymentTrigger().triggerReadyJobs(); // Schedules the blocked production job(s)
+        tester.deploymentTrigger().triggerReadyJobs(); // Schedules staging test for the blocked production job(s)
+        // TODO jvenstad: Required now because changes during block window used empty source -- improvement to use first untested production job with change to test :)
+        tester.deployAndNotify(app, changedApplication, true, stagingTest);
         assertEquals(singletonList(buildJob(app, productionUsWest1)), tester.buildService().jobs());
     }
 
     @Test
+    public void testCompletionOfPartOfChangeDuringBlockWindow() {
+        ManualClock clock = new ManualClock(Instant.parse("2017-09-26T17:30:00.00Z")); // Tuesday, 17:30
+        DeploymentTester tester = new DeploymentTester(new ControllerTester(clock));
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .blockChange(false, true, "tue", "18", "UTC")
+                .region("us-west-1")
+                .region("us-east-3")
+                .build();
+        Application application = tester.createAndDeploy("app1", 1, applicationPackage);
+
+        // Application on (6.1, 1.0.42)
+        Version v1 = Version.fromString("6.1");
+
+        // Application is mid-upgrade when block window begins, and has an outstanding change.
+        Version v2 = Version.fromString("6.2");
+        tester.upgradeSystem(v2);
+        tester.jobCompletion(component).application(application).nextBuildNumber().uploadArtifact(applicationPackage).submit();
+
+        tester.deployAndNotify(application, applicationPackage, true, stagingTest);
+        tester.deployAndNotify(application, applicationPackage, true, systemTest);
+
+        // Entering block window will keep the outstanding change in place, but a new component run triggers a new change.
+        // This component completion should remove the older outstanding change, to avoid a later downgrade.
+        clock.advance(Duration.ofHours(1));
+        tester.deployAndNotify(application, applicationPackage, true, productionUsWest1);
+        assertEquals((Long) BuildJob.defaultBuildNumber, tester.application(application.id()).deploymentJobs().jobStatus()
+                                                               .get(productionUsWest1).lastSuccess().get().application().buildNumber().get());
+        assertEquals((Long) (BuildJob.defaultBuildNumber + 1), tester.application(application.id()).outstandingChange().application().get().buildNumber().get());
+
+        tester.readyJobTrigger().maintain();
+        assertTrue(tester.buildService().jobs().isEmpty());
+
+        // New component triggers a full deployment of new application version, leaving platform versions alone.
+        tester.jobCompletion(component).application(application).nextBuildNumber().nextBuildNumber().uploadArtifact(applicationPackage).submit();
+        tester.deployAndNotify(application, applicationPackage, true, stagingTest);
+        tester.deployAndNotify(application, applicationPackage, true, systemTest);
+        tester.deployAndNotify(application, applicationPackage, true, productionUsWest1);
+        tester.deployAndNotify(application, applicationPackage, true, systemTest);
+        tester.deployAndNotify(application, applicationPackage, true, stagingTest);
+        tester.deployAndNotify(application, applicationPackage, true, productionUsEast3);
+        tester.deployAndNotify(application, applicationPackage, true, systemTest);
+        tester.deployAndNotify(application, applicationPackage, true, stagingTest);
+
+        // All tests are done for now, and only the platform change remains.
+        assertTrue(tester.buildService().jobs().isEmpty());
+        assertEquals(Change.of(v2), tester.application(application.id()).change());
+
+        // Exiting block window, staging test is re-run for the last prod zone, which has the old platform.
+        clock.advance(Duration.ofHours(1));
+        tester.readyJobTrigger().maintain();
+        tester.deployAndNotify(application, applicationPackage, true, stagingTest);
+        tester.deployAndNotify(application, applicationPackage, true, productionUsEast3);
+
+        assertFalse(tester.application(application.id()).change().isPresent());
+        assertFalse(tester.application(application.id()).outstandingChange().isPresent());
+    }
+
+    @Test
     public void testUpgradingButNoJobStarted() {
         DeploymentTester tester = new DeploymentTester();
         ReadyJobsTrigger readyJobsTrigger = new ReadyJobsTrigger(tester.controller(),