Reuse empty exclusive hosts when nodes fit exactly on the host

2 files changed, 57 insertions, 9 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 2334ff48aac..265040823b2 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
@@ -38,8 +38,6 @@ public class NodePrioritizer {
     private final NameResolver nameResolver;
     private final Nodes nodes;
     private final boolean dynamicProvisioning;
-    /** Whether node specification allows new nodes to be allocated. */
-    private final boolean canAllocateNew;
     private final boolean canAllocateToSpareHosts;
     private final boolean topologyChange;
     private final int currentClusterSize;
@@ -81,9 +79,6 @@ public class NodePrioritizer {
         // NodeCandidate::compareTo will ensure that they will not be used until there is no room elsewhere.
         // In non-dynamically provisioned zones, we only allow allocating to spare hosts to replace failed nodes.
         this.canAllocateToSpareHosts = dynamicProvisioning || isReplacement(nodesInCluster, clusterSpec.group());
-        // Do not allocate new nodes for exclusive deployments in dynamically provisioned zones: provision new host instead.
-        this.canAllocateNew = requestedNodes instanceof NodeSpec.CountNodeSpec
-                              && (!dynamicProvisioning || !requestedNodes.isExclusive());
     }
 
     /** Collects all node candidates for this application and returns them in the most-to-least preferred order */
@@ -136,15 +131,14 @@ public class NodePrioritizer {
 
     /** Add a node on each host with enough capacity for the requested flavor  */
     private void addCandidatesOnExistingHosts() {
-        if ( !canAllocateNew) return;
+        if (requestedNodes.resources().isEmpty()) return;
 
         for (Node host : allNodes) {
             if ( ! nodes.canAllocateTenantNodeTo(host, dynamicProvisioning)) continue;
             if (nodes.suspended(host)) continue; // Hosts that are suspended may be down for some time, e.g. for OS upgrade
             if (host.reservedTo().isPresent() && !host.reservedTo().get().equals(application.tenant())) continue;
             if (host.reservedTo().isPresent() && application.instance().isTester()) continue;
-            // TODO jonmv: allow reusing exclusive hosts after all, if the host matches what we'd provision for that node anyway.
-            if (host.exclusiveToApplicationId().isPresent()) continue; // Never allocate new nodes to exclusive hosts
+            if (host.exclusiveToApplicationId().isPresent() && ! fitsPerfectly(host)) continue;
             if ( ! host.exclusiveToClusterType().map(clusterSpec.type()::equals).orElse(true)) continue;
             if (spareHosts.contains(host) && !canAllocateToSpareHosts) continue;
             if ( ! capacity.hasCapacity(host, requestedNodes.resources().get())) continue;
@@ -160,6 +154,10 @@ public class NodePrioritizer {
         }
     }
 
+    private boolean fitsPerfectly(Node host) {
+        return requestedNodes.resources().get().compatibleWith(host.resources());
+    }
+
     /** Add existing nodes allocated to the application */
     private void addApplicationNodes() {
         EnumSet<Node.State> legalStates = EnumSet.of(Node.State.active, Node.State.inactive, Node.State.reserved);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
index 60ac30293a3..d4a911fa1ab 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/DynamicProvisioningTest.java
@@ -21,6 +21,7 @@ import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.flags.PermanentFlags;
 import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.Node.State;
 import com.yahoo.vespa.hosted.provision.NodeList;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.Agent;
@@ -31,6 +32,7 @@ import org.junit.Test;
 import java.time.Instant;
 import java.util.Collection;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Collectors;
@@ -85,7 +87,6 @@ public class DynamicProvisioningTest {
         assertEquals(20, tester.nodeRepository().nodes().list().size());
         assertEquals(8, tester.nodeRepository().nodes().list(Node.State.active).nodeType(NodeType.host).size());
         assertEquals(12, tester.nodeRepository().nodes().list(Node.State.active).nodeType(NodeType.tenant).size());
-        assert false: "TODO: test with TTL";
     }
 
     @Test
@@ -117,6 +118,55 @@ public class DynamicProvisioningTest {
     }
 
     @Test
+    public void empty_exclusive_to_hosts_reused_iff_new_allocation_fits_perfectly() {
+        var tester = tester(true);
+
+        NodeResources highResources = new NodeResources(4, 80, 100, 1);
+        NodeResources lowResources = new NodeResources(2, 20, 50, 1);
+
+        ApplicationId application = ProvisioningTester.applicationId();
+        prepareAndActivate(application, clusterSpec("mycluster", true), 2, 1, highResources, tester);
+
+        // Total of 4 nodes should now be in node-repo, 2 active hosts and 2 active nodes.
+        assertEquals(4, tester.nodeRepository().nodes().list().size());
+        assertEquals(2, tester.nodeRepository().nodes().list(Node.State.active).nodeType(NodeType.tenant).size());
+
+        // Redeploying the application causes no changes at all.
+        prepareAndActivate(application, clusterSpec("mycluster", true), 2, 1, highResources, tester);
+        assertEquals(4, tester.nodeRepository().nodes().list().size());
+        assertEquals(2, tester.nodeRepository().nodes().list(Node.State.active).nodeType(NodeType.tenant).size());
+
+        // Deploying with a smaller node flavour causes new, smaller hosts to be provisioned.
+        prepareAndActivate(application, clusterSpec("mycluster", true), 2, 1, lowResources, tester);
+
+        // Total of 8 nodes should now be in node-repo, 4 active hosts and 4 active nodes, of which 2 are retired.
+        NodeList nodes = tester.nodeRepository().nodes().list();
+        assertEquals(8, nodes.size());
+        assertEquals(4, nodes.nodeType(NodeType.host).state(Node.State.active).size());
+        assertEquals(4, nodes.nodeType(NodeType.tenant).state(Node.State.active).size());
+        assertEquals(2, nodes.retired().size());
+
+        // Remove the child nodes, and redeploy with the original flavour. This should reuse the existing hosts.
+        tester.nodeRepository().database().writeTo(State.deprovisioned, nodes.retired().asList(), Agent.operator, Optional.empty());
+        tester.nodeRepository().nodes().list().state(State.deprovisioned).forEach(tester.nodeRepository().nodes()::forget);
+
+        // Total of 6 nodes should now be in node-repo, 4 active hosts and 2 active nodes.
+        nodes = tester.nodeRepository().nodes().list();
+        assertEquals(6, nodes.size());
+        assertEquals(4, nodes.nodeType(NodeType.host).state(Node.State.active).size());
+        assertEquals(2, nodes.nodeType(NodeType.tenant).state(Node.State.active).size());
+        assertEquals(0, nodes.retired().size());
+
+        // Deploy again with high resources.
+        prepareAndActivate(application, clusterSpec("mycluster", true), 2, 1, highResources, tester);
+        // Total of 8 nodes should now be in node-repo, 4 active hosts and 4 active nodes.
+        nodes = tester.nodeRepository().nodes().list();
+        assertEquals(8, nodes.size());
+        assertEquals(4, nodes.nodeType(NodeType.host).state(Node.State.active).size());
+        assertEquals(4, nodes.nodeType(NodeType.tenant).state(Node.State.active).size());
+    }
+
+    @Test
     public void avoids_allocating_to_empty_hosts() {
         var tester = tester(false);
         tester.makeReadyHosts(6, new NodeResources(12, 12, 200, 12));