Implement recursive delete in node-repo

8 files changed, 100 insertions, 45 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 68bdffd75f9..4800383b2bb 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
@@ -468,28 +468,37 @@ public class NodeRepository extends AbstractComponent {
     }
 
     /**
-     * Removes a node. A node must be in a legal state before it can be removed.
+     * Removes all the nodes that are children of hostname before finally removing the hostname itself.
+     *
+     * @return List of all the nodes that have been removed
      */
-    public void remove(String hostname) {
-        Node nodeToRemove = getNode(hostname).orElseThrow(() ->  new NotFoundException("No node with hostname \"" + hostname + '"'));
-        List<Node.State> legalStates = dynamicAllocationEnabled() ?
-                Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked, Node.State.dirty) :
-                Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
+    public List<Node> removeRecursively(String hostname) {
+        Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"'));
 
-        if (! legalStates.contains(nodeToRemove.state())) {
-            throw new IllegalArgumentException("Can only remove node from following states: " +
-                    legalStates.stream().map(Node.State::name).collect(Collectors.joining(", ")));
+        try (Mutex lock = lockUnallocated()) {
+            List<Node> removed = getChildNodes(hostname).stream()
+                    .filter(this::allowedToRemove)
+                    .collect(Collectors.toList());
+            if (allowedToRemove(node)) removed.add(node);
+            db.removeNodes(removed);
+
+            return removed;
+        } catch (RuntimeException e) {
+            throw new IllegalArgumentException("Failed to delete " + node.hostname(), e);
         }
+    }
 
-        if (nodeToRemove.state().equals(Node.State.dirty)) {
-            if (!(nodeToRemove.flavor().getType().equals(Flavor.Type.DOCKER_CONTAINER))) {
-                throw new IllegalArgumentException("Only docker nodes can be deleted from state dirty");
-            }
-        }
+    private boolean allowedToRemove(Node nodeToRemove) {
+        List<Node.State> legalStates = nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER ?
+                Arrays.asList(Node.State.provisioned, Node.State.ready) :
+                Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
 
-        try (Mutex lock = lock(nodeToRemove)) {
-            db.removeNode(nodeToRemove.state(), hostname);
+        if (! legalStates.contains(nodeToRemove.state())) {
+            throw new IllegalArgumentException(String.format("%s can only be removed from following states: %s",
+                    nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
         }
+
+        return true;
     }
 
     /**
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
index 8f218d7e6fc..d38c9179986 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
@@ -106,18 +106,21 @@ public class CuratorDatabaseClient {
     }
 
     /**
-     * Removes a node.
+     * Removes multiple nodes in a single transaction.
      *
-     * @param state the current state of the node
-     * @param hostName the host name of the node to remove
+     * @param nodes list of the nodes to remove
      */
-    public void removeNode(Node.State state, String hostName) {
-        Path path = toPath(state, hostName);
+    public void removeNodes(List<Node> nodes) {
         NestedTransaction transaction = new NestedTransaction();
-        CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction);
-        curatorTransaction.add(CuratorOperations.delete(path.getAbsolute()));
+
+        for (Node node : nodes) {
+            Path path = toPath(node.state(), node.hostname());
+            CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction);
+            curatorTransaction.add(CuratorOperations.delete(path.getAbsolute()));
+        }
+
         transaction.commit();
-        log.log(LogLevel.INFO, "Removed: " + state + " node " + hostName);
+        nodes.forEach(node -> log.log(LogLevel.INFO, "Removed node " + node.hostname() + " in state " + node.state()));
     }
 
     /**
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
index 4e7bb1f7d16..1cb997b48fb 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/NodesApiHandler.java
@@ -125,20 +125,21 @@ public class NodesApiHandler extends LoggingRequestHandler {
             return new MessageResponse("Moved " + lastElement(path) + " to active");
         }
         else if (path.startsWith("/nodes/v2/state/availablefornewallocations/")) {
-            /**
+            /*
              * This is a temporary "state" or rest call that we use to enable a smooth rollout of
              * dynamic docker flavor allocations. Once we have switch everything we remove this
-             * and change the code in the nodeadmin to delete directly (remember to allow deletion of dirty nodes then).
+             * and change the code in the nodeadmin to delete directly (remember to allow deletion of ready nodes then).
              *
              * Should only be called by node-admin for docker containers (the docker constraint is
              * enforced in the remove method)
              */
             String hostname = lastElement(path);
+            nodeRepository.setReady(hostname);
+
             if (nodeRepository.dynamicAllocationEnabled()) {
-                nodeRepository.remove(hostname);
-                return new MessageResponse("Removed " + hostname);
+                List<Node> removedNodes = nodeRepository.removeRecursively(hostname);
+                return new MessageResponse("Removed " + removedNodes.stream().map(Node::hostname).collect(Collectors.joining(", ")));
             } else {
-                nodeRepository.setReady(hostname);
                 return new MessageResponse("Moved " + hostname + " to ready");
             }
         }
@@ -180,8 +181,8 @@ public class NodesApiHandler extends LoggingRequestHandler {
         String path = request.getUri().getPath();
         if (path.startsWith("/nodes/v2/node/")) {
             String hostname = lastElement(path);
-            nodeRepository.remove(hostname);
-            return new MessageResponse("Removed " + hostname);
+            List<Node> removedNodes = nodeRepository.removeRecursively(hostname);
+            return new MessageResponse("Removed " + removedNodes.stream().map(Node::hostname).collect(Collectors.joining(", ")));
         }
         else if (path.startsWith("/nodes/v2/maintenance/inactive/")) {
             return setActive(lastElement(path), true);
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 bcae9d293f6..f7168e132f2 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
@@ -28,18 +28,18 @@ public class NodeRepositoryTest {
     @Test
     public void nodeRepositoryTest() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
-        assertEquals(0, tester.getNodes(NodeType.tenant).size());
+        assertEquals(0, tester.nodeRepository().getNodes().size());
 
         tester.addNode("id1", "host1", "default", NodeType.tenant);
         tester.addNode("id2", "host2", "default", NodeType.tenant);
         tester.addNode("id3", "host3", "default", NodeType.tenant);
 
-        assertEquals(3, tester.getNodes(NodeType.tenant).size());
+        assertEquals(3, tester.nodeRepository().getNodes().size());
         
         tester.nodeRepository().park("host2", Agent.system, "Parking to unit test");
-        tester.nodeRepository().remove("host2");
+        tester.nodeRepository().removeRecursively("host2");
 
-        assertEquals(2, tester.getNodes(NodeType.tenant).size());
+        assertEquals(2, tester.nodeRepository().getNodes().size());
     }
 
     @Test
@@ -70,20 +70,57 @@ public class NodeRepositoryTest {
     }
 
     @Test
-    public void only_allow_to_delete_dirty_nodes_when_dynamic_allocation_feature_enabled() {
+    public void only_allow_docker_containers_remove_in_provisioned_or_ready() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
-        tester.addNode("id1", "host1", "default", NodeType.host);
+        tester.addNode("id1", "host1", "docker", NodeType.tenant);
         tester.addNode("id2", "host2", "docker", NodeType.tenant);
+        tester.nodeRepository().fail("host2", Agent.system, "Failed for testing");
+
+        tester.nodeRepository().removeRecursively("host1"); // host1 is in state provisioned
+        try {
+            tester.nodeRepository().removeRecursively("host2");
+            fail("Should not be able to delete docker tenant node in state dirty");
+        } catch (IllegalArgumentException ignored) {
+            // Expected
+        }
+
         tester.nodeRepository().setDirty("host2");
+        tester.nodeRepository().setReady("host2");
+        tester.nodeRepository().removeRecursively("host2");
+    }
+
+    @Test
+    public void delete_host_only_after_all_the_children_have_been_deleted() {
+        NodeRepositoryTester tester = new NodeRepositoryTester();
+
+        tester.addNode("id1", "host1", "default", NodeType.host);
+        tester.addNode("id2", "host2", "default", NodeType.host);
+        tester.addNode("node10", "node10", "host1", "docker", NodeType.tenant);
+        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());
+
+        tester.nodeRepository().setDirty("node11");
 
         try {
-            tester.nodeRepository().remove("host2");
-            fail("Should not be able to delete tenant node in state dirty");
+            tester.nodeRepository().removeRecursively("host1");
+            fail("Should not be able to delete host node, one of the children are in state dirty");
         } catch (IllegalArgumentException ignored) {
             // Expected
         }
+        assertEquals(6, tester.nodeRepository().getNodes().size());
 
-        tester.curator().set(Path.fromString("/provision/v1/dynamicDockerAllocation"), new byte[0]);
-        tester.nodeRepository().remove("host2");
+        // Should be OK to delete host2 as both host2 and its only child, node20, are in state provisioned
+        tester.nodeRepository().removeRecursively("host2");
+        assertEquals(4, tester.nodeRepository().getNodes().size());
+
+        // Now node10 and node12 are in provisioned, set node11 to ready, and it should be OK to delete host1
+        tester.nodeRepository().setReady("node11");
+        tester.nodeRepository().removeRecursively("node12"); // Remove one of the children first instead
+        assertEquals(3, tester.nodeRepository().getNodes().size());
+
+        tester.nodeRepository().removeRecursively("host1");
+        assertEquals(0, tester.nodeRepository().getNodes().size());
     }
 }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
index 784fc1a274a..3d01bde4291 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTester.java
@@ -51,6 +51,12 @@ public class NodeRepositoryTester {
         return nodeRepository.addNodes(Collections.singletonList(node)).get(0);
     }
 
+    public Node addNode(String id, String hostname, String parentHostname, String flavor, NodeType type) {
+        Node node = nodeRepository.createNode(id, hostname, Optional.of(parentHostname),
+                nodeFlavors.getFlavorOrThrow(flavor), type);
+        return nodeRepository.addNodes(Collections.singletonList(node)).get(0);
+    }
+
     private FlavorsConfig createConfig() {
         FlavorConfigBuilder b = new FlavorConfigBuilder();
         b.addFlavor("default", 2., 4., 100, Flavor.Type.BARE_METAL).cost(3);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
index e304aac5463..8fd67f949d9 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailerTest.java
@@ -342,7 +342,7 @@ public class NodeFailerTest {
         assertEquals(15, tester.nodeRepository.getNodes(NodeType.proxy, Node.State.active).size());
 
         // The first down host is removed, which causes the second one to be moved to failed
-        tester.nodeRepository.remove(failedHost1);
+        tester.nodeRepository.removeRecursively(failedHost1);
         tester.failer.run();
         assertEquals( 2, tester.deployer.redeployments);
         assertEquals(14, tester.nodeRepository.getNodes(NodeType.proxy, Node.State.active).size());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java
index cb46d0a4624..bba5aa2db8d 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/ZooKeeperAccessMaintainerTest.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.config.provision.NodeType;
-import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepositoryTester;
 import com.yahoo.vespa.hosted.provision.node.Agent;
 import com.yahoo.vespa.zookeeper.ZooKeeperServer;
@@ -49,7 +48,7 @@ public class ZooKeeperAccessMaintainerTest {
         assertEquals(asSet("host1,host2,host3,host4,host5,server1,server2"), ZooKeeperServer.getAllowedClientHostnames());
 
         tester.nodeRepository().park("host2", Agent.system, "Parking to unit test");
-        tester.nodeRepository().remove("host2");
+        tester.nodeRepository().removeRecursively("host2");
         maintainer.maintain();
 
         assertEquals(2, tester.getNodes(NodeType.tenant).size());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
index 565dfe0457e..40ae1e1c2c5 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/RestApiTest.java
@@ -401,7 +401,7 @@ public class RestApiTest {
         // Attempt to DELETE a node which is not put in a deletable state first
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host2.yahoo.com",
                                    new byte[0], Request.Method.DELETE),
-                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Can only remove node from following states: provisioned, failed, parked, dirty\"}");
+                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Failed to delete host2.yahoo.com: host2.yahoo.com can only be removed from following states: provisioned, failed, parked\"}");
 
         // PUT current restart generation with string instead of long
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",