Merge pull request #11100 from vespa-engine/revert-11085-mpolden/revert-brokenness

Revert "Revert "stop using deprecated deployment spec methods""

10 files changed, 102 insertions, 73 deletions

diff --git a/config-model-api/abi-spec.json b/config-model-api/abi-spec.json
index 4ddf51c2a0c..e3f9153e05d 100644
--- a/config-model-api/abi-spec.json
+++ b/config-model-api/abi-spec.json
@@ -331,8 +331,6 @@
     "methods": [
       "public void <init>(java.util.List, java.util.Optional, java.util.Optional, java.util.Optional, java.lang.String)",
       "public void <init>(java.util.Optional, com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy, java.util.Optional, java.util.List, java.util.List, java.lang.String, java.util.Optional, java.util.Optional, com.yahoo.config.application.api.Notifications, java.util.List)",
-      "public java.util.Optional globalServiceId()",
-      "public com.yahoo.config.application.api.DeploymentSpec$UpgradePolicy upgradePolicy()",
       "public java.util.Optional majorVersion()",
       "public java.util.List steps()",
       "public java.util.List zones()",
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 39453de5cdf..53f21354c3c 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
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.config.application.api;
 
+import com.yahoo.collections.Comparables;
 import com.yahoo.config.application.api.xml.DeploymentSpecXmlReader;
 import com.yahoo.config.provision.AthenzDomain;
 import com.yahoo.config.provision.AthenzService;
@@ -72,6 +73,7 @@ public class DeploymentSpec {
         this.athenzService = athenzService;
         this.xmlForm = xmlForm;
         validateTotalDelay(steps);
+        validateUpgradePoliciesOfIncreasingConservativeness(steps);
     }
 
     // TODO: Remove after October 2019
@@ -147,6 +149,23 @@ public class DeploymentSpec {
                                                " but max 24 hours is allowed");
     }
 
+    /** Throws an IllegalArgumentException if any instance has a looser upgrade policy than the previous */
+    private void validateUpgradePoliciesOfIncreasingConservativeness(List<Step> steps) {
+        UpgradePolicy previous = Collections.min(List.of(UpgradePolicy.values()));
+        for (Step step : steps) {
+            UpgradePolicy strictest = previous;
+            List<DeploymentInstanceSpec> specs = instances(List.of(step));
+            for (DeploymentInstanceSpec spec : specs) {
+                if (spec.upgradePolicy().compareTo(previous) < 0)
+                    throw new IllegalArgumentException("Instance '" + spec.name() + "' cannot have a looser upgrade " +
+                                                       "policy than the previous of '" + previous + "'");
+
+                strictest = Comparables.max(strictest, spec.upgradePolicy());
+            }
+            previous = strictest;
+        }
+    }
+
     // TODO: Remove after October 2019
     private DeploymentInstanceSpec singleInstance() {
         return singleInstance(steps);
@@ -161,12 +180,6 @@ public class DeploymentSpec {
                                            instances.stream().map(Step::toString).collect(Collectors.joining(",")));
     }
 
-    // TODO: Remove after October 2019
-    public Optional<String> globalServiceId() { return singleInstance().globalServiceId(); }
-
-    // TODO: Remove after October 2019
-    public UpgradePolicy upgradePolicy() { return singleInstance().upgradePolicy(); }
-
     /** Returns the major version this application is pinned to, or empty (default) to allow all major versions */
     public Optional<Integer> majorVersion() { return majorVersion; }
 
