Merge pull request #20895 from vespa-engine/jonmv/deployment-orchestration-for-long-pipelines

Jonmv/deployment orchestration for long pipelines

10 files changed, 132 insertions, 15 deletions

diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
index cac9d21ee1f..d946dd972f4 100644
--- a/config-model-api/abi-spec.json
+++ b/config-model-api/abi-spec.json
@@ -194,9 +194,10 @@
       "public"
     ],
     "methods": [
-      "public void <init>(com.yahoo.config.provision.InstanceName, java.util.List, com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy, com.yahoo.config.application.api.DeploymentSpec$UpgradeRollout, java.util.List, java.util.Optional, java.util.Optional, com.yahoo.config.application.api.Notifications, java.util.List, java.time.Instant)",
+      "public void <init>(com.yahoo.config.provision.InstanceName, java.util.List, com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy, com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision, com.yahoo.config.application.api.DeploymentSpec$UpgradeRollout, java.util.List, java.util.Optional, java.util.Optional, com.yahoo.config.application.api.Notifications, java.util.List, java.time.Instant)",
       "public com.yahoo.config.provision.InstanceName name()",
       "public com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy upgradePolicy()",
+      "public com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision upgradeRevision()",
       "public com.yahoo.config.application.api.DeploymentSpec$UpgradeRollout upgradeRollout()",
       "public java.util.List changeBlocker()",
       "public java.util.Optional globalServiceId()",
@@ -365,6 +366,23 @@
       "public static final enum com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy conservative"
     ]
   },
