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.

2 files changed, 59 insertions, 15 deletions

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, " +