Merge pull request #3253 from vespa-engine/freva/recursive-delete

Implement recursive delete in node-repo

8 files changed, 146 insertions, 56 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..7f595b4a541 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
@@ -467,29 +467,88 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
+    /*
+     * This method is used to enable a smooth rollout of dynamic docker flavor allocations. Once we have switch
+     * everything this can be simplified to only deleting the node.
+     *
+     * Should only be called by node-admin for docker containers
+     */
+    public List<Node> markNodeAvailableForNewAllocation(String hostname) {
+        Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"'));
+        if (node.flavor().getType() != Flavor.Type.DOCKER_CONTAINER) {
+            throw new IllegalArgumentException(
+                    "Cannot make " + hostname + " available for new allocation, must be a docker container node");
+        } else if (node.state() != Node.State.dirty) {
+            throw new IllegalArgumentException(
+                    "Cannot make " + hostname + " available for new allocation, must be in state dirty, but was in " + node.state());
+        }
+
+        if (dynamicAllocationEnabled()) {
+            return removeRecursively(node, true);
+        } else {
+            return setReady(Collections.singletonList(node));
+        }
+    }
+
     /**
-     * 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);
-
-        if (! legalStates.contains(nodeToRemove.state())) {
-            throw new IllegalArgumentException("Can only remove node from following states: " +
-                    legalStates.stream().map(Node.State::name).collect(Collectors.joining(", ")));
+    public List<Node> removeRecursively(String hostname) {
+        Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"'));
+        return removeRecursively(node, false);
+    }
+
+    private List<Node> removeRecursively(Node node, boolean force) {
+        try (Mutex lock = lockUnallocated()) {
+            List<Node> removed = node.type() != NodeType.host ?
+                    new ArrayList<>() :
+                    getChildNodes(node.hostname()).stream()
+                            .filter(child -> force || verifyRemovalIsAllowed(child, true))
+                            .collect(Collectors.toList());
+
+            if (force || verifyRemovalIsAllowed(node, false)) 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");
+    /**
+     * Allowed to a node delete if:
+     *  Non-docker-container node: iff in state provisioned|failed|parked
+     *  Docker-container-node:
+     *    If only removing the container node: node in state ready
+     *    If also removing the parent node: child is in state provisioned|failed|parked|ready
+     */
+    private boolean verifyRemovalIsAllowed(Node nodeToRemove, boolean deletingAsChild) {
+        // TODO: Enable once controller no longer deletes child nodes manually
+        /*if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !deletingAsChild) {
+            if (nodeToRemove.state() != Node.State.ready) {
+                throw new IllegalArgumentException(
+                        String.format("Docker container node %s can only be removed when in state ready", nodeToRemove.hostname()));
             }
-        }
 
-        try (Mutex lock = lock(nodeToRemove)) {
-            db.removeNode(nodeToRemove.state(), hostname);
+        } else */ if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) {
+            List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked, Node.State.ready);
+
+            if (! legalStates.contains(nodeToRemove.state())) {
+                throw new IllegalArgumentException(String.format("Child node %s can only be removed from following states: %s",
+                        nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", "))));
+            }
+        } else {
+            List<Node.State> legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked);
+
+            if (! legalStates.contains(nodeToRemove.state())) {
+                throw new IllegalArgumentException(String.format("Node %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..b16ce5f818e 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,22 +125,10 @@ 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).
-             *
-             * Should only be called by node-admin for docker containers (the docker constraint is
-             * enforced in the remove method)
-             */
             String hostname = lastElement(path);
-            if (nodeRepository.dynamicAllocationEnabled()) {
-                nodeRepository.remove(hostname);
-                return new MessageResponse("Removed " + hostname);
-            } else {
-                nodeRepository.setReady(hostname);
-                return new MessageResponse("Moved " + hostname + " to ready");
-            }
+            List<Node> available = nodeRepository.markNodeAvailableForNewAllocation(hostname);
+            return new MessageResponse("Marked following nodes as available for new allocation: " +
+                    available.stream().map(Node::hostname).collect(Collectors.joining(", ")));
         }
 
         throw new NotFoundException("Cannot put to path '" + path + "'");
@@ -180,8 +168,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..8eec56a1c00 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
@@ -8,6 +8,7 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.path.Path;
 import com.yahoo.vespa.hosted.provision.node.Agent;
+import org.junit.Ignore;
 import org.junit.Test;
 
 import java.nio.charset.StandardCharsets;
@@ -28,18 +29,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
@@ -69,21 +70,55 @@ public class NodeRepositoryTest {
         assertTrue(tester.nodeRepository().dynamicAllocationEnabled());
     }
 
+    @Test @Ignore // TODO: Enable once controller no longer deletes child nodes manually
+    public void only_allow_docker_containers_remove_in_ready() {
+        NodeRepositoryTester tester = new NodeRepositoryTester();
+        tester.addNode("id1", "host1", "docker", NodeType.tenant);
+
+        try {
+            tester.nodeRepository().removeRecursively("host1"); // host1 is in state provisioned
+            fail("Should not be able to delete docker container node by itself in state provisioned");
+        } catch (IllegalArgumentException ignored) {
+            // Expected
+        }
+
+        tester.nodeRepository().setDirty("host1");
+        tester.nodeRepository().setReady("host1");
+        tester.nodeRepository().removeRecursively("host1");
+    }
+
     @Test
-    public void only_allow_to_delete_dirty_nodes_when_dynamic_allocation_feature_enabled() {
+    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", "docker", NodeType.tenant);
-        tester.nodeRepository().setDirty("host2");
+        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 is 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("node11"); // 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..38d3bf46028 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: Node 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",