Compute outstanding change based on previous instances, when any

3 files changed, 130 insertions, 4 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 1cef9d7d855..ad237a6aa6e 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
@@ -13,6 +13,7 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.Instance;
+import com.yahoo.vespa.hosted.controller.api.integration.deployment.ApplicationVersion;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.application.Change;
@@ -26,7 +27,6 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -216,12 +216,29 @@ public class DeploymentStatus {
      * and has not yet started rolling out, due to some other change or a block window being present at the time of submission.
      */
     public Change outstandingChange(InstanceName instance) {
-        return application.latestVersion().map(Change::of)
+        return nextVersion(instance).map(Change::of)
                           .filter(change -> application.require(instance).change().application().map(change::upgrades).orElse(true))
                           .filter(change -> ! jobsToRun(Map.of(instance, change)).isEmpty())
                           .orElse(Change.empty());
     }
 
+    /** The next application version to roll out to instance. */
+    private Optional<ApplicationVersion> nextVersion(InstanceName instance) {
+        return Optional.ofNullable(instanceSteps().get(instance)).stream()
+                       .flatMap(this::allDependencies)
+                       .flatMap(step -> step.instance.latestDeployed().stream())
+                       .min(naturalOrder())
+                       .or(application::latestVersion);
+    }
+
+    private Stream<InstanceStatus> allDependencies(StepStatus step) {
+        return step.dependencies.stream()
+                                .flatMap(dep -> Stream.concat(Stream.of(dep), allDependencies(dep)))
+                                .filter(InstanceStatus.class::isInstance)
+                                .map(InstanceStatus.class::cast)
+                                .distinct();
+    }
+
     /**
      * True if the job has already been triggered on the given versions, or if all test types (systemTest, stagingTest),
      * restricted to the job's instance if declared in that instance, have successful runs on the given versions.
@@ -310,7 +327,7 @@ public class DeploymentStatus {
     /** Adds the primitive steps contained in the given step, which depend on the given previous primitives, to the dependency graph. */
     private List<StepStatus> fillStep(Map<JobId, StepStatus> dependencies, List<StepStatus> allSteps,
                                       DeploymentSpec.Step step, List<StepStatus> previous, InstanceName instance) {
-        if (step.steps().isEmpty()) {
+        if (step.steps().isEmpty() && ! (step instanceof DeploymentInstanceSpec)) {
             if (instance == null)
                 return previous; // Ignore test and staging outside all instances.
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatusList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatusList.java
index c6e4d02acfa..22df5ca559e 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatusList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentStatusList.java
@@ -24,7 +24,7 @@ public class DeploymentStatusList extends AbstractFilteringList<DeploymentStatus
 
     /** Returns the subset of applications which have changes left to deploy; blocked, or deploying */
     public DeploymentStatusList withChanges() {
-        return matching(status -> status.application().instances().values().stream()
+        return matching(status -> status.application().productionInstances().values().stream()
                                         .anyMatch(instance -> instance.change().hasTargets() || status.outstandingChange(instance.name()).hasTargets()));
     }
 
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 a1b427fbeaa..52521775ddd 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
@@ -819,6 +819,115 @@ public class DeploymentTriggerTest {
     }
 
     @Test
+    public void testMultipleInstancesWithDifferentChanges() {
+        DeploymentContext i1 = tester.newDeploymentContext("t", "a", "i1");
+        DeploymentContext i2 = tester.newDeploymentContext("t", "a", "i2");
+        DeploymentContext i3 = tester.newDeploymentContext("t", "a", "i3");
+        DeploymentContext i4 = tester.newDeploymentContext("t", "a", "i4");
+        ApplicationPackage applicationPackage = ApplicationPackageBuilder
+                .fromDeploymentXml("<deployment version='1'>\n" +
+                                   "  <parallel>\n" +
+                                   "    <instance id='i1'>\n" +
+                                   "      <prod>\n" +
+                                   "        <region>us-east-3</region>\n" +
+                                   "        <delay hours='6' />\n" +
+                                   "      </prod>\n" +
+                                   "    </instance>\n" +
+                                   "    <instance id='i2'>\n" +
+                                   "      <prod>\n" +
+                                   "        <region>us-east-3</region>\n" +
+                                   "      </prod>\n" +
+                                   "    </instance>\n" +
+                                   "  </parallel>\n" +
+                                   "  <instance id='i3'>\n" +
+                                   "    <prod>\n" +
+                                   "      <region>us-east-3</region>\n" +
+                                   "        <delay hours='18' />\n" +
+                                   "      <test>us-east-3</test>\n" +
+                                   "    </prod>\n" +
+                                   "  </instance>\n" +
+                                   "  <instance id='i4'>\n" +
+                                   "    <test />\n" +
+                                   "    <staging />\n" +
+                                   "    <prod>\n" +
+                                   "      <region>us-east-3</region>\n" +
+                                   "    </prod>\n" +
+                                   "  </instance>\n" +
+                                   "</deployment>\n");
+
+        // Package is submitted, and change propagated to the two first instances.
+        i1.submit(applicationPackage);
+        Optional<ApplicationVersion> v0 = i1.lastSubmission();
+        tester.outstandingChangeDeployer().run();
+        assertEquals(v0, i1.instance().change().application());
+        assertEquals(v0, i2.instance().change().application());
+        assertEquals(Optional.empty(), i3.instance().change().application());
+        assertEquals(Optional.empty(), i4.instance().change().application());
+
+        // Tests run in i4, as they're declared there, and i1 and i2 get to work
+        i4.runJob(systemTest).runJob(stagingTest);
+        i1.runJob(productionUsEast3);
+        i2.runJob(productionUsEast3);
+
+        // Since the post-deployment delay of i1 is incomplete, i3 doesn't yet get the change.
+        tester.outstandingChangeDeployer().run();
+        assertEquals(v0, i1.instance().latestDeployed());
+        assertEquals(v0, i2.instance().latestDeployed());
+        assertEquals(Optional.empty(), i1.instance().change().application());
+        assertEquals(Optional.empty(), i2.instance().change().application());
+        assertEquals(Optional.empty(), i3.instance().change().application());
+        assertEquals(Optional.empty(), i4.instance().change().application());
+
+        // When the delay is done, i3 gets the change.
+        tester.clock().advance(Duration.ofHours(6));
+        tester.outstandingChangeDeployer().run();
+        assertEquals(Optional.empty(), i1.instance().change().application());
+        assertEquals(Optional.empty(), i2.instance().change().application());
+        assertEquals(v0, i3.instance().change().application());
+        assertEquals(Optional.empty(), i4.instance().change().application());
+
+        // v0 begins roll-out in i3, and v1 is submitted and rolls out in i1 and i2 some time later
+        i3.runJob(productionUsEast3); // v0
+        tester.clock().advance(Duration.ofHours(12));
+        i1.submit(applicationPackage);
+        Optional<ApplicationVersion> v1 = i1.lastSubmission();
+        i4.runJob(systemTest).runJob(stagingTest);
+        i1.runJob(productionUsEast3); // v1
+        i2.runJob(productionUsEast3); // v1
+        assertEquals(v1, i1.instance().latestDeployed());
+        assertEquals(v1, i2.instance().latestDeployed());
+        assertEquals(Optional.empty(), i1.instance().change().application());
+        assertEquals(Optional.empty(), i2.instance().change().application());
+        assertEquals(v0, i3.instance().change().application());
+        assertEquals(Optional.empty(), i4.instance().change().application());
+
+        // After some time, v2 also starts rolling out to i1 and i2, but does not complete in i2
+        tester.clock().advance(Duration.ofHours(3));
+        i1.submit(applicationPackage);
+        Optional<ApplicationVersion> v2 = i1.lastSubmission();
+        i4.runJob(systemTest).runJob(stagingTest);
+        i1.runJob(productionUsEast3); // v2
+        tester.clock().advance(Duration.ofHours(3));
+
+        // v1 is all done in i1 and i2, but does not yet roll out in i3; v2 is not completely rolled out there yet.
+        // TODO jonmv: thie belowh new revision policy, but must be faked for now, as v1 would not wait for v0 to complete.
+        //tester.outstandingChangeDeployer().run();
+        assertEquals(v0, i3.instance().change().application());
+
+        // i3 completes v0, which rolls out to i4; v1 is ready for i3, but v2 is not.
+        i3.runJob(testUsEast3);
+        assertEquals(Optional.empty(), i3.instance().change().application());
+        tester.outstandingChangeDeployer().run();
+        assertEquals(v2, i1.instance().latestDeployed());
+        assertEquals(v1, i2.instance().latestDeployed());
+        assertEquals(v0, i3.instance().latestDeployed());
+        assertEquals(Optional.empty(), i1.instance().change().application());
+        assertEquals(v2, i2.instance().change().application());
+        assertEquals(v1, i3.instance().change().application());
+        assertEquals(v0, i4.instance().change().application());
+    }
+
+    @Test
     public void testMultipleInstances() {
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .instances("instance1,instance2")