Revert "Revert "Dont override test status if its needed for a production job""

This reverts commit bf75b7789716e31d658763ea40fc46350dbe0a77.

2 files changed, 17 insertions, 11 deletions

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 bb9323ddbd8..0e32b55ae9b 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
@@ -50,6 +50,7 @@ import static com.yahoo.vespa.hosted.controller.api.integration.BuildService.Job
 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.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static java.util.Comparator.comparing;
 import static java.util.Comparator.naturalOrder;
@@ -285,7 +286,7 @@ public class DeploymentTrigger {
                                                 application.deploymentJobs().statusOf(stagingTest)
                                                         .<Instant>flatMap(job -> job.lastSuccess().map(JobRun::at)));
             String reason = "New change available";
-            List<Job> testJobs = Collections.emptyList();
+            List<Job> testJobs = null; // null means "uninitialised", while empty means "don't run any jobs".
 
             if (change.isPresent())
                 for (Step step : productionSteps) {
@@ -299,8 +300,10 @@ public class DeploymentTrigger {
                                     && jobStateContains(application, job, idle)
                                     && stepJobs.containsAll(runningProductionJobs(application)))
                                     jobs.add(deploymentJob(application, versions, change, job, reason, completedAt.get()));
+                                if ( ! alreadyTriggered(application, versions))
+                                    testJobs = emptyList();
                             }
-                            else if (testJobs.isEmpty()) {
+                            else if (testJobs == null) {
                                 testJobs = testJobs(application, versions, String.format("Testing deployment for %s (%s)", job.jobName(), versions.toString()),
                                                     completedAt.orElse(clock.instant()));
                             }
@@ -319,7 +322,7 @@ public class DeploymentTrigger {
                         }
                     }
                 }
-            if (testJobs.isEmpty())
+            if (testJobs == null)
                 testJobs = testJobs(application, versions(application, application.change(), Optional
                         .empty()), "Testing last changes outside prod", clock.instant());
             jobs.addAll(testJobs);
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 31fa78d4b5f..1c5e686fab1 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
@@ -528,7 +528,6 @@ public class DeploymentTriggerTest {
         Supplier<Application> app = () -> tester.application(application.id());
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .environment(Environment.prod)
-                .region("us-central-1")
                 .parallel("eu-west-1", "us-east-3")
                 .build();
         // Application version 42 and platform version 6.1.
@@ -541,12 +540,8 @@ public class DeploymentTriggerTest {
         tester.upgradeSystem(v2);
         tester.deployAndNotify(application, empty(), true, systemTest);
         tester.deployAndNotify(application, empty(), true, stagingTest);
-        tester.deployAndNotify(application, empty(), true, productionUsCentral1);
         tester.deploymentTrigger().cancelChange(application.id(), true);
         tester.deploy(productionEuWest1, application, applicationPackage);
-        tester.deployAndNotify(application, applicationPackage, false, productionEuWest1);
-        tester.deployAndNotify(application, applicationPackage, false, productionUsEast3);
-        assertEquals(v2, app.get().deployments().get(productionUsCentral1.zone(main).get()).version());
         assertEquals(v2, app.get().deployments().get(productionEuWest1.zone(main).get()).version());
         assertEquals(v1, app.get().deployments().get(productionUsEast3.zone(main).get()).version());
 
@@ -556,18 +551,26 @@ public class DeploymentTriggerTest {
         tester.deployAndNotify(application, empty(), true, systemTest);
         assertEquals(v2, app.get().deploymentJobs().jobStatus().get(stagingTest).lastTriggered().get().platform());
         tester.deployAndNotify(application, empty(), true, stagingTest);
-        tester.deployAndNotify(application, empty(), true, productionUsCentral1);
+
+        // Tests are not re-triggered, because eu-west-1 needs their success to trigger
+        assertEquals(v2, app.get().deploymentJobs().jobStatus().get(systemTest).lastTriggered().get().platform());
+        assertEquals(v2, app.get().deploymentJobs().jobStatus().get(stagingTest).lastTriggered().get().platform());
+
+         // Finish old runs of the production jobs, which fail.
+        tester.deployAndNotify(application, applicationPackage, false, productionEuWest1);
+        tester.deployAndNotify(application, applicationPackage, false, productionUsEast3);
+
+        // New upgrade is already tested for eu-west-1, and tests may now test the upgrade for us-east-3
         assertEquals(v1, app.get().deploymentJobs().jobStatus().get(systemTest).lastTriggered().get().platform());
         tester.deployAndNotify(application, empty(), true, systemTest);
         assertEquals(v1, app.get().deploymentJobs().jobStatus().get(stagingTest).lastTriggered().get().platform());
         tester.deployAndNotify(application, empty(), true, stagingTest);
+        tester.deployAndNotify(application, empty(), false, productionEuWest1);
 
         // The production job on version 6.2 fails and must retry -- this is OK, even though staging now has a different version.
-        tester.deployAndNotify(application, empty(), false, productionEuWest1);
         tester.deployAndNotify(application, empty(), true, productionUsEast3);
         tester.deployAndNotify(application, empty(), true, productionEuWest1);
         assertFalse(app.get().change().isPresent());
-        assertEquals(43, app.get().deploymentJobs().jobStatus().get(productionUsCentral1).lastSuccess().get().application().buildNumber().get().longValue());
         assertEquals(43, app.get().deploymentJobs().jobStatus().get(productionEuWest1).lastSuccess().get().application().buildNumber().get().longValue());
         assertEquals(43, app.get().deploymentJobs().jobStatus().get(productionUsEast3).lastSuccess().get().application().buildNumber().get().longValue());
     }