Cleanup

3 files changed, 54 insertions, 44 deletions

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 c8482af1c2e..280d4c7f7e4 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,9 +2,7 @@
 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;
 import java.util.StringJoiner;
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 c366b8c19ae..22b6728adf2 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
@@ -26,9 +26,9 @@ import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.NoSuchElementException;
@@ -38,7 +38,6 @@ import java.util.OptionalLong;
 import java.util.Set;
 import java.util.function.Supplier;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static com.yahoo.config.provision.Environment.prod;
@@ -54,7 +53,6 @@ import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobTy
 import static java.util.Collections.singletonList;
 import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
-import static java.util.Optional.empty;
 import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.joining;
 import static java.util.stream.Collectors.partitioningBy;
@@ -95,11 +93,13 @@ public class DeploymentTrigger {
     }
 
     /**
-     * Called each time a job completes (successfully or not) to record information used when deciding what to trigger.
+     * Records information when a job completes (successfully or not). This information is used when deciding what to
+     * trigger next.
      */
     public void notifyOfCompletion(JobReport report) {
-        log.log(LogLevel.INFO, String.format("Got notified of %s for %s of %s (%d).",
-                                             report.jobError().map(JobError::toString).orElse("success"),
+        log.log(LogLevel.INFO, String.format("Notified of %s for %s of %s (%d).",
+                                             report.jobError().map(e -> e.toString() + " error")
+                                                   .orElse("success"),
                                              report.jobType(),
                                              report.applicationId(),
                                              report.projectId()));
@@ -113,7 +113,8 @@ public class DeploymentTrigger {
             JobRun triggering;
             if (report.jobType() == component) {
                 ApplicationVersion applicationVersion = ApplicationVersion.from(report.sourceRevision().get(), report.buildNumber());
-                triggering = JobRun.triggering(controller.systemVersion(), applicationVersion, empty(), empty(), "Application commit", clock.instant());
+                triggering = JobRun.triggering(controller.systemVersion(), applicationVersion, Optional
+                        .empty(), Optional.empty(), "Application commit", clock.instant());
                 if (report.success()) {
                     if (acceptNewApplicationVersion(application))
                         application = application.withChange(application.change().with(applicationVersion))
@@ -121,10 +122,11 @@ public class DeploymentTrigger {
                     else
                         application = application.withOutstandingChange(Change.of(applicationVersion));
                 }
+            } else {
+                triggering = application.deploymentJobs().statusOf(report.jobType()).flatMap(JobStatus::lastTriggered)
+                                        .orElseThrow(() -> new IllegalStateException("Notified of completion of " + report.jobType().jobName() + " for " +
+                                                                                     report.applicationId() + ", but that has neither been triggered nor deployed"));
             }
-            else triggering = application.deploymentJobs().statusOf(report.jobType()).flatMap(JobStatus::lastTriggered)
-                    .orElseThrow(() -> new IllegalStateException("Got notified about completion of " + report.jobType().jobName() + " for " +
-                                                                 report.applicationId() + ", but that has neither been triggered nor deployed"));
             application = application.withJobCompletion(report.projectId(),
                                                         report.jobType(),
                                                         triggering.completion(report.buildNumber(), clock.instant()),
@@ -134,6 +136,7 @@ public class DeploymentTrigger {
         });
     }
 
+    /** Returns a map of jobs that are scheduled to be run, grouped by the job type */
     public Map<JobType, ? extends List<? extends BuildJob>> jobsToRun() {
         return computeReadyJobs().stream().collect(groupingBy(Job::jobType));
     }
@@ -172,7 +175,7 @@ public class DeploymentTrigger {
      * the project id is removed from the application owning the job, to prevent further trigger attemps.
      */
     public boolean trigger(Job job) {
-        log.log(LogLevel.INFO, String.format("Attempting to trigger %s: %s", job, job.triggering));
+        log.log(LogLevel.INFO, String.format("Triggering %s: %s", job, job.triggering));
         try {
             buildService.trigger(job);
             applications().lockOrThrow(job.applicationId(), application ->
@@ -188,16 +191,17 @@ public class DeploymentTrigger {
         }
     }
 
+    /** Force triggering of a job for given application */
     public List<JobType> forceTrigger(ApplicationId applicationId, JobType jobType) {
         Application application = applications().require(applicationId);
         if (jobType == component) {
             buildService.trigger(BuildJob.of(applicationId, application.deploymentJobs().projectId().getAsLong(), jobType.jobName()));
             return singletonList(component);
         }
-        Versions versions = versionsFor(application, application.change(), deploymentFor(application, jobType));
+        Versions versions = versions(application, application.change(), deploymentFor(application, jobType));
         String reason = ">:o:< Triggered by force! (-o-) |-o-| (=oo=)";
         return (jobType.isProduction() && ! isTested(application, versions)
-                ? testJobsFor(application, versions, reason, clock.instant()).stream()
+                ? testJobs(application, versions, reason, clock.instant()).stream()
                 : Stream.of(deploymentJob(application, versions, application.change(), jobType, reason, clock.instant())))
                 .peek(this::trigger)
                 .map(Job::jobType).collect(toList());
@@ -287,20 +291,20 @@ public class DeploymentTrigger {
                 for (Step step : productionSteps) {
                     Set<JobType> stepJobs = step.zones().stream().map(order::toJob).collect(toSet());
                     Map<Instant, List<JobType>> jobsByCompletion = stepJobs.stream().collect(groupingBy(job -> completedAt(change, application, job).orElse(Instant.EPOCH)));
-                    if (jobsByCompletion.containsKey(Instant.EPOCH)) { // Step not complete, because some jobs remain -- trigger these if the previous step was done, or theirs tests if needed.
+                    if (jobsByCompletion.containsKey(Instant.EPOCH)) { // Step incomplete because some jobs remain, trigger those if the previous step was done, or required test steps.
                         for (JobType job : jobsByCompletion.get(Instant.EPOCH)) {
-                            Versions versions = versionsFor(application, change, deploymentFor(application, job));
+                            Versions versions = versions(application, change, deploymentFor(application, job));
                             if (isTested(application, versions)) {
-                                if (   completedAt.isPresent()
-                                    && jobStateIsAmong(application, job, idle)
-                                    && stepJobs.containsAll(runningProductionJobsFor(application)))
+                                if (completedAt.isPresent()
+                                    && jobStateContains(application, job, idle)
+                                    && stepJobs.containsAll(runningProductionJobs(application)))
                                     jobs.add(deploymentJob(application, versions, change, job, reason, completedAt.get()));
                             }
                             else if (testJobs.isEmpty()) {
-                                testJobs = testJobsFor(application, versions, "Testing deployment for " + job.jobName(), completedAt.orElse(clock.instant()));
+                                testJobs = testJobs(application, versions, "Testing deployment for " + job.jobName(), completedAt.orElse(clock.instant()));
                             }
                         }
-                        completedAt = empty();
+                        completedAt = Optional.empty();
                     }
                     else { // All jobs are complete -- find the time of completion of this step.
                         if (stepJobs.isEmpty()) { // No jobs means this is delay step.
@@ -315,7 +319,8 @@ public class DeploymentTrigger {
                     }
                 }
             if (testJobs.isEmpty())
-                testJobs = testJobsFor(application, versionsFor(application, application.change(), empty()), "Testing last changes outside prod", clock.instant());
+                testJobs = testJobs(application, versions(application, application.change(), Optional
+                        .empty()), "Testing last changes outside prod", clock.instant());
             jobs.addAll(testJobs);
         });
         return jobs;
@@ -323,7 +328,7 @@ public class DeploymentTrigger {
 
     // ---------- Job state helpers ----------
 
-    private List<JobType> runningProductionJobsFor(Application application) {
+    private List<JobType> runningProductionJobs(Application application) {
         return application.deploymentJobs().jobStatus().keySet().parallelStream()
                           .filter(JobType::isProduction)
                           .filter(job -> isRunning(application, job))
@@ -332,16 +337,16 @@ public class DeploymentTrigger {
 
     /** Returns whether the given job is currently running; false if completed since last triggered, asking the build service otherwise. */
     private boolean isRunning(Application application, JobType jobType) {
-        return    ! application.deploymentJobs().statusOf(jobType)
+        return ! application.deploymentJobs().statusOf(jobType)
                                .flatMap(job -> job.lastCompleted().map(run -> run.at().isAfter(job.lastTriggered().get().at())))
                                .orElse(false)
-                  &&   jobStateIsAmong(application, jobType, running, queued);
+               && jobStateContains(application, jobType, running, queued);
     }
 
-    private boolean jobStateIsAmong(Application application, JobType jobType, JobState state, JobState... states) {
-        return EnumSet.of(state, states).contains(buildService.stateOf(BuildJob.of(application.id(),
-                                                                                   application.deploymentJobs().projectId().getAsLong(),
-                                                                                   jobType.jobName())));
+    private boolean jobStateContains(Application application, JobType jobType, JobState... states) {
+        return Arrays.asList(states).contains(buildService.stateOf(BuildJob.of(application.id(),
+                                                                               application.deploymentJobs().projectId().getAsLong(),
+                                                                               jobType.jobName())));
     }
 
     // ---------- Completion logic ----------
@@ -356,22 +361,26 @@ public class DeploymentTrigger {
      * version is broken somehow, such that the job may be locked in failure until a new version is released.
      */
     private Optional<Instant> completedAt(Change change, Application application, JobType jobType) {
-        Versions versions = versionsFor(application, change, deploymentFor(application, jobType));
+        Versions versions = versions(application, change, deploymentFor(application, jobType));
         Optional<JobRun> lastSuccess = successOn(application, jobType, versions);
         if (lastSuccess.isPresent() || ! jobType.isProduction())
             return lastSuccess.map(JobRun::at);
 
         return deploymentFor(application, jobType)
-                .filter(deployment ->    ! (   change.upgrades(deployment.version())
-                                            || change.upgrades(deployment.applicationVersion()))
-                                      &&   (   application.change().downgrades(deployment.version())
-                                            || application.change().downgrades(deployment.applicationVersion())))
+                .filter(deployment -> !isUpgrade(change, deployment) && isDowngrade(application.change(), deployment))
                 .map(Deployment::at);
     }
 
+    private static boolean isUpgrade(Change change, Deployment deployment) {
+        return change.upgrades(deployment.version()) || change.upgrades(deployment.applicationVersion());
+    }
+
+    private static boolean isDowngrade(Change change, Deployment deployment) {
+        return change.downgrades(deployment.version()) || change.downgrades(deployment.applicationVersion());
+    }
+
     private boolean isTested(Application application, Versions versions) {
-        return    testedAt(application, versions).isPresent()
-                  || alreadyTriggered(application, versions);
+        return testedAt(application, versions).isPresent() || alreadyTriggered(application, versions);
     }
 
     private Optional<Instant> testedAt(Application application, Versions versions) {
@@ -391,7 +400,8 @@ public class DeploymentTrigger {
                                               .isPresent());
     }
 
-    /** If the given state's sources are present and differ from its targets, returns whether they are equal to those of the given job run. */
+    /** If the given state's sources are present and differ from its targets, returns whether they are equal to those
+     * of the given job run. */
     private static boolean sourcesMatchIfPresent(Versions versions, JobRun jobRun) {
         return   (   ! versions.sourcePlatform.filter(version -> ! version.equals(versions.targetPlatform)).isPresent()
                      ||   versions.sourcePlatform.equals(jobRun.sourcePlatform()))
@@ -438,7 +448,7 @@ public class DeploymentTrigger {
     /**
      * Returns the list of test jobs that should run now, and that need to succeed on the given versions for it to be considered tested.
      */
-    private List<Job> testJobsFor(Application application, Versions versions, String reason, Instant availableSince) {
+    private List<Job> testJobs(Application application, Versions versions, String reason, Instant availableSince) {
         List<Step> steps = application.deploymentSpec().steps();
         if (steps.isEmpty()) steps = singletonList(new DeploymentSpec.DeclaredZone(test));
         List<Job> jobs = new ArrayList<>();
@@ -446,7 +456,7 @@ public class DeploymentTrigger {
             for (JobType jobType : step.zones().stream().map(order::toJob).collect(toList())) {
                 Optional<JobRun> completion = successOn(application, jobType, versions)
                         .filter(run -> jobType != stagingTest || sourcesMatchIfPresent(versions, run));
-                if ( ! completion.isPresent() && jobStateIsAmong(application, jobType, idle))
+                if (! completion.isPresent() && jobStateContains(application, jobType, idle))
                     jobs.add(deploymentJob(application, versions, application.change(), jobType, reason, availableSince));
             }
         }
@@ -462,7 +472,7 @@ public class DeploymentTrigger {
         return new Job(application, triggering, jobType, availableSince, isRetry, change.application().isPresent());
     }
 
-    private Versions versionsFor(Application application, Change change, Optional<Deployment> deployment) {
+    private Versions versions(Application application, Change change, Optional<Deployment> deployment) {
         return new Versions(targetPlatform(application, change, deployment),
                             targetApplication(application, change, deployment),
                             deployment.map(Deployment::version),
@@ -492,7 +502,8 @@ public class DeploymentTrigger {
         private final boolean isRetry;
         private final boolean isApplicationUpgrade;
 
-        private Job(Application application, JobRun triggering, JobType jobType, Instant availableSince, boolean isRetry, boolean isApplicationUpgrade) {
+        private Job(Application application, JobRun triggering, JobType jobType, Instant availableSince,
+                    boolean isRetry, boolean isApplicationUpgrade) {
             super(application.id(), application.deploymentJobs().projectId().getAsLong(), jobType.jobName());
             this.jobType = jobType;
             this.triggering = triggering;
@@ -516,7 +527,8 @@ public class DeploymentTrigger {
         private final Optional<Version> sourcePlatform;
         private final Optional<ApplicationVersion> sourceApplication;
 
-        private Versions(Version targetPlatform, ApplicationVersion targetApplication, Optional<Version> sourcePlatform, Optional<ApplicationVersion> sourceApplication) {
+        private Versions(Version targetPlatform, ApplicationVersion targetApplication, Optional<Version> sourcePlatform,
+                         Optional<ApplicationVersion> sourceApplication) {
             this.targetPlatform = targetPlatform;
             this.targetApplication = targetApplication;
             this.sourcePlatform = sourcePlatform;
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/jobreport-unexpected-completion.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/jobreport-unexpected-completion.json
index bac6ffbd724..72123e7ae41 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/jobreport-unexpected-completion.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/responses/jobreport-unexpected-completion.json
@@ -1,4 +1,4 @@
 {
   "error-code": "BAD_REQUEST",
-  "message": "Got notified about completion of production-us-east-3 for tenant1.application1, but that has neither been triggered nor deployed"
+  "message": "Notified of completion of production-us-east-3 for tenant1.application1, but that has neither been triggered nor deployed"
 }