Run test jobs only in clouds for which there are dependent deployments

2 files changed, 141 insertions, 19 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 cb59cc2b663..d96384946f7 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
@@ -255,30 +255,63 @@ public class DeploymentStatus {
             if (change == null || ! change.hasTargets())
                 return;
 
-            Collection<Optional<JobId>> firstProductionJobsWithDeployment = jobSteps.keySet().stream()
-                                                                                   .filter(jobId -> jobId.type().isProduction() && jobId.type().isDeployment())
-                                                                                   .filter(jobId -> deploymentFor(jobId).isPresent())
-                                                                                   .collect(groupingBy(jobId -> findCloud(jobId.type()),
-                                                                                                       Collectors.reducing((o, n) -> o))) // Take the first.
-                                                                                   .values();
-            if (firstProductionJobsWithDeployment.isEmpty())
-                firstProductionJobsWithDeployment = List.of(Optional.empty());
-
-            for (Optional<JobId> firstProductionJobWithDeploymentInCloud : firstProductionJobsWithDeployment) {
+            Map<CloudName, Optional<JobId>> firstProductionJobsWithDeployment = firstDependentProductionJobsWithDeployment(job.application().instance());
+            firstProductionJobsWithDeployment.forEach((cloud, firstProductionJobWithDeploymentInCloud) -> {
                 Versions versions = Versions.from(change,
                                                   application,
                                                   firstProductionJobWithDeploymentInCloud.flatMap(this::deploymentFor),
                                                   fallbackPlatform(change, job));
                 if (step.completedAt(change, firstProductionJobWithDeploymentInCloud).isEmpty()) {
-                    CloudName cloud = firstProductionJobWithDeploymentInCloud.map(JobId::type).map(this::findCloud).orElse(zones.systemZone().getCloudName());
                     JobType typeWithZone = job.type().isSystemTest() ? JobType.systemTest(zones, cloud) : JobType.stagingTest(zones, cloud);
                     jobs.merge(job, List.of(new Job(typeWithZone, versions, step.readyAt(change), change)), DeploymentStatus::union);
                 }
-            }
+            });
         });
         return Collections.unmodifiableMap(jobs);
     }
 
