Verify deployment versions, not just change versions. More tests fail.

7 files changed, 65 insertions, 64 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 87578e6639b..6891477d142 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
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.application;
 
 import com.google.common.collect.ImmutableMap;
+import com.yahoo.component.Version;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.RegionName;
@@ -98,20 +99,6 @@ public class DeploymentJobs {
         return ! JobList.from(status.values()).failing().isEmpty();
     }
 
-    /** Returns whether change can be deployed to the given environment */
-    public boolean isDeployableTo(Environment environment, Change change) {
-        // TODO jvenstad: Rewrite to verify versions when deployment is already decided.
-        if (environment == null || ! change.isPresent()) {
-            return true;
-        }
-        if (environment == Environment.staging) {
-            return successAt(change, JobType.systemTest).isPresent();
-        } else if (environment == Environment.prod) {
-            return successAt(change, JobType.stagingTest).isPresent();
-        }
-        return true; // other environments do not have any preconditions
-    }
-
     /** Returns the JobStatus of the given JobType, or empty. */
     public Optional<JobStatus> statusOf(JobType jobType) {
         return Optional.ofNullable(jobStatus().get(jobType));
@@ -126,14 +113,6 @@ public class DeploymentJobs {
 
     public Optional<IssueId> issueId() { return issueId; }
 
-    /** Returns the time of success for the given change for the given job type, or empty if unsuccessful. */
-    public Optional<Instant> successAt(Change change, JobType jobType) {
-        return statusOf(jobType)
-                .flatMap(JobStatus::lastSuccess)
-                .filter(status -> status.lastCompletedWas(change))
-                .map(JobStatus.JobRun::at);
-    }
-
     private static OptionalLong requireId(OptionalLong id, String message) {
         Objects.requireNonNull(id, message);
         if ( ! id.isPresent()) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
index bb90370f6c0..27992d43eba 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
@@ -146,6 +146,11 @@ public class JobList {
         }
 
         /** Returns the subset of jobs where the run of the given type was on the given version */
+        public JobList on(ApplicationVersion version) {
+            return filter(run -> run.applicationVersion().equals(version));
+        }
+
+        /** Returns the subset of jobs where the run of the given type was on the given version */
         public JobList on(Version version) {
             return filter(run -> run.version().equals(version));
         }
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 c60871100ac..9250bffd7d0 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
@@ -185,29 +185,6 @@ public class JobStatus {
         /** Returns the time if this triggering or completion */
         public Instant at() { return at; }
 
-        /** Returns whether the job last completed for the given change */
-        public boolean lastCompletedWas(Change change) {
-            if (change.platform().isPresent() && ! change.platform().get().equals(version())) return false;
-            if (change.application().isPresent() && ! change.application().get().equals(applicationVersion)) return false;
-            return true;
-        }
-
-        @Override
-        public int hashCode() {
-            return Objects.hash(version, applicationVersion, at);
-        }
-
-        @Override
-        public boolean equals(Object o) {
-            if (this == o) return true;
-            if ( ! (o instanceof JobRun)) return false;
-            JobRun jobRun = (JobRun) o;
-            return id == jobRun.id &&
-                   Objects.equals(version, jobRun.version) &&
-                   Objects.equals(applicationVersion, jobRun.applicationVersion) &&
-                   Objects.equals(at, jobRun.at);
-        }
-
         @Override
         public String toString() { return "job run " + id + " of version " + version + " "
                                           + applicationVersion + " at " + at; }
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 2723b7f23c1..653d1bc8f22 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
@@ -18,6 +18,7 @@ import com.yahoo.vespa.hosted.controller.application.Deployment;
 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.JobList;
 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;
@@ -28,6 +29,7 @@ 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;
 import java.util.NoSuchElementException;
@@ -40,8 +42,11 @@ import java.util.logging.Logger;
 import java.util.stream.Collectors;
 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.vespa.hosted.controller.api.integration.BuildService.BuildJob;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.component;
+import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.stagingTest;
 import static com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType.systemTest;
 import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
@@ -221,8 +226,9 @@ public class DeploymentTrigger {
         Application application = applications().require(applicationId);
         if (jobType == component)
             buildService.trigger(BuildJob.of(applicationId, application.deploymentJobs().projectId().getAsLong(), jobType.jobName()));
-        else
+        else if (isVerified(application, jobType))
             trigger(deploymentJob(application, jobType, ">:o:< Triggered by force! (-o-) |-o-| (=oo=) ", clock.instant(), Collections.emptySet()));
+        // TODO jvenstad: No need to throw here, since it will rather trigger test jobs (soon!)
     }
 
     private Job deploymentJob(Application application, JobType jobType, String reason, Instant availableSince, Collection<JobType> concurrentlyWith) {
@@ -230,18 +236,20 @@ public class DeploymentTrigger {
                                      .filter(JobError.outOfCapacity::equals).isPresent();
         if (isRetry) reason += "; retrying on out of capacity";
 
-        Change change = application.change();
-        Optional<Deployment> deployment = deploymentFor(application, jobType);
-
-        Version platform = max(deployment.map(Deployment::version), change.platform())
-                .orElse(application.oldestDeployedPlatform()
-                                   .orElse(controller.systemVersion()));
+        return new Job(application, jobType, reason, availableSince, concurrentlyWith, isRetry, application.change(),
+                       targetPlatform(application, application.change(), jobType), targetApplication(application, application.change(), jobType));
+    }
 
-        ApplicationVersion applicationVersion = max(deployment.map(Deployment::applicationVersion), change.application())
+    private ApplicationVersion targetApplication(Application application, Change change, JobType jobType) {
+        return max(deploymentFor(application, jobType).map(Deployment::applicationVersion), change.application())
                 .orElse(application.oldestDeployedApplication()
                                    .orElse(application.deploymentJobs().jobStatus().get(component).lastSuccess().get().applicationVersion()));
+    }
 
-        return new Job(application, jobType, reason, availableSince, concurrentlyWith, isRetry, change, platform, applicationVersion);
+    private Version targetPlatform(Application application, Change change, JobType jobType) {
+        return max(deploymentFor(application, jobType).map(Deployment::version), change.platform())
+                .orElse(application.oldestDeployedPlatform()
+                                   .orElse(controller.systemVersion()));
     }
 
     private static <T extends Comparable<T>> Optional<T> max(Optional<T> o1, Optional<T> o2) {
@@ -277,7 +285,8 @@ public class DeploymentTrigger {
                 }
                 else if (completedAt.isPresent()) { // Step not complete, because some jobs remain -- trigger these if the previous step was done.
                     for (JobType job : remainingJobs)
-                        jobs.add(deploymentJob(application, job, reason, completedAt.get(), stepJobs));
+                        if (isVerified(application, job))
+                            jobs.add(deploymentJob(application, job, reason, completedAt.get(), stepJobs));
                     completedAt = Optional.empty();
                     break;
                 }
@@ -289,6 +298,26 @@ public class DeploymentTrigger {
         return jobs;
     }
 
+    private boolean isVerified(Application application, JobType jobType) {
+        Version targetPlatform = targetPlatform(application, application.change(), jobType);
+        ApplicationVersion targetApplication = targetApplication(application, application.change(), jobType);
+        if (jobType.environment() == staging)
+            return ! JobList.from(application).type(systemTest)
+                            .lastSuccess().on(targetPlatform)
+                            .lastSuccess().on(targetApplication)
+                            .isEmpty();
+        if (jobType.environment() == prod)
+            return    ! JobList.from(application).type(stagingTest)
+                               .lastSuccess().on(targetPlatform)
+                               .lastSuccess().on(targetApplication)
+                               .isEmpty()
+                   || ! JobList.from(application).production()
+                               .lastTriggered().on(targetPlatform)
+                               .lastTriggered().on(targetApplication)
+                               .isEmpty();
+        return true;
+    }
+
     /**
      * Returns the instant when the given change is complete for the given application for the given job.
      *
@@ -298,9 +327,11 @@ public class DeploymentTrigger {
      * part is a downgrade, regardless of the status of the job.
      */
     private Optional<Instant> completedAt(Change change, Application application, JobType jobType) {
-        Optional<Instant> lastSuccess = application.deploymentJobs().successAt(change, jobType);
+        Optional<JobRun> lastSuccess = application.deploymentJobs().statusOf(jobType).flatMap(JobStatus::lastSuccess)
+                                                  .filter(last ->    change.platform().map(last.version()::equals).orElse(true)
+                                                                  && change.application().map(last.applicationVersion()::equals).orElse(true));
         if (lastSuccess.isPresent() || ! jobType.isProduction())
-            return lastSuccess;
+            return lastSuccess.map(JobRun::at);
 
         return deploymentFor(application, jobType)
                 .filter(deployment ->    ! (   change.upgrades(deployment.version())
@@ -312,10 +343,6 @@ public class DeploymentTrigger {
 
     private boolean canTrigger(Job job) {
         Application application = applications().require(job.applicationId());
-        // TODO jvenstad: Check versions, not change.
-        if ( ! application.deploymentJobs().isDeployableTo(job.jobType.environment(), application.change()))
-            return false;
-
         if (isRunning(application, job.jobType))
             return false;
 
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 28522b154d6..6a5201f4fdb 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
@@ -92,7 +92,7 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
 
         Slime slime = new Slime();
         Cursor cursor = slime.setObject();
-        cursor.setString("message", "Triggered " + jobType.jobName() + " for " + tenantName + "." + applicationName + ".default");
+        cursor.setString("message", "Triggered " + jobType.jobName() + " for " + tenantName + "." + applicationName);
         return new SlimeJsonResponse(slime);
     }
 
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 47cc6454121..084ffe7cbdc 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
@@ -379,6 +379,7 @@ public class DeploymentTriggerTest {
         Version version1 = new Version("6.2");
         tester.upgradeSystem(version1);
         tester.jobCompletion(productionUsCentral1).application(application).unsuccessful().submit();
+        // TODO jvenstad: Fails here now, because job isn't triggered any more, as deploy target is not verified.
         tester.completeUpgrade(application, version1, applicationPackage);
         assertEquals(appVersion1, app.get().deployments().get(ZoneId.from("prod.us-central-1")).applicationVersion());
     }
@@ -426,6 +427,7 @@ public class DeploymentTriggerTest {
 
         assertEquals(v2, app.get().deployments().get(productionUsCentral1.zone(main).get()).version());
         assertEquals((Long) 42L, app.get().deployments().get(productionUsCentral1.zone(main).get()).applicationVersion().buildNumber().get());
+        // TODO jvenstad: Fails here now, because job isn't triggered any more, as deploy target is not verified.
         assertNotEquals(triggered, app.get().deploymentJobs().jobStatus().get(productionUsCentral1).lastTriggered().get().at());
 
         // Change has a higher application version than what is deployed -- deployment should trigger.
@@ -434,7 +436,7 @@ public class DeploymentTriggerTest {
         assertEquals(v2, app.get().deployments().get(productionUsCentral1.zone(main).get()).version());
         assertEquals((Long) 43L, app.get().deployments().get(productionUsCentral1.zone(main).get()).applicationVersion().buildNumber().get());
 
-        // Change is again strictly dominated, and us-central-1 should be skipped, even though it is still a failure.
+        // Change is again strictly dominated, and us-central-1 is skipped, even though it is still failing.
         tester.deployAndNotify(application, applicationPackage, false, productionUsCentral1);
         tester.deployAndNotify(application, applicationPackage, true, productionEuWest1);
         assertFalse(app.get().change().isPresent());
@@ -471,6 +473,7 @@ public class DeploymentTriggerTest {
         assertEquals(v1, app.get().deployments().get(productionUsEast3.zone(main).get()).version());
 
         // New application version should run system and staging tests first against 6.2, then against 6.1.
+        // TODO jvenstad: Make triggering happen at 6.2 here, since that is the first production job. Currently it tests 6.1.
         tester.jobCompletion(component).application(application).nextBuildNumber().uploadArtifact(applicationPackage).submit();
         assertEquals(v2, app.get().deploymentJobs().jobStatus().get(systemTest).lastTriggered().get().version());
         tester.deployAndNotify(application, Optional.empty(), true, systemTest);
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
index 6e79f53cae6..91d5d687531 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
@@ -3,6 +3,9 @@ package com.yahoo.vespa.hosted.controller.restapi.screwdriver;
 
 import com.yahoo.application.container.handler.Request;
 import com.yahoo.vespa.hosted.controller.Application;
+import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
+import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport;
+import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.restapi.ContainerControllerTester;
 import com.yahoo.vespa.hosted.controller.restapi.ControllerContainerTest;
 import com.yahoo.vespa.hosted.controller.versions.VersionStatus;
@@ -57,12 +60,19 @@ public class ScrewdriverApiTest extends ControllerContainerTest {
                                    app.id().tenant().value() + "/application/" + app.id().application().value(),
                                    new byte[0], Request.Method.POST),
                        200, "{\"message\":\"Triggered component for tenant1.application1\"}");
+        tester.controller().applications().deploymentTrigger().notifyOfCompletion(new JobReport(app.id(),
+                                                                                                DeploymentJobs.JobType.component,
+                                                                                                1,
+                                                                                                42,
+                                                                                                Optional.of(new SourceRevision("repo", "branch", "commit")),
+                                                                                                Optional.empty()));
 
         // Triggers specific job when given -- fails here because the job has never run before, and so application version can't be resolved.
         assertResponse(new Request("http://localhost:8080/screwdriver/v1/trigger/tenant/" +
                                    app.id().tenant().value() + "/application/" + app.id().application().value(),
                                    "staging-test".getBytes(StandardCharsets.UTF_8), Request.Method.POST),
-                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Cannot determine application version to use for stagingTest\"}");
+                       200, "{\"message\":\"Triggered staging-test for tenant1.application1\"}");
+        // TODO jvenstad: This should trigger system-test instead, unless they are allowed to run in parallel.
     }
 
 }