Bug fix priority on reserved vs ready nodes

3 files changed, 4 insertions, 18 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePriority.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePriority.java
index c9afa19c161..b5a87fffa81 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePriority.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePriority.java
@@ -59,8 +59,8 @@ class NodePriority {
         if (n2.node.state().equals(Node.State.inactive) && !n1.node.state().equals(Node.State.inactive)) return 1;
 
         // Choose reserved nodes from a previous allocation attempt (the exist in node repo)
-        if (isInNodeRepoAndReserved(n1) && isInNodeRepoAndReserved(n2)) return -1;
-        if (isInNodeRepoAndReserved(n2) && isInNodeRepoAndReserved(n1)) return 1;
+        if (isInNodeRepoAndReserved(n1) && !isInNodeRepoAndReserved(n2)) return -1;
+        if (isInNodeRepoAndReserved(n2) && !isInNodeRepoAndReserved(n1)) return 1;
 
         // Choose ready nodes
         if (n1.node.state().equals(Node.State.ready) && !n2.node.state().equals(Node.State.ready)) return -1;
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
index 160c7b874db..4a20cb3e318 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/MultigroupProvisioningTest.java
@@ -60,20 +60,6 @@ public class MultigroupProvisioningTest {
         deploy(application1, 6, 3, tester);
     }
 
-    @Test
-    public void test_provisioning_of_multiple_groups2() {
-        ProvisioningTester tester = new ProvisioningTester(new Zone(Environment.prod, RegionName.from("us-east")));
-
-        ApplicationId application1 = tester.makeApplicationId();
-
-        tester.makeReadyNodes(6, "default");
-
-        deploy(application1,4, 2, tester);
-        deploy(application1, 2, 2, tester);
-        deploy(application1,4, 2, tester);
-
-    }
-
     /**
      * This demonstrates a case where we end up provisioning new nodes rather than reusing retired nodes
      * due to asymmetric group sizes after step 2 (second group has 3 additional retired nodes).
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
index 73ef95ca4c8..bd50a9fc581 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTester.java
@@ -136,9 +136,9 @@ public class ProvisioningTester implements AutoCloseable {
         Set<String> reservedBefore = toHostNames(nodeRepository.getNodes(application, Node.State.reserved));
         Set<String> inactiveBefore = toHostNames(nodeRepository.getNodes(application, Node.State.inactive));
         // prepare twice to ensure idempotence
-        //List<HostSpec> hosts1 = provisioner.prepare(application, cluster, capacity, groups, provisionLogger);
+        List<HostSpec> hosts1 = provisioner.prepare(application, cluster, capacity, groups, provisionLogger);
         List<HostSpec> hosts2 = provisioner.prepare(application, cluster, capacity, groups, provisionLogger);
-        //assertEquals(hosts1, hosts2);
+        assertEquals(hosts1, hosts2);
         Set<String> newlyActivated = toHostNames(nodeRepository.getNodes(application, Node.State.reserved));
         newlyActivated.removeAll(reservedBefore);
         newlyActivated.removeAll(inactiveBefore);