Ensure all parallel deployments complete before next step

4 files changed, 42 insertions, 16 deletions

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 1d1d07d9513..02e0d94920e 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
@@ -120,9 +120,9 @@ public class DeploymentJobs {
             return true;
         }
         if (environment == Environment.staging) {
-            return isSuccessful(JobType.systemTest, change.get());
+            return isSuccessful(change.get(), JobType.systemTest);
         } else if (environment == Environment.prod) {
-            return isSuccessful(JobType.stagingTest, change.get());
+            return isSuccessful(change.get(), JobType.stagingTest);
         }
         return true; // other environments do not have any preconditions
     }
@@ -131,7 +131,15 @@ public class DeploymentJobs {
     public boolean isDeployed(Change change) {
         return status.values().stream()
                 .filter(status -> status.type().isProduction())
-                .allMatch(status -> isSuccessful(status.type(), change));
+                .allMatch(status -> isSuccessful(change, status.type()));
+    }
+
+    /** Returns whether job has completed successfully */
+    public boolean isSuccessful(Change change, JobType jobType) {
+        return Optional.ofNullable(jobStatus().get(jobType))
+                .filter(JobStatus::isSuccess)
+                .filter(status -> status.lastCompletedFor(change))
+                .isPresent();
     }
     
     /** Returns the oldest failingSince time of the jobs of this, or null if none are failing */
@@ -154,13 +162,6 @@ public class DeploymentJobs {
 
     public Optional<String> jiraIssueId() { return jiraIssueId; }
 
-    private boolean isSuccessful(JobType jobType, Change change) {
-        return Optional.ofNullable(jobStatus().get(jobType))
-                .filter(JobStatus::isSuccess)
-                .filter(status -> status.lastCompletedFor(change))
-                .isPresent();
-    }
-
     /** Job types that exist in the build system */
     public enum JobType {
 
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 b20a459b04a..448ab419853 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
@@ -57,6 +57,11 @@ public class DeploymentOrder {
             return Collections.emptyList();
         }
 
+        // Postpone if step hasn't completed all it's jobs for this change
+        if (!completedSuccessfully(currentStep.get(), application)) {
+            return Collections.emptyList();
+        }
+
         // Postpone next job if delay has not passed yet
         Duration delay = delayAfter(currentStep.get(), application);
         if (postponeDeployment(delay, job, application)) {
@@ -88,11 +93,23 @@ public class DeploymentOrder {
     /** Returns jobs for given deployment spec, in the order they are declared */
     public List<JobType> jobsFrom(DeploymentSpec deploymentSpec) {
         return deploymentSpec.steps().stream()
-                .flatMap(step -> step.zones().stream())
+                .flatMap(step -> jobsFrom(step).stream())
+                .collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
+    }
+
+    /** Returns jobs for the given step */
+    private List<JobType> jobsFrom(DeploymentSpec.Step step) {
+        return step.zones().stream()
                 .map(this::toJob)
                 .collect(Collectors.collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
     }
 
+    /** Returns whether all jobs have completed successfully for given step */
+    private boolean completedSuccessfully(DeploymentSpec.Step step, Application application) {
+        return jobsFrom(step).stream()
+                .allMatch(job -> application.deploymentJobs().isSuccessful(application.deploying().get(), job));
+    }
+
     /** Resolve deployment step from job */
     private Optional<DeploymentSpec.Step> fromJob(JobType job, Application application) {
         for (DeploymentSpec.Step step : application.deploymentSpec().steps()) {
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 e04ff8576e3..9b05101b5eb 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
@@ -179,7 +179,6 @@ public class DeploymentTester {
             assertEquals(job.id(), buildJob.get().jobName());
         }
         buildSystem().removeJobs(application.id());
-
     }
 
     private Optional<BuildService.BuildJob> findJob(Application application, JobType jobType) {
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 298cc1982d3..7ed0ad843cc 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
@@ -158,6 +158,7 @@ public class DeploymentTriggerTest {
                 .environment(Environment.prod)
                 .region("us-central-1")
                 .parallel("us-west-1", "us-east-3")
+                .region("eu-west-1")
                 .build();
 
         // Component job finishes
@@ -171,14 +172,22 @@ public class DeploymentTriggerTest {
         assertEquals(1, tester.buildSystem().jobs().size());
         tester.deployAndNotify(application, applicationPackage, true, JobType.productionUsCentral1);
 
-        // The two next regions are triggered in parallel
+        // Deploys in two regions in parallel
         assertEquals(2, tester.buildSystem().jobs().size());
         assertEquals(JobType.productionUsEast3.id(), tester.buildSystem().jobs().get(0).jobName());
         assertEquals(JobType.productionUsWest1.id(), tester.buildSystem().jobs().get(1).jobName());
+        tester.buildSystem().takeJobsToRun();
+
+        tester.deploy(JobType.productionUsWest1, application, applicationPackage, false);
+        tester.notifyJobCompletion(JobType.productionUsWest1, application, true);
+        assertTrue("No more jobs triggered at this time", tester.buildSystem().jobs().isEmpty());
 
-        // Deployment completes
-        tester.deployAndNotify(application, applicationPackage, true, JobType.productionUsWest1,
-                               JobType.productionUsEast3);
+        tester.deploy(JobType.productionUsEast3, application, applicationPackage, false);
+        tester.notifyJobCompletion(JobType.productionUsEast3, application, true);
+
+        // Last region completes
+        assertEquals(1, tester.buildSystem().jobs().size());
+        tester.deployAndNotify(application, applicationPackage, true, JobType.productionEuWest1);
         assertTrue("All jobs consumed", tester.buildSystem().jobs().isEmpty());
     }