Support a default BCP deadline

5 files changed, 85 insertions, 55 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/Bcp.java b/config-model-api/src/main/java/com/yahoo/config/application/api/Bcp.java
index 48464904f44..7464373df9e 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/Bcp.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/Bcp.java
@@ -6,6 +6,7 @@ import java.time.Duration;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -39,23 +40,30 @@ import java.util.stream.Collectors;
  */
 public class Bcp {
 
-    private static final Bcp empty = new Bcp(List.of());
+    private static final Bcp empty = new Bcp(List.of(), Optional.empty());
 
+    private final Optional<Duration> defaultDeadline;
     private final List<Group> groups;
 
-    public Bcp(List<Group> groups) {
+    public Bcp(List<Group> groups, Optional<Duration> defaultDeadline) {
         totalMembershipSumsToOne(groups);
+        this.defaultDeadline = defaultDeadline;
         this.groups = List.copyOf(groups);
     }
 
+    public Optional<Duration> defaultDeadline() { return defaultDeadline; }
     public List<Group> groups() { return groups; }
 
+    public Bcp withGroups(List<Group> groups) {
+        return new Bcp(groups, defaultDeadline);
+    }
+
     /** Returns the set of regions declared in the groups of this. */
     public Set<RegionName> regions() {
         return groups.stream().flatMap(group -> group.members().stream()).map(member -> member.region()).collect(Collectors.toSet());
     }
 
-    public boolean isEmpty() { return groups.isEmpty(); }
+    public boolean isEmpty() { return groups.isEmpty() && defaultDeadline.isEmpty(); }
 
     /** Returns this bcp spec, or if it is empty, the given bcp spec. */
     public Bcp orElse(Bcp other) {
@@ -81,7 +89,9 @@ public class Bcp {
     @Override
     public String toString() {
         if (isEmpty()) return "empty BCP";
-        return "BCP of " + groups;
+        return "BCP of " +
+               ( groups.isEmpty() ? "no groups" : groups ) +
+               (defaultDeadline.isEmpty() ? "" : ", deadline: " + defaultDeadline.get());
     }
 
     public static class Group {
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 4b30734365d..7c0c8b27e62 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
@@ -107,6 +107,7 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
         validateZones(new HashSet<>(), new HashSet<>(), this);
         validateEndpoints(globalServiceId, this.endpoints);
         validateChangeBlockers(changeBlockers, now);
+        validateBcp(bcp);
     }
 
     public InstanceName name() { return name; }
@@ -181,7 +182,6 @@ public class DeploymentInstanceSpec extends DeploymentSpec.Steps {
                       .collect(Collectors.toSet());
      }
 
-
     private void validateChangeBlockers(List<DeploymentSpec.ChangeBlocker> changeBlockers, Instant now) {
         // Find all possible dates an upgrade block window can start
         Stream<Instant> blockingFrom = changeBlockers.stream()
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 699010417bf..1f44e599e11 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
@@ -46,7 +46,6 @@ public class DeploymentSpec {
                                                                   Optional.empty(),
                                                                   Optional.empty(),
                                                                   List.of(),
-                                                                  Bcp.empty(),
                                                                   "<deployment version='1.0'/>",
                                                                   List.of());
 
@@ -58,7 +57,6 @@ public class DeploymentSpec {
     private final Optional<AthenzService> athenzService;
     private final Optional<CloudAccount> cloudAccount;
     private final List<Endpoint> endpoints;
-    private final Bcp bcp;
     private final List<DeprecatedElement> deprecatedElements;
 
     private final String xmlForm;
@@ -69,7 +67,6 @@ public class DeploymentSpec {
                           Optional<AthenzService> athenzService,
                           Optional<CloudAccount> cloudAccount,
                           List<Endpoint> endpoints,
-                          Bcp bcp,
                           String xmlForm,
                           List<DeprecatedElement> deprecatedElements) {
         this.steps = List.copyOf(Objects.requireNonNull(steps));
@@ -79,13 +76,11 @@ public class DeploymentSpec {
         this.cloudAccount = Objects.requireNonNull(cloudAccount);
         this.xmlForm = Objects.requireNonNull(xmlForm);
         this.endpoints = List.copyOf(Objects.requireNonNull(endpoints));
-        this.bcp = Objects.requireNonNull(bcp);
         this.deprecatedElements = List.copyOf(Objects.requireNonNull(deprecatedElements));
         validateTotalDelay(steps);
         validateUpgradePoliciesOfIncreasingConservativeness(steps);
         validateAthenz();
         validateApplicationEndpoints();
-        validateBcp();
     }
 
     public boolean isEmpty() { return this == empty; }
@@ -164,11 +159,6 @@ public class DeploymentSpec {
         }
     }
 
-    private void validateBcp() {
-        for (var instance : instances())
-            instance.validateBcp(instance.bcp().orElse(bcp()));
-    }
-
     /** Returns the major version this application is pinned to, or empty (default) to allow all major versions */
     public Optional<Integer> majorVersion() { return majorVersion; }
 
@@ -212,8 +202,9 @@ public class DeploymentSpec {
                                  .orElse(ZoneEndpoint.defaultEndpoint);
     }
 
-    /** Returns the default BCP spec for instances, or Bcp.empty() if none are defined. */
-    public Bcp bcp() { return bcp; }
+    /** @deprecated returns Bcp.empty(). */
+    @Deprecated // Remove after June 2023
+    public Bcp bcp() { return Bcp.empty(); }
 
     /** Returns the XML form of this spec, or null if it was not created by fromXml, nor is empty */
     public String xmlForm() { return xmlForm; }
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 d04bb7ecfe0..40fc5086340 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
@@ -44,6 +44,7 @@ import java.io.Reader;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
+import java.time.temporal.TemporalUnit;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -59,6 +60,7 @@ import java.util.Optional;
 import java.util.OptionalDouble;
 import java.util.Set;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 import static java.util.Comparator.comparingInt;
@@ -138,10 +140,8 @@ public class DeploymentSpecXmlReader {
 
         List<Step> steps = new ArrayList<>();
         List<Endpoint> applicationEndpoints = new ArrayList<>();
-        Bcp defaultBcp;
         if ( ! containsTag(instanceTag, root)) { // deployment spec skipping explicit instance -> "default" instance
-            steps.addAll(readInstanceContent("default", root, new HashMap<>(), root));
-            defaultBcp = Bcp.empty();
+            steps.addAll(readInstanceContent("default", root, new HashMap<>(), root, Bcp.empty()));
         }
         else {
             if (XML.getChildren(root).stream().anyMatch(child -> child.getTagName().equals(prodTag)))
@@ -151,14 +151,14 @@ public class DeploymentSpecXmlReader {
 
             for (Element child : XML.getChildren(root)) {
                 String tagName = child.getTagName();
+                Bcp defaultBcp = readBcp(root, Optional.empty(), List.of(), List.of(), Map.of());
                 if (tagName.equals(instanceTag)) {
-                    steps.addAll(readInstanceContent(child.getAttribute(idAttribute), child, new HashMap<>(), root));
+                    steps.addAll(readInstanceContent(child.getAttribute(idAttribute), child, new HashMap<>(), root, defaultBcp));
                 } else {
-                    steps.addAll(readNonInstanceSteps(child, new HashMap<>(), root)); // (No global service id here)
+                    steps.addAll(readNonInstanceSteps(child, new HashMap<>(), root, defaultBcp)); // (No global service id here)
                 }
             }
             readEndpoints(root, Optional.empty(), steps, applicationEndpoints, Map.of());
-            defaultBcp = readBcp(root, Optional.empty(), steps, List.of(), Map.of());
         }
 
         return new DeploymentSpec(steps,
@@ -167,7 +167,6 @@ public class DeploymentSpecXmlReader {
                                   stringAttribute(athenzServiceAttribute, root).map(AthenzService::from),
                                   stringAttribute(cloudAccountAttribute, root).map(CloudAccount::from),
                                   applicationEndpoints,
-                                  defaultBcp,
                                   xmlForm,
                                   deprecatedElements);
     }
@@ -183,7 +182,8 @@ public class DeploymentSpecXmlReader {
     private List<DeploymentInstanceSpec> readInstanceContent(String instanceNameString,
                                                              Element instanceElement,
                                                              Map<String, String> prodAttributes,
-                                                             Element parentTag) {
+                                                             Element parentTag,
+                                                             Bcp defaultBcp) {
         if (instanceNameString.isBlank())
             illegal("<instance> attribute 'id' must be specified, and not be blank");
 
@@ -211,11 +211,11 @@ public class DeploymentSpecXmlReader {
         Tags tags = XML.attribute(tagsTag, instanceElement).map(Tags::fromString).orElse(Tags.empty());
         List<Step> steps = new ArrayList<>();
         for (Element instanceChild : XML.getChildren(instanceElement))
-            steps.addAll(readNonInstanceSteps(instanceChild, prodAttributes, instanceChild));
+            steps.addAll(readNonInstanceSteps(instanceChild, prodAttributes, instanceChild, defaultBcp));
         List<Endpoint> endpoints = new ArrayList<>();
         Map<ClusterSpec.Id, Map<ZoneId, ZoneEndpoint>> zoneEndpoints = new LinkedHashMap<>();
         readEndpoints(instanceElement, Optional.of(instanceNameString), steps, endpoints, zoneEndpoints);
-        Bcp bcp = readBcp(instanceElement, Optional.of(instanceNameString), steps, endpoints, zoneEndpoints);
+        Bcp bcp = complete(readBcp(instanceElement, Optional.of(instanceNameString), steps, endpoints, zoneEndpoints).orElse(defaultBcp), steps);
         validateEndpoints(endpoints);
 
         // Build and return instances with these values
@@ -250,16 +250,16 @@ public class DeploymentSpecXmlReader {
         }
     }
 
-    private List<Step> readSteps(Element stepTag, Map<String, String> prodAttributes, Element parentTag) {
+    private List<Step> readSteps(Element stepTag, Map<String, String> prodAttributes, Element parentTag, Bcp defaultBcp) {
         if (stepTag.getTagName().equals(instanceTag))
-            return new ArrayList<>(readInstanceContent(stepTag.getAttribute(idAttribute), stepTag, prodAttributes, parentTag));
+            return new ArrayList<>(readInstanceContent(stepTag.getAttribute(idAttribute), stepTag, prodAttributes, parentTag, defaultBcp));
         else
-            return readNonInstanceSteps(stepTag, prodAttributes, parentTag);
+            return readNonInstanceSteps(stepTag, prodAttributes, parentTag, defaultBcp);
 
     }
 
     // Consume the given tag as 0-N steps. 0 if it is not a step, >1 if it contains multiple nested steps that should be flattened
-    private List<Step> readNonInstanceSteps(Element stepTag, Map<String, String> prodAttributes, Element parentTag) {
+    private List<Step> readNonInstanceSteps(Element stepTag, Map<String, String> prodAttributes, Element parentTag, Bcp defaultBcp) {
         Optional<AthenzService> athenzService = mostSpecificAttribute(stepTag, athenzServiceAttribute).map(AthenzService::from);
         Optional<String> testerFlavor = mostSpecificAttribute(stepTag, testerFlavorAttribute);
 
@@ -281,7 +281,7 @@ public class DeploymentSpecXmlReader {
                 return List.of(new DeclaredZone(Environment.from(stepTag.getTagName()), Optional.empty(), false, athenzService, testerFlavor, readCloudAccount(stepTag)));
             case prodTag: // regions, delay and parallel may be nested within, but we can flatten them
                 return XML.getChildren(stepTag).stream()
-                                               .flatMap(child -> readNonInstanceSteps(child, prodAttributes, stepTag).stream())
+                                               .flatMap(child -> readNonInstanceSteps(child, prodAttributes, stepTag, defaultBcp).stream())
                                                .toList();
             case delayTag:
                 return List.of(new Delay(Duration.ofSeconds(longAttribute("hours", stepTag) * 60 * 60 +
@@ -289,11 +289,11 @@ public class DeploymentSpecXmlReader {
                                                             longAttribute("seconds", stepTag))));
             case parallelTag: // regions and instances may be nested within
                 return List.of(new ParallelSteps(XML.getChildren(stepTag).stream()
-                                                    .flatMap(child -> readSteps(child, prodAttributes, parentTag).stream())
+                                                    .flatMap(child -> readSteps(child, prodAttributes, parentTag, defaultBcp).stream())
                                                     .toList()));
             case stepsTag: // regions and instances may be nested within
                 return List.of(new Steps(XML.getChildren(stepTag).stream()
-                                            .flatMap(child -> readSteps(child, prodAttributes, parentTag).stream())
+                                            .flatMap(child -> readSteps(child, prodAttributes, parentTag, defaultBcp).stream())
                                             .toList()));
             case regionTag:
                 return List.of(readDeclaredZone(Environment.prod, athenzService, testerFlavor, stepTag));
@@ -496,15 +496,15 @@ public class DeploymentSpecXmlReader {
                        List<Endpoint> endpoints, Map<ClusterSpec.Id, Map<ZoneId, ZoneEndpoint>> zoneEndpoints) {
         Element bcpElement = XML.getChild(parent, "bcp");
         if (bcpElement == null) return Bcp.empty();
+        Optional<Duration> defaultDeadline = XML.attribute("deadline", bcpElement).map(value -> toDuration(value, "deadline"));
 
         List<Bcp.Group> groups = new ArrayList<>();
         Map<String, Map<RegionName, List<ZoneEndpoint>>> endpointsByZone = new LinkedHashMap<>();
         for (Element groupElement : XML.getChildren(bcpElement, "group")) {
             List<Bcp.RegionMember> regions = new ArrayList<>();
             for (Element regionElement : XML.getChildren(groupElement, "region")) {
-                RegionName region = RegionName.from(XML.getValue(regionElement));
                 double fraction = toDouble(XML.attribute("fraction", regionElement).orElse(null), "fraction").orElse(1.0);
-                regions.add(new Bcp.RegionMember(region, fraction));
+                regions.add(new Bcp.RegionMember(RegionName.from(XML.getValue(regionElement)), fraction));
             }
             for (Element endpointElement : XML.getChildren(groupElement, "endpoint")) {
                 if (instance.isEmpty()) illegal("The default <bcp> element at the root cannot define endpoints");
@@ -518,11 +518,32 @@ public class DeploymentSpecXmlReader {
                 endpoint.ifPresent(e -> endpoints.add(e));
             }
 
-            Duration deadline = XML.attribute("deadline", groupElement).map(value -> toDuration(value, "deadline")).orElse(Duration.ZERO);
+            Duration deadline = XML.attribute("deadline", groupElement).map(value -> toDuration(value, "deadline"))
+                                   .orElse(defaultDeadline.orElse(Duration.ZERO));
             groups.add(new Bcp.Group(regions, deadline));
         }
         validateAndConsolidate(endpointsByZone, zoneEndpoints);
-        return new Bcp(groups);
+        return new Bcp(groups, defaultDeadline);
+    }
+
+    /**
+     * A bcp instance as written and imported may either specify groups containing all regions,
+     * or no groups, meaning it should contain one group with all regions.
+     * This adds that missing implicit group when appropriate.
+     */
+    private Bcp complete(Bcp bcp, List<Step> steps) {
+        if ( ! bcp.groups().isEmpty()) return bcp; // has explicit groups
+        var group = new Bcp.Group(prodRegions(steps).stream().map(region -> new Bcp.RegionMember(region, 1.0)).toList(),
+                                  bcp.defaultDeadline().orElse(Duration.ZERO));
+        return bcp.withGroups(List.of(group));
+    }
+
+    private Set<RegionName> prodRegions(List<Step> steps) {
+        return steps.stream()
+                    .flatMap(s -> s.zones().stream())
+                    .filter(zone -> zone.environment().isProduction())
+                    .flatMap(z -> z.region().stream())
+                    .collect(Collectors.toSet());
     }
 
     static void validateAndConsolidate(Map<String, Map<RegionName, List<ZoneEndpoint>>> in, Map<ClusterSpec.Id, Map<ZoneId, ZoneEndpoint>> out) {
@@ -784,24 +805,32 @@ public class DeploymentSpecXmlReader {
     }
 
     /**
-     * Returns a string consisting of a number followed by "m" or "M" to a duration of that number of minutes,
+     * Returns a string consisting of a number followed by "m" or "h" to a duration given in that unit,
      * or zero duration if null of blank.
      */
-    private static Duration toDuration(String minutesSpec, String sourceDescription) {
+    private static Duration toDuration(String durationSpec, String sourceDescription) {
         try {
-            if (minutesSpec == null || minutesSpec.isBlank()) return Duration.ZERO;
-            minutesSpec = minutesSpec.trim().toLowerCase();
-            if ( ! minutesSpec.endsWith("m"))
-                throw new IllegalArgumentException("Must end by 'm'");
-            try {
-                return Duration.ofMinutes(Integer.parseInt(minutesSpec.substring(0, minutesSpec.length() - 1)));
-            }
-            catch (NumberFormatException e) {
-                throw new IllegalArgumentException("Must be an integer number of minutes followed by 'm'");
-            }
+            if (durationSpec == null || durationSpec.isBlank()) return Duration.ZERO;
+            durationSpec = durationSpec.trim().toLowerCase();
+            var magnitude = toMagnitude(durationSpec);
+            return switch (durationSpec.substring(durationSpec.length() - 1)) {
+                case "m" -> Duration.ofMinutes(magnitude);
+                case "h" -> Duration.ofHours(magnitude);
+                case "d" -> Duration.ofDays(magnitude);
+                default -> throw new IllegalArgumentException("Must end by 'm', 'h' or 'd'");
+            };
         }
         catch (IllegalArgumentException e) {
-            throw new IllegalArgumentException("Illegal " + sourceDescription + " '" + minutesSpec + "'", e);
+            throw new IllegalArgumentException("Illegal " + sourceDescription + " '" + durationSpec + "'", e);
+        }
+    }
+
+    private static int toMagnitude(String durationSpec) {
+        try {
+            return Integer.parseInt(durationSpec.substring(0, durationSpec.length() - 1));
+        }
+        catch (NumberFormatException e) {
+            throw new IllegalArgumentException("Must be an integer followed by 'm' or 'h'");
         }
     }
 
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithBcpTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithBcpTest.java
index a78f53e2084..9fe1d08b904 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithBcpTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecWithBcpTest.java
@@ -104,7 +104,7 @@ public class DeploymentSpecWithBcpTest {
 
         var spec = DeploymentSpec.fromXml(r);
 
-        var betaBcp = spec.requireInstance("beta").bcp().orElse(spec.bcp());
+        var betaBcp = spec.requireInstance("beta").bcp();
         assertEquals(1, betaBcp.groups().size());
         var betaGroup = betaBcp.groups().get(0);
         assertEquals(2, betaGroup.members().size());
@@ -112,7 +112,7 @@ public class DeploymentSpecWithBcpTest {
         assertEquals(new Bcp.RegionMember(RegionName.from("us-east1"), 1.0), betaGroup.members().get(0));
         assertEquals(new Bcp.RegionMember(RegionName.from("us-east2"), 1.0), betaGroup.members().get(1));
 
-        var mainBcp = spec.requireInstance("main").bcp().orElse(spec.bcp());
+        var mainBcp = spec.requireInstance("main").bcp();
         assertEquals(7, mainBcp.regions().size());
         assertEquals(3, mainBcp.groups().size());
 
@@ -214,7 +214,7 @@ public class DeploymentSpecWithBcpTest {
             fail();
         }
         catch (IllegalArgumentException e) {
-            assertEquals("Illegal deadline 'fast': Must end by 'm'", Exceptions.toMessageString(e));
+            assertEquals("Illegal deadline 'fast': Must be an integer followed by 'm' or 'h'", Exceptions.toMessageString(e));
         }
     }
 
@@ -403,7 +403,7 @@ public class DeploymentSpecWithBcpTest {
 
     }
     private void assertTwoRegions(DeploymentSpec spec) {
-        var bcp = spec.requireInstance("default").bcp().orElse(spec.bcp());
+        var bcp = spec.requireInstance("default").bcp();
         assertEquals(1, bcp.groups().size());
         var group = bcp.groups().get(0);
         assertEquals(2, group.members().size());