Fix parent host state order

2 files changed, 52 insertions, 3 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
index e97f263e233..22058105f90 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNode.java
@@ -33,7 +33,7 @@ class PrioritizableNode implements Comparable<PrioritizableNode> {
     /** This node does not exist in the node repository yet */
     final boolean isNewNode;
 
-    private PrioritizableNode(Node node, NodeResources freeParentCapacity, Optional<Node> parent, boolean violatesSpares, boolean isSurplusNode, boolean isNewNode) {
+    PrioritizableNode(Node node, NodeResources freeParentCapacity, Optional<Node> parent, boolean violatesSpares, boolean isSurplusNode, boolean isNewNode) {
         this.node = node;
         this.freeParentCapacity = freeParentCapacity;
         this.parent = parent;
@@ -73,13 +73,13 @@ class PrioritizableNode implements Comparable<PrioritizableNode> {
         if (this.node.state() == Node.State.ready && other.node.state() != Node.State.ready) return -1;
         if (other.node.state() == Node.State.ready && this.node.state() != Node.State.ready) return 1;
 
-        if ( ! this.node.state().equals(other.node.state()))
+        if (this.node.state() != other.node.state())
             throw new IllegalStateException("Nodes " + this.node + " and " + other.node + " have different states");
 
         // Choose nodes where host is in more desirable state
         int thisHostStatePri = this.parent.map(host -> ALLOCATABLE_HOST_STATES.indexOf(host.state())).orElse(-2);
         int otherHostStatePri = other.parent.map(host -> ALLOCATABLE_HOST_STATES.indexOf(host.state())).orElse(-2);
-        if (thisHostStatePri != otherHostStatePri) return thisHostStatePri - otherHostStatePri;
+        if (thisHostStatePri != otherHostStatePri) return otherHostStatePri - thisHostStatePri;
 
         // Choose docker node over non-docker node (is this to differentiate between docker replaces non-docker flavors?)
         if (this.parent.isPresent() && !other.parent.isPresent()) return -1;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java
new file mode 100644
index 00000000000..1a373222bc5
--- /dev/null
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/PrioritizableNodeTest.java
@@ -0,0 +1,49 @@
+package com.yahoo.vespa.hosted.provision.provisioning;
+
+import com.yahoo.config.provision.Flavor;
+import com.yahoo.config.provision.NodeResources;
+import com.yahoo.config.provision.NodeType;
+import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.node.History;
+import com.yahoo.vespa.hosted.provision.node.IP;
+import com.yahoo.vespa.hosted.provision.node.Reports;
+import com.yahoo.vespa.hosted.provision.node.Status;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+
+import static org.junit.Assert.assertEquals;
+
+public class PrioritizableNodeTest {
+
+    @Test
+    public void test_order() {
+        List<PrioritizableNode> expected = List.of(
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.empty(), false, true, false),
+                new PrioritizableNode(node("abc123", Node.State.active), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false),
+                new PrioritizableNode(node("abc123", Node.State.inactive), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false),
+                new PrioritizableNode(node("abc123", Node.State.reserved), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, false),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.active)), true, false, true),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.ready)), true, false, true),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.provisioned)), true, false, true),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.of(node("host1", Node.State.failed)), true, false, true),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(1, 1, 1, 1), Optional.empty(), true, false, true),
+                new PrioritizableNode(node("abc123", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, true),
+                new PrioritizableNode(node("xyz789", Node.State.ready), new NodeResources(2, 2, 2, 2), Optional.empty(), true, false, true)
+        );
+
+        List<PrioritizableNode> copy = new ArrayList<>(expected);
+        Collections.shuffle(copy);
+        Collections.sort(copy);
+        assertEquals(expected, copy);
+    }
+
+    private static Node node(String hostname, Node.State state) {
+        return new Node(hostname, new IP.Config(Set.of("::1"), Set.of()), hostname, Optional.empty(), new Flavor(new NodeResources(2, 2, 2, 2)),
+                Status.initial(), state, Optional.empty(), History.empty(), NodeType.tenant, new Reports(), Optional.empty());
+    }
+}
\ No newline at end of file