From 13561abb96f65f3ee52fa89d9c16881c3f18dabd Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Fri, 2 Jun 2017 13:25:22 +0200 Subject: Revert "Bratseth/deployment delay" --- .../config/application/api/DeploymentSpec.java | 206 +++++---------------- .../config/application/api/DeploymentSpecTest.java | 75 +++----- 2 files changed, 66 insertions(+), 215 deletions(-) (limited to 'config-model-api') 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 556bcb78d31..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,12 +12,10 @@ 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; /** @@ -25,9 +23,6 @@ import java.util.Optional; * 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 @@ -42,73 +37,20 @@ public class DeploymentSpec { private final Optional globalServiceId; private final UpgradePolicy upgradePolicy; - private final List steps; + private final List zones; private final String xmlForm; - public DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, List steps) { - this(globalServiceId, upgradePolicy, steps, null); + public DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, List zones) { + this(globalServiceId, upgradePolicy, zones, null); } private DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, - List steps, String xmlForm) { - validateTotalDelay(steps); + List 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 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 completeSteps(List 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 ZoneDeployment(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 ZoneDeployment(Environment.test)); - } - - // Enforce order test, staging, prod - ZoneDeployment testStep = remove(Environment.test, steps); - if (testStep != null) - steps.add(0, testStep); - ZoneDeployment 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 ZoneDeployment remove(Environment environment, List steps) { - for (int i = 0; i < steps.size(); i++) { - if (steps.get(i).deploysTo(environment)) - return (ZoneDeployment)steps.remove(i); - } - return null; - } /** Returns the ID of the service to expose through global routing, if present */ public Optional globalServiceId() { @@ -118,16 +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 steps() { return steps; } + /** Returns the zones this declares as a read-only list. */ + public List 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 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; } @@ -151,49 +93,30 @@ public class DeploymentSpec { * @throws IllegalArgumentException if the XML is invalid */ public static DeploymentSpec fromXml(String xmlForm) { - List steps = new ArrayList<>(); + List zones = new ArrayList<>(); Optional 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 ZoneDeployment(environment, - Optional.of(RegionName.from(XML.getValue(stepTag).trim())), - readActive(stepTag))); + List 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 ZoneDeployment(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) { @@ -250,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"); @@ -272,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 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 region) { return false; } - - } - - /** A deployment step which is to run deployment in a particular zone */ - public static class ZoneDeployment extends Step { + public static class DeclaredZone { private final Environment environment; @@ -310,15 +203,7 @@ public class DeploymentSpec { private final boolean active; - public ZoneDeployment(Environment environment) { - this(environment, Optional.empty(), false); - } - - public ZoneDeployment(Environment environment, Optional 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"); + public DeclaredZone(Environment environment, Optional region, boolean active) { this.environment = environment; this.region = region; this.active = active; @@ -332,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 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 ZoneDeployment)) return false; - ZoneDeployment other = (ZoneDeployment)o; - if (this.environment != other.environment) return false; - if ( ! this.region.equals(other.region())) return false; - return true; + public boolean matches(Environment environment, Optional 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 bc1d0c55654..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.ZoneDeployment)spec.steps().get(2)).active()); - - assertTrue(spec.steps().get(3).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1")))); - assertTrue(((DeploymentSpec.ZoneDeployment)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,32 +91,28 @@ public class DeploymentSpecTest { StringReader r = new StringReader( "" + " " + - " " + " " + " " + " us-east1" + - " us-east1" + - " " + " us-west1" + " " + "" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(5, spec.steps().size()); - - assertTrue(spec.steps().get(0).deploysTo(Environment.test)); + assertEquals(4, spec.zones().size()); - 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.ZoneDeployment)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.ZoneDeployment)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")))); @@ -199,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( - "" + - " " + - " " + - " us-west-1" + - " " + - " us-central-1" + - " " + - " us-east-3" + - " " + - "" - ); - 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("", DeploymentSpec.empty.xmlForm()); } -- cgit v1.2.3