Handle when an instance has only declared tests

5 files changed, 90 insertions, 12 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java
index 773c927b588..2a1874140a8 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/InstanceList.java
@@ -66,10 +66,17 @@ public class InstanceList extends AbstractFilteringList<ApplicationId, InstanceL
         return matching(id -> instance(id).productionDeployments().size() > 0);
     }
 
-    /** Returns the subset of instances which have at least one deployment on a lower version than the given one */
+    /** Returns the subset of instances which contain declared jobs */
+    public InstanceList withDeclaredJobs() {
+        return matching(id -> statuses.get(id).jobSteps().values().stream()
+                                      .anyMatch(job -> job.isDeclared() && job.job().get().application().equals(id)));
+    }
+
+    /** Returns the subset of instances which have at least one deployment on a lower version than the given one, or which have no production deployments */
     public InstanceList onLowerVersionThan(Version version) {
-        return matching(id -> instance(id).productionDeployments().values().stream()
-                                          .anyMatch(deployment -> deployment.version().isBefore(version)));
+        return matching(id ->    instance(id).productionDeployments().isEmpty()
+                              || instance(id).productionDeployments().values().stream()
+                                             .anyMatch(deployment -> deployment.version().isBefore(version)));
     }
 
     /** Returns the subset of instances which have changes left to deploy; blocked, or deploying */
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 3d5a181b987..07dc8dbe7e8 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
@@ -151,13 +151,31 @@ public class DeploymentStatus {
                                                 ImmutableMap::copyOf));
     }
 
-    /** 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 changes to be considered complete. */
     public Map<JobId, List<Versions>> jobsToRun(Map<InstanceName, Change> changes) {
         Map<JobId, Versions> productionJobs = new LinkedHashMap<>();
         changes.forEach((instance, change) -> productionJobs.putAll(productionJobs(instance, change)));
         Map<JobId, List<Versions>> testJobs = testJobs(productionJobs);
         Map<JobId, List<Versions>> jobs = new LinkedHashMap<>(testJobs);
         productionJobs.forEach((job, versions) -> jobs.put(job, List.of(versions)));
+        // Add runs for idle, declared test jobs if they have no successes on their instance's change's versions.
+        jobSteps.forEach((job, step) -> {
+            if ( ! step.isDeclared() || jobs.containsKey(job))
+                return;
+
+            Change change = changes.get(job.application().instance());
+            if (change == null || ! change.hasTargets())
+                return;
+
+            Optional<JobId> firstProductionJobWithDeployment = jobSteps.keySet().stream()
+                                                                       .filter(jobId -> jobId.type().isProduction() && jobId.type().isDeployment())
+                                                                       .filter(jobId -> deploymentFor(jobId).isPresent())
+                                                                       .findFirst();
+
+            Versions versions = Versions.from(change, application, firstProductionJobWithDeployment.flatMap(this::deploymentFor), systemVersion);
+            if (step.completedAt(change, firstProductionJobWithDeployment).isEmpty())
+            jobs.merge(job, List.of(versions), DeploymentStatus::union);
+        });
         return ImmutableMap.copyOf(jobs);
     }
 
@@ -237,6 +255,7 @@ public class DeploymentStatus {
                                                   && testJobs.get(test).contains(versions)))
                     testJobs.merge(firstDeclaredOrElseImplicitTest(testType), List.of(versions), DeploymentStatus::union);
             });
+            // Add runs for declared tests in instances without production jobs, if no successes exist for given change.
         }
         return ImmutableMap.copyOf(testJobs);
     }
