Revert "Prefer lower cost nodes over even distribution"

6 files changed, 71 insertions, 9 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
index 52e689441d1..c611438fcbe 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java
@@ -82,7 +82,7 @@ class GroupPreparer {
             // Use new, ready nodes. Lock ready pool to ensure that nodes are not grabbed by others.
             try (Mutex readyLock = nodeRepository.lockUnallocated()) {
                 List<Node> readyNodes = nodeRepository.getNodes(requestedNodes.type(), Node.State.ready);
-                accepted = nodeList.offer(prioritizeNodes(readyNodes, requestedNodes), !canChangeGroup);
+                accepted = nodeList.offer(stripeOverHosts(prioritizeNodes(readyNodes, requestedNodes)), !canChangeGroup);
                 nodeList.update(nodeRepository.reserve(accepted));
             }
 
@@ -109,18 +109,17 @@ class GroupPreparer {
      * to use comes first 
      */
     private List<Node> prioritizeNodes(List<Node> nodeList, NodeSpec nodeSpec) {
-        if ( nodeSpec.specifiesNonStockFlavor()) { // prefer exact flavor, docker hosts, lower cost, tie break by hostname
+        if ( nodeSpec.specifiesNonStockFlavor()) { // sort by exact before inexact flavor match, increasing cost, hostname
             Collections.sort(nodeList, (n1, n2) -> ComparisonChain.start()
                     .compareTrueFirst(nodeSpec.matchesExactly(n1.flavor()), nodeSpec.matchesExactly(n2.flavor()))
-                    .compareTrueFirst(n1.parentHostname().isPresent(), n2.parentHostname().isPresent())
                     .compare(n1.flavor().cost(), n2.flavor().cost())
                     .compare(n1.hostname(), n2.hostname())
                     .result()
             );
         }
-        else { // prefer docker hosts, lower cost, tie break by hostname
+        else { // sort by increasing cost, hostname
             Collections.sort(nodeList, (n1, n2) -> ComparisonChain.start()
-                    .compareTrueFirst(n1.parentHostname().isPresent(), n2.parentHostname().isPresent())
+                    .compareTrueFirst(nodeSpec.matchesExactly(n1.flavor()), nodeSpec.matchesExactly(n1.flavor()))
                     .compare(n1.flavor().cost(), n2.flavor().cost())
                     .compare(n1.hostname(), n2.hostname())
                     .result()
@@ -129,6 +128,44 @@ class GroupPreparer {
         return nodeList;
     }
 
+    /** Return the input nodes in an order striped over their parent hosts */
+    static List<Node> stripeOverHosts(List<Node> input) {
+        List<Node> output = new ArrayList<>(input.size());
+
+        // first deal with nodes having a parent host
+        long nodesHavingParent = input.stream()
+            .filter(n -> n.parentHostname().isPresent())
+            .collect(Collectors.counting());
+        if (nodesHavingParent > 0) {
+            // Make a map where each parent host maps to its list of child nodes
+            Map<String, List<Node>> byParentHosts = input.stream()
+                .filter(n -> n.parentHostname().isPresent())
+                .collect(Collectors.groupingBy(n -> n.parentHostname().get()));
+
+            // sort keys, those parent hosts with the most (remaining) ready nodes first
+            List<String> sortedParentHosts = byParentHosts
+                .keySet().stream()
+                .sorted((k1, k2) -> byParentHosts.get(k2).size() - byParentHosts.get(k1).size())
+                .collect(Collectors.toList());
+            while (nodesHavingParent > 0) {
+                // take one node from each parent host, round-robin.
+                for (String k : sortedParentHosts) {
+                    List<Node> leftFromHost = byParentHosts.get(k);
+                    if (! leftFromHost.isEmpty()) {
+                        output.add(leftFromHost.remove(0));
+                        --nodesHavingParent;
+                    }
+                }
+            }
+        }
+
+        // now add non-VMs (nodes without a parent):
+        input.stream()
+            .filter(n -> ! n.parentHostname().isPresent())
+            .forEach(n -> output.add(n));
+        return output;
+    }
+
     /** Used to manage a list of nodes during the node reservation process */
     private class NodeList {
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
index 59601832e59..6ba2c40186d 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/AclProvisioningTest.java
@@ -23,7 +23,6 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
-import java.util.Set;
 import java.util.stream.Collectors;
 
 import static com.yahoo.vespa.hosted.provision.provisioning.ProvisioningTester.createConfig;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
index d62deac2855..1cb27df3972 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/VirtualNodeProvisioningTest.java
@@ -45,6 +45,32 @@ public class VirtualNodeProvisioningTest {
     }
 
     @Test
+    public void optimize_sorts_by_more_vms() {
+        List<Node> readyList = new ArrayList<>();
+        readyList.addAll(tester.makeReadyNodes(2, flavor));
+        readyList.addAll(tester.makeReadyVirtualNodes(2, flavor, "parentHost1"));
+        readyList.addAll(tester.makeReadyNodes(2, flavor));
+        readyList.addAll(tester.makeReadyVirtualNodes(3, flavor, "parentHost2"));
+        readyList.addAll(tester.makeReadyVirtualNodes(2, flavor, "parentHost3"));
+        readyList.addAll(tester.makeReadyVirtualNodes(1, flavor, "parentHost4"));
+        readyList.addAll(tester.makeReadyNodes(3, flavor));
+        assertEquals(15, readyList.size());
+        List<Node> optimized = GroupPreparer.stripeOverHosts(readyList);
+        assertEquals(15, optimized.size());
+        assertEquals("parentHost2", optimized.get(0).parentHostname().get());
+        assertEquals("parentHost4", optimized.get(3).parentHostname().get());
+        assertEquals("parentHost2", optimized.get(4).parentHostname().get());
+        assertEquals("parentHost2", optimized.get(7).parentHostname().get());
+        assertEquals(false, optimized.get(8).parentHostname().isPresent());
+        assertEquals(false, optimized.get(9).parentHostname().isPresent());
+        assertEquals(false, optimized.get(10).parentHostname().isPresent());
+        assertEquals(false, optimized.get(11).parentHostname().isPresent());
+        assertEquals(false, optimized.get(12).parentHostname().isPresent());
+        assertEquals(false, optimized.get(13).parentHostname().isPresent());
+        assertEquals(false, optimized.get(14).parentHostname().isPresent());
+    }
+
+    @Test
     public void distinct_parent_host_for_each_node_in_a_cluster() {
         tester.makeReadyVirtualNodes(2, flavor, "parentHost1");
         tester.makeReadyVirtualNodes(2, flavor, "parentHost2");
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node10.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node10.json
index 7f40c08c9df..9d37e94e623 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node10.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node10.json
@@ -22,7 +22,7 @@
     "clustertype": "container",
     "clusterid": "id1",
     "group": "0",
-    "index": 0,
+    "index": 1,
     "retired": false
   },
   "restartGeneration": 0,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
index 2e24c3f03b8..6e0ac0c9e2b 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4-after-changes.json
@@ -22,7 +22,7 @@
     "clustertype": "container",
     "clusterid": "id1",
     "group": "0",
-    "index": 1,
+    "index": 0,
     "retired": false
   },
   "restartGeneration": 0,
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4.json b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4.json
index 306cccd227c..5278f0dd6bf 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4.json
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/responses/node4.json
@@ -22,7 +22,7 @@
     "clustertype": "container",
     "clusterid": "id1",
     "group": "0",
-    "index": 1,
+    "index": 0,
     "retired": false
   },
   "restartGeneration": 0,