+    /**
+     * Returns the clouds, and their first production deployments, that depend on this instance; or,
+     * if no such deployments exist, all clouds the application deploy to, and their first production deployments; or
+     * if no clouds are deployed to at all, the system default cloud.
+     */
+    Map<CloudName, Optional<JobId>> firstDependentProductionJobsWithDeployment(InstanceName testInstance) {
+        // Find instances' dependencies on each other: these are topologically ordered, so a simple traversal does it.
+        Map<InstanceName, Set<InstanceName>> dependencies = new HashMap<>();
+        instanceSteps().forEach((name, step) -> {
+            dependencies.put(name, new HashSet<>());
+            dependencies.get(name).add(name);
+            for (StepStatus dependency : step.dependencies()) {
+                dependencies.get(name).add(dependency.instance());
+                dependencies.get(name).addAll(dependencies.get(dependency.instance));
+            }
+        });
+
+        Map<CloudName, Optional<JobId>> independentJobsPerCloud = new HashMap<>();
+        Map<CloudName, Optional<JobId>> jobsPerCloud = new HashMap<>();
+        jobSteps.forEach((job, step) -> {
+            if ( ! job.type().isProduction() || ! job.type().isDeployment())
+                return;
+
+            (dependencies.get(step.instance()).contains(testInstance) ? jobsPerCloud
+                                                                      : independentJobsPerCloud)
+                    .merge(findCloud(job.type()),
+                           Optional.of(job),
+                           (o, n) -> o.filter(v -> deploymentFor(v).isPresent())             // Keep first if its deployment is present.
+                                      .or(() -> n.filter(v -> deploymentFor(v).isPresent())) // Use next if only its deployment is present.
+                                      .or(() -> o));                                         // Keep first if none have deployments.
+        });
+
+        if (jobsPerCloud.isEmpty())
+            jobsPerCloud.putAll(independentJobsPerCloud);
+
+        if (jobsPerCloud.isEmpty())
+            jobsPerCloud.put(zones.systemZone().getCloudName(), Optional.empty());
+
+        return jobsPerCloud;
+    }
+
+
     /** Fall back to the newest, deployable platform, which is compatible with what we want to deploy. */
     public Version fallbackPlatform(Change change, JobId job) {
         Optional<Version> compileVersion = change.revision().map(application.revisions()::get).flatMap(ApplicationVersion::compileVersion);
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 238489e521a..c18b333a91b 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
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.controller.deployment;
 
 import com.yahoo.component.Version;
+import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.CloudName;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.InstanceName;
@@ -41,6 +42,7 @@ import java.util.stream.Collectors;
 
 import static ai.vespa.validation.Validation.require;
 import static com.yahoo.config.provision.SystemName.cd;
+import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.applicationPackage;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.productionApNortheast1;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.productionApNortheast2;
 import static com.yahoo.vespa.hosted.controller.deployment.DeploymentContext.productionApSoutheast1;
@@ -64,8 +66,10 @@ import static java.util.Collections.emptyList;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 /**
  * Tests a wide variety of deployment scenarios and configurations
@@ -2050,16 +2054,16 @@ public class DeploymentTriggerTest {
         var conservative = tester.newDeploymentContext("t", "a", "default");
 
         canary.runJob(systemTest)
-                .runJob(stagingTest);
+              .runJob(stagingTest);
         conservative.runJob(productionEuWest1)
-                .runJob(testEuWest1);
+                    .runJob(testEuWest1);
 
         canary.submit(applicationPackage)
-                .runJob(systemTest)
-                .runJob(stagingTest);
+              .runJob(systemTest)
+              .runJob(stagingTest);
         tester.outstandingChangeDeployer().run();
         conservative.runJob(productionEuWest1)
-                .runJob(testEuWest1);
+                    .runJob(testEuWest1);
 
         tester.controllerTester().upgradeSystem(new Version("6.7.7"));
         tester.upgrader().maintain();
@@ -2068,7 +2072,7 @@ public class DeploymentTriggerTest {
               .runJob(stagingTest);
         tester.upgrader().maintain();
         conservative.runJob(productionEuWest1)
-                .runJob(testEuWest1);
+                    .runJob(testEuWest1);
 
     }
 
@@ -2349,7 +2353,7 @@ public class DeploymentTriggerTest {
         Version version3 = new Version("6.4");
         tester.controllerTester().upgradeSystem(version3);
         tests.runJob(systemTest)            // Success in default cloud.
-                .failDeployment(systemTest);   // Failure in centauri cloud.
+             .failDeployment(systemTest);   // Failure in centauri cloud.
         tester.upgrader().run();
 
         assertEquals(Change.of(version3), tests.instance().change());
@@ -2445,6 +2449,91 @@ public class DeploymentTriggerTest {
         assertEquals(Set.of(), tests.deploymentStatus().jobsToRun().keySet());
     }
 
+
+    @Test
+    void testInstancesWithMultipleClouds() {
+        String spec = """
+                      <deployment>
+                        <parallel>
+                          <instance id='independent'>
+                            <test />
+                          </instance>
+                          <steps>
+                            <parallel>
+                              <instance id='alpha'>
+                                <test />
+                                <prod>
+                                  <region>us-east-3</region>
+                                </prod>
+                              </instance>
+                              <instance id='beta'>
+                                <test />
+                                <prod>
+                                  <region>alpha-centauri</region>
+                                </prod>
+                              </instance>
+                              <instance id='gamma'>
+                                <test />
+                              </instance>
+                            </parallel>
+                            <instance id='nu'>
+                              <staging />
+                            </instance>
+                            <instance id='omega'>
+                              <prod>
+                                <region>alpha-centauri</region>
+                              </prod>
+                            </instance>
+                          </steps>
+                          <instance id='separate'>
+                            <staging />
+                            <prod>
+                              <region>alpha-centauri</region>
+                            </prod>
+                          </instance>
+                        </parallel>
+                      </deployment>
+                      """;
+
+        RegionName alphaCentauri = RegionName.from("alpha-centauri");
+        ZoneApiMock.Builder builder = ZoneApiMock.newBuilder().withCloud("centauri").withSystem(tester.controller().system());
+        ZoneApi testAlphaCentauri = builder.with(ZoneId.from(Environment.test, alphaCentauri)).build();
+        ZoneApi stagingAlphaCentauri = builder.with(ZoneId.from(Environment.staging, alphaCentauri)).build();
+        ZoneApi prodAlphaCentauri = builder.with(ZoneId.from(Environment.prod, alphaCentauri)).build();
+
+        tester.controllerTester().zoneRegistry().addZones(testAlphaCentauri, stagingAlphaCentauri, prodAlphaCentauri);
+        tester.controllerTester().setRoutingMethod(tester.controllerTester().zoneRegistry().zones().all().ids(), RoutingMethod.sharedLayer4);
+        tester.configServer().bootstrap(tester.controllerTester().zoneRegistry().zones().all().ids(), SystemApplication.notController());
+
+        ApplicationPackage appPackage = ApplicationPackageBuilder.fromDeploymentXml(spec);
+        DeploymentContext app = tester.newDeploymentContext("tenant", "application", "alpha").submit(appPackage);
+        Map<JobId, List<DeploymentStatus.Job>> jobs = app.deploymentStatus().jobsToRun();
+
+        JobType centauriTest = JobType.systemTest(tester.controller().zoneRegistry(), CloudName.from("centauri"));
+        JobType centauriStaging = JobType.stagingTest(tester.controller().zoneRegistry(), CloudName.from("centauri"));
+        assertQueued("independent", jobs, systemTest, centauriTest);
+        assertQueued("alpha", jobs, systemTest);
+        assertQueued("beta", jobs, centauriTest);
+        assertQueued("gamma", jobs, centauriTest);
+        assertQueued("nu", jobs, stagingTest);
+        assertQueued("separate", jobs, centauriStaging);
+
+        // Once alpha runs its default system test, it also runs the centauri system test, as omega depends on it.
+        app.runJob(systemTest);
+        assertQueued("alpha", app.deploymentStatus().jobsToRun(), centauriTest);
+    }
+
+    private static void assertQueued(String instance, Map<JobId, List<DeploymentStatus.Job>> jobs, JobType... expected) {
+        List<DeploymentStatus.Job> queued = jobs.get(new JobId(ApplicationId.from("tenant", "application", instance), expected[0]));
+        Set<ZoneId> remaining = new HashSet<>();
+        for (JobType ex : expected) remaining.add(ex.zone());
+        for (DeploymentStatus.Job q : queued)
+            if ( ! remaining.remove(q.type().zone()))
+                fail("unexpected queued job for " + instance + ": " + q.type());
+        if ( ! remaining.isEmpty())
+            fail("expected tests for " + instance + " were not queued in : " + remaining);
+    }
+
     @Test
     void testNoTests() {
         DeploymentContext app = tester.newDeploymentContext();