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

Implement recursive delete in node-repo

3 files changed, 91 insertions, 41 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);