Fix corner case for system/staging test jobs

This fixes a corner case where we compute versions to run in system and staging tests, where the first production deployment used to compute the versions has a newer platform that what we are deploying — this confused the algorithm into believing it needed both a run which was a success both on the deploying change, AND on the newer version, which is obviously impossible. When a newer version exists in the production deployment, this is now used instead of, not in addition to, the deploying change.

2 files changed, 43 insertions, 6 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 124b913eb01..b4904ca3cf8 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
@@ -646,12 +646,10 @@ public class DeploymentStatus {
                 @Override
                 public Optional<Instant> completedAt(Change change, Optional<JobId> dependent) {
                     return RunList.from(job)
-                                  .matching(run -> change.platform().map(run.versions().targetPlatform()::equals).orElse(true))
-                                  .matching(run -> change.application().map(run.versions().targetApplication()::equals).orElse(true))
-                                  .matching(run -> dependent.flatMap(status::deploymentFor)
-                                                            .map(deployment -> Versions.from(change, deployment))
-                                                            .map(run.versions()::targetsMatch)
-                                                            .orElse(true))
+                                  .matching(run -> run.versions().targetsMatch(Versions.from(change,
+                                                                                             status.application,
+                                                                                             dependent.flatMap(status::deploymentFor),
+                                                                                             status.systemVersion)))
                                   .status(RunStatus.success)
                                   .asList().stream()
                                   .map(run -> run.end().get())
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 f1306b51b39..e0241e90244 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
@@ -24,6 +24,7 @@ import java.time.Instant;
 import java.util.Collection;
 import java.util.EnumSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalLong;
 import java.util.stream.Collectors;
@@ -1216,4 +1217,42 @@ public class DeploymentTriggerTest {
         app.assertNotRunning(stagingTest);
     }
 
+    @Test
+    public void test() {
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder().systemTest()
+                                                                               .stagingTest()
+                                                                               .region("us-east-3")
+                                                                               .region("us-west-1")
+                                                                               .build();
+        var app = tester.newDeploymentContext().submit(applicationPackage).deploy();
+        var appToAvoidVersionGC = tester.newDeploymentContext("g", "c", "default").submit().deploy();
+
+        Version version2 = new Version("7.8.9");
+        Version version3 = new Version("8.9.10");
+        tester.controllerTester().upgradeSystem(version2);
+        tester.deploymentTrigger().triggerChange(appToAvoidVersionGC.instanceId(), Change.of(version2));
+        appToAvoidVersionGC.deployPlatform(version2);
+
+        // app upgrades first zone to version3, and then the other two to version2.
+        tester.controllerTester().upgradeSystem(version3);
+        tester.deploymentTrigger().triggerChange(app.instanceId(), Change.of(version3));
+        app.runJob(systemTest).runJob(stagingTest);
+        tester.triggerJobs();
+        tester.upgrader().overrideConfidence(version3, VespaVersion.Confidence.broken);
+        tester.controllerTester().computeVersionStatus();
+        tester.upgrader().run();
+        assertEquals(Optional.of(version2), app.instance().change().platform());
+
+        app.runJob(systemTest)
+           .runJob(productionUsEast3)
+           .runJob(stagingTest)
+           .runJob(productionUsWest1);
+
+        assertEquals(version3, app.instanceJobs().get(productionUsEast3).lastSuccess().get().versions().targetPlatform());
+        assertEquals(version2, app.instanceJobs().get(productionUsWest1).lastSuccess().get().versions().targetPlatform());
+        assertEquals(Map.of(), app.deploymentStatus().jobsToRun());
+        assertEquals(Change.empty(), app.instance().change());
+        assertEquals(List.of(), tester.jobs().active());
+    }
+
 }