+  "com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision": {
+    "superClass": "java.lang.Enum",
+    "interfaces": [],
+    "attributes": [
+      "public",
+      "final",
+      "enum"
+    ],
+    "methods": [
+      "public static com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision[] values()",
+      "public static com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision valueOf(java.lang.String)"
+    ],
+    "fields": [
+      "public static final enum com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision separate",
+      "public static final enum com.yahoo.config.application.api.DeploymentSpec$UpgradeRevision latest"
+    ]
+  },
   "com.yahoo.config.application.api.DeploymentSpec$UpgradeRollout": {
     "superClass": "java.lang.Enum",
     "interfaces": [],
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
index 67ddb9ef83c..ea38860c29b 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentInstanceSpec.java
@@ -31,6 +31,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
     private final InstanceName name;
 
     private final DeploymentSpec.UpgradePolicy upgradePolicy;
+    private final DeploymentSpec.UpgradeRevision upgradeRevision;
     private final DeploymentSpec.UpgradeRollout upgradeRollout;
     private final List<DeploymentSpec.ChangeBlocker> changeBlockers;
     private final Optional<String> globalServiceId;
@@ -41,6 +42,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
     public DeploymentInstanceSpec(InstanceName name,
                                   List<DeploymentSpec.Step> steps,
                                   DeploymentSpec.UpgradePolicy upgradePolicy,
+                                  DeploymentSpec.UpgradeRevision upgradeRevision,
                                   DeploymentSpec.UpgradeRollout upgradeRollout,
                                   List<DeploymentSpec.ChangeBlocker> changeBlockers,
                                   Optional<String> globalServiceId,
@@ -51,6 +53,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
         super(steps);
         this.name = name;
         this.upgradePolicy = upgradePolicy;
+        this.upgradeRevision = upgradeRevision;
         this.upgradeRollout = upgradeRollout;
         this.changeBlockers = changeBlockers;
         this.globalServiceId = globalServiceId;
@@ -150,6 +153,9 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
     /** Returns the upgrade policy of this, which is defaultPolicy if none is specified */
     public DeploymentSpec.UpgradePolicy upgradePolicy() { return upgradePolicy; }
 
+    /** Returns the upgrade revision strategy of this, which is separate if none is specified */
+    public DeploymentSpec.UpgradeRevision upgradeRevision() { return upgradeRevision; }
+
     /** Returns the upgrade rollout strategy of this, which is separate if none is specified */
     public DeploymentSpec.UpgradeRollout upgradeRollout() { return upgradeRollout; }
 
@@ -198,6 +204,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
         DeploymentInstanceSpec other = (DeploymentInstanceSpec) o;
         return globalServiceId.equals(other.globalServiceId) &&
                upgradePolicy == other.upgradePolicy &&
+               upgradeRevision == other.upgradeRevision &&
                upgradeRollout == other.upgradeRollout &&
                changeBlockers.equals(other.changeBlockers) &&
                steps().equals(other.steps()) &&
@@ -208,7 +215,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
 
     @Override
     public int hashCode() {
-        return Objects.hash(globalServiceId, upgradePolicy, upgradeRollout, changeBlockers, steps(), athenzService, notifications, endpoints);
+        return Objects.hash(globalServiceId, upgradePolicy, upgradeRevision, upgradeRollout, changeBlockers, steps(), athenzService, notifications, endpoints);
     }
 
     @Override
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
index 88363db6e49..4b019bd9f7a 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
@@ -550,6 +550,15 @@ public class DeploymentSpec {
     }
 
 
+    /** Determines when application changes deploy, when an older revision is already rolling out. */
+    public enum UpgradeRevision {
+        /** Separate: Application changes wait for previous application changes to complete, unless they fail. */
+        separate,
+        /** Latest: Application changes immediately supersede previous application changes, unless currently blocked. */
+        latest
+    }
+
+
     /** Determines when application changes deploy, when there is already an ongoing platform upgrade. */
     public enum UpgradeRollout {
         /** Separate: Application changes wait for upgrade to complete, unless upgrade fails. */
diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
index 8f866654d56..b031af9faf2 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/xml/DeploymentSpecXmlReader.java
@@ -165,6 +165,7 @@ public class DeploymentSpecXmlReader {
 
         // Values where the parent may provide a default
         DeploymentSpec.UpgradePolicy upgradePolicy = readUpgradePolicy(instanceTag, parentTag);
+        DeploymentSpec.UpgradeRevision upgradeRevision = readUpgradeRevision(instanceTag, parentTag);
         DeploymentSpec.UpgradeRollout upgradeRollout = readUpgradeRollout(instanceTag, parentTag);
         List<DeploymentSpec.ChangeBlocker> changeBlockers = readChangeBlockers(instanceTag, parentTag);
         Optional<AthenzService> athenzService = mostSpecificAttribute(instanceTag, athenzServiceAttribute).map(AthenzService::from);
@@ -183,6 +184,7 @@ public class DeploymentSpecXmlReader {
                      .map(name -> new DeploymentInstanceSpec(InstanceName.from(name),
                                                              steps,
                                                              upgradePolicy,
+                                                             upgradeRevision,
                                                              upgradeRollout,
                                                              changeBlockers,
                                                              globalServiceId.asOptional(),
@@ -472,6 +474,25 @@ public class DeploymentSpecXmlReader {
         }
     }
 
+    private DeploymentSpec.UpgradeRevision readUpgradeRevision(Element parent, Element fallbackParent) {
+        Element upgradeElement = XML.getChild(parent, upgradeTag);
+        if (upgradeElement == null)
+            upgradeElement = XML.getChild(fallbackParent, upgradeTag);
+        if (upgradeElement == null)
+            return DeploymentSpec.UpgradeRevision.separate;
+
+        String revision = upgradeElement.getAttribute("revision");
+        if (revision.isEmpty())
+            return DeploymentSpec.UpgradeRevision.separate;
+
+        switch (revision) {
+            case "separate": return DeploymentSpec.UpgradeRevision.separate;
+            case "latest": return DeploymentSpec.UpgradeRevision.latest;
+            default: throw new IllegalArgumentException("Illegal upgrade revision '" + revision + "': " +
+                                                        "Must be one of " + Arrays.toString(DeploymentSpec.UpgradeRevision.values()));
+        }
+    }
+
     private DeploymentSpec.UpgradeRollout readUpgradeRollout(Element parent, Element fallbackParent) {
         Element upgradeElement = XML.getChild(parent, upgradeTag);
         if (upgradeElement == null)
@@ -487,8 +508,8 @@ public class DeploymentSpecXmlReader {
             case "separate": return DeploymentSpec.UpgradeRollout.separate;
             case "leading": return DeploymentSpec.UpgradeRollout.leading;
             // case "simultaneous": return DeploymentSpec.UpgradePolicy.conservative;
-            default: throw new IllegalArgumentException("Illegal upgrade policy '" + rollout + "': " +
-                                                        "Must be one of " + Arrays.toString(DeploymentSpec.UpgradePolicy.values()));
+            default: throw new IllegalArgumentException("Illegal upgrade rollout '" + rollout + "': " +
+                                                        "Must be one of " + Arrays.toString(DeploymentSpec.UpgradeRollout.values()));
         }
     }
 
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index 2fa2ba83291..a97faf5995d 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -121,6 +121,7 @@ public class DeploymentSpecTest {
         assertFalse(spec.requireInstance("default").globalServiceId().isPresent());
 
         assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, spec.requireInstance("default").upgradePolicy());
+        assertEquals(DeploymentSpec.UpgradeRevision.separate, spec.requireInstance("default").upgradeRevision());
         assertEquals(DeploymentSpec.UpgradeRollout.separate, spec.requireInstance("default").upgradeRollout());
     }
 
@@ -364,6 +365,21 @@ public class DeploymentSpecTest {
     }
 
     @Test
+    public void productionSpecWithUpgradeRevision() {
+        StringReader r = new StringReader(
+                "<deployment>" +
+                "   <instance id='default'>" +
+                "      <upgrade revision='latest' />" +
+                "   </instance>" +
+                "   <instance id='custom'/>" +
+                "</deployment>"
+        );
+        DeploymentSpec spec = DeploymentSpec.fromXml(r);
+        assertEquals("latest", spec.requireInstance("default").upgradeRevision().toString());
+        assertEquals("separate", spec.requireInstance("custom").upgradeRevision().toString());
+    }
+
+    @Test
     public void productionSpecWithUpgradeRollout() {
         StringReader r = new StringReader(
                 "<deployment>" +
@@ -397,10 +413,10 @@ public class DeploymentSpecTest {
     public void upgradePolicyDefault() {
         StringReader r = new StringReader(
                 "<deployment version='1.0'>" +
-                "   <upgrade policy='canary' rollout='leading'/>" +
+                "   <upgrade policy='canary' rollout='leading' revision='latest' />" +
                 "   <instance id='instance1'/>" +
                 "   <instance id='instance2'>" +
-                "      <upgrade policy='conservative' rollout='separate'/>" +
+                "      <upgrade policy='conservative' rollout='separate' revision='separate'/>" +
                 "   </instance>" +
                 "</deployment>"
         );
@@ -408,6 +424,8 @@ public class DeploymentSpecTest {
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         assertEquals("canary", spec.requireInstance("instance1").upgradePolicy().toString());
         assertEquals("conservative", spec.requireInstance("instance2").upgradePolicy().toString());
+        assertEquals("latest", spec.requireInstance("instance1").upgradeRevision().toString());
+        assertEquals("separate", spec.requireInstance("instance2").upgradeRevision().toString());
         assertEquals("leading", spec.requireInstance("instance1").upgradeRollout().toString());
         assertEquals("separate", spec.requireInstance("instance2").upgradeRollout().toString());
     }
diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc
index 819e6b79fbb..3e751a379d4 100644
--- a/config-model/src/main/resources/schema/deployment.rnc
+++ b/config-model/src/main/resources/schema/deployment.rnc
@@ -52,6 +52,7 @@ ParallelInstances = element parallel {
 
 Upgrade = element upgrade {
     attribute policy { xsd:string }? &
+    attribute revision { xsd:string }? &
     attribute rollout { xsd:string }?
 }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
index 3fe5240ce34..cc68d47666d 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/Instance.java
@@ -188,7 +188,7 @@ public class Instance {
         return change;
     }
 
-    /** Returns the application version that last completedd roll-out to this instance. */
+    /** Returns the application version that last rolled out to this instance. */
     public Optional<ApplicationVersion> latestDeployed() {
         return latestDeployed;
     }
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 d3f2d4e4501..9790ece421e 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
@@ -364,11 +364,13 @@ public class DeploymentTrigger {
     // ---------- Change management o_O ----------
 
     private boolean acceptNewApplicationVersion(DeploymentStatus status, InstanceName instance) {
-        if (status.application().require(instance).change().application().isPresent()) return true; // Replacing a previous application change is ok.
-        if (status.hasFailures()) return true; // Allow changes to fix upgrade problems.
-        if (status.application().deploymentSpec().instance(instance) // Leading upgrade allows app change to join in.
-                  .map(spec -> spec.upgradeRollout() == DeploymentSpec.UpgradeRollout.leading).orElse(false)) return true;
-        return status.application().require(instance).change().platform().isEmpty();
+        if (status.application().deploymentSpec().instance(instance).isEmpty()) return false; // Unknown instance.
+        if (status.hasFailures()) return true; // Allow changes to fix upgrade or previous revision problems.
+        DeploymentInstanceSpec spec = status.application().deploymentSpec().requireInstance(instance);
+        Change change = status.application().require(instance).change();
+        if (change.platform().isPresent() && spec.upgradeRollout() == DeploymentSpec.UpgradeRollout.separate) return false;
+        if (change.application().isPresent() && spec.upgradeRevision() == DeploymentSpec.UpgradeRevision.separate) return false;
+        return true;
     }
 
     private Instance withRemainingChange(Instance instance, Change change, DeploymentStatus status) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
index a338efd856c..bc5a70b1fa0 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/ApplicationPackageBuilder.java
@@ -57,6 +57,7 @@ public class ApplicationPackageBuilder {
     private OptionalInt majorVersion = OptionalInt.empty();
     private String instances = "default";
     private String upgradePolicy = null;
+    private String upgradeRevision = "latest";
     private String upgradeRollout = null;
     private String globalServiceId = null;
     private String athenzIdentityAttributes = "athenz-domain='domain' athenz-service='service'";
@@ -80,6 +81,11 @@ public class ApplicationPackageBuilder {
         return this;
     }
 
+    public ApplicationPackageBuilder upgradeRevision(String upgradeRevision) {
+        this.upgradeRevision = upgradeRevision;
+        return this;
+    }
+
     public ApplicationPackageBuilder upgradeRollout(String upgradeRollout) {
         this.upgradeRollout = upgradeRollout;
         return this;
@@ -253,9 +259,10 @@ public class ApplicationPackageBuilder {
         }
         xml.append(">\n");
         xml.append("  <instance id='").append(instances).append("'>\n");
-        if (upgradePolicy != null || upgradeRollout != null) {
+        if (upgradePolicy != null || upgradeRevision != null || upgradeRollout != null) {
             xml.append("    <upgrade ");
             if (upgradePolicy != null) xml.append("policy='").append(upgradePolicy).append("' ");
+            if (upgradeRevision != null) xml.append("revision='").append(upgradeRevision).append("' ");
             if (upgradeRollout != null) xml.append("rollout='").append(upgradeRollout).append("' ");
             xml.append("/>\n");
         }
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 52521775ddd..cd1e12312a8 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
@@ -107,6 +107,40 @@ public class DeploymentTriggerTest {
     }
 
     @Test
+    public void separateRevisionMakesApplicationChangeWaitForPreviousToComplete() {
+        DeploymentContext app = tester.newDeploymentContext();
+        ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
+                .upgradeRevision(null) // separate by default, but we override this in test builder
+                .region("us-east-3")
+                .test("us-east-3")
+                .build();
+
+        app.submit(applicationPackage).runJob(systemTest).runJob(stagingTest).runJob(productionUsEast3);
+        Optional<ApplicationVersion> v0 = app.lastSubmission();
+
+        app.submit(applicationPackage);
+        Optional<ApplicationVersion> v1 = app.lastSubmission();
+        assertEquals(v0, app.instance().change().application());
+
+        // Eager tests still run before new revision rolls out.
+        app.runJob(systemTest).runJob(stagingTest);
+
+        // v0 rolls out completely.
+        app.runJob(testUsEast3);
+        assertEquals(Optional.empty(), app.instance().change().application());
+
+        // v1 starts rolling when v0 is done.
+        tester.outstandingChangeDeployer().run();
+        assertEquals(v1, app.instance().change().application());
+
+        // v1 fails, so v2 starts immediately.
+        app.runJob(productionUsEast3).failDeployment(testUsEast3);
+        app.submit(applicationPackage);
+        Optional<ApplicationVersion> v2 = app.lastSubmission();
+        assertEquals(v2, app.instance().change().application());
+    }
+
+    @Test
     public void leadingUpgradeAllowsApplicationChangeWhileUpgrading() {
         var applicationPackage = new ApplicationPackageBuilder().region("us-east-3")
                                                                 .upgradeRollout("leading")
@@ -826,6 +860,7 @@ public class DeploymentTriggerTest {
         DeploymentContext i4 = tester.newDeploymentContext("t", "a", "i4");
         ApplicationPackage applicationPackage = ApplicationPackageBuilder
                 .fromDeploymentXml("<deployment version='1'>\n" +
+                                   "  <upgrade revision='separate' />\n" +
                                    "  <parallel>\n" +
                                    "    <instance id='i1'>\n" +
                                    "      <prod>\n" +
@@ -910,8 +945,7 @@ public class DeploymentTriggerTest {
         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();
+        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.