Node with allocation must be parked to allow deprovisioning

2 files changed, 48 insertions, 23 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 cdc4416f90b..1d11d67c7cb 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
@@ -186,24 +186,37 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer {
                 .collect(Collectors.toList());
     }
 
-    private List<Node> candidatesForRemoval(List<Node> nodes) {
-        Map<String, Node> hostsByHostname = new HashMap<>(nodes.stream()
-                .filter(node -> switch (node.type()) {
-                    case host ->
-                        // TODO: Mark empty tenant hosts as wanttoretire & wanttodeprovision elsewhere, then handle as confighost here
-                        node.state() != Node.State.parked || node.status().wantToDeprovision();
-                    case confighost, controllerhost -> node.state() == Node.State.parked && node.status().wantToDeprovision();
-                    default -> false;
-                })
-                .collect(Collectors.toMap(Node::hostname, Function.identity())));
+    private static List<Node> candidatesForRemoval(List<Node> nodes) {
+        Map<String, Node> removableHostsByHostname = new HashMap<>();
+        for (var node : nodes) {
+            if (canRemoveHost(node)) {
+                removableHostsByHostname.put(node.hostname(), node);
+            }
+        }
+        for (var node : nodes) {
+            if (node.parentHostname().isPresent() && !canRemoveNode(node)) {
+                removableHostsByHostname.remove(node.parentHostname().get());
+            }
+        }
+        return List.copyOf(removableHostsByHostname.values());
+    }
 
-        nodes.stream()
-             .filter(node -> node.allocation().isPresent() && !node.status().wantToDeprovision())
-             .flatMap(node -> node.parentHostname().stream())
-             .distinct()
-             .forEach(hostsByHostname::remove);
+    private static boolean canRemoveHost(Node host) {
+        return switch (host.type()) {
+            // TODO: Mark empty tenant hosts as wanttoretire & wanttodeprovision elsewhere, then handle as confighost here
+            case host -> host.state() != Node.State.parked || host.status().wantToDeprovision();
+            case confighost, controllerhost -> canDeprovision(host);
+            default -> false;
+        };
+    }
+
+    private static boolean canRemoveNode(Node node) {
+        if (node.type().isHost()) throw new IllegalArgumentException("Node " + node + " is not a child");
+        return node.allocation().isEmpty() || canDeprovision(node);
+    }
 
-        return List.copyOf(hostsByHostname.values());
+    private static boolean canDeprovision(Node node) {
+        return node.status().wantToDeprovision() && node.state() == Node.State.parked;
     }
 
     private Map<String, Node> findSharedHosts(NodeList nodeList) {
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 7f4171ee8c0..8b363881fde 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
@@ -576,17 +576,29 @@ public class DynamicProvisioningMaintainerTest {
         tester.hostProvisioner.with(Behaviour.failProvisioning);
         Node host4 = tester.addNode("host4", Optional.empty(), NodeType.host, Node.State.parked);
         Node host41 = tester.addNode("host4-1", Optional.of("host4"), NodeType.tenant, Node.State.parked, DynamicProvisioningTester.tenantApp);
-        tester.nodeRepository.nodes().deprovision("host4", Agent.operator, Instant.now());
+        Node host42 = tester.addNode("host4-2", Optional.of("host4"), NodeType.tenant, Node.State.active, DynamicProvisioningTester.tenantApp);
 
-        assertEquals(Optional.of(true), tester.nodeRepository.nodes().node("host4").map(n -> n.status().wantToDeprovision()));
-        assertEquals(Optional.of(Node.State.parked), tester.nodeRepository.nodes().node("host4").map(Node::state));
-        assertEquals(Optional.of(true), tester.nodeRepository.nodes().node("host4-1").map(n -> n.status().wantToDeprovision()));
-        assertEquals(Optional.of(Node.State.parked), tester.nodeRepository.nodes().node("host4-1").map(Node::state));
+        // Host and children are marked for deprovisioning
+        tester.nodeRepository.nodes().deprovision("host4", Agent.operator, Instant.now());
+        for (var node : List.of(host4, host41, host42)) {
+            assertTrue(tester.nodeRepository.nodes().node(node.hostname()).map(n -> n.status().wantToDeprovision()).get());
+        }
 
+        // Host and children remain parked because one child is still active
         tester.maintainer.maintain();
+        for (var node : List.of(host4, host41)) {
+            assertEquals(Node.State.parked, tester.nodeRepository.nodes().node(node.hostname()).get().state());
+        }
+        assertEquals(Node.State.active, tester.nodeRepository.nodes().node(host42.hostname()).get().state());
 
-        assertEquals(Optional.empty(), tester.nodeRepository.nodes().node("host4"));
-        assertEquals(Optional.empty(), tester.nodeRepository.nodes().node("host4-1"));
+        // Last child is parked
+        tester.nodeRepository.nodes().park(host42.hostname(), true, Agent.system, getClass().getSimpleName());
+
+        // Host and children can now be removed
+        tester.maintainer.maintain();
+        for (var node : List.of(host4, host41, host42)) {
+            assertTrue(node.hostname() + " removed", tester.nodeRepository.nodes().node(node.hostname()).isEmpty());
+        }
     }
 
     private void assertCfghost3IsActive(DynamicProvisioningTester tester) {