Move deployment spec helper methods to DeploymentSteps

4 files changed, 103 insertions, 74 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
index 42243ff279e..d399ef977ac 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentSteps.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.deployment;
 
 import com.yahoo.config.application.api.DeploymentSpec;
+import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
@@ -16,63 +17,97 @@ import java.util.Objects;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import static java.util.Collections.singletonList;
 import static java.util.Comparator.comparingInt;
 import static java.util.stream.Collectors.collectingAndThen;
 
 /**
- * This class provides helper methods for mapping a deployment spec to jobs.
+ * This class provides helper methods for reading a deployment spec.
  *
  * @author mpolden
  */
 public class DeploymentSteps {
 
+    private final DeploymentSpec spec;
     private final Supplier<SystemName> system;
 
-    public DeploymentSteps(Supplier<SystemName> system) {
-        this.system = Objects.requireNonNull(system, "system may not be null");
+    public DeploymentSteps(DeploymentSpec spec, Supplier<SystemName> system) {
+        this.spec = Objects.requireNonNull(spec, "spec cannot be null");
+        this.system = Objects.requireNonNull(system, "system cannot be null");
     }
 
-    /** 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())
-                             .map(this::toJob)
-                             .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
+    /** Returns jobs for this, in the order they are declared */
+    public List<JobType> jobs() {
+        return spec.steps().stream()
+                   .flatMap(step -> step.zones().stream())
+                   .map(this::toJob)
+                   .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
     }
 
     /** Returns job status sorted according to deployment spec */
-    public List<JobStatus> sortBy(DeploymentSpec deploymentSpec, Collection<JobStatus> jobStatus) {
-        List<DeploymentJobs.JobType> sortedJobs = jobsFrom(deploymentSpec);
+    public List<JobStatus> sortBy(Collection<JobStatus> jobStatus) {
+        List<DeploymentJobs.JobType> sortedJobs = jobs();
         return jobStatus.stream()
                         .sorted(comparingInt(job -> sortedJobs.indexOf(job.type())))
                         .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
     }
 
     /** Returns deployments sorted according to declared zones */
-    public List<Deployment> sortBy(List<DeploymentSpec.DeclaredZone> zones, Collection<Deployment> deployments) {
-        List<ZoneId> productionZones = zones.stream()
-                                            .filter(z -> z.region().isPresent())
-                                            .map(z -> ZoneId.from(z.environment(), z.region().get()))
-                                            .collect(Collectors.toList());
+    public List<Deployment> sortBy2(Collection<Deployment> deployments) {
+        List<ZoneId> productionZones = spec.zones().stream()
+                                           .filter(z -> z.region().isPresent())
+                                           .map(z -> ZoneId.from(z.environment(), z.region().get()))
+                                           .collect(Collectors.toList());
         return deployments.stream()
                           .sorted(comparingInt(deployment -> productionZones.indexOf(deployment.zone())))
                           .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
     }
 
+    /** Resolve jobs from step */
+    public List<JobType> toJobs(DeploymentSpec.Step step) {
+        return step.zones().stream()
+                   .map(this::toJob)
+                   .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
+    }
+
+    /** Returns test jobs in this */
+    public List<JobType> testJobs() {
+        return toJobs(test());
+    }
+
+    /** Returns production jobs in this */
+    public List<JobType> productionJobs() {
+        return toJobs(production());
+    }
+
+    /** Returns test steps in this */
+    public List<DeploymentSpec.Step> test() {
+        if (spec.steps().isEmpty()) {
+            return singletonList(new DeploymentSpec.DeclaredZone(Environment.test));
+        }
+        return spec.steps().stream()
+                   .filter(step -> step.deploysTo(Environment.test) || step.deploysTo(Environment.staging))
+                   .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
+    }
+
+    /** Returns production steps in this */
+    public List<DeploymentSpec.Step> production() {
+        return spec.steps().stream()
+                   .filter(step -> step.deploysTo(Environment.prod) || step.zones().isEmpty())
+                   .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
+    }
+
     /** Resolve job from deployment zone */
-    public JobType toJob(DeploymentSpec.DeclaredZone zone) {
+    private JobType toJob(DeploymentSpec.DeclaredZone zone) {
         return JobType.from(system.get(), zone.environment(), zone.region().orElse(null))
                       .orElseThrow(() -> new IllegalArgumentException("Invalid zone " + zone));
     }
 
-    /** Resolve jobs from step */
-    public List<JobType> toJobs(DeploymentSpec.Step step) {
-        return step.zones().stream().map(this::toJob).collect(Collectors.toList());
-    }
-
     /** Resolve jobs from steps */
-    public List<JobType> toJobs(List<DeploymentSpec.Step> steps) {
-        return steps.stream().flatMap(step -> toJobs(step).stream()).collect(Collectors.toList());
+    private List<JobType> toJobs(List<DeploymentSpec.Step> steps) {
+        return steps.stream()
+                    .flatMap(step -> toJobs(step).stream())
+                    .collect(collectingAndThen(Collectors.toList(), Collections::unmodifiableList));
     }
 
 }
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 4a5a71c3bd2..a6f99ddd38c 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
@@ -25,6 +25,7 @@ import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
@@ -36,9 +37,6 @@ import java.util.function.Supplier;
 import java.util.logging.Logger;
 import java.util.stream.Stream;
 
-import static com.yahoo.config.provision.Environment.prod;
-import static com.yahoo.config.provision.Environment.staging;
-import static com.yahoo.config.provision.Environment.test;
 import static com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
 import static com.yahoo.vespa.hosted.controller.api.integration.BuildService.JobState.idle;
 import static com.yahoo.vespa.hosted.controller.api.integration.BuildService.JobState.queued;
@@ -71,18 +69,16 @@ public class DeploymentTrigger {
 
     private final Controller controller;
     private final Clock clock;
-    private final DeploymentSteps steps;
     private final BuildService buildService;
 
     public DeploymentTrigger(Controller controller, BuildService buildService, Clock clock) {
         this.controller = Objects.requireNonNull(controller, "controller cannot be null");
         this.buildService = Objects.requireNonNull(buildService, "buildService cannot be null");
         this.clock = Objects.requireNonNull(clock, "clock cannot be null");
-        this.steps = new DeploymentSteps(controller::system);
     }
 
-    public DeploymentSteps deploymentSteps() {
-        return steps;
+    public DeploymentSteps steps(DeploymentSpec spec) {
+        return new DeploymentSteps(spec, controller::system);
     }
 
     /**
@@ -277,48 +273,59 @@ public class DeploymentTrigger {
                                                         .<Instant>flatMap(job -> job.lastSuccess().map(JobRun::at)));
             String reason = "New change available";
             List<Job> testJobs = null; // null means "uninitialised", while empty means "don't run any jobs".
+            DeploymentSteps steps = steps(application.deploymentSpec());
 
-            if (change.isPresent())
-                for (Step step : productionStepsOf(application)) {
+            if (change.isPresent()) {
+                for (Step step : steps.production()) {
                     List<JobType> stepJobs = steps.toJobs(step);
-                    List<JobType> remainingJobs = stepJobs.stream().filter(job -> ! isComplete(change, application, job)).collect(toList());
-                    if ( ! remainingJobs.isEmpty()) { // Step is incomplete; trigger remaining jobs if ready, or their test jobs if untested.
+                    List<JobType> remainingJobs = stepJobs.stream().filter(job -> !isComplete(change, application, job)).collect(toList());
+                    if (!remainingJobs.isEmpty()) { // Step is incomplete; trigger remaining jobs if ready, or their test jobs if untested.
                         for (JobType job : remainingJobs) {
                             Versions versions = Versions.from(change, application, deploymentFor(application, job),
                                                               controller.systemVersion());
                             if (isTested(application, versions)) {
-                                if (   completedAt.isPresent()
-                                    && jobStateOf(application, job) == idle
-                                    && stepJobs.containsAll(runningProductionJobs(application)))
+                                if (canTrigger(job, application, stepJobs, completedAt)) {
                                     jobs.add(deploymentJob(application, versions, change, job, reason, completedAt.get()));
-                                if ( ! alreadyTriggered(application, versions))
+                                }
+                                if (!alreadyTriggered(application, versions)) {
                                     testJobs = emptyList();
-                            }
-                            else if (testJobs == null) {
-                                testJobs = testJobs(application, versions, String.format("Testing deployment for %s (%s)", job.jobName(), versions.toString()),
-                                                    completedAt.orElse(clock.instant()));
+                                }
+                            } else if (testJobs == null) {
+                                testJobs = testJobs(application, versions,
+                                                    String.format("Testing deployment for %s (%s)",
+                                                                  job.jobName(), versions.toString()),
+                                                    completedAt.orElseGet(clock::instant));
                             }
                         }
                         completedAt = Optional.empty();
-                    }
-                    else { // All jobs are complete; find the time of completion of this step.
+                    } 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()));
+                            completedAt = completedAt.map(at -> at.plus(delay)).filter(at -> !at.isAfter(clock.instant()));
                             reason += " after a delay of " + delay;
-                        }
-                        else {
+                        } else {
                             completedAt = stepJobs.stream().map(job -> application.deploymentJobs().statusOf(job).get().lastCompleted().get().at()).max(naturalOrder());
                             reason = "Available change in " + stepJobs.stream().map(JobType::jobName).collect(joining(", "));
                         }
                     }
                 }
-            if (testJobs == null)
+            }
+            if (testJobs == null) {
                 testJobs = testJobs(application, Versions.from(application, controller.systemVersion()),
                                     "Testing last changes outside prod", clock.instant());
+            }
             jobs.addAll(testJobs);
         });
-        return jobs;
+        return Collections.unmodifiableList(jobs);
+    }
+
+    /** Returns whether given job can be triggered at this point in time */
+    private boolean canTrigger(JobType job, Application application, List<JobType> stepJobs, Optional<Instant> completedAt) {
+        if (!completedAt.isPresent()) return false;
+        if (jobStateOf(application, job) != idle) return false;
+        if (!stepJobs.containsAll(runningProductionJobs(application))) return false;
+
+        return true;
     }
 
     // ---------- Job state helpers ----------
@@ -401,9 +408,10 @@ public class DeploymentTrigger {
     }
 
     private Change remainingChange(Application application) {
-        List<JobType> jobs = productionStepsOf(application).isEmpty()
-                ? steps.toJobs(testStepsOf(application))
-                : steps.toJobs(productionStepsOf(application));
+        DeploymentSteps steps = steps(application.deploymentSpec());
+        List<JobType> jobs = steps.production().isEmpty()
+                ? steps.testJobs()
+                : steps.productionJobs();
 
         Change change = application.change();
         if (jobs.stream().allMatch(job -> isComplete(application.change().withoutApplication(), application, job)))
@@ -422,7 +430,7 @@ public class DeploymentTrigger {
      */
     private List<Job> testJobs(Application application, Versions versions, String reason, Instant availableSince) {
         List<Job> jobs = new ArrayList<>();
-        for (JobType jobType : steps.toJobs(testStepsOf(application))) {
+        for (JobType jobType : steps(application.deploymentSpec()).testJobs()) {
             Optional<JobRun> completion = successOn(application, jobType, versions)
                     .filter(run -> versions.sourcesMatchIfPresent(run) || jobType == systemTest);
             if ( ! completion.isPresent() && jobStateOf(application, jobType) == idle)
@@ -431,20 +439,6 @@ public class DeploymentTrigger {
         return jobs;
     }
 
-    private List<Step> testStepsOf(Application application) {
-        return application.deploymentSpec().steps().isEmpty()
-                ? singletonList(new DeploymentSpec.DeclaredZone(test))
-                : application.deploymentSpec().steps().stream()
-                             .filter(step -> step.deploysTo(test) || step.deploysTo(staging))
-                             .collect(toList());
-    }
-
-    private List<Step> productionStepsOf(Application application) {
-        return application.deploymentSpec().steps().stream()
-                .filter(step -> step.deploysTo(prod) || step.zones().isEmpty())
-                .collect(toList());
-    }
-
     private Job deploymentJob(Application application, Versions versions, Change change, JobType jobType, String reason, Instant availableSince) {
         boolean isRetry = application.deploymentJobs().statusOf(jobType).flatMap(JobStatus::jobError)
                                      .filter(JobError.outOfCapacity::equals).isPresent();
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 71f87d7e960..76a8eaec5d9 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
@@ -355,8 +355,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
 
         // Jobs sorted according to deployment spec
         List<JobStatus> jobStatus = controller.applications().deploymentTrigger()
-                .deploymentSteps()
-                .sortBy(application.deploymentSpec(), application.deploymentJobs().jobStatus().values());
+                .steps(application.deploymentSpec())
+                .sortBy(application.deploymentJobs().jobStatus().values());
 
         Cursor deploymentsArray = object.setArray("deploymentJobs");
         for (JobStatus job : jobStatus) {
@@ -396,8 +396,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
 
         // Deployments sorted according to deployment spec
         List<Deployment> deployments = controller.applications().deploymentTrigger()
-                .deploymentSteps()
-                .sortBy(application.deploymentSpec().zones(), application.deployments().values());
+                .steps(application.deploymentSpec())
+                .sortBy2(application.deployments().values());
         Cursor instancesArray = object.setArray("instances");
         for (Deployment deployment : deployments) {
             Cursor deploymentObject = instancesArray.addObject();
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 1c2eda6de81..fe425607b87 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
@@ -193,8 +193,8 @@ public class DeploymentTester {
 
     private void completeDeployment(Application application, ApplicationPackage applicationPackage,
                                     Optional<JobType> failOnJob, boolean includingProductionZones) {
-        DeploymentSteps order = new DeploymentSteps(controller()::system);
-        List<JobType> jobs = order.jobsFrom(applicationPackage.deploymentSpec());
+        DeploymentSteps steps = controller().applications().deploymentTrigger().steps(applicationPackage.deploymentSpec());
+        List<JobType> jobs = steps.jobs();
         if ( ! includingProductionZones)
             jobs = jobs.stream().filter(job -> ! job.isProduction()).collect(Collectors.toList());
         for (JobType job : jobs) {