@@ -250,7 +263,7 @@ public class DeploymentSpec {
 
     private static List<DeploymentInstanceSpec> instances(List<DeploymentSpec.Step> steps) {
         return steps.stream()
-                    .flatMap(step -> step instanceof ParallelZones ? ((ParallelZones)step).steps.stream() : List.of(step).stream())
+                    .flatMap(step -> step instanceof ParallelZones ? ((ParallelZones) step).steps.stream() : List.of(step).stream())
                     .filter(step -> step instanceof DeploymentInstanceSpec).map(DeploymentInstanceSpec.class::cast)
                     .collect(Collectors.toList());
     }
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecDeprecatedAPITest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecDeprecatedAPITest.java
index 2724cecec6a..729fc4dc5af 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecDeprecatedAPITest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecDeprecatedAPITest.java
@@ -45,7 +45,6 @@ public class DeploymentSpecDeprecatedAPITest {
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertFalse(spec.includes(Environment.staging, Optional.empty()));
         assertFalse(spec.includes(Environment.prod, Optional.empty()));
-        assertFalse(spec.globalServiceId().isPresent());
     }
 
     @Test
@@ -78,7 +77,6 @@ public class DeploymentSpecDeprecatedAPITest {
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertTrue(spec.includes(Environment.staging, Optional.empty()));
         assertFalse(spec.includes(Environment.prod, Optional.empty()));
-        assertFalse(spec.globalServiceId().isPresent());
     }
 
     @Test
@@ -111,9 +109,6 @@ public class DeploymentSpecDeprecatedAPITest {
         assertTrue(spec.includes(Environment.prod, Optional.of(RegionName.from("us-east1"))));
         assertTrue(spec.includes(Environment.prod, Optional.of(RegionName.from("us-west1"))));
         assertFalse(spec.includes(Environment.prod, Optional.of(RegionName.from("no-such-region"))));
-        assertFalse(spec.globalServiceId().isPresent());
-        
-        assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, spec.upgradePolicy());
     }
 
     @Test
@@ -208,7 +203,6 @@ public class DeploymentSpecDeprecatedAPITest {
         assertTrue(spec.includes(Environment.prod, Optional.of(RegionName.from("us-east1"))));
         assertTrue(spec.includes(Environment.prod, Optional.of(RegionName.from("us-west1"))));
         assertFalse(spec.includes(Environment.prod, Optional.of(RegionName.from("no-such-region"))));
-        assertFalse(spec.globalServiceId().isPresent());
     }
 
     @Test
@@ -223,7 +217,6 @@ public class DeploymentSpecDeprecatedAPITest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(spec.globalServiceId(), Optional.of("query"));
     }
 
     @Test(expected=IllegalArgumentException.class)
@@ -247,41 +240,6 @@ public class DeploymentSpecDeprecatedAPITest {
     }
 
     @Test
-    public void productionSpecWithGlobalServiceIdBeforeStaging() {
-        StringReader r = new StringReader(
-            "<deployment>" +
-            "  <test/>" +
-            "  <prod global-service-id='qrs'>" +
-            "    <region active='true'>us-west-1</region>" +
-            "    <region active='true'>us-central-1</region>" +
-            "    <region active='true'>us-east-3</region>" +
-            "  </prod>" +
-            "  <staging/>" +
-            "</deployment>"
-        );
-
-        DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals("qrs", spec.globalServiceId().get());
-    }
-
-    @Test
-    public void productionSpecWithUpgradePolicy() {
-        StringReader r = new StringReader(
-                "<deployment>" +
-                "  <upgrade policy='canary'/>" +
-                "  <prod>" +
-                "    <region active='true'>us-west-1</region>" +
-                "    <region active='true'>us-central-1</region>" +
-                "    <region active='true'>us-east-3</region>" +
-                "  </prod>" +
-                "</deployment>"
-        );
-
-        DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals("canary", spec.upgradePolicy().toString());
-    }
-
-    @Test
     public void maxDelayExceeded() {
         try {
             StringReader r = new StringReader(
@@ -307,8 +265,6 @@ public class DeploymentSpecDeprecatedAPITest {
 
     @Test
     public void testEmpty() {
-        assertFalse(DeploymentSpec.empty.globalServiceId().isPresent());
-        assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, DeploymentSpec.empty.upgradePolicy());
         assertTrue(DeploymentSpec.empty.steps().isEmpty());
         assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm());
     }
@@ -366,7 +322,7 @@ public class DeploymentSpecDeprecatedAPITest {
                 "  <block-change days='mon,tue' hours='15-16'/>\n" +
                 "</deployment>"
         );
-        DeploymentSpec spec = DeploymentSpec.fromXml(r);
+        DeploymentSpec.fromXml(r);
     }
 
     @Test(expected = IllegalArgumentException.class)
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 ff51c336658..f181b02a071 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
@@ -310,16 +310,16 @@ public class DeploymentSpecTest {
                 "<deployment version='1.0'>" +
                 "   <upgrade policy='canary'/>" +
                 "   <instance id='instance1'>" +
-                "      <upgrade policy='conservative'/>" +
                 "   </instance>" +
                 "   <instance id='instance2'>" +
+                "      <upgrade policy='conservative'/>" +
                 "   </instance>" +
                 "</deployment>"
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals("conservative", spec.requireInstance("instance1").upgradePolicy().toString());
-        assertEquals("canary", spec.requireInstance("instance2").upgradePolicy().toString());
+        assertEquals("canary", spec.requireInstance("instance1").upgradePolicy().toString());
+        assertEquals("conservative", spec.requireInstance("instance2").upgradePolicy().toString());
     }
 
     @Test
@@ -351,7 +351,6 @@ public class DeploymentSpecTest {
     @Test
     public void testEmpty() {
         assertFalse(DeploymentSpec.empty.requireInstance("default").globalServiceId().isPresent());
-        assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, DeploymentSpec.empty.upgradePolicy());
         assertTrue(DeploymentSpec.empty.steps().isEmpty());
         assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm());
     }