@@ -312,7 +331,7 @@ public class DeploymentStatus {
 
         if (step instanceof DeploymentInstanceSpec) {
             DeploymentInstanceSpec spec = ((DeploymentInstanceSpec) step);
-            StepStatus instanceStatus = new InstanceStatus(spec, previous, now, application.require(spec.name()));
+            StepStatus instanceStatus = new InstanceStatus(spec, previous, now, application.require(spec.name()), this);
             instance = spec.name();
             allSteps.add(instanceStatus);
             previous = List.of(instanceStatus);
@@ -458,20 +477,26 @@ public class DeploymentStatus {
         private final DeploymentInstanceSpec spec;
         private final Instant now;
         private final Instance instance;
+        private final DeploymentStatus status;
 
         private InstanceStatus(DeploymentInstanceSpec spec, List<StepStatus> dependencies, Instant now,
-                               Instance instance) {
+                               Instance instance, DeploymentStatus status) {
             super(StepType.instance, spec, dependencies, spec.name());
             this.spec = spec;
             this.now = now;
             this.instance = instance;
+            this.status = status;
         }
 
-        /** Time of completion of its dependencies, if all parts of the given change are contained in the change for this instance. */
+        /**
+         * Time of completion of its dependencies, if all parts of the given change are contained in the change
+         * for this instance, or if no more jobs should run for this instance for the given change.
+         */
         @Override
         public Optional<Instant> completedAt(Change change, Optional<JobId> dependent) {
-            return    (change.platform().isEmpty() || change.platform().equals(instance.change().platform()))
-                   && (change.application().isEmpty() || change.application().equals(instance.change().application()))
+            return    (   (change.platform().isEmpty() || change.platform().equals(instance.change().platform()))
+                       && (change.application().isEmpty() || change.application().equals(instance.change().application()))
+                   || status.jobsToRun(Map.of(instance.name(), change)).isEmpty())
                       ? dependenciesCompletedAt(change, dependent)
                       : Optional.empty();
         }
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 e2fb74ec0b4..0ac9e17c24f 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
@@ -420,8 +420,10 @@ public class DeploymentTrigger {
 
         @Override
         public String toString() {
-            return jobType + " for " + instanceId + " on (" + versions.targetPlatform() + ", " +
-                   versions.targetApplication().id() + "), ready since " + availableSince;
+            return jobType + " for " + instanceId +
+                   " on (" + versions.targetPlatform() + versions.sourcePlatform().map(version -> " <-- " + version).orElse("") +
+                   ", " + versions.targetApplication().id()  + versions.sourceApplication().map(version -> " <-- " + version.id()).orElse("") +
+                   "), ready since " + availableSince;
         }
 
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index bdb7b0860e5..5f0f2e4ba4e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -107,7 +107,7 @@ public class Upgrader extends ControllerMaintainer {
     /** Returns a list of all production application instances, except those which are pinned, which we should not manipulate here. */
     private InstanceList instances() {
         return InstanceList.from(controller().jobController().deploymentStatuses(ApplicationList.from(controller().applications().readable())))
-                           .withProductionDeployment()
+                           .withDeclaredJobs()
                            .unpinned();
     }
 
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 2b5e8b3e91c..6417119618b 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
@@ -1128,4 +1128,48 @@ public class DeploymentTriggerTest {
            .runJob(productionCdAwsUsEast1a);
     }
 
+    @Test
+    public void testsInSeparateInstance() {
+        String deploymentSpec =
+                "<deployment version='1.0'>\n" +
+                "    <instance id='canary'>\n" +
+                "        <upgrade policy='canary' />\n" +
+                "        <test />\n" +
+                "        <staging />\n" +
+                "    </instance>\n" +
+                "    <instance id='default'>\n" +
+                "        <prod>\n" +
+                "            <region active='true'>eu-west-1</region>\n" +
+                "            <test>eu-west-1</test>\n" +
+                "        </prod>\n" +
+                "    </instance>\n" +
+                "</deployment>\n";
+
+        ApplicationPackage applicationPackage = ApplicationPackageBuilder.fromDeploymentXml(deploymentSpec);
+        var canary = tester.newDeploymentContext("t", "a", "canary").submit(applicationPackage);
+        var conservative = tester.newDeploymentContext("t", "a", "default");
+
+        canary.runJob(systemTest)
+              .runJob(stagingTest);
+        conservative.runJob(productionEuWest1)
+                    .runJob(testEuWest1);
+
+        canary.submit(applicationPackage)
+              .runJob(systemTest)
+              .runJob(stagingTest);
+        tester.outstandingChangeDeployer().run();
+        conservative.runJob(productionEuWest1)
+                    .runJob(testEuWest1);
+
+        tester.controllerTester().upgradeSystem(new Version("7.7.7"));
+        tester.upgrader().maintain();
+
+        canary.runJob(systemTest)
+              .runJob(stagingTest);
+        tester.upgrader().maintain();
+        conservative.runJob(productionEuWest1)
+                    .runJob(testEuWest1);
+
+    }
+
 }