Support recursive removal of all host types

2 files changed, 35 insertions, 13 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index b5e36abd076..b41820a461b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -650,7 +650,7 @@ public class NodeRepository extends AbstractComponent {
         try (Mutex lock = lockUnallocated()) {
             requireRemovable(node, false, force);
 
-            if (node.type() == NodeType.host) {
+            if (node.type().isDockerHost()) {
                 List<Node> children = list().childrenOf(node).asList();
                 children.forEach(child -> requireRemovable(child, true, force));
                 db.removeNodes(children);
@@ -665,8 +665,9 @@ public class NodeRepository extends AbstractComponent {
                 return removed;
             }
             else {
-                db.removeNodes(List.of(node));
-                return List.of(node);
+                List<Node> removed = List.of(node);
+                db.removeNodes(removed);
+                return removed;
             }
         }
     }
@@ -681,8 +682,8 @@ public class NodeRepository extends AbstractComponent {
     /**
      * Throws if the given node cannot be removed. Removal is allowed if:
      *  - Tenant node: node is unallocated
-     *  - Non-Docker-container node: iff in state provisioned|failed|parked
-     *  - Docker-container-node:
+     *  - Host node: iff in state provisioned|failed|parked
+     *  - Child node:
      *      If only removing the container node: node in state ready
      *      If also removing the parent node: child is in state provisioned|failed|parked|dirty|ready
      */
@@ -694,7 +695,7 @@ public class NodeRepository extends AbstractComponent {
 
         if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !removingAsChild) {
             if (node.state() != State.ready)
-                illegal(node + " can not be removed as it is not in the state [ready]");
+                illegal(node + " can not be removed as it is not in the state " + State.ready);
         }
         else if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) { // removing a child node
             Set<State> legalStates = EnumSet.of(State.provisioned, State.failed, State.parked, State.dirty, State.ready);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
index 97f1eda866a..9d5e7691fe8 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
@@ -32,7 +32,7 @@ import static org.junit.Assert.fail;
 public class NodeRepositoryTest {
 
     @Test
-    public void nodeRepositoryTest() {
+    public void add_and_remove() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
         assertEquals(0, tester.nodeRepository().getNodes().size());
 
@@ -120,13 +120,8 @@ public class NodeRepositoryTest {
         tester.addNode("node11", "node11", "host1", "docker", NodeType.tenant);
         tester.addNode("node12", "node12", "host1", "docker", NodeType.tenant);
         tester.addNode("node20", "node20", "host2", "docker", NodeType.tenant);
-        assertEquals(6, tester.nodeRepository().getNodes().size());
-
-        Node node = tester.nodeRepository().getNode("host1").get();
-        IP.Config cfg = new IP.Config(Set.of("127.0.0.1"), Set.of());
-        node = node.with(cfg);
-
         tester.setNodeState("node11", Node.State.active);
+        assertEquals(6, tester.nodeRepository().getNodes().size());
 
         try {
             tester.nodeRepository().removeRecursively("host1");
@@ -152,6 +147,32 @@ public class NodeRepositoryTest {
     }
 
     @Test
+    public void delete_config_host() {
+        NodeRepositoryTester tester = new NodeRepositoryTester();
+
+        String cfghost1 = "cfghost1";
+        String cfg1 = "cfg1";
+        tester.addNode("id1", cfghost1, "default", NodeType.confighost);
+        tester.addNode("id2", cfg1, cfghost1, "docker", NodeType.config);
+        tester.setNodeState(cfghost1, Node.State.active);
+        tester.setNodeState(cfg1, Node.State.active);
+        assertEquals(2, tester.nodeRepository().getNodes().size());
+
+        try {
+            tester.nodeRepository().removeRecursively(cfghost1);
+            fail("Should not be able to delete host node, one of the children is in state active");
+        } catch (IllegalArgumentException ignored) { }
+        assertEquals(2, tester.nodeRepository().getNodes().size());
+
+        // Fail host and container
+        tester.nodeRepository().failRecursively(cfghost1, Agent.system, getClass().getSimpleName());
+
+        // Remove recursively
+        tester.nodeRepository().removeRecursively(cfghost1);
+        assertEquals(0, tester.nodeRepository().list().not().state(Node.State.deprovisioned).size());
+    }
+
+    @Test
     public void relevant_information_from_deprovisioned_hosts_are_merged_into_readded_host() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
         Instant testStart = tester.nodeRepository().clock().instant();