Revert "Revert "Prefer lower cost nodes over even distribution""

6 files changed, 9 insertions, 71 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 c611438fcbe..52e689441d1 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(stripeOverHosts(prioritizeNodes(readyNodes, requestedNodes)), !canChangeGroup);
+                accepted = nodeList.offer(prioritizeNodes(readyNodes, requestedNodes), !canChangeGroup);
                 nodeList.update(nodeRepository.reserve(accepted));
             }
 
@@ -109,17 +109,18 @@ class GroupPreparer {
      * to use comes first 
      */
     private List<Node> prioritizeNodes(List<Node> nodeList, NodeSpec nodeSpec) {
-        if ( nodeSpec.specifiesNonStockFlavor()) { // sort by exact before inexact flavor match, increasing cost, hostname
+        if ( nodeSpec.specifiesNonStockFlavor()) { // prefer exact flavor, docker hosts, lower cost, tie break by 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 { // sort by increasing cost, hostname
+        else { // prefer docker hosts, lower cost, tie break by hostname
             Collections.sort(nodeList, (n1, n2) -> ComparisonChain.start()
-                    .compareTrueFirst(nodeSpec.matchesExactly(n1.flavor()), nodeSpec.matchesExactly(n1.flavor()))
+                    .compareTrueFirst(n1.parentHostname().isPresent(), n2.parentHostname().isPresent())
                     .compare(n1.flavor().cost(), n2.flavor().cost())
                     .compare(n1.hostname(), n2.hostname())
                     .result()
@@ -128,44 +129,6 @@ 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 6ba2c40186d..59601832e59 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,6 +23,7 @@ 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 1cb27df3972..d62deac2855 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,32 +45,6 @@ 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 9d37e94e623..7f40c08c9df 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": 1,
+    "index": 0,
     "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 6e0ac0c9e2b..2e24c3f03b8 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": 0,
+    "index": 1,
     "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 5278f0dd6bf..306cccd227c 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": 0,
+    "index": 1,
     "retired": false
   },
   "restartGeneration": 0,