Ensure that capacity reduction does not remove non-empty hosts

2 files changed, 35 insertions, 31 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java
index b1031715c24..44bfed90106 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java
@@ -95,14 +95,7 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer {
     /** Converge zone to wanted capacity */
     private void convergeToCapacity(NodeList nodes) {
         List<NodeResources> capacity = targetCapacity();
-        List<Node> existingHosts;
-        if (nodeRepository().zone().getCloud().dynamicProvisioning()) {
-            existingHosts = removableHostsOf(nodes);
-        } else {
-            if (capacity.isEmpty()) return;
-            existingHosts = availableHostsOf(nodes);
-        }
-        List<Node> excessHosts = provision(capacity, existingHosts);
+        List<Node> excessHosts = provision(capacity, nodes);
         excessHosts.forEach(host -> {
             try {
                 hostProvisioner.deprovision(host);
@@ -117,9 +110,15 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer {
     /**
      * Provision the nodes necessary to satisfy given capacity.
      *
-     * @return Excess hosts that can be deprovisioned, if any.
+     * @return Excess hosts that can safely be deprovisioned, if any.
      */
-    private List<Node> provision(List<NodeResources> capacity, List<Node> existingHosts) {
+    private List<Node> provision(List<NodeResources> capacity, NodeList nodes) {
+        List<Node> existingHosts = availableHostsOf(nodes);
+        if (nodeRepository().zone().getCloud().dynamicProvisioning()) {
+            existingHosts = removableHostsOf(existingHosts, nodes);
+        } else if (capacity.isEmpty()) {
+            return List.of();
+        }
         List<Node> excessHosts = new ArrayList<>(existingHosts);
         for (Iterator<NodeResources> it = capacity.iterator(); it.hasNext() && !excessHosts.isEmpty(); ) {
             NodeResources resources = it.next();
@@ -150,7 +149,7 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer {
                 log.log(Level.WARNING, "Failed to pre-provision " + resources + ", will retry in " + interval(), e);
             }
         });
-        return excessHosts;
+        return removableHostsOf(excessHosts, nodes);
     }
 
 
@@ -173,17 +172,17 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer {
                     .asList();
     }
 
-    /** Returns hosts that have no containers and are thus removable */
-    private static List<Node> removableHostsOf(NodeList nodes) {
-        Map<String, Node> hostsByHostname = availableHostsOf(nodes).stream()
-                                                                   .collect(Collectors.toMap(Node::hostname,
-                                                                                             Function.identity()));
-
-        nodes.asList().stream()
-             .filter(node -> node.allocation().isPresent())
-             .flatMap(node -> node.parentHostname().stream())
-             .distinct()
-             .forEach(hostsByHostname::remove);
+    /** Returns the subset of given hosts that have no containers and are thus removable */
+    private static List<Node> removableHostsOf(List<Node> hosts, NodeList allNodes) {
+        Map<String, Node> hostsByHostname = hosts.stream()
+                                                 .collect(Collectors.toMap(Node::hostname,
+                                                                           Function.identity()));
+
+        allNodes.asList().stream()
+                .filter(node -> node.allocation().isPresent())
+                .flatMap(node -> node.parentHostname().stream())
+                .distinct()
+                .forEach(hostsByHostname::remove);
 
         return List.copyOf(hostsByHostname.values());
     }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
index 6b61c1b2774..ba859655ab7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
@@ -160,9 +160,7 @@ public class DynamicProvisioningMaintainerTest {
         assertEquals(2, tester.provisionedHostsMatching(resources2));
 
         // Next maintenance run does nothing
-        List<Node> nodes = tester.nodeRepository.getNodes();
-        tester.maintainer.maintain();
-        assertEquals(nodes, tester.nodeRepository.getNodes());
+        tester.assertNodesUnchanged();
 
         // Target capacity is changed
         NodeResources resources3 = new NodeResources(48, 128, 1000, 1);
@@ -188,15 +186,16 @@ public class DynamicProvisioningMaintainerTest {
         tester.provisioningTester.deploy(application,
                                          Capacity.from(new ClusterResources(2, 1, new NodeResources(4, 8, 50, 0.1))));
         assertEquals(2, tester.nodeRepository.list().owner(application).size());
-        nodes = tester.nodeRepository.getNodes();
-        tester.maintainer.maintain();
-        assertEquals(nodes, tester.nodeRepository.getNodes());
+        tester.assertNodesUnchanged();
 
         // Clearing flag does nothing
         tester.flagSource.withListFlag(Flags.TARGET_CAPACITY.id(), List.of(), HostCapacity.class);
-        nodes = tester.nodeRepository.getNodes();
-        tester.maintainer.maintain();
-        assertEquals(nodes, tester.nodeRepository.getNodes());
+        tester.assertNodesUnchanged();
+
+        // Capacity reduction does not remove host with children
+        tester.flagSource.withListFlag(Flags.TARGET_CAPACITY.id(), List.of(new HostCapacity(resources1.vcpu(), resources1.memoryGb(), resources1.diskGb(), 1)),
+                                       HostCapacity.class);
+        tester.assertNodesUnchanged();
     }
 
     private static class DynamicProvisioningTester {
@@ -282,6 +281,12 @@ public class DynamicProvisioningMaintainerTest {
                                                    .count();
         }
 
+        private void assertNodesUnchanged() {
+            List<Node> nodes = nodeRepository.getNodes();
+            maintainer.maintain();
+            assertEquals("Nodes are unchanged after maintenance run", nodes, nodeRepository.getNodes());
+        }
+
     }
 
     static class MockHostProvisioner implements HostProvisioner {