Make sorting order for preferred re-allocation node stable

3 files changed, 36 insertions, 11 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
index aa3870bd61c..1ac86ad9f4b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
@@ -264,7 +264,7 @@ class NodePrioritizer {
                 pri.violatesSpares = true;
             }
 
-            if (headroomHosts.containsKey(parent) && isSmallestNodeOnParent(node, parent)) {
+            if (headroomHosts.containsKey(parent) && isPreferredNodeToBeReloacted(allNodes, node, parent)) {
                 ResourceCapacity neededCapacity = headroomHosts.get(parent);
 
                 // If the node is new then we need to check the headroom requirement after it has been added
@@ -278,15 +278,13 @@ class NodePrioritizer {
         return pri;
     }
 
-    private boolean isSmallestNodeOnParent(Node node, Node parent) {
-        NodeList list = new NodeList(allNodes);
-        ResourceCapacity nodeSize = new ResourceCapacity(node);
-        for (Node child : list.childNodes(parent).asList()) {
-            if (new ResourceCapacity(child).compare(nodeSize) < 0) {
-                return false;
-            }
-        }
-        return true;
+    static boolean isPreferredNodeToBeReloacted(List<Node> nodes, Node node, Node parent) {
+        NodeList list = new NodeList(nodes);
+        return list.childNodes(parent).asList().stream()
+                .sorted(NodePrioritizer::compareForRelocation)
+                .findFirst()
+                .filter(n -> n.equals(node))
+                .isPresent();
     }
 
     private boolean isReplacement(long nofNodesInCluster, long nodeFailedNodes) {
@@ -320,4 +318,27 @@ class NodePrioritizer {
                 .filter(n -> n.hostname().equals(node.parentHostname().orElse(" NOT A NODE")))
                 .findAny();
     }
+
+    private static int compareForRelocation(Node a, Node b) {
+        // Choose smallest node
+        int capacity = ResourceCapacity.of(a).compare(ResourceCapacity.of(b));
+        if (capacity != 0) return capacity;
+
+        // Choose unallocated over allocated (this case is when we have ready docker nodes)
+        if (!a.allocation().isPresent() && b.allocation().isPresent()) return -1;
+        if (a.allocation().isPresent() && !b.allocation().isPresent()) return 1;
+
+        // Choose container over content nodes
+        if (a.allocation().isPresent() && a.allocation().isPresent()) {
+            if (a.allocation().get().membership().cluster().type().equals(ClusterSpec.Type.container) &&
+                    !b.allocation().get().membership().cluster().type().equals(ClusterSpec.Type.container))
+                return -1;
+            if (!a.allocation().get().membership().cluster().type().equals(ClusterSpec.Type.container) &&
+                    b.allocation().get().membership().cluster().type().equals(ClusterSpec.Type.container))
+                return 1;
+        }
+
+        // To get a stable algorithm - choose lexicographical from hostname
+        return a.hostname().compareTo(b.hostname());
+    }
 }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ResourceCapacity.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ResourceCapacity.java
index 362e83da80a..8373cf9e17f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ResourceCapacity.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ResourceCapacity.java
@@ -36,6 +36,10 @@ public class ResourceCapacity {
         return capacity;
     }
 
+    static ResourceCapacity of(Node node) {
+        return new ResourceCapacity(node);
+    }
+
     public double getMemory() {
         return memory;
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
index 62eb462115f..c26865d5690 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicDockerProvisioningTest.java
@@ -211,7 +211,7 @@ public class DynamicDockerProvisioningTest {
      * |    |     |      |      |         |    |     |  1a   |  1b   |
      */
     @Test
-    public void only_smallest_container_is_moved_from_hosts_with_headroom_violations() {
+    public void only_preferred_container_is_moved_from_hosts_with_headroom_violations() {
         ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.perf, RegionName.from("us-east")), flavorsConfig(true));
         enableDynamicAllocation(tester);
         tester.makeReadyNodes(4, "host", "host-medium", NodeType.host, 32);