@@ -502,6 +501,72 @@ public class DeploymentSpecTest {
     }
 
     @Test(expected = IllegalArgumentException.class)
+    public void deploymentSpecWithIncreasinglyStrictUpgradePolicies() {
+        StringReader r = new StringReader(
+                "<deployment version='1.0'>" +
+                "   <instance id='instance1'>" +
+                "      <upgrade policy='conservative'/>" +
+                "   </instance>" +
+                "   <instance id='instance2' />" +
+                "</deployment>"
+        );
+        DeploymentSpec.fromXml(r);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void deploymentSpecWithIncreasinglyStrictUpgradePoliciesInParallel() {
+        StringReader r = new StringReader(
+                "<deployment version='1.0'>" +
+                "  <instance />" +
+                "  <parallel>" +
+                "     <instance id='instance1'>" +
+                "        <upgrade policy='conservative'/>" +
+                "     </instance>" +
+                "     <instance id='instance2'>" +
+                "        <upgrade policy='canary'/>" +
+                "     </instance>" +
+                "  </parallel>" +
+                "</deployment>"
+        );
+        DeploymentSpec.fromXml(r);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void deploymentSpecWithIncreasinglyStrictUpgradePoliciesAfterParallel() {
+        StringReader r = new StringReader(
+                "<deployment version='1.0'>" +
+                "  <parallel>" +
+                "     <instance id='instance1'>" +
+                "        <upgrade policy='conservative'/>" +
+                "     </instance>" +
+                "     <instance id='instance2'>" +
+                "        <upgrade policy='canary'/>" +
+                "     </instance>" +
+                "  </parallel>" +
+                "  <instance />" +
+                "</deployment>"
+        );
+        DeploymentSpec.fromXml(r);
+    }
+
+    @Test
+    public void deploymentSpecWithDifferentUpgradePoliciesInParallel() {
+        StringReader r = new StringReader(
+                "<deployment version='1.0'>" +
+                "  <parallel>" +
+                "     <instance id='instance1'>" +
+                "        <upgrade policy='conservative'/>" +
+                "     </instance>" +
+                "     <instance id='instance2' />" +
+                "  </parallel>" +
+                "</deployment>"
+        );
+        DeploymentSpec spec = DeploymentSpec.fromXml(r);
+        assertEquals(DeploymentSpec.UpgradePolicy.conservative, spec.requireInstance("instance1").upgradePolicy());
+        assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, spec.requireInstance("instance2").upgradePolicy());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
     public void deploymentSpecWithIllegallyOrderedDeploymentSpec1() {
         StringReader r = new StringReader(
                 "<deployment>" +
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
index b5af354aead..95e13ee2748 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithoutInstanceTest.java
@@ -253,7 +253,6 @@ public class DeploymentSpecWithoutInstanceTest {
     @Test
     public void testEmpty() {
         assertFalse(DeploymentSpec.empty.requireInstance("default").globalServiceId().isPresent());
-        assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, DeploymentSpec.empty.upgradePolicy());
         assertTrue(DeploymentSpec.empty.steps().isEmpty());
         assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm());
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationList.java
index 638f406409f..46e9a08a4bd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/ApplicationList.java
@@ -154,13 +154,17 @@ public class ApplicationList {
     }
 
     /** Returns the subset of applications which has the given upgrade policy */
+    // TODO jonmv: Make this instance based when instances are orchestrated, and deployments reported per instance.
     public ApplicationList with(UpgradePolicy policy) {
-        return filteredOn(application ->  application.deploymentSpec().upgradePolicy() == policy);
+        return filteredOn(application ->  application.deploymentSpec().instances().stream()
+                                                     .anyMatch(instance -> instance.upgradePolicy() == policy));
     }
 
     /** Returns the subset of applications which does not have the given upgrade policy */
+    // TODO jonmv: Make this instance based when instances are orchestrated, and deployments reported per instance.
     public ApplicationList without(UpgradePolicy policy) {
-        return filteredOn(application ->  application.deploymentSpec().upgradePolicy() != policy);
+        return filteredOn(application ->  application.deploymentSpec().instances().stream()
+                                                     .allMatch(instance -> instance.upgradePolicy() != policy));
     }
 
     /** Returns the subset of applications which have at least one deployment on a lower version than the given one */
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 9573f5d07f5..9c6ebd22c43 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
@@ -441,7 +441,7 @@ public class DeploymentTrigger {
         if (jobStatus.get().lastCompleted().isEmpty()) return true; // Never completed
         if (jobStatus.get().firstFailing().isEmpty()) return true; // Should not happen as firstFailing should be set for an unsuccessful job
         if ( ! versions.targetsMatch(jobStatus.get().lastCompleted().get())) return true; // Always trigger as targets have changed
-        if (deploymentSpec.upgradePolicy() == DeploymentSpec.UpgradePolicy.canary) return true; // Don't throttle canaries
+        if (deploymentSpec.requireInstance(instance.name()).upgradePolicy() == DeploymentSpec.UpgradePolicy.canary) return true; // Don't throttle canaries
 
         Instant firstFailing = jobStatus.get().firstFailing().get().at();
         Instant lastCompleted = jobStatus.get().lastCompleted().get().at();
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index c20904710ea..08fa3abcd9f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -102,10 +102,9 @@ public class Upgrader extends Maintainer {
         applications = applications.notFailingOn(version); // try to upgrade only if it hasn't failed on this version
         applications = applications.canUpgradeAt(controller().clock().instant()); // wait with applications that are currently blocking upgrades
         applications = applications.byIncreasingDeployedVersion(); // start with lowest versions
-        if (!containsOnlyCanaries(applications)) { // throttle upgrades of non-canaries
-            applications = applications.first(numberOfApplicationsToUpgrade());
-        }
-        for (Application application : applications.asList())
+        for (Application application : applications.with(UpgradePolicy.canary).asList())
+            controller().applications().deploymentTrigger().triggerChange(application.id(), Change.of(version));
+        for (Application application : applications.without(UpgradePolicy.canary).first(numberOfApplicationsToUpgrade()).asList())
             controller().applications().deploymentTrigger().triggerChange(application.id(), Change.of(version));
     }
 
@@ -173,9 +172,4 @@ public class Upgrader extends Maintainer {
         controller().removeConfidenceOverride(version::equals);
     }
 
-    /** Returns whether all given applications are canaries */
-    private static boolean containsOnlyCanaries(ApplicationList applications) {
-        return applications.asList().stream().allMatch(application -> application.deploymentSpec().upgradePolicy() == UpgradePolicy.canary);
-    }
-
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java
index 08d6b5602fe..c7dec08d85a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/JobControllerApiHandlerHelper.java
@@ -163,7 +163,7 @@ class JobControllerApiHandlerHelper {
         VespaVersion lastVespa = controller.versionStatus().version(controller.systemVersion());
         VespaVersion.Confidence targetConfidence = Map.of(defaultPolicy, normal,
                                                           conservative, high)
-                                                      .getOrDefault(application.deploymentSpec().upgradePolicy(), broken);
+                                                      .getOrDefault(application.deploymentSpec().requireInstance(instance.name()).upgradePolicy(), broken);
         for (VespaVersion version : controller.versionStatus().versions())
             if (   ! version.versionNumber().isAfter(controller.systemVersion())
                 &&   version.confidence().equalOrHigherThan(targetConfidence))
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
index 2adf6ce95e1..4ac7ff4d6d4 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
@@ -142,7 +142,7 @@ public class DeploymentApiHandler extends LoggingRequestHandler {
                                                                    "/application/" +
                                                                    instance.id().application().value()).toString());
         object.setString("upgradePolicy", toString(controller.applications().requireApplication(TenantAndApplicationId.from(instance.id()))
-                                                             .deploymentSpec().upgradePolicy()));
+                                                             .deploymentSpec().requireInstance(instance.name()).upgradePolicy()));
     }
 
     private static String toString(DeploymentSpec.UpgradePolicy upgradePolicy) {