Only include non-retired nodes in redundancy computation

Usually redundancy is down-adjusted to match the nodes per group,
but not if the node count requested is high enough but downscaled
by capacity policies. The redundancy is also adjusted down if there
are not enough nodes in the actual allocation, which normally
handles this case, but not if there are retired nodes.
That is fixed in this PR by not counting retired nodes
when making that adjustment.

4 files changed, 80 insertions, 28 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 188504edd18..e41e7309986 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
@@ -63,6 +63,8 @@ public class InMemoryProvisioner implements HostProvisioner {
 
     private final boolean useMaxResources;
 
+    private final boolean alwaysReturnOneNode;
+
     private Provisioned provisioned = new Provisioned();
 
     /** Creates this with a number of nodes with resources 1, 3, 9, 1 */
@@ -72,37 +74,39 @@ public class InMemoryProvisioner implements HostProvisioner {
 
     /** Creates this with a number of nodes with given resources */
     public InMemoryProvisioner(int nodeCount, NodeResources resources, boolean sharedHosts) {
-        this(Map.of(resources, createHostInstances(nodeCount)), true, false, sharedHosts, 0);
+        this(Map.of(resources, createHostInstances(nodeCount)), true, false, false, sharedHosts, 0);
     }
 
     /** Creates this with a set of host names of the flavor 'default' */
     public InMemoryProvisioner(boolean failOnOutOfCapacity, boolean sharedHosts, String... hosts) {
-        this(Map.of(defaultResources, toHostInstances(hosts)), failOnOutOfCapacity, false, sharedHosts, 0);
+        this(Map.of(defaultResources, toHostInstances(hosts)), failOnOutOfCapacity, false, false, sharedHosts, 0);
     }
 
     /** Creates this with a set of host names of the flavor 'default' */
     public InMemoryProvisioner(boolean failOnOutOfCapacity, boolean sharedHosts, List<String> hosts) {
-        this(Map.of(defaultResources, toHostInstances(hosts.toArray(new String[0]))), failOnOutOfCapacity, false, sharedHosts, 0);
+        this(Map.of(defaultResources, toHostInstances(hosts.toArray(new String[0]))), failOnOutOfCapacity, false, false, sharedHosts, 0);
     }
 
     /** Creates this with a set of hosts of the flavor 'default' */
     public InMemoryProvisioner(Hosts hosts, boolean failOnOutOfCapacity, boolean sharedHosts, String ... retiredHostNames) {
-        this(Map.of(defaultResources, hosts.asCollection()), failOnOutOfCapacity, false, sharedHosts, 0, retiredHostNames);
+        this(Map.of(defaultResources, hosts.asCollection()), failOnOutOfCapacity, false, false, sharedHosts, 0, retiredHostNames);
     }
 
     /** Creates this with a set of hosts of the flavor 'default' */
     public InMemoryProvisioner(Hosts hosts, boolean failOnOutOfCapacity, boolean sharedHosts, int startIndexForClusters, String ... retiredHostNames) {
-        this(Map.of(defaultResources, hosts.asCollection()), failOnOutOfCapacity, false, sharedHosts, startIndexForClusters, retiredHostNames);
+        this(Map.of(defaultResources, hosts.asCollection()), failOnOutOfCapacity, false, false, sharedHosts, startIndexForClusters, retiredHostNames);
     }
 
     public InMemoryProvisioner(Map<NodeResources, Collection<Host>> hosts,
                                boolean failOnOutOfCapacity,
                                boolean useMaxResources,
+                               boolean alwaysReturnOneNode,
                                boolean sharedHosts,
                                int startIndexForClusters,
                                String ... retiredHostNames) {
         this.failOnOutOfCapacity = failOnOutOfCapacity;
         this.useMaxResources = useMaxResources;
+        this.alwaysReturnOneNode = alwaysReturnOneNode;
         for (Map.Entry<NodeResources, Collection<Host>> hostsWithResources : hosts.entrySet())
             for (Host host : hostsWithResources.getValue())
                 freeNodes.put(hostsWithResources.getKey(), host);
@@ -142,16 +146,20 @@ public class InMemoryProvisioner implements HostProvisioner {
     public List<HostSpec> prepare(ClusterSpec cluster, ClusterResources requested, boolean required, boolean canFail) {
         if (cluster.group().isPresent() && requested.groups() > 1)
             throw new IllegalArgumentException("Cannot both be specifying a group and ask for groups to be created");
-        int capacity = failOnOutOfCapacity || required
+
+        int nodes = failOnOutOfCapacity || required
                        ? requested.nodes()
                        : Math.min(requested.nodes(), freeNodes.get(defaultResources).size() + totalAllocatedTo(cluster));
-        int groups = requested.groups() > capacity ? capacity : requested.groups();
+        if (alwaysReturnOneNode)
+            nodes = 1;
+
+        int groups = requested.groups() > nodes ? nodes : requested.groups();
 
         List<HostSpec> allocation = new ArrayList<>();
         if (groups == 1) {
             allocation.addAll(allocateHostGroup(cluster.with(Optional.of(ClusterSpec.Group.from(0))),
                                                 requested.nodeResources(),
-                                                capacity,
+                                                nodes,
                                                 startIndexForClusters,
                                                 canFail));
         }
@@ -159,7 +167,7 @@ public class InMemoryProvisioner implements HostProvisioner {
             for (int i = 0; i < groups; i++) {
                 allocation.addAll(allocateHostGroup(cluster.with(Optional.of(ClusterSpec.Group.from(i))),
                                                     requested.nodeResources(),
-                                                    capacity / groups,
+                                                    nodes / groups,
                                                     allocation.size(),
                                                     canFail));
             }
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 f106b1f7bd5..44365bbbe27 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
@@ -162,10 +162,10 @@ public class StorageGroup {
     }
 
     /** Returns the total number of nodes below this group */
-    public int countNodes() {
-        int nodeCount = nodes.size();
+    public int countNodes(boolean includeRetired) {
+        int nodeCount = (int)nodes.stream().filter(node -> includeRetired || ! node.isRetired()).count();
         for (StorageGroup group : subgroups)
-            nodeCount += group.countNodes();
+            nodeCount += group.countNodes(includeRetired);
         return nodeCount;
     }
 
@@ -220,7 +220,7 @@ public class StorageGroup {
                     ? groupBuilder.buildHosted(deployState, owner, Optional.empty())
                     : groupBuilder.buildNonHosted(deployState, owner, Optional.empty());
             Redundancy redundancy = redundancyBuilder.build(owner.getName(), owner.isHosted(), storageGroup.subgroups.size(),
-                                                            storageGroup.getNumberOfLeafGroups(), storageGroup.countNodes(),
+                                                            storageGroup.getNumberOfLeafGroups(), storageGroup.countNodes(false),
                                                             maxRedundancy);
             owner.setRedundancy(redundancy);
             if (storageGroup.partitions.isEmpty() && (redundancy.groups() > 1)) {
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 10019b00f61..499ec94f8ee 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
@@ -35,7 +35,6 @@ import com.yahoo.vespa.model.test.VespaModelTester;
 import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
 import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithMockPkg;
 import com.yahoo.yolean.Exceptions;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import java.io.StringReader;
@@ -986,6 +985,35 @@ public class ModelProvisioningTest {
     }
 
     @Test
+    public void testRedundancy2DownscaledToOneNodeButOneRetired() {
+        String services =
+                "<?xml version='1.0' encoding='utf-8' ?>" +
+                "<services>" +
+                "  <content version='1.0' id='bar'>" +
+                "     <redundancy>2</redundancy>" +
+                "     <documents>" +
+                "       <document type='type1' mode='index'/>" +
+                "     </documents>" +
+                "     <nodes count='2'/>" +
+                "  </content>" +
+                "</services>";
+
+        int numberOfHosts = 3;
+        VespaModelTester tester = new VespaModelTester();
+        tester.addHosts(numberOfHosts);
+        VespaModel model = tester.createModel(services, false, false, true, "node-1-3-10-03");
+        assertEquals(numberOfHosts, model.getRoot().hostSystem().getHosts().size());
+
+        ContentCluster cluster = model.getContentClusters().get("bar");
+        assertEquals(2, cluster.getStorageNodes().getChildren().size());
+        assertEquals(1, cluster.redundancy().effectiveInitialRedundancy());
+        assertEquals(1, cluster.redundancy().effectiveFinalRedundancy());
+        assertEquals(1, cluster.redundancy().effectiveReadyCopies());
+        assertEquals(2, cluster.getRootGroup().getNodes().size());
+        assertEquals(0, cluster.getRootGroup().getSubgroups().size());
+    }
+
+    @Test
     public void testUsingNodesCountAttributesAndGettingTooFewNodes() {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>" +
@@ -1451,7 +1479,7 @@ public class ModelProvisioningTest {
         assertEquals("We get 1 node per cluster and no admin node apart from the dedicated cluster controller", 3, model.getHosts().size());
         assertEquals(1, model.getContainerClusters().size());
         assertEquals(1, model.getContainerClusters().get("foo").getContainers().size());
-        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes());
+        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes(true));
         assertEquals(1, model.getAdmin().getClusterControllers().getContainers().size());
     }
 
@@ -1504,7 +1532,7 @@ public class ModelProvisioningTest {
         assertEquals(6, model.getRoot().hostSystem().getHosts().size());
         assertEquals(5, model.getAdmin().getSlobroks().size());
         assertEquals(2, model.getContainerClusters().get("foo").getContainers().size());
-        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes());
+        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes(true));
     }
 
     @Test
@@ -1574,7 +1602,7 @@ public class ModelProvisioningTest {
         assertEquals(1, model.getRoot().hostSystem().getHosts().size());
         assertEquals(1, model.getAdmin().getSlobroks().size());
         assertEquals(1, model.getContainerClusters().get("foo").getContainers().size());
-        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes());
+        assertEquals(1, model.getContentClusters().get("bar").getRootGroup().countNodes(true));
     }
 
     /** Recreate the combination used in some factory tests */
@@ -1857,7 +1885,7 @@ public class ModelProvisioningTest {
             assertTrue("Initial servers are not joining", config.build().server().stream().noneMatch(ZookeeperServerConfig.Server::joining));
         }
         {
-            VespaModel nextModel = tester.createModel(Zone.defaultZone(), servicesXml.apply(5), true, false, 0, Optional.of(model), new DeployState.Builder());
+            VespaModel nextModel = tester.createModel(Zone.defaultZone(), servicesXml.apply(5), true, false, false, 0, Optional.of(model), new DeployState.Builder());
             ApplicationContainerCluster cluster = nextModel.getContainerClusters().get("zk");
             ZookeeperServerConfig.Builder config = new ZookeeperServerConfig.Builder();
             cluster.getContainers().forEach(c -> c.getConfig(config));
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 b72ae088484..ba975e52d1a 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
@@ -52,6 +52,7 @@ public class VespaModelTester {
     private final Map<NodeResources, Collection<Host>> hostsByResources = new HashMap<>();
     private ApplicationId applicationId = ApplicationId.defaultId();
     private boolean useDedicatedNodeForLogserver = false;
+    private HostProvisioner provisioner;
 
     public VespaModelTester() {
         this(new NullConfigModelRegistry());
@@ -61,6 +62,12 @@ public class VespaModelTester {
         this.configModelRegistry = configModelRegistry;
     }
 
+    public HostProvisioner provisioner() {
+        if (provisioner instanceof ProvisionerAdapter)
+            return ((ProvisionerAdapter)provisioner).provisioner();
+        return provisioner;
+    }
+
     /** Adds some nodes with resources 1, 3, 10 */
     public Hosts addHosts(int count) { return addHosts(InMemoryProvisioner.defaultResources, count); }
 
@@ -108,37 +115,43 @@ public class VespaModelTester {
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(String services, boolean failOnOutOfCapacity, String ... retiredHostNames) {
-        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, 0,
+        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, false, 0,
                            Optional.empty(), new DeployState.Builder(), retiredHostNames);
     }
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(String services, boolean failOnOutOfCapacity, DeployState.Builder builder) {
-        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, 0, Optional.empty(), builder);
+        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, false, 0, Optional.empty(), builder);
     }
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(String services, boolean failOnOutOfCapacity, boolean useMaxResources, String ... retiredHostNames) {
-        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, useMaxResources, 0,
+        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, useMaxResources, false, 0,
+                           Optional.empty(), new DeployState.Builder(), retiredHostNames);
+    }
+
+    /** Creates a model which uses 0 as start index */
+    public VespaModel createModel(String services, boolean failOnOutOfCapacity, boolean useMaxResources, boolean alwaysReturnOneNode, String ... retiredHostNames) {
+        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, useMaxResources, alwaysReturnOneNode, 0,
                            Optional.empty(), new DeployState.Builder(), retiredHostNames);
     }
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(String services, boolean failOnOutOfCapacity, int startIndexForClusters, String ... retiredHostNames) {
-        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, startIndexForClusters,
+        return createModel(Zone.defaultZone(), services, failOnOutOfCapacity, false, false, startIndexForClusters,
                            Optional.empty(), new DeployState.Builder(), retiredHostNames);
     }
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(Zone zone, String services, boolean failOnOutOfCapacity, String ... retiredHostNames) {
-        return createModel(zone, services, failOnOutOfCapacity, false, 0,
+        return createModel(zone, services, failOnOutOfCapacity, false, false, 0,
                            Optional.empty(), new DeployState.Builder(), retiredHostNames);
     }
 
     /** Creates a model which uses 0 as start index */
     public VespaModel createModel(Zone zone, String services, boolean failOnOutOfCapacity,
                                   DeployState.Builder deployStateBuilder, String ... retiredHostNames) {
-        return createModel(zone, services, failOnOutOfCapacity, false, 0,
+        return createModel(zone, services, failOnOutOfCapacity, false, false, 0,
                            Optional.empty(), deployStateBuilder, retiredHostNames);
     }
 
@@ -152,15 +165,16 @@ public class VespaModelTester {
      * @return the resulting model
      */
     public VespaModel createModel(Zone zone, String services, boolean failOnOutOfCapacity, boolean useMaxResources,
+                                  boolean alwaysReturnOneNode,
                                   int startIndexForClusters, Optional<VespaModel> previousModel,
                                   DeployState.Builder deployStatebuilder, String ... retiredHostNames) {
         VespaModelCreatorWithMockPkg modelCreatorWithMockPkg = new VespaModelCreatorWithMockPkg(null, services, ApplicationPackageUtils.generateSearchDefinition("type1"));
         ApplicationPackage appPkg = modelCreatorWithMockPkg.appPkg;
 
-        HostProvisioner provisioner = hosted ?
-                                      new ProvisionerAdapter(new InMemoryProvisioner(hostsByResources,
+        provisioner = hosted ?        new ProvisionerAdapter(new InMemoryProvisioner(hostsByResources,
                                                                                      failOnOutOfCapacity,
                                                                                      useMaxResources,
+                                                                                     alwaysReturnOneNode,
                                                                                      false,
                                                                                      startIndexForClusters,
                                                                                      retiredHostNames)) :
@@ -184,12 +198,14 @@ public class VespaModelTester {
     /** To verify that we don't call allocateHost(alias) in hosted environments */
     private static class ProvisionerAdapter implements HostProvisioner {
 
-        private final HostProvisioner provisioner;
+        private final InMemoryProvisioner provisioner;
 
-        public ProvisionerAdapter(HostProvisioner provisioner) {
+        public ProvisionerAdapter(InMemoryProvisioner provisioner) {
             this.provisioner = provisioner;
         }
 
+        public InMemoryProvisioner provisioner() { return provisioner; }
+
         @Override
         public HostSpec allocateHost(String alias) {
             throw new UnsupportedOperationException("Allocating hosts using <node> tags is not supported in hosted environments, " +