Revert "Revert "Revert "Bratseth/deployment delay"""

2 files changed, 65 insertions, 222 deletions

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 d0e6b4a8580..3c00702bd8f 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
@@ -12,23 +12,17 @@ import java.io.BufferedReader;
 import java.io.FileReader;
 import java.io.IOException;
 import java.io.Reader;
-import java.time.Duration;
+import java.io.UncheckedIOException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.LinkedHashSet;
 import java.util.List;
-import java.util.Objects;
 import java.util.Optional;
-import java.util.stream.Collectors;
 
 /**
  * Specifies the environments and regions to which an application should be deployed.
  * This may be used both for inspection as part of an application model and to answer
  * queries about deployment from the command line. A main method is included for the latter usage.
  * 
- * A deployment consists of a number of steps executed in the order listed in deployment xml, as
- * well as some additional settings.
- * 
  * This is immutable.
  *
  * @author bratseth
@@ -43,73 +37,20 @@ public class DeploymentSpec {
     
     private final Optional<String> globalServiceId;
     private final UpgradePolicy upgradePolicy;
-    private final List<Step> steps;
+    private final List<DeclaredZone> zones;
     private final String xmlForm;
 
-    public DeploymentSpec(Optional<String> globalServiceId, UpgradePolicy upgradePolicy, List<Step> steps) {
-        this(globalServiceId, upgradePolicy, steps, null);
+    public DeploymentSpec(Optional<String> globalServiceId, UpgradePolicy upgradePolicy, List<DeclaredZone> zones) {
+        this(globalServiceId, upgradePolicy, zones, null);
     }
 
     private DeploymentSpec(Optional<String> globalServiceId, UpgradePolicy upgradePolicy, 
-                           List<Step> steps, String xmlForm) {
-        validateTotalDelay(steps);
+                           List<DeclaredZone> zones, String xmlForm) {
         this.globalServiceId = globalServiceId;
         this.upgradePolicy = upgradePolicy;
-        this.steps = ImmutableList.copyOf(completeSteps(new ArrayList<>(steps)));
+        this.zones = ImmutableList.copyOf(zones);
         this.xmlForm = xmlForm;
     }
-    
-    /** Throw an IllegalArgumentException if the total delay exceeds 24 hours */
-    private static void validateTotalDelay(List<Step> steps) {
-        long totalDelaySeconds = steps.stream().filter(step -> step instanceof Delay)
-                                               .mapToLong(delay -> ((Delay)delay).duration().getSeconds())
-                                               .sum();
-        if (totalDelaySeconds > Duration.ofHours(24).getSeconds())
-            throw new IllegalArgumentException("The total delay specified is " + Duration.ofSeconds(totalDelaySeconds) +
-                                               " but max 24 hours is allowed");
-    }
-    
-    /** Adds missing required steps and reorders steps to a permissible order */
-    private static List<Step> completeSteps(List<Step> steps) {
-        // Ensure no duplicate deployments to the same zone
-        steps = new ArrayList<>(new LinkedHashSet<>(steps));
-        
-        // Add staging if required and missing
-        if (steps.stream().anyMatch(step -> step.deploysTo(Environment.prod)) &&
-            steps.stream().noneMatch(step -> step.deploysTo(Environment.staging))) {
-            steps.add(new DeclaredZone(Environment.staging));
-        }
-        
-        // Add test if required and missing
-        if (steps.stream().anyMatch(step -> step.deploysTo(Environment.staging)) &&
-            steps.stream().noneMatch(step -> step.deploysTo(Environment.test))) {
-            steps.add(new DeclaredZone(Environment.test));
-        }
-        
-        // Enforce order test, staging, prod
-        DeclaredZone testStep = remove(Environment.test, steps);
-        if (testStep != null)
-            steps.add(0, testStep);
-        DeclaredZone stagingStep = remove(Environment.staging, steps);
-        if (stagingStep != null)
-            steps.add(1, stagingStep);
-        
-        return steps;
-    }
-
-    /** 
-     * Removes the first occurrence of a deployment step to the given environment and returns it.
-     * 
-     * @param environment
-     * @return the removed step, or null if it is not present
-     */
-    private static DeclaredZone remove(Environment environment, List<Step> steps) {
-        for (int i = 0; i < steps.size(); i++) {
-            if (steps.get(i).deploysTo(environment))
-                return (DeclaredZone)steps.remove(i);
-        }
-        return null;
-    }
 
     /** Returns the ID of the service to expose through global routing, if present */
     public Optional<String> globalServiceId() {
@@ -119,22 +60,16 @@ public class DeploymentSpec {
     /** Returns the upgrade policy of this, which is defaultPolicy if none is specified */
     public UpgradePolicy upgradePolicy() { return upgradePolicy; }
 
-    /** Returns the deployment steps of this in the order they will be performed */
-    public List<Step> steps() { return steps; }
-
-    /** Returns only the DeclaredZone deployment steps of this in the order they will be performed */
-    public List<DeclaredZone> zones() { 
-        return steps.stream().filter(step -> step instanceof DeclaredZone).map(DeclaredZone.class::cast)
-                             .collect(Collectors.toList());
-    }
-
+    /** Returns the zones this declares as a read-only list. */
+    public List<DeclaredZone> zones() { return zones; }
+    
     /** Returns the XML form of this spec, or null if it was not created by fromXml or is the empty spec */
     public String xmlForm() { return xmlForm; }
 
     /** Returns whether this deployment spec specifies the given zone, either implicitly or explicitly */
     public boolean includes(Environment environment, Optional<RegionName> region) {
-        for (Step step : steps)
-            if (step.deploysTo(environment, region)) return true;
+        for (DeclaredZone declaredZone : zones)
+            if (declaredZone.matches(environment, region)) return true;
         return false;
     }
 
@@ -158,49 +93,30 @@ public class DeploymentSpec {
      * @throws IllegalArgumentException if the XML is invalid
      */
     public static DeploymentSpec fromXml(String xmlForm) {
-        List<Step> steps = new ArrayList<>();
+        List<DeclaredZone> zones = new ArrayList<>();
         Optional<String> globalServiceId = Optional.empty();
         Element root = XML.getDocument(xmlForm).getDocumentElement();
         for (Element environmentTag : XML.getChildren(root)) {
-            if ( ! isEnvironmentName(environmentTag.getTagName())) continue;
-
+            if (!isEnvironmentName(environmentTag.getTagName())) continue;
             Environment environment = Environment.from(environmentTag.getTagName());
-
-            if (environment == Environment.prod) {
-                for (Element stepTag : XML.getChildren(environmentTag)) {
-                    if (stepTag.getTagName().equals("delay"))
-                        steps.add(new Delay(Duration.ofSeconds(longAttribute("hours",   stepTag) * 60 * 60 +
-                                                               longAttribute("minutes", stepTag) * 60 +
-                                                               longAttribute("seconds", stepTag))));
-                    else // a region: deploy step
-                        steps.add(new DeclaredZone(environment,
-                                                   Optional.of(RegionName.from(XML.getValue(stepTag).trim())),
-                                                   readActive(stepTag)));
+            List<Element> regionTags = XML.getChildren(environmentTag, "region");
+            if (regionTags.isEmpty()) {
+                zones.add(new DeclaredZone(environment, Optional.empty(), false));
+            } else {
+                for (Element regionTag : regionTags) {
+                    RegionName region = RegionName.from(XML.getValue(regionTag).trim());
+                    boolean active = environment == Environment.prod && readActive(regionTag);
+                    zones.add(new DeclaredZone(environment, Optional.of(region), active));
                 }
             }
-            else {
-                steps.add(new DeclaredZone(environment));
-            }
 
-            if (environment == Environment.prod)
+            if (Environment.prod.equals(environment)) {
                 globalServiceId = readGlobalServiceId(environmentTag);
-            else if (readGlobalServiceId(environmentTag).isPresent())
+            } else if (readGlobalServiceId(environmentTag).isPresent()) {
                 throw new IllegalArgumentException("Attribute 'global-service-id' is only valid on 'prod' tag.");
+            }
         }
-        return new DeploymentSpec(globalServiceId, readUpgradePolicy(root), steps, xmlForm);
-    }
-    
-    /** Returns the given attribute as an integer, or 0 if it is not present */
-    private static long longAttribute(String attributeName, Element tag) {
-        String value = tag.getAttribute(attributeName);
-        if (value == null || value.isEmpty()) return 0;
-        try {
-            return Long.parseLong(value);
-        }
-        catch (NumberFormatException e) {
-            throw new IllegalArgumentException("Expected an integer for attribute '" + attributeName + 
-                                               "' but got '" + value + "'");
-        }
+        return new DeploymentSpec(globalServiceId, readUpgradePolicy(root), zones, xmlForm);
     }
 
     private static boolean isEnvironmentName(String tagName) {
@@ -257,7 +173,7 @@ public class DeploymentSpec {
     }
 
     /** This may be invoked by a continuous build */
-    public static void main(String[] args) {
+    public static void main (String[] args) {
         if (args.length != 2 && args.length != 3) {
             System.err.println("Usage: DeploymentSpec [file] [environment] [region]?" +
                                "Returns 0 if the specified zone matches the deployment spec, 1 otherwise");
@@ -279,37 +195,7 @@ public class DeploymentSpec {
         }
     }
 
-    /** A delpoyment step */
-    public abstract static class Step {
-        
-        /** Returns whether this step deploys to the given region */
-        public final boolean deploysTo(Environment environment) {
-            return deploysTo(environment, Optional.empty());
-        }
-
-        /** Returns whether this step deploys to the given environment, and (if specified) region */
-        public abstract boolean deploysTo(Environment environment, Optional<RegionName> region);
-
-    }
-
-    /** A deployment step which is to wait for some time before progressing to the next step */
-    public static class Delay extends Step {
-        
-        private final Duration duration;
-        
-        public Delay(Duration duration) {
-            this.duration = duration;
-        }
-        
-        public Duration duration() { return duration; }
-
-        @Override
-        public boolean deploysTo(Environment environment, Optional<RegionName> region) { return false; }
-
-    }
-
-    /** A deployment step which is to run deployment in a particular zone */
-    public static class DeclaredZone extends Step {
+    public static class DeclaredZone {
 
         private final Environment environment;
 
@@ -317,15 +203,7 @@ public class DeploymentSpec {
 
         private final boolean active;
 
-        public DeclaredZone(Environment environment) {
-            this(environment, Optional.empty(), false);
-        }
-
         public DeclaredZone(Environment environment, Optional<RegionName> region, boolean active) {
-            if (environment != Environment.prod && region.isPresent())
-                throw new IllegalArgumentException("Non-prod environments cannot specify a region");
-            if (environment == Environment.prod && ! region.isPresent())
-                throw new IllegalArgumentException("Prod environments must be specified with a region");
             this.environment = environment;
             this.region = region;
             this.active = active;
@@ -339,26 +217,23 @@ public class DeploymentSpec {
         /** Returns whether this zone should receive production traffic */
         public boolean active() { return active; }
 
-        @Override
-        public boolean deploysTo(Environment environment, Optional<RegionName> region) {
-            if (environment != this.environment) return false;
-            if (region.isPresent() && ! region.equals(this.region)) return false;
-            return true;
-        }
-        
-        @Override
-        public int hashCode() {
-            return Objects.hash(environment, region);
-        }
-        
-        @Override
-        public boolean equals(Object o) {
-            if (o == this) return true;
-            if ( !  (o instanceof DeclaredZone)) return false;
-            DeclaredZone other = (DeclaredZone)o;
-            if (this.environment != other.environment) return false;
-            if ( ! this.region.equals(other.region())) return false;
-            return true;
+        public boolean matches(Environment environment, Optional<RegionName> region) {
+            if (environment.equals(this.environment) && region.equals(this.region)) return true;
+            if ( ! region.isPresent() && prerequisite(environment)) return true;
+            return false;
+        }
+
+        /**
+         * Returns whether deployment in the given environment is a prerequisite of deployment in this environment
+         *
+         * The required progression leading to prerequisites is test, staging, prod.
+         */
+        private boolean prerequisite(Environment environment) {
+            if (this.environment == Environment.prod)
+                return environment == Environment.staging || environment == Environment.test;
+            if (this.environment == Environment.staging)
+                return environment == Environment.test;
+            return false;
         }
 
     }
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 8de8a9c2da4..ffde5b21458 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
@@ -26,8 +26,8 @@ public class DeploymentSpecTest {
         StringReader r = new StringReader(specXml);
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         assertEquals(specXml, spec.xmlForm());
-        assertEquals(1, spec.steps().size());
-        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
+        assertEquals(1, spec.zones().size());
+        assertEquals(Environment.test, spec.zones().get(0).environment());
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertFalse(spec.includes(Environment.staging, Optional.empty()));
@@ -44,9 +44,8 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(2, spec.steps().size());
-        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
-        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
+        assertEquals(1, spec.zones().size());
+        assertEquals(Environment.staging, spec.zones().get(0).environment());
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertTrue(spec.includes(Environment.staging, Optional.empty()));
@@ -66,17 +65,15 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(4, spec.steps().size());
+        assertEquals(2, spec.zones().size());
 
-        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
+        assertEquals(Environment.prod, spec.zones().get(0).environment());
+        assertEquals("us-east1", spec.zones().get(0).region().get().value());
+        assertFalse(spec.zones().get(0).active());
 
-        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
-
-        assertTrue(spec.steps().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
-        assertFalse(((DeploymentSpec.DeclaredZone)spec.steps().get(2)).active());
-
-        assertTrue(spec.steps().get(3).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
-        assertTrue(((DeploymentSpec.DeclaredZone)spec.steps().get(3)).active());
+        assertEquals(Environment.prod, spec.zones().get(1).environment());
+        assertEquals("us-west1", spec.zones().get(1).region().get().value());
+        assertTrue(spec.zones().get(1).active());
 
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
@@ -94,33 +91,28 @@ public class DeploymentSpecTest {
         StringReader r = new StringReader(
         "<deployment version='1.0'>" +
         "   <test/>" +
-        "   <test/>" +
         "   <staging/>" +
         "   <prod>" +
         "      <region active='false'>us-east1</region>" +
-        "      <region active='false'>us-east1</region>" +
-        "      <delay hours='3' minutes='30'/>" + 
         "      <region active='true'>us-west1</region>" +
         "   </prod>" +
         "</deployment>"
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(5, spec.steps().size());
         assertEquals(4, spec.zones().size());
 
-        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
-
-        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
+        assertEquals(Environment.test, spec.zones().get(0).environment());
 
-        assertTrue(spec.steps().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
-        assertFalse(((DeploymentSpec.DeclaredZone)spec.steps().get(2)).active());
+        assertEquals(Environment.staging, spec.zones().get(1).environment());
 
-        assertTrue(spec.steps().get(3) instanceof DeploymentSpec.Delay);
-        assertEquals(3 * 60 * 60 + 30 * 60, ((DeploymentSpec.Delay)spec.steps().get(3)).duration().getSeconds());
+        assertEquals(Environment.prod, spec.zones().get(2).environment());
+        assertEquals("us-east1", spec.zones().get(2).region().get().value());
+        assertFalse(spec.zones().get(2).active());
 
-        assertTrue(spec.steps().get(4).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
-        assertTrue(((DeploymentSpec.DeclaredZone)spec.steps().get(4)).active());
+        assertEquals(Environment.prod, spec.zones().get(3).environment());
+        assertEquals("us-west1", spec.zones().get(3).region().get().value());
+        assertTrue(spec.zones().get(3).active());
 
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
@@ -200,36 +192,12 @@ public class DeploymentSpecTest {
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         assertEquals("canary", spec.upgradePolicy().toString());
     }
-
-    @Test
-    public void maxDelayExceeded() {
-        try {
-            StringReader r = new StringReader(
-                    "<deployment>" +
-                    "  <upgrade policy='canary'/>" +
-                    "  <prod>" +
-                    "    <region active='true'>us-west-1</region>" +
-                    "    <delay hours='23'/>" +
-                    "    <region active='true'>us-central-1</region>" +
-                    "    <delay minutes='59' seconds='61'/>" +
-                    "    <region active='true'>us-east-3</region>" +
-                    "  </prod>" +
-                    "</deployment>"
-            );
-            DeploymentSpec.fromXml(r);
-            fail("Expected exception due to exceeding the max total delay");
-        }
-        catch (IllegalArgumentException e) {
-            // success
-            assertEquals("The total delay specified is PT24H1S but max 24 hours is allowed", e.getMessage());
-        }
-    }
-
+    
     @Test
     public void testEmpty() {
         assertFalse(DeploymentSpec.empty.globalServiceId().isPresent());
         assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, DeploymentSpec.empty.upgradePolicy());
-        assertTrue(DeploymentSpec.empty.steps().isEmpty());
+        assertTrue(DeploymentSpec.empty.zones().isEmpty());
         assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm());
     }