From 8af3bc056d712a3a1bc6b99bef3ddf5be3b89e94 Mon Sep 17 00:00:00 2001 From: valerijf Date: Tue, 29 Aug 2017 14:39:22 +0200 Subject: Implement recursive delete in node-repo --- .../vespa/hosted/provision/NodeRepository.java | 41 ++++++++++------ .../persistence/CuratorDatabaseClient.java | 19 +++++--- .../provision/restapi/v2/NodesApiHandler.java | 15 +++--- .../vespa/hosted/provision/NodeRepositoryTest.java | 57 ++++++++++++++++++---- .../hosted/provision/NodeRepositoryTester.java | 6 +++ .../provision/maintenance/NodeFailerTest.java | 2 +- .../maintenance/ZooKeeperAccessMaintainerTest.java | 3 +- .../hosted/provision/restapi/v2/RestApiTest.java | 2 +- 8 files changed, 100 insertions(+), 45 deletions(-) (limited to 'node-repository') 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 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 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 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 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 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 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 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", -- cgit v1.2.3 From 388de264b5e1c775fbf603ae5db16e0121aad27c Mon Sep 17 00:00:00 2001 From: valerijf Date: Wed, 30 Aug 2017 09:24:26 +0200 Subject: Code review fixes --- .../vespa/hosted/provision/NodeRepository.java | 45 ++++++++++++++++------ .../vespa/hosted/provision/NodeRepositoryTest.java | 19 ++++----- .../hosted/provision/restapi/v2/RestApiTest.java | 2 +- 3 files changed, 43 insertions(+), 23 deletions(-) (limited to 'node-repository') 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 4800383b2bb..b36921d37ac 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 @@ -476,10 +476,13 @@ public class NodeRepository extends AbstractComponent { Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"')); try (Mutex lock = lockUnallocated()) { - List removed = getChildNodes(hostname).stream() - .filter(this::allowedToRemove) - .collect(Collectors.toList()); - if (allowedToRemove(node)) removed.add(node); + List removed = node.type() != NodeType.host ? + new ArrayList<>() : + getChildNodes(hostname).stream() + .filter(child -> allowedToRemove(child, true)) + .collect(Collectors.toList()); + + if (allowedToRemove(node, false)) removed.add(node); db.removeNodes(removed); return removed; @@ -488,14 +491,34 @@ public class NodeRepository extends AbstractComponent { } } - private boolean allowedToRemove(Node nodeToRemove) { - List 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); + /** + * 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 allowedToRemove(Node nodeToRemove, boolean deletingAsChild) { + 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())); + } + + } else if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) { + List 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("%s can only be removed from following states: %s", - nodeToRemove.hostname(), legalStates.stream().map(Node.State::name).collect(Collectors.joining(", ")))); + 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 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/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java index f7168e132f2..fe3f12b105e 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 @@ -70,23 +70,20 @@ public class NodeRepositoryTest { } @Test - public void only_allow_docker_containers_remove_in_provisioned_or_ready() { + public void only_allow_docker_containers_remove_in_ready() { NodeRepositoryTester tester = new NodeRepositoryTester(); 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"); + 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("host2"); - tester.nodeRepository().setReady("host2"); - tester.nodeRepository().removeRecursively("host2"); + tester.nodeRepository().setDirty("host1"); + tester.nodeRepository().setReady("host1"); + tester.nodeRepository().removeRecursively("host1"); } @Test @@ -105,7 +102,7 @@ public class NodeRepositoryTest { try { tester.nodeRepository().removeRecursively("host1"); - fail("Should not be able to delete host node, one of the children are in state dirty"); + fail("Should not be able to delete host node, one of the children is in state dirty"); } catch (IllegalArgumentException ignored) { // Expected } @@ -117,7 +114,7 @@ public class NodeRepositoryTest { // 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 + tester.nodeRepository().removeRecursively("node11"); // Remove one of the children first instead assertEquals(3, tester.nodeRepository().getNodes().size()); tester.nodeRepository().removeRecursively("host1"); 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 40ae1e1c2c5..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\":\"Failed to delete host2.yahoo.com: host2.yahoo.com can only be removed from following states: provisioned, failed, parked\"}"); + 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", -- cgit v1.2.3 From 861004271aee7457466086d96ea00e715b7751b7 Mon Sep 17 00:00:00 2001 From: valerijf Date: Wed, 30 Aug 2017 13:44:37 +0200 Subject: Code review fixes --- .../admin/noderepository/NodeRepositoryImpl.java | 1 + .../vespa/hosted/provision/NodeRepository.java | 39 ++++++++++++++++++---- .../provision/restapi/v2/NodesApiHandler.java | 19 ++--------- .../vespa/hosted/provision/NodeRepositoryTest.java | 3 +- 4 files changed, 39 insertions(+), 23 deletions(-) (limited to 'node-repository') diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java index 7d73d05ca36..83787bcd5c3 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/noderepository/NodeRepositoryImpl.java @@ -175,6 +175,7 @@ public class NodeRepositoryImpl implements NodeRepository { NodeMessageResponse.class); NODE_ADMIN_LOGGER.info(response.message); + System.out.println(response.message); if (response.errorCode == null || response.errorCode.isEmpty()) { return; } 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 b36921d37ac..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,6 +467,29 @@ 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 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 all the nodes that are children of hostname before finally removing the hostname itself. * @@ -474,15 +497,18 @@ public class NodeRepository extends AbstractComponent { */ public List removeRecursively(String hostname) { Node node = getNode(hostname).orElseThrow(() -> new NotFoundException("No node with hostname \"" + hostname + '"')); + return removeRecursively(node, false); + } + private List removeRecursively(Node node, boolean force) { try (Mutex lock = lockUnallocated()) { List removed = node.type() != NodeType.host ? new ArrayList<>() : - getChildNodes(hostname).stream() - .filter(child -> allowedToRemove(child, true)) + getChildNodes(node.hostname()).stream() + .filter(child -> force || verifyRemovalIsAllowed(child, true)) .collect(Collectors.toList()); - if (allowedToRemove(node, false)) removed.add(node); + if (force || verifyRemovalIsAllowed(node, false)) removed.add(node); db.removeNodes(removed); return removed; @@ -498,14 +524,15 @@ public class NodeRepository extends AbstractComponent { * 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 allowedToRemove(Node nodeToRemove, boolean deletingAsChild) { - if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && !deletingAsChild) { + 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())); } - } else if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) { + } else */ if (nodeToRemove.flavor().getType() == Flavor.Type.DOCKER_CONTAINER) { List legalStates = Arrays.asList(Node.State.provisioned, Node.State.failed, Node.State.parked, Node.State.ready); if (! legalStates.contains(nodeToRemove.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 1cb997b48fb..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,23 +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 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()) { - List removedNodes = nodeRepository.removeRecursively(hostname); - return new MessageResponse("Removed " + removedNodes.stream().map(Node::hostname).collect(Collectors.joining(", "))); - } else { - return new MessageResponse("Moved " + hostname + " to ready"); - } + List 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 + "'"); 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 fe3f12b105e..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; @@ -69,7 +70,7 @@ public class NodeRepositoryTest { assertTrue(tester.nodeRepository().dynamicAllocationEnabled()); } - @Test + @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); -- cgit v1.2.3