Merge with master

9 files changed, 48 insertions, 46 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 1885b54e9c0..68bdffd75f9 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
@@ -25,6 +25,7 @@ import com.yahoo.vespa.hosted.provision.node.filter.StateFilter;
 import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient;
 import com.yahoo.vespa.hosted.provision.persistence.DnsNameResolver;
 import com.yahoo.vespa.hosted.provision.persistence.NameResolver;
+import com.yahoo.vespa.hosted.provision.restapi.v2.NotFoundException;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -468,24 +469,26 @@ public class NodeRepository extends AbstractComponent {
 
     /**
      * Removes a node. A node must be in a legal state before it can be removed.
-     *
-     * @return true if the node was removed, false if it was not found in one of the legal states
      */
-    public boolean remove(String hostname) {
-
-        Node.State[] legalStates = {Node.State.provisioned, Node.State.failed, Node.State.parked};
-        Node.State[] legalDynamicStates = {Node.State.provisioned, Node.State.failed, Node.State.parked, Node.State.dirty};
-
-        Optional<Node> nodeToRemove = getNode(hostname, dynamicAllocationEnabled() ? legalDynamicStates : legalStates);
-        if ( ! nodeToRemove.isPresent()) return false;
+    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(", ")));
+        }
 
-        // Only docker nodes are allowed to be deleted in state dirty.
-        if ( nodeToRemove.get().state().equals(Node.State.dirty)) {
-            if (!(nodeToRemove.get().flavor().getType().equals(Flavor.Type.DOCKER_CONTAINER))) return false;
+        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");
+            }
         }
 
-        try (Mutex lock = lock(nodeToRemove.get())) {
-            return db.removeNode(nodeToRemove.get().state(), hostname);
+        try (Mutex lock = lock(nodeToRemove)) {
+            db.removeNode(nodeToRemove.state(), hostname);
         }
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
index c8f64a65415..8f7317b28eb 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
@@ -112,7 +112,7 @@ public class History {
 
         public enum Type { 
             // State move events
-            readied, reserved, activated, deactivated, deallocated, parked,
+            provisioned(false), readied, reserved, activated, deactivated, deallocated, parked,
             // The active node was retired
             retired,
             // The active node went down according to the service monitor
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 cd63599fed6..651ee44415f 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
@@ -108,16 +108,14 @@ public class CuratorDatabaseClient {
      *
      * @param state the current state of the node
      * @param hostName the host name of the node to remove
-     * @return true if the node was removed, false if it was not found
      */
-    public boolean removeNode(Node.State state, String hostName) {
+    public void removeNode(Node.State state, String hostName) {
         Path path = toPath(state, hostName);
         NestedTransaction transaction = new NestedTransaction();
         CuratorTransaction curatorTransaction = curatorDatabase.newCuratorTransactionIn(transaction);
         curatorTransaction.add(CuratorOperations.delete(path.getAbsolute()));
         transaction.commit();
         log.log(LogLevel.INFO, "Removed: " + state + " node " + hostName);
-        return true;
     }
 
     /**
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
index 8056bd787db..9393dc5ead4 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
@@ -257,6 +257,7 @@ public class NodeSerializer {
     /** Returns the event type, or null if this event type should be ignored */
     private History.Event.Type eventTypeFromString(String eventTypeString) {
         switch (eventTypeString) {
+            case "provisioned" : return History.Event.Type.provisioned;
             case "readied" : return History.Event.Type.readied;
             case "reserved" : return History.Event.Type.reserved;
             case "activated" : return History.Event.Type.activated;
@@ -273,6 +274,7 @@ public class NodeSerializer {
     }
     private String toString(History.Event.Type nodeEventType) {
         switch (nodeEventType) {
+            case provisioned : return "provisioned";
             case readied : return "readied";
             case reserved : return "reserved";
             case activated : return "activated";
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 594983be698..4e7bb1f7d16 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
@@ -135,10 +135,8 @@ public class NodesApiHandler extends LoggingRequestHandler {
              */
             String hostname = lastElement(path);
             if (nodeRepository.dynamicAllocationEnabled()) {
-                if (nodeRepository.remove(hostname))
-                    return new MessageResponse("Removed " + hostname);
-                else
-                    throw new NotFoundException("No node in the provisioned, parked, dirty or failed state with hostname " + hostname);
+                nodeRepository.remove(hostname);
+                return new MessageResponse("Removed " + hostname);
             } else {
                 nodeRepository.setReady(hostname);
                 return new MessageResponse("Moved " + hostname + " to ready");
@@ -182,10 +180,8 @@ public class NodesApiHandler extends LoggingRequestHandler {
         String path = request.getUri().getPath();
         if (path.startsWith("/nodes/v2/node/")) {
             String hostname = lastElement(path);
-            if (nodeRepository.remove(hostname))
-                return new MessageResponse("Removed " + hostname);
-            else
-                throw new NotFoundException("No node in the provisioned, parked or failed state with hostname " + hostname);
+            nodeRepository.remove(hostname);
+            return new MessageResponse("Removed " + hostname);
         }
         else if (path.startsWith("/nodes/v2/maintenance/inactive/")) {
             return setActive(lastElement(path), true);
@@ -196,13 +192,8 @@ public class NodesApiHandler extends LoggingRequestHandler {
     }
 
     private Node nodeFromRequest(HttpRequest request) {
-        // TODO: The next 4 lines can be a oneliner when updateNodeAttribute is removed (as we won't allow path suffixes)
         String path = request.getUri().getPath();
-        String prefixString = "/nodes/v2/node/";
-        int beginIndex = path.indexOf(prefixString) + prefixString.length();
-        int endIndex = path.indexOf("/", beginIndex);
-        if (endIndex < 0) endIndex = path.length(); // path ends by ip
-        String hostname = path.substring(beginIndex, endIndex);
+        String hostname = path.substring(path.lastIndexOf("/"));
 
         return nodeRepository.getNode(hostname).orElseThrow(() ->
                 new NotFoundException("No node found with hostname " + hostname));
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 12c76638604..bcae9d293f6 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
@@ -13,6 +13,7 @@ import org.junit.Test;
 import java.nio.charset.StandardCharsets;
 import java.util.Optional;
 
+import static junit.framework.TestCase.fail;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -36,7 +37,7 @@ public class NodeRepositoryTest {
         assertEquals(3, tester.getNodes(NodeType.tenant).size());
         
         tester.nodeRepository().park("host2", Agent.system, "Parking to unit test");
-        assertTrue(tester.nodeRepository().remove("host2"));
+        tester.nodeRepository().remove("host2");
 
         assertEquals(2, tester.getNodes(NodeType.tenant).size());
     }
@@ -71,17 +72,18 @@ public class NodeRepositoryTest {
     @Test
     public void only_allow_to_delete_dirty_nodes_when_dynamic_allocation_feature_enabled() {
         NodeRepositoryTester tester = new NodeRepositoryTester();
-        try {
-            tester.addNode("id1", "host1", "default", NodeType.host);
-            tester.addNode("id2", "host2", "docker", NodeType.tenant);
-            tester.nodeRepository().setDirty("host2");
-
-            assertFalse(tester.nodeRepository().remove("host2"));
+        tester.addNode("id1", "host1", "default", NodeType.host);
+        tester.addNode("id2", "host2", "docker", NodeType.tenant);
+        tester.nodeRepository().setDirty("host2");
 
-            tester.curator().set(Path.fromString("/provision/v1/dynamicDockerAllocation"), new byte[0]);
-            assertTrue(tester.nodeRepository().remove("host2"));
-        } finally {
-            tester.curator().delete(Path.fromString("/provision/v1/dynamicDockerAllocation"));
+        try {
+            tester.nodeRepository().remove("host2");
+            fail("Should not be able to delete tenant node in state dirty");
+        } catch (IllegalArgumentException ignored) {
+            // Expected
         }
+
+        tester.curator().set(Path.fromString("/provision/v1/dynamicDockerAllocation"), new byte[0]);
+        tester.nodeRepository().remove("host2");
     }
 }
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 ee0b8f55a4b..cb46d0a4624 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
@@ -49,7 +49,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");
-        assertTrue(tester.nodeRepository().remove("host2"));
+        tester.nodeRepository().remove("host2");
         maintainer.maintain();
 
         assertEquals(2, tester.getNodes(NodeType.tenant).size());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
index 1b19b57317e..f91b4863eeb 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/SerializationTest.java
@@ -117,6 +117,10 @@ public class SerializationTest {
                 "   \"flavor\" : \"large\",\n" +
                 "   \"history\" : [\n" +
                 "      {\n" +
+                "         \"type\" : \"provisioned\",\n" +
+                "         \"at\" : 1444391401389\n" +
+                "      },\n" +
+                "      {\n" +
                 "         \"type\" : \"reserved\",\n" +
                 "         \"at\" : 1444391402611\n" +
                 "      }\n" +
@@ -143,6 +147,8 @@ public class SerializationTest {
         assertEquals(2, node.status().reboot().current());
         assertEquals(3, node.allocation().get().restartGeneration().wanted());
         assertEquals(4, node.allocation().get().restartGeneration().current());
+        assertEquals(Arrays.asList(History.Event.Type.provisioned, History.Event.Type.reserved),
+                node.history().events().stream().map(History.Event::type).collect(Collectors.toList()));
         assertTrue(node.allocation().get().isRemovable());
         assertEquals(NodeType.tenant, node.type());
     }
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 00595d5b833..565dfe0457e 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
@@ -176,7 +176,7 @@ public class RestApiTest {
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
                                    Utf8.toBytes("{\"currentRestartGeneration\": 1}"), Request.Method.PATCH),
                        "{\"message\":\"Updated host4.yahoo.com\"}");
-        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com/currentRebootGeneration",
+        assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
                                    Utf8.toBytes("{\"currentRebootGeneration\": 1}"), Request.Method.PATCH),
                        "{\"message\":\"Updated host4.yahoo.com\"}");
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",
@@ -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),
-                       404, "{\"error-code\":\"NOT_FOUND\",\"message\":\"No node in the provisioned, parked or failed state with hostname host2.yahoo.com\"}");
+                       400, "{\"error-code\":\"BAD_REQUEST\",\"message\":\"Can only remove node from following states: provisioned, failed, parked, dirty\"}");
 
         // PUT current restart generation with string instead of long
         assertResponse(new Request("http://localhost:8080/nodes/v2/node/host4.yahoo.com",