From 6505f290574579cb490536d9f42786d4cac1f157 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Mon, 5 Jun 2017 09:53:33 +0200 Subject: Revert "Revert "Revert "Revert "Bratseth/deployment delay"""" --- .../config/application/api/DeploymentSpec.java | 213 ++++++++++++++++----- .../config/application/api/DeploymentSpecTest.java | 74 +++++-- .../vespa/model/container/ContainerCluster.java | 6 +- .../src/main/resources/schema/deployment.rnc | 24 ++- .../src/test/schema-test-files/deployment.xml | 2 + vespajlib/src/main/java/com/yahoo/text/XML.java | 2 +- 6 files changed, 244 insertions(+), 77 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 3c00702bd8f..d0e6b4a8580 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,17 +12,23 @@ import java.io.BufferedReader; import java.io.FileReader; import java.io.IOException; import java.io.Reader; -import java.io.UncheckedIOException; +import java.time.Duration; 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 @@ -37,20 +43,73 @@ public class DeploymentSpec { private final Optional globalServiceId; private final UpgradePolicy upgradePolicy; - private final List zones; + private final List steps; private final String xmlForm; - public DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, List zones) { - this(globalServiceId, upgradePolicy, zones, null); + public DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, List steps) { + this(globalServiceId, upgradePolicy, steps, null); } private DeploymentSpec(Optional globalServiceId, UpgradePolicy upgradePolicy, - List zones, String xmlForm) { + List steps, String xmlForm) { + validateTotalDelay(steps); this.globalServiceId = globalServiceId; this.upgradePolicy = upgradePolicy; - this.zones = ImmutableList.copyOf(zones); + this.steps = ImmutableList.copyOf(completeSteps(new ArrayList<>(steps))); 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 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 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 globalServiceId() { @@ -60,16 +119,22 @@ public class DeploymentSpec { /** Returns the upgrade policy of this, which is defaultPolicy if none is specified */ public UpgradePolicy upgradePolicy() { return upgradePolicy; } - /** Returns the zones this declares as a read-only list. */ - public List zones() { return zones; } - + /** Returns the deployment steps of this in the order they will be performed */ + public List steps() { return steps; } + + /** Returns only the DeclaredZone deployment steps of this in the order they will be performed */ + public List zones() { + return steps.stream().filter(step -> step instanceof DeclaredZone).map(DeclaredZone.class::cast) + .collect(Collectors.toList()); + } + /** 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 (DeclaredZone declaredZone : zones) - if (declaredZone.matches(environment, region)) return true; + for (Step step : steps) + if (step.deploysTo(environment, region)) return true; return false; } @@ -93,30 +158,49 @@ public class DeploymentSpec { * @throws IllegalArgumentException if the XML is invalid */ public static DeploymentSpec fromXml(String xmlForm) { - List zones = new ArrayList<>(); + List steps = 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()); - 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)); + + 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))); } } + else { + steps.add(new DeclaredZone(environment)); + } - if (Environment.prod.equals(environment)) { + if (environment == Environment.prod) 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), zones, xmlForm); + 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 + "'"); + } } private static boolean isEnvironmentName(String tagName) { @@ -173,7 +257,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"); @@ -195,7 +279,37 @@ public class DeploymentSpec { } } - public static class DeclaredZone { + /** 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 DeclaredZone extends Step { private final Environment environment; @@ -203,7 +317,15 @@ public class DeploymentSpec { private final boolean active; + public DeclaredZone(Environment environment) { + this(environment, Optional.empty(), false); + } + public DeclaredZone(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"); this.environment = environment; this.region = region; this.active = active; @@ -217,23 +339,26 @@ public class DeploymentSpec { /** Returns whether this zone should receive production traffic */ public boolean active() { return active; } - 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; + @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 DeclaredZone)) return false; + DeclaredZone other = (DeclaredZone)o; + if (this.environment != other.environment) return false; + if ( ! this.region.equals(other.region())) return false; + return true; } } 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 ffde5b21458..8de8a9c2da4 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.zones().size()); - assertEquals(Environment.test, spec.zones().get(0).environment()); + assertEquals(1, spec.steps().size()); + assertTrue(spec.steps().get(0).deploysTo(Environment.test)); 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,8 +44,9 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(1, spec.zones().size()); - assertEquals(Environment.staging, spec.zones().get(0).environment()); + assertEquals(2, spec.steps().size()); + assertTrue(spec.steps().get(0).deploysTo(Environment.test)); + assertTrue(spec.steps().get(1).deploysTo(Environment.staging)); assertTrue(spec.includes(Environment.test, Optional.empty())); assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1")))); assertTrue(spec.includes(Environment.staging, Optional.empty())); @@ -65,15 +66,17 @@ public class DeploymentSpecTest { ); DeploymentSpec spec = DeploymentSpec.fromXml(r); - assertEquals(2, spec.zones().size()); + assertEquals(4, spec.steps().size()); - 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(0).deploysTo(Environment.test)); - 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.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()); assertTrue(spec.includes(Environment.test, Optional.empty())); assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1")))); @@ -91,28 +94,33 @@ public class DeploymentSpecTest { StringReader r = new StringReader( "" + " " + + " " + " " + " " + " us-east1" + + " us-east1" + + " " + " us-west1" + " " + "" ); DeploymentSpec spec = DeploymentSpec.fromXml(r); + assertEquals(5, spec.steps().size()); assertEquals(4, spec.zones().size()); - assertEquals(Environment.test, spec.zones().get(0).environment()); + assertTrue(spec.steps().get(0).deploysTo(Environment.test)); + + assertTrue(spec.steps().get(1).deploysTo(Environment.staging)); - assertEquals(Environment.staging, spec.zones().get(1).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.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(3) instanceof DeploymentSpec.Delay); + assertEquals(3 * 60 * 60 + 30 * 60, ((DeploymentSpec.Delay)spec.steps().get(3)).duration().getSeconds()); - 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.steps().get(4).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1")))); + assertTrue(((DeploymentSpec.DeclaredZone)spec.steps().get(4)).active()); assertTrue(spec.includes(Environment.test, Optional.empty())); assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1")))); @@ -192,12 +200,36 @@ 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.zones().isEmpty()); + assertTrue(DeploymentSpec.empty.steps().isEmpty()); assertEquals("", DeploymentSpec.empty.xmlForm()); } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java index addf14d5e6b..dbae6851977 100755 --- a/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/container/ContainerCluster.java @@ -418,10 +418,10 @@ public final class ContainerCluster private boolean zoneHasActiveRotation(Zone zone) { return getDeploymentSpec() - .flatMap(spec -> spec.zones().stream() - .filter(dz -> dz.matches(zone.environment(), Optional.of(zone.region()))) + .flatMap(spec -> spec.steps().stream() + .filter(dz -> dz.deploysTo(zone.environment(), Optional.of(zone.region()))) .findFirst()) - .map(DeploymentSpec.DeclaredZone::active) + .map(step -> ((DeploymentSpec.DeclaredZone)step).active()) .orElse(false); } diff --git a/config-model/src/main/resources/schema/deployment.rnc b/config-model/src/main/resources/schema/deployment.rnc index c8bd11d7184..d34255c7127 100644 --- a/config-model/src/main/resources/schema/deployment.rnc +++ b/config-model/src/main/resources/schema/deployment.rnc @@ -21,11 +21,19 @@ Staging = element staging { text } -Prod = - element prod { - attribute global-service-id { text }?, - element region { - attribute active { xsd:boolean }, - text - }* - } +Prod = element prod { + attribute global-service-id { text }? & + Region* & + Delay* +} + +Region = element region { + attribute active { xsd:boolean }, + text +} + +Delay = element delay { + attribute hours { xsd:long }? & + attribute minutes { xsd:long }? & + attribute seconds { xsd:long }? +} diff --git a/config-model/src/test/schema-test-files/deployment.xml b/config-model/src/test/schema-test-files/deployment.xml index 92d59abbe53..286466eff57 100644 --- a/config-model/src/test/schema-test-files/deployment.xml +++ b/config-model/src/test/schema-test-files/deployment.xml @@ -4,7 +4,9 @@ us-west-1 + us-central-1 + us-east-3 diff --git a/vespajlib/src/main/java/com/yahoo/text/XML.java b/vespajlib/src/main/java/com/yahoo/text/XML.java index c688d5f9722..1b3d690cb2c 100644 --- a/vespajlib/src/main/java/com/yahoo/text/XML.java +++ b/vespajlib/src/main/java/com/yahoo/text/XML.java @@ -25,7 +25,7 @@ import org.xml.sax.SAXParseException; * @author Bjorn Borud * @author Vegard Havdal * @author bratseth - * @author Steinar Knutsen + * @author Steinar Knutsen */ public class XML { /** -- cgit v1.2.3