Implement recursive delete in node-repo

5 files changed, 56 insertions, 14 deletions

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",