Missed some cleanup, and moved validation at deployment

4 files changed, 48 insertions, 57 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index fce36406830..84d821b9f46 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -296,6 +296,7 @@ public class ApplicationController {
                         ? application.oldestDeployedApplication().orElse(triggered.get().applicationVersion())
                         : triggered.get().applicationVersion();
                 applicationPackage = new ApplicationPackage(artifactRepository.getApplicationPackage(application.id(), applicationVersion.id()));
+                validateRun(application, zone, platformVersion, applicationVersion);
             }
 
             validate(applicationPackage.deploymentSpec());
@@ -316,12 +317,6 @@ public class ApplicationController {
                 store(application); // store missing information even if we fail deployment below
             }
 
-            // TODO jvenstad: Use code from DeploymentTrigger? Also, validate application version.
-            // Validate the change being deployed
-            if ( ! canDeployDirectly) {
-                validateChange(application, zone, platformVersion);
-            }
-
             // Assign global rotation
             application = withRotation(application, zone);
             Set<String> rotationNames = new HashSet<>();
@@ -602,26 +597,20 @@ public class ApplicationController {
                 .forEach(zone -> {
                     if ( ! controller.zoneRegistry().hasZone(ZoneId.from(zone.environment(),
                                                                        zone.region().orElse(null)))) {
-                        throw new IllegalArgumentException("Zone " + zone + " in deployment spec was not found in " +
-                                                           "this system!");
+                        throw new IllegalArgumentException("Zone " + zone + " in deployment spec was not found in this system!");
                     }
                 });
     }
 
-    /** Verify that what we want to deploy is tested and that we aren't downgrading */
-    private void validateChange(Application application, ZoneId zone, Version version) {
-        if ( ! application.deploymentJobs().isDeployableTo(zone.environment(), application.change())) {
-            throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
-                                               " as " + application.change() + " is not tested");
-        }
-        // TODO jvenstad: Rewrite to use decided versions. Simplifies the below.
-        Deployment existingDeployment = application.deployments().get(zone);
-        if (zone.environment().isProduction() && existingDeployment != null &&
-            existingDeployment.version().isAfter(version)) {
-            throw new IllegalArgumentException("Rejecting deployment of " + application + " to " + zone +
-                                               " as the requested version " + version + " is older than" +
-                                               " the current version " + existingDeployment.version());
-        }
+    /** Verify that we don't downgrade an existing production deployment. */
+    private void validateRun(Application application, ZoneId zone, Version platformVersion, ApplicationVersion applicationVersion) {
+        Deployment deployment = application.deployments().get(zone);
+        if (   zone.environment().isProduction() && deployment != null
+            && (   platformVersion.compareTo(deployment.version()) < 0
+                || applicationVersion.compareTo(deployment.applicationVersion()) < 0))
+            throw new IllegalArgumentException(String.format("Rejecting deployment of %s to %s, as the requested versions (platform: %s, application: %s)" +
+                                                             " are older than the currently deployed (platform: %s, application: %s).",
+                                                             application, zone, platformVersion, applicationVersion, deployment.version(), deployment.applicationVersion()));
     }
 
     public RotationRepository rotationRepository() {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
index 5c325865dd0..c60871100ac 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
@@ -8,6 +8,8 @@ import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * The last known build status of a particular deployment job for a particular application.
  * This is immutable.
@@ -33,12 +35,12 @@ public class JobStatus {
     public JobStatus(DeploymentJobs.JobType type, Optional<DeploymentJobs.JobError> jobError,
                      Optional<JobRun> lastTriggered, Optional<JobRun> lastCompleted,
                      Optional<JobRun> firstFailing, Optional<JobRun> lastSuccess) {
-        Objects.requireNonNull(type, "jobType cannot be null");
-        Objects.requireNonNull(jobError, "jobError cannot be null");
-        Objects.requireNonNull(lastTriggered, "lastTriggered cannot be null");
-        Objects.requireNonNull(lastCompleted, "lastCompleted cannot be null");
-        Objects.requireNonNull(firstFailing, "firstFailing cannot be null");
-        Objects.requireNonNull(lastSuccess, "lastSuccess cannot be null");
+        requireNonNull(type, "jobType cannot be null");
+        requireNonNull(jobError, "jobError cannot be null");
+        requireNonNull(lastTriggered, "lastTriggered cannot be null");
+        requireNonNull(lastCompleted, "lastCompleted cannot be null");
+        requireNonNull(firstFailing, "firstFailing cannot be null");
+        requireNonNull(lastSuccess, "lastSuccess cannot be null");
 
         this.type = type;
         this.jobError = jobError;
@@ -160,16 +162,12 @@ public class JobStatus {
         private final String reason;
         private final Instant at;
 
-        public JobRun(long id, Version version, ApplicationVersion applicationVersion,String reason, Instant at) {
-            Objects.requireNonNull(version, "version cannot be null");
-            Objects.requireNonNull(applicationVersion, "applicationVersion cannot be null");
-            Objects.requireNonNull(reason, "Reason cannot be null");
-            Objects.requireNonNull(at, "at cannot be null");
+        public JobRun(long id, Version version, ApplicationVersion applicationVersion, String reason, Instant at) {
             this.id = id;
-            this.version = version;
-            this.applicationVersion = applicationVersion;
-            this.reason = reason;
-            this.at = at;
+            this.version = requireNonNull(version);
+            this.applicationVersion = requireNonNull(applicationVersion);
+            this.reason = requireNonNull(reason);
+            this.at = requireNonNull(at);
         }
 
         /** Returns the id of this run of this job, or -1 if not known */
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 b9ecaa61e6d..e3941edadea 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
@@ -19,6 +19,7 @@ import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
 import com.yahoo.vespa.hosted.controller.application.JobStatus;
+import com.yahoo.vespa.hosted.controller.application.JobStatus.JobRun;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
 import java.time.Clock;
@@ -27,7 +28,9 @@ import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.Optional;
@@ -36,6 +39,7 @@ 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 java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
@@ -116,8 +120,7 @@ public class DeploymentTrigger {
      * Only one job is triggered each run for test jobs, since their environments have limited capacity.
      */
     public long triggerReadyJobs() {
-        return computeReadyJobs().stream()
-                                 .collect(partitioningBy(job -> job.jobType().isTest()))
+        return computeReadyJobs().collect(partitioningBy(job -> job.jobType().isTest()))
                                  .entrySet().stream()
                                  .flatMap(entry -> (entry.getKey()
                                          // True for capacity constrained zones -- sort by priority and make a task for each job type.
@@ -144,18 +147,19 @@ 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 for %s, deploying %s: %s (platform: %s, application: %s)", job.jobType, job.id, job.change, job.reason, job.platformVersion, job.applicationVersion.id()));
+        log.log(LogLevel.INFO, String.format("Attempting to trigger %s, deploying %s: %s (platform: %s, application: %s)", job, job.change, job.reason, job.platformVersion, job.applicationVersion.id()));
 
         try {
             buildService.trigger(job);
-            applications().lockOrThrow(job.id, application -> applications().store(application.withJobTriggering(
-                    job.jobType, new JobStatus.JobRun(-1, job.platformVersion, job.applicationVersion, job.reason, clock.instant()))));
+            applications().lockOrThrow(job.applicationId(), application -> applications().store(application.withJobTriggering(
+                    job.jobType, new JobRun(-1, job.platformVersion, job.applicationVersion, job.reason, clock.instant()))));
             return true;
         }
         catch (RuntimeException e) {
-            log.log(LogLevel.WARNING, String.format("Exception triggering %s for %s (%s): %s", job.jobType, job.id, job.projectId, e));
+            log.log(LogLevel.WARNING, "Exception triggering " + job + ": " + e);
             if (e instanceof NoSuchElementException || e instanceof IllegalArgumentException)
-                applications().lockOrThrow(job.id, application -> applications().store(application.withProjectId(OptionalLong.empty())));
+                applications().lockOrThrow(job.applicationId(), application ->
+                        applications().store(application.withProjectId(OptionalLong.empty())));
             return false;
         }
     }
@@ -189,16 +193,19 @@ public class DeploymentTrigger {
         });
     }
 
+    public Map<JobType, ? extends List<? extends BuildService.BuildJob>> jobsToRun() {
+        return computeReadyJobs().collect(groupingBy(Job::jobType));
+    }
+
     /** Returns the set of all jobs which have changes to propagate from the upstream steps. */
-    public List<Job> computeReadyJobs() {
+    public Stream<Job> computeReadyJobs() {
         return ApplicationList.from(applications().asList())
                               .notPullRequest()
                               .withProjectId()
                               .deploying()
                               .idList().stream()
                               .map(this::computeReadyJobs)
-                              .flatMap(List::stream)
-                .collect(Collectors.toList());
+                              .flatMap(List::stream);
     }
 
     /** Returns whether the given job is currently running; false if completed since last triggered, asking the build service othewise. */
@@ -209,6 +216,7 @@ public class DeploymentTrigger {
     }
 
     public Job forcedDeploymentJob(Application application, JobType jobType, String reason) {
+        // TODO jvenstad: Verification here!
         return deploymentJob(application, jobType, reason, clock.instant(), Collections.emptySet());
     }
 
@@ -342,7 +350,8 @@ public class DeploymentTrigger {
         return Optional.ofNullable(application.deployments().get(jobType.zone(controller.system()).get()));
     }
 
-    public static class Job extends BuildService.BuildJob {
+
+    private static class Job extends BuildService.BuildJob {
 
         private final JobType jobType;
         private final String reason;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
index 960b1c84c3f..0d8339e04ca 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
@@ -10,6 +10,7 @@ import com.yahoo.slime.Cursor;
 import com.yahoo.slime.Slime;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
+import com.yahoo.vespa.hosted.controller.api.integration.BuildService;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger;
 import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse;
@@ -19,7 +20,6 @@ import com.yahoo.vespa.hosted.controller.versions.VespaVersion;
 import com.yahoo.yolean.Exceptions;
 
 import java.io.InputStream;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -70,7 +70,7 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
             return vespaVersion();
         }
         if (path.matches("/screwdriver/v1/jobsToRun"))
-            return buildJobs(controller.applications().deploymentTrigger().computeReadyJobs().stream().collect(groupingBy(job -> job.jobType())));
+            return buildJobs(controller.applications().deploymentTrigger().jobsToRun());
         return notFound(request);
     }
 
@@ -112,20 +112,15 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
         return new SlimeJsonResponse(slime);
     }
 
-    private HttpResponse buildJobs(Map<JobType, List<DeploymentTrigger.Job>> jobLists) {
+    private HttpResponse buildJobs(Map<JobType, ? extends List<? extends BuildService.BuildJob>> jobLists) {
         Slime slime = new Slime();
         Cursor jobTypesObject = slime.setObject();
         jobLists.forEach((jobType, jobs) -> {
             Cursor jobArray = jobTypesObject.setArray(jobType.jobName());
             jobs.forEach(job -> {
                 Cursor buildJobObject = jobArray.addObject();
-                buildJobObject.setString("id", job.applicationId().toString());
+                buildJobObject.setString("applicationId", job.applicationId().toString());
                 buildJobObject.setLong("projectId", job.projectId());
-                buildJobObject.setString("reason", job.reason());
-                buildJobObject.setString("change", job.change().toString());
-                buildJobObject.setLong("availableSince", job.availableSince().toEpochMilli());
-                buildJobObject.setString("platform", job.platform().toString());
-                buildJobObject.setString("application", job.application().toString());
             });
         });
         return new SlimeJsonResponse(slime);