Pick 1 cluster controller by index

3 files changed, 67 insertions, 20 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
index bec20daf120..e6ed91165ca 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/HostResource.java
@@ -270,22 +270,25 @@ public class HostResource implements Comparable<HostResource> {
      * Picks hosts by some mixture of host name and index 
      * (where the mix of one or the other is decided by the last parameter).
      */
-    public static List<HostResource> pickHosts(Collection<HostResource> hosts, int count, int targetHostsSortedByIndex) {
-        targetHostsSortedByIndex = Math.min(Math.min(targetHostsSortedByIndex, count), hosts.size());
+    public static List<HostResource> pickHosts(Collection<HostResource> hosts, int count, int targetHostsSelectedByIndex) {
+        targetHostsSelectedByIndex = Math.min(Math.min(targetHostsSelectedByIndex, count), hosts.size());
 
         List<HostResource> hostsSortedByName = new ArrayList<>(hosts);
         Collections.sort(hostsSortedByName);
 
         List<HostResource> hostsSortedByIndex = new ArrayList<>(hosts);
         hostsSortedByIndex.sort(Comparator.comparingInt(host -> host.primaryClusterMembership().get().index()));
-
-        hostsSortedByName = hostsSortedByName.subList(0, Math.min(count - targetHostsSortedByIndex, hostsSortedByName.size()));
-        hostsSortedByIndex.removeAll(hostsSortedByName);
-        hostsSortedByIndex = hostsSortedByIndex.subList(0, Math.min(targetHostsSortedByIndex, hostsSortedByIndex.size()));
+        return pickHosts(hostsSortedByName, hostsSortedByIndex, count, targetHostsSelectedByIndex);
+    }
+    public static List<HostResource> pickHosts(List<HostResource> hostsSelectedByName, List<HostResource> hostsSelectedByIndex, 
+                                               int count, int targetHostsSelectedByIndex) {
+        hostsSelectedByName = hostsSelectedByName.subList(0, Math.min(count - targetHostsSelectedByIndex, hostsSelectedByName.size()));
+        hostsSelectedByIndex.removeAll(hostsSelectedByName);
+        hostsSelectedByIndex = hostsSelectedByIndex.subList(0, Math.min(targetHostsSelectedByIndex, hostsSelectedByIndex.size()));
 
         List<HostResource> finalHosts = new ArrayList<>();
-        finalHosts.addAll(hostsSortedByName);
-        finalHosts.addAll(hostsSortedByIndex);
+        finalHosts.addAll(hostsSelectedByName);
+        finalHosts.addAll(hostsSelectedByIndex);
         return finalHosts;
     }
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
index 376b7ea595a..6e82379d00b 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/cluster/ContentCluster.java
@@ -10,6 +10,7 @@ import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.Zone;
+import com.yahoo.lang.MutableInteger;
 import com.yahoo.vespa.config.content.MessagetyperouteselectorpolicyConfig;
 import com.yahoo.vespa.config.content.FleetcontrollerConfig;
 import com.yahoo.vespa.config.content.StorDistributionConfig;
@@ -328,9 +329,11 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri
         }
 
         private List<HostResource> drawControllerHosts(int count, StorageGroup rootGroup, Collection<ContainerModel> containers) {
-            List<HostResource> hosts = drawContentHostsRecursively(count, rootGroup);
+            List<HostResource> hostsByName = drawContentHostsRecursively(count, false, rootGroup);
+            List<HostResource> hostsByIndex = drawContentHostsRecursively(count, true, rootGroup);
             // if (hosts.size() < count) // supply with containers TODO: Currently disabled due to leading to topology change problems
             //     hosts.addAll(drawContainerHosts(count - hosts.size(), containers, new HashSet<>(hosts)));
+            List<HostResource> hosts = HostResource.pickHosts(hostsByName, hostsByIndex, count, 1);
             if (hosts.size() % 2 == 0) // ZK clusters of even sizes are less available (even in the size=2 case)
                 hosts = hosts.subList(0, hosts.size()-1);
             return hosts;
@@ -403,20 +406,26 @@ public class ContentCluster extends AbstractConfigProducer implements StorDistri
          */
         // Note: This method cannot be changed to draw different nodes without ensuring that it will draw nodes
         //       which overlaps with previously drawn nodes as this will prevent rolling upgrade
-        private List<HostResource> drawContentHostsRecursively(int count, StorageGroup group) {
+        private List<HostResource> drawContentHostsRecursively(int count, boolean byIndex, StorageGroup group) {
             Set<HostResource> hosts = new HashSet<>();
             if (group.getNodes().isEmpty()) {
                 int hostsPerSubgroup = (int)Math.ceil((double)count / group.getSubgroups().size());
                 for (StorageGroup subgroup : group.getSubgroups())
-                    hosts.addAll(drawContentHostsRecursively(hostsPerSubgroup, subgroup));
+                    hosts.addAll(drawContentHostsRecursively(hostsPerSubgroup, byIndex, subgroup));
             }
             else {
                 hosts.addAll(group.getNodes().stream()
-                    .filter(node -> ! node.isRetired()) // Avoid retired controllers to avoid surprises on expiry
-                    .map(StorageNode::getHostResource).collect(Collectors.toList()));
+                     .filter(node -> ! node.isRetired()) // Avoid retired controllers to avoid surprises on expiry
+                     .map(StorageNode::getHostResource).collect(Collectors.toList()));
             }
 
-            return HostResource.pickHosts(hosts, count, 0);
+            List<HostResource> sortedHosts = new ArrayList<>(hosts);
+            if (byIndex)
+                sortedHosts.sort(Comparator.comparingInt(host -> host.primaryClusterMembership().get().index()));
+            else // by name
+                Collections.sort(sortedHosts);
+            sortedHosts = sortedHosts.subList(0, Math.min(count, hosts.size()));
+            return sortedHosts;
         }
 
         private ContainerCluster createClusterControllers(AbstractConfigProducer parent, Collection<HostResource> hosts, String name, boolean multitenant) {
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 9ed3c853b19..6ea15788cf3 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
@@ -393,7 +393,7 @@ public class ModelProvisioningTest {
         assertEquals("bar-controllers", clusterControllers.getName());
         assertEquals("default28", clusterControllers.getContainers().get(0).getHostName());
         assertEquals("default31", clusterControllers.getContainers().get(1).getHostName());
-        assertEquals("default34", clusterControllers.getContainers().get(2).getHostName());
+        assertEquals("default54", clusterControllers.getContainers().get(2).getHostName());
         assertEquals(0, cluster.getRootGroup().getNodes().size());
         assertEquals(9, cluster.getRootGroup().getSubgroups().size());
         assertThat(cluster.getRootGroup().getSubgroups().get(0).getIndex(), is("0"));
@@ -432,7 +432,7 @@ public class ModelProvisioningTest {
         assertEquals("baz-controllers", clusterControllers.getName());
         assertEquals("default01", clusterControllers.getContainers().get(0).getHostName());
         assertEquals("default02", clusterControllers.getContainers().get(1).getHostName());
-        assertEquals("default03", clusterControllers.getContainers().get(2).getHostName());
+        assertEquals("default27", clusterControllers.getContainers().get(2).getHostName());
         assertEquals(0, cluster.getRootGroup().getNodes().size());
         assertEquals(27, cluster.getRootGroup().getSubgroups().size());
         assertThat(cluster.getRootGroup().getSubgroups().get(0).getIndex(), is("0"));
@@ -485,7 +485,7 @@ public class ModelProvisioningTest {
         assertEquals("bar-controllers", clusterControllers.getName());
         assertEquals("default01", clusterControllers.getContainers().get(0).getHostName());
         assertEquals("default02", clusterControllers.getContainers().get(1).getHostName());
-        assertEquals("default03", clusterControllers.getContainers().get(2).getHostName());
+        assertEquals("default08", clusterControllers.getContainers().get(2).getHostName());
         assertEquals(0, cluster.getRootGroup().getNodes().size());
         assertEquals(8, cluster.getRootGroup().getSubgroups().size());
         assertEquals(8, cluster.distributionBits());
@@ -538,12 +538,13 @@ public class ModelProvisioningTest {
         ContentCluster cluster = model.getContentClusters().get("bar");
         ContainerCluster clusterControllers = cluster.getClusterControllers();
         assertEquals( 8, cluster.distributionBits());
-        assertEquals("We get the closest odd numer", 5, clusterControllers.getContainers().size());
+        assertEquals("We get the closest odd number", 5, clusterControllers.getContainers().size());
         assertEquals("bar-controllers", clusterControllers.getName());
         assertEquals("default01", clusterControllers.getContainers().get(0).getHostName());
         assertEquals("default02", clusterControllers.getContainers().get(1).getHostName());
         assertEquals("default04", clusterControllers.getContainers().get(2).getHostName());
-        assertEquals("default05", clusterControllers.getContainers().get(3).getHostName()); // Should be 16 for perfect distribution ...
+        assertEquals("default05", clusterControllers.getContainers().get(3).getHostName());
+        assertEquals("default07", clusterControllers.getContainers().get(4).getHostName());
         assertEquals("default09", cluster.getRootGroup().getSubgroups().get(0).getNodes().get(0).getHostName());
         assertEquals("default08", cluster.getRootGroup().getSubgroups().get(0).getNodes().get(1).getHostName());
         assertEquals("default06", cluster.getRootGroup().getSubgroups().get(1).getNodes().get(0).getHostName());
@@ -551,6 +552,40 @@ public class ModelProvisioningTest {
     }
 
     @Test
+    public void testClusterControllersWithGroupSize2() {
+        String services =
+                "<?xml version='1.0' encoding='utf-8' ?>\n" +
+                "<services>" +
+                "  <admin version='4.0'/>" +
+                "  <container version='1.0' id='foo'>" +
+                "     <nodes count='10'/>" +
+                "  </container>" +
+                "  <content version='1.0' id='bar'>" +
+                "     <redundancy>2</redundancy>" +
+                "     <documents>" +
+                "       <document type='type1' mode='index'/>" +
+                "     </documents>" +
+                "     <nodes count='8' groups='4'/>" +
+                "  </content>" +
+                "</services>";
+
+        int numberOfHosts = 18;
+        VespaModelTester tester = new VespaModelTester();
+        tester.addHosts(numberOfHosts);
+        VespaModel model = tester.createModel(services, true);
+        assertThat(model.getRoot().getHostSystem().getHosts().size(), is(numberOfHosts));
+
+        // Check content clusters
+        ContentCluster cluster = model.getContentClusters().get("bar");
+        ContainerCluster clusterControllers = cluster.getClusterControllers();
+        assertEquals("We get the closest odd number", 3, clusterControllers.getContainers().size());
+        assertEquals("bar-controllers", clusterControllers.getName());
+        assertEquals("default01", clusterControllers.getContainers().get(0).getHostName());
+        assertEquals("default03", clusterControllers.getContainers().get(1).getHostName());
+        assertEquals("default08", clusterControllers.getContainers().get(2).getHostName());
+    }
+
+    @Test
     public void testClusterControllersCanSupplementWithAllContainerClusters() throws ParseException {
         String services =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
@@ -614,7 +649,7 @@ public class ModelProvisioningTest {
         assertEquals("bar-controllers", clusterControllers.getName());
         assertEquals("Skipping retired default09", "default01", clusterControllers.getContainers().get(0).getHostName());
         assertEquals("Skipping retired default03", "default04", clusterControllers.getContainers().get(1).getHostName());
-        assertEquals("Skipping retired default06", "default07", clusterControllers.getContainers().get(2).getHostName());
+        assertEquals("Skipping retired default06", "default08", clusterControllers.getContainers().get(2).getHostName());
     }
 
     @Test