Clarify and simplify little bits

2 files changed, 22 insertions, 16 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
index 6948e1f1dd0..6054e13ebde 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatus.java
@@ -90,20 +90,24 @@ public class DeploymentStatus {
         this.allSteps = List.copyOf(allSteps);
     }
 
+    /** The application this deployment status concerns. */
     public Application application() {
         return application;
     }
 
+    /** A filterable list of the status of all jobs for this application. */
     public JobList jobs() {
         return allJobs;
     }
 
+    /** Whether any jobs of this application are failing with other errors than lack of capacity in a test zone. */
     public boolean hasFailures() {
         return ! allJobs.failing()
                         .not().withStatus(RunStatus.outOfCapacity)
                         .isEmpty();
     }
 
+    /** All job statuses, by job type, for the given instance. */
     public Map<JobType, JobStatus> instanceJobs(InstanceName instance) {
         return allJobs.asList().stream()
                       .filter(job -> job.id().application().equals(application.id().instance(instance)))
@@ -111,13 +115,17 @@ public class DeploymentStatus {
                                                          job -> job));
     }
 
+    /** Filterable job status lists for each instance of this application. */
     public Map<ApplicationId, JobList> instanceJobs() {
         return allJobs.asList().stream()
                       .collect(groupingBy(job -> job.id().application(),
                                           collectingAndThen(toUnmodifiableList(), JobList::from)));
     }
 
-    /** Returns the set of jobs that need to run for the application's current change to be considered complete. */
+    /**
+     * The set of jobs that need to run for the application's current change to be considered complete,
+     * and any test jobs for any oustanding change, which will likely be needed to lated deploy this change.
+     */
     public Map<JobId, List<Versions>> jobsToRun() {
         Map<JobId, List<Versions>> jobs = jobsToRun(application().change());
         if (outstandingChange().isEmpty())
@@ -136,7 +144,7 @@ public class DeploymentStatus {
                                                 ImmutableMap::copyOf));
     }
 
-    /** Returns the set of jobs that need to run for the given change to be considered complete. */
+    /** The set of jobs that need to run for the given change to be considered complete. */
     public Map<JobId, List<Versions>> jobsToRun(Change change) {
         Map<JobId, Versions> productionJobs = productionJobs(change);
         Map<JobId, List<Versions>> testJobs = testJobs(productionJobs);
@@ -145,8 +153,10 @@ public class DeploymentStatus {
         return ImmutableMap.copyOf(jobs);
     }
 
+    /** The step status for all steps in the deployment spec of this, which are jobs, in the same order as in the deployment spec. */
     public Map<JobId, StepStatus> jobSteps() { return jobSteps; }
 
+    /** The step status for all steps in the deployment spec of this, in the same order as in the deployment spec. */
     public List<StepStatus> allSteps() { return allSteps; }
 
     public Optional<Deployment> deploymentFor(JobId job) {
@@ -155,7 +165,7 @@ public class DeploymentStatus {
     }
 
     /**
-     * Returns the change of this application's latest submission, if this upgrades any of its production deployments,
+     * The change of this application's latest submission, if this upgrades any of its production deployments,
      * and has not yet started rolling out, due to some other change or a block window being present at the time of submission.
      */
     public Change outstandingChange() {
@@ -178,6 +188,7 @@ public class DeploymentStatus {
                                                                                         .successOn(versions).isEmpty());
     }
 
+    /** The production jobs that need to run to complete roll-out of the given change to production. */
     public Map<JobId, Versions> productionJobs(Change change) {
         Map<JobId, Versions> jobs = new LinkedHashMap<>();
         jobSteps.forEach((job, step) -> {
@@ -188,11 +199,12 @@ public class DeploymentStatus {
         return ImmutableMap.copyOf(jobs);
     }
 
+    /** The test jobs that need to run prior to the given production deployment jobs. */
     public Map<JobId, List<Versions>> testJobs(Map<JobId, Versions> jobs) {
         Map<JobId, List<Versions>> testJobs = new LinkedHashMap<>();
         for (JobType testType : List.of(systemTest, stagingTest)) {
             jobs.forEach((job, versions) -> {
-                if (job.type().isDeployment()) {
+                if (job.type().isProduction() && job.type().isDeployment()) {
                     declaredTest(job.application(), testType).ifPresent(testJob -> {
                         if (allJobs.successOn(versions).get(testJob).isEmpty())
                             testJobs.merge(testJob, List.of(versions), DeploymentStatus::union);
@@ -200,7 +212,7 @@ public class DeploymentStatus {
                 }
             });
             jobs.forEach((job, versions) -> {
-                if (   job.type().isDeployment()
+                if (   job.type().isProduction() && job.type().isDeployment()
                     && allJobs.successOn(versions).type(testType).isEmpty()
                     && testJobs.keySet().stream()
                                .noneMatch(test ->    test.type() == testType
@@ -211,12 +223,13 @@ public class DeploymentStatus {
         return ImmutableMap.copyOf(testJobs);
     }
 
+    /** JobId of any declared test of the given type, for the given instance. */
     private Optional<JobId> declaredTest(ApplicationId instanceId, JobType testJob) {
         JobId jobId = new JobId(instanceId, testJob);
         return jobSteps.get(jobId).isDeclared() ? Optional.of(jobId) : Optional.empty();
     }
 
-    /** Returns a DAG of the dependencies between the primitive steps in the spec, with iteration order equal to declaration order. */
+    /** A DAG of the dependencies between the primitive steps in the spec, with iteration order equal to declaration order. */
     private Map<JobId, StepStatus> jobDependencies(DeploymentSpec spec, List<StepStatus> allSteps) {
         if (DeploymentSpec.empty.equals(spec))
             return Map.of();
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 dfd25c57e70..05b9bbbadd0 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
@@ -4,8 +4,6 @@ package com.yahoo.vespa.hosted.controller.deployment;
 import com.yahoo.config.application.api.DeploymentInstanceSpec;
 import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.ApplicationId;
-import com.yahoo.config.provision.Environment;
-import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
@@ -31,13 +29,9 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.OptionalLong;
-import java.util.function.Predicate;
 import java.util.function.Supplier;
 import java.util.logging.Logger;
-import java.util.stream.Stream;
 
-import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.stagingTest;
-import static com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType.systemTest;
 import static java.util.Comparator.comparing;
 import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.partitioningBy;
@@ -153,13 +147,12 @@ public class DeploymentTrigger {
     public List<JobId> forceTrigger(ApplicationId applicationId, JobType jobType, String user, boolean requireTests) {
         Application application = applications().requireApplication(TenantAndApplicationId.from(applicationId));
         Instance instance = application.require(applicationId.instance());
-        Versions versions = Versions.from(application.change(), application, deploymentFor(instance, jobType),
-                                          controller.systemVersion());
-        String reason = "Job triggered manually by " + user;
         DeploymentStatus status = jobs.deploymentStatus(application);
         JobId job = new JobId(instance.id(), jobType);
+        Versions versions = Versions.from(application.change(), application, status.deploymentFor(job), controller.systemVersion());
+        String reason = "Job triggered manually by " + user;
         Map<JobId, List<Versions>> jobs = status.testJobs(Map.of(job, versions));
-        if (jobs.isEmpty() || ! requireTests || ! jobType.isProduction())
+        if (jobs.isEmpty() || ! requireTests)
             jobs = Map.of(job, List.of(versions));
         jobs.forEach((jobId, versionsList) -> {
             trigger(deploymentJob(instance, versionsList.get(0), application.change(), jobId.type(), status.jobs().get(jobId).get(), reason, clock.instant()));