Merge pull request #4687 from vespa-engine/mpolden/source-revision-in-job-report

Add source revision to job report

9 files changed, 29 insertions, 25 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 1d04106d7bb..a728786e2ce 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
@@ -7,9 +7,9 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
-import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 
 import java.time.Instant;
 import java.util.Collection;
@@ -244,18 +244,21 @@ public class DeploymentJobs {
         private final JobType jobType;
         private final long projectId;
         private final long buildNumber;
+        private final Optional<SourceRevision> sourceRevision;
         private final Optional<JobError> jobError;
 
         public JobReport(ApplicationId applicationId, JobType jobType, long projectId, long buildNumber,
-                         Optional<JobError> jobError) {
+                         Optional<SourceRevision> sourceRevision, Optional<JobError> jobError) {
             Objects.requireNonNull(applicationId, "applicationId cannot be null");
             Objects.requireNonNull(jobType, "jobType cannot be null");
+            Objects.requireNonNull(sourceRevision, "sourceRevision cannot be null");
             Objects.requireNonNull(jobError, "jobError cannot be null");
 
             this.applicationId = applicationId;
             this.projectId = projectId;
             this.buildNumber = buildNumber;
             this.jobType = jobType;
+            this.sourceRevision = sourceRevision; // TODO: Require non-empty source revision if jobType == component
             this.jobError = jobError;
         }
 
@@ -264,6 +267,7 @@ public class DeploymentJobs {
         public long projectId() { return projectId; }
         public long buildNumber() { return buildNumber; }
         public boolean success() { return ! jobError.isPresent(); }
+        public Optional<SourceRevision> sourceRevision() { return sourceRevision; }
         public Optional<JobError> jobError() { return jobError; }
 
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
index 25b3cd0fd1e..d506e8f3dcd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentOrder.java
@@ -6,7 +6,6 @@ import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.LockedApplication;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
-import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
@@ -116,12 +115,6 @@ public class DeploymentOrder {
                 .collect(collectingAndThen(toList(), Collections::unmodifiableList));
     }
 
-    /** Returns whether all jobs have completed successfully for given step */
-    private boolean completedSuccessfully(DeploymentSpec.Step step, Change change, Application application) {
-        return jobsFrom(step).stream()
-                .allMatch(job -> application.deploymentJobs().isSuccessful(change, job));
-    }
-
     /** Resolve deployment step from job */
     private Optional<DeploymentSpec.Step> fromJob(JobType job, Application application) {
         for (DeploymentSpec.Step step : application.deploymentSpec().steps()) {
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 08676a4d1b5..7908f9b095a 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
@@ -399,11 +399,11 @@ public class DeploymentTrigger {
         if ( ! job.isProduction())
             throw new IllegalArgumentException(job + " is not a production job!");
 
-        return    lastSuccessfulIs(version, job, application)
-               || job.zone(controller.system())
-                       .map(zone -> application.deployments().get(zone))
-                       .map(deployment -> deployment.version().isAfter(version))
-                       .orElse(false);
+        return lastSuccessfulIs(version, job, application) ||
+               job.zone(controller.system())
+                  .map(zone -> application.deployments().get(zone))
+                  .map(deployment -> deployment.version().isAfter(version))
+                  .orElse(false);
     }
 
     private boolean acceptNewRevisionNow(LockedApplication application) {
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 17181950a29..44f3197643a 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
@@ -16,6 +16,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
 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.SourceRevision;
 import com.yahoo.vespa.hosted.controller.restapi.ErrorResponse;
 import com.yahoo.vespa.hosted.controller.restapi.Path;
 import com.yahoo.vespa.hosted.controller.restapi.SlimeJsonResponse;
@@ -173,10 +174,21 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
                 JobType.fromJobName(report.field("jobName").asString()),
                 report.field("projectId").asLong(),
                 report.field("buildNumber").asLong(),
+                toSourceRevision(report.field("sourceRevision")),
                 jobError
         );
     }
 
+    private static Optional<SourceRevision> toSourceRevision(Inspector object) {
+        if (!object.field("repository").valid() ||
+            !object.field("branch").valid() ||
+            !object.field("commit").valid()) {
+            return Optional.empty();
+        }
+        return Optional.of(new SourceRevision(object.field("repository").asString(), object.field("branch").asString(),
+                                              object.field("commit").asString()));
+    }
+
     private static String asString(InputStream in) {
         Scanner scanner = new Scanner(in).useDelimiter("\\A");
         if (scanner.hasNext()) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
index 13eec52b97a..c1e23e91eac 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/versions/VersionStatus.java
@@ -5,13 +5,11 @@ import com.google.common.collect.ImmutableList;
 import com.yahoo.collections.ListMap;
 import com.yahoo.component.Version;
 import com.yahoo.component.Vtag;
-import com.yahoo.config.provision.RegionName;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.github.GitSha;
 import com.yahoo.vespa.hosted.controller.application.ApplicationList;
 import com.yahoo.vespa.hosted.controller.application.Deployment;
-import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.JobList;
 
 import java.net.URI;
@@ -95,8 +93,7 @@ public class VersionStatus {
         Version systemVersion = infrastructureVersions.stream().sorted().findFirst().get();
 
         Collection<DeploymentStatistics> deploymentStatistics = computeDeploymentStatistics(infrastructureVersions,
-                                                                                            controller.applications().asList(),
-                                                                                            controller.applications().deploymentTrigger().jobTimeoutLimit());
+                                                                                            controller.applications().asList());
         List<VespaVersion> versions = new ArrayList<>();
 
         for (DeploymentStatistics statistics : deploymentStatistics) {
@@ -132,8 +129,7 @@ public class VersionStatus {
     }
 
     private static Collection<DeploymentStatistics> computeDeploymentStatistics(Set<Version> infrastructureVersions,
-                                                                                List<Application> applications,
-                                                                                Instant jobTimeoutLimit) {
+                                                                                List<Application> applications) {
         Map<Version, DeploymentStatistics> versionMap = new HashMap<>();
 
         for (Version infrastructureVersion : infrastructureVersions) {
@@ -141,8 +137,6 @@ public class VersionStatus {
         }
 
         for (Application application : ApplicationList.from(applications).notPullRequest().asList()) {
-            DeploymentJobs jobs = application.deploymentJobs();
-
             // Note that each version deployed on this application in production exists
             // (ignore non-production versions)
             for (Deployment deployment : application.productionDeployments().values()) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 0fa0189f506..cec4ef7e400 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -10,7 +10,6 @@ import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.api.Tenant;
 import com.yahoo.vespa.hosted.controller.api.application.v4.model.DeployOptions;
@@ -25,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
 import com.yahoo.vespa.hosted.controller.api.integration.athenz.NToken;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
 import com.yahoo.vespa.hosted.controller.application.ApplicationRevision;
 import com.yahoo.vespa.hosted.controller.application.Change;
@@ -36,7 +36,6 @@ import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.BuildSystem;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester;
-import com.yahoo.vespa.hosted.controller.persistence.ApplicationSerializer;
 import com.yahoo.vespa.hosted.controller.rotation.RotationId;
 import com.yahoo.vespa.hosted.controller.rotation.RotationLock;
 import com.yahoo.vespa.hosted.controller.versions.DeploymentStatistics;
@@ -569,7 +568,6 @@ public class ControllerTest {
         assertEquals(version, tester.controller().versionStatus().systemVersion().get().versionNumber());
 
         // Load test data data
-        ApplicationSerializer serializer = new ApplicationSerializer();
         byte[] json = Files.readAllBytes(Paths.get("src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/canary-with-stale-data.json"));
         Application application = tester.controllerTester().createApplication(SlimeUtils.jsonToSlime(json));
 
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 de8b6b9cd18..c9f0c6cba1d 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
@@ -163,6 +163,7 @@ public class DeploymentTester {
                 jobType,
                 application.deploymentJobs().projectId().get(),
                 42,
+                Optional.empty(),
                 jobError
         );
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java
index 1b1a4feaa4e..b7da2af263c 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/MockBuildService.java
@@ -162,6 +162,7 @@ public class MockBuildService implements BuildService {
                                 jobType,
                                 projectId,
                                 42,
+                                Optional.empty(),
                                 Optional.ofNullable(success ? null : JobError.unknown)
                         ));
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
index ab1dde996e6..b3ed069c32e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
@@ -95,6 +95,7 @@ public class ContainerControllerTester {
         }
         controller().applications().notifyJobCompletion(new DeploymentJobs.JobReport(applicationId, job, projectId,
                                                                                      42,
+                                                                                     Optional.empty(),
                                                                                      success ? Optional.empty() : Optional.of(DeploymentJobs.JobError.unknown)
         ));
     }