Disallow impossible redundancy

6 files changed, 67 insertions, 50 deletions

diff --git a/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java b/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java
index a6bcf6b0fd2..102023ba1fd 100644
--- a/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java
+++ b/config-model/src/main/java/com/yahoo/config/model/provision/InMemoryProvisioner.java
@@ -9,6 +9,7 @@ import com.yahoo.config.provision.Capacity;
 import com.yahoo.config.provision.ClusterMembership;
 import com.yahoo.config.provision.ClusterResources;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.ProvisionLogger;
@@ -67,6 +68,8 @@ public class InMemoryProvisioner implements HostProvisioner {
 
     private Provisioned provisioned = new Provisioned();
 
+    private Environment environment = Environment.prod;
+
     /** Creates this with a number of nodes with resources 1, 3, 9, 1 */
     public InMemoryProvisioner(int nodeCount, boolean sharedHosts) {
         this(nodeCount, defaultResources, sharedHosts);
@@ -115,6 +118,12 @@ public class InMemoryProvisioner implements HostProvisioner {
         this.retiredHostNames = Set.of(retiredHostNames);
     }
 
+    /** May affect e.g. the number of nodes/cluster. */
+    public InMemoryProvisioner setEnvironment(Environment environment) {
+        this.environment = environment;
+        return this;
+    }
+
     private static Collection<Host> toHostInstances(String[] hostnames) {
         return Arrays.stream(hostnames).map(Host::new).collect(Collectors.toList());
     }
@@ -137,6 +146,10 @@ public class InMemoryProvisioner implements HostProvisioner {
     @Override
     public List<HostSpec> prepare(ClusterSpec cluster, Capacity requested, ProvisionLogger logger) {
         provisioned.add(cluster.id(), requested);
+        if (environment == Environment.dev) {
+            requested = requested.withLimits(requested.minResources().withNodes(1),
+                                             requested.maxResources().withNodes(1));
+        }
         if (useMaxResources)
             return prepare(cluster, requested.maxResources(), requested.isRequired(), requested.canFail());
         else
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
index 640bede6b62..67b2e4b81b5 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/builder/xml/dom/NodesSpecification.java
@@ -60,13 +60,17 @@ public class NodesSpecification {
     /** The cloud account to use for nodes in this spec, if any */
     private final Optional<CloudAccount> cloudAccount;
 
+    /* Whether the count attribute was present on the nodes element. */
+    private final boolean hasCountAttribute;
+
     private NodesSpecification(ClusterResources min,
                                ClusterResources max,
                                boolean dedicated, Version version,
                                boolean required, boolean canFail, boolean exclusive,
                                Optional<DockerImage> dockerImageRepo,
                                Optional<String> combinedId,
-                               Optional<CloudAccount> cloudAccount) {
+                               Optional<CloudAccount> cloudAccount,
+                               boolean hasCountAttribute) {
         if (max.smallerThan(min))
             throw new IllegalArgumentException("Min resources must be larger or equal to max resources, but " +
                                                max + " is smaller than " + min);
@@ -89,6 +93,7 @@ public class NodesSpecification {
         this.dockerImageRepo = dockerImageRepo;
         this.combinedId = combinedId;
         this.cloudAccount = cloudAccount;
+        this.hasCountAttribute = hasCountAttribute;
     }
 
     private static NodesSpecification create(boolean dedicated, boolean canFail, Version version,
@@ -97,6 +102,7 @@ public class NodesSpecification {
         var resolvedElement = resolveElement(nodesElement);
         var combinedId = findCombinedId(nodesElement, resolvedElement);
         var resources = toResources(resolvedElement);
+        boolean hasCountAttribute = resolvedElement.stringAttribute("count") != null;
         return new NodesSpecification(resources.getFirst(),
                                       resources.getSecond(),
                                       dedicated,
@@ -106,7 +112,8 @@ public class NodesSpecification {
                                       resolvedElement.booleanAttribute("exclusive", false),
                                       dockerImageToUse(resolvedElement, dockerImageRepo),
                                       combinedId,
-                                      cloudAccount);
+                                      cloudAccount,
+                                      hasCountAttribute);
     }
 
     private static Pair<ClusterResources, ClusterResources> toResources(ModelElement nodesElement) {
@@ -180,7 +187,8 @@ public class NodesSpecification {
                                       false,
                                       context.getDeployState().getWantedDockerImageRepo(),
                                       Optional.empty(),
-                                      context.getDeployState().getProperties().cloudAccount());
+                                      context.getDeployState().getProperties().cloudAccount(),
+                                      false);
     }
 
     /** Returns a requirement from <code>count</code> dedicated nodes in one group */
@@ -194,7 +202,8 @@ public class NodesSpecification {
                                       false,
                                       context.getDeployState().getWantedDockerImageRepo(),
                                       Optional.empty(),
-                                      context.getDeployState().getProperties().cloudAccount());
+                                      context.getDeployState().getProperties().cloudAccount(),
+                                      false);
     }
 
     /**
@@ -219,7 +228,8 @@ public class NodesSpecification {
                                       false,
                                       context.getDeployState().getWantedDockerImageRepo(),
                                       Optional.empty(),
-                                      context.getDeployState().getProperties().cloudAccount());
+                                      context.getDeployState().getProperties().cloudAccount(),
+                                      false);
     }
 
     public ClusterResources minResources() { return min; }
@@ -238,6 +248,11 @@ public class NodesSpecification {
      */
     public boolean isExclusive() { return exclusive; }
 
+    /** Returns whether the count attribute was present on the {@code <nodes>} element. */
+    public boolean hasCountAttribute() {
+        return hasCountAttribute;
+    }
+
     public Map<HostResource, ClusterMembership> provision(HostSystem hostSystem,
                                                           ClusterSpec.Type clusterType,
                                                           ClusterSpec.Id clusterId,
@@ -451,5 +466,4 @@ public class NodesSpecification {
         return "specification of " + (dedicated ? "dedicated " : "") +
                (min.equals(max) ? min : "min " + min + " max " + max);
     }
-
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/StorageGroup.java b/config-model/src/main/java/com/yahoo/vespa/model/content/StorageGroup.java
index 975abad9960..8a8d2742df1 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/StorageGroup.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/StorageGroup.java
@@ -6,6 +6,7 @@ import com.yahoo.config.model.ConfigModelContext;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.config.provision.ClusterMembership;
 import com.yahoo.config.provision.ClusterSpec;
+import com.yahoo.config.provision.Environment;
 import com.yahoo.vespa.config.content.StorDistributionConfig;
 import com.yahoo.vespa.model.HostResource;
 import com.yahoo.vespa.model.HostSystem;
@@ -203,9 +204,8 @@ public class StorageGroup {
         }
 
         public StorageGroup buildRootGroup(DeployState deployState, RedundancyBuilder redundancyBuilder, ContentCluster owner) {
-            Optional<Integer> maxRedundancy = Optional.empty();
             if (owner.isHosted())
-                maxRedundancy = validateRedundancyAndGroups();
+                validateRedundancyAndGroups(deployState.zone().environment());
 
             Optional<ModelElement> group = Optional.ofNullable(clusterElement.child("group"));
             Optional<ModelElement> nodes = getNodes(clusterElement);
@@ -222,8 +222,7 @@ public class StorageGroup {
                                         : groupBuilder.buildNonHosted(deployState, owner, Optional.empty());
 
             Redundancy redundancy = redundancyBuilder.build(owner.getName(), owner.isHosted(), storageGroup.subgroups.size(),
-                                                            storageGroup.getNumberOfLeafGroups(), storageGroup.countNodes(false),
-                                                            maxRedundancy);
+                                                            storageGroup.getNumberOfLeafGroups(), storageGroup.countNodes(false));
             owner.setRedundancy(redundancy);
             if (storageGroup.partitions.isEmpty() && (redundancy.groups() > 1)) {
                 storageGroup.partitions = Optional.of(computePartitions(redundancy.finalRedundancy(), redundancy.groups()));
@@ -231,27 +230,24 @@ public class StorageGroup {
             return storageGroup;
         }
 
-        private Optional<Integer> validateRedundancyAndGroups() {
+        private void validateRedundancyAndGroups(Environment environment) {
             var redundancyElement = clusterElement.child("redundancy");
-            if (redundancyElement == null) return Optional.empty();
+            if (redundancyElement == null) return;
             long redundancy = redundancyElement.asLong();
 
             var nodesElement = clusterElement.child("nodes");
-            if (nodesElement == null) return Optional.empty();
+            if (nodesElement == null) return;
             var nodesSpec = NodesSpecification.from(nodesElement, context);
 
+            // Allow dev deployment of self-hosted app (w/o count attribute): absent count => 1 node
+            if (!nodesSpec.hasCountAttribute() && environment == Environment.dev) return;
+
             int minNodesPerGroup = (int)Math.ceil((double)nodesSpec.minResources().nodes() / nodesSpec.minResources().groups());
 
-            if (minNodesPerGroup < redundancy) { // TODO: Fail on this on Vespa 8, and simplify? But see ModelProvisioningTest.testThatStandaloneSyntaxWorksOnHostedManuallyDeployed
-                context.getDeployLogger()
-                       .logApplicationPackage(Level.WARNING,
-                                              "Cluster '" + clusterElement.stringAttribute("id") + "' " +
-                                              "specifies redundancy " + redundancy + " but cannot be higher than " +
-                                              "the minimum nodes per group, which is " + minNodesPerGroup);
-                return Optional.of(minNodesPerGroup);
-            }
-            else {
-                return Optional.empty();
+            if (minNodesPerGroup < redundancy) {
+                throw new IllegalArgumentException("Cluster '" + clusterElement.stringAttribute("id") + "' " +
+                                                   "specifies redundancy " + redundancy + ", but it cannot be higher than " +
+                                                   "the minimum nodes per group, which is " + minNodesPerGroup);
             }
         }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/RedundancyBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/RedundancyBuilder.java
index 7b124593354..1948cc1bd71 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/RedundancyBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/RedundancyBuilder.java
@@ -5,8 +5,6 @@ import com.yahoo.vespa.model.builder.xml.dom.ModelElement;
 import com.yahoo.vespa.model.content.IndexedHierarchicDistributionValidator;
 import com.yahoo.vespa.model.content.Redundancy;
 
-import java.util.Optional;
-
 /**
  * Builds redundancy config for a content cluster.
  */
@@ -39,13 +37,7 @@ public class RedundancyBuilder {
             }
         }
     }
-    public Redundancy build(String clusterName, boolean isHosted, int subGroups, int leafGroups,  int totalNodes,
-                            Optional<Integer> maxRedundancy) {
-        if (maxRedundancy.isPresent()) {
-            initialRedundancy = Math.min(initialRedundancy, maxRedundancy.get());
-            finalRedundancy = Math.min(finalRedundancy, maxRedundancy.get());
-            readyCopies = Math.min(readyCopies, maxRedundancy.get());
-        }
+    public Redundancy build(String clusterName, boolean isHosted, int subGroups, int leafGroups,  int totalNodes) {
         if (isHosted) {
             return new Redundancy(initialRedundancy, finalRedundancy, readyCopies, leafGroups, totalNodes);
         } else {
diff --git a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
index f183020cb22..ffc38ba932d 100644
--- a/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
+++ b/config-model/src/test/java/com/yahoo/config/model/provision/ModelProvisioningTest.java
@@ -1107,16 +1107,13 @@ public class ModelProvisioningTest {
         int numberOfHosts = 3;
         VespaModelTester tester = new VespaModelTester();
         tester.addHosts(numberOfHosts);
-        VespaModel model = tester.createModel(services, false, "node-1-3-50-03");
-        assertEquals(numberOfHosts, model.getRoot().hostSystem().getHosts().size());
-
-        ContentCluster cluster = model.getContentClusters().get("bar");
-        assertEquals(2, cluster.redundancy().effectiveInitialRedundancy());
-        assertEquals(2, cluster.redundancy().effectiveFinalRedundancy());
-        assertEquals(2, cluster.redundancy().effectiveReadyCopies());
-        assertEquals("1|*", cluster.getRootGroup().getPartitions().get());
-        assertEquals(0, cluster.getRootGroup().getNodes().size());
-        assertEquals(2, cluster.getRootGroup().getSubgroups().size());
+        try {
+            VespaModel model = tester.createModel(services, false, "node-1-3-50-03");
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("Cluster 'bar' specifies redundancy 2, but it cannot be higher than the minimum nodes per group, which is 1", Exceptions.toMessageString(e));
+        }
     }
 
     @Test
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
index 41f9b4c7b55..ed0cedd4e87 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/test/VespaModelTester.java
@@ -192,14 +192,19 @@ public class VespaModelTester {
         VespaModelCreatorWithMockPkg modelCreatorWithMockPkg = new VespaModelCreatorWithMockPkg(hosts, services, generateSchemas("type1"));
         ApplicationPackage appPkg = modelCreatorWithMockPkg.appPkg;
 
-        provisioner = hosted ?        new ProvisionerAdapter(new InMemoryProvisioner(hostsByResources,
-                                                                                     failOnOutOfCapacity,
-                                                                                     useMaxResources,
-                                                                                     alwaysReturnOneNode,
-                                                                                     false,
-                                                                                     startIndexForClusters,
-                                                                                     retiredHostNames)) :
-                                      new SingleNodeProvisioner();
+        if (hosted) {
+            InMemoryProvisioner provisioner = new InMemoryProvisioner(hostsByResources,
+                                                                      failOnOutOfCapacity,
+                                                                      useMaxResources,
+                                                                      alwaysReturnOneNode,
+                                                                      false,
+                                                                      startIndexForClusters,
+                                                                      retiredHostNames);
+            provisioner.setEnvironment(zone.environment());
+            this.provisioner = new ProvisionerAdapter(provisioner);
+        } else {
+            provisioner = new SingleNodeProvisioner();
+        }
 
         TestProperties properties = new TestProperties()
                 .setMultitenant(hosted) // Note: system tests are multitenant but not hosted