Read the node after taking the lock when moving to ready

2 files changed, 29 insertions, 36 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
index 230d646699d..084f1f5a9ef 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
@@ -189,29 +189,15 @@ public class Nodes {
         }
     }
 
-    /** Sets a list of nodes ready and returns the nodes in the ready state */
-    public List<Node> setReady(List<Node> nodes, Agent agent, String reason) {
-        try (Mutex lock = lockUnallocated()) {
-            List<Node> nodesWithResetFields = nodes.stream()
-                                                   .map(node -> {
-                                                       if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty)
-                                                           illegal("Can not set " + node + " ready. It is not provisioned or dirty.");
-                                                       if (node.status().wantToDeprovision()) return node; // Do not reset status if wantToDeprovision
-                                                       return node.withWantToRetire(false,
-                                                                                    false,
-                                                                                    false,
-                                                                                    Agent.system,
-                                                                                    clock.instant());
-                                                   })
-                                                   .collect(Collectors.toList());
-            return db.writeTo(Node.State.ready, nodesWithResetFields, agent, Optional.of(reason));
-        }
-    }
+    /** Sets a node to ready and returns the node in the ready state */
+    public Node setReady(NodeMutex nodeMutex, Agent agent, String reason) {
+        Node node = nodeMutex.node();
+        if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty)
+            illegal("Can not set " + node + " ready. It is not provisioned or dirty.");
+        if (!node.status().wantToDeprovision()) // Do not reset status if wantToDeprovision
+            node = node.withWantToRetire(false, false, false, agent, clock.instant());
 
-    public Node setReady(String hostname, Agent agent, String reason) {
-        Node nodeToReady = requireNode(hostname);
-        if (nodeToReady.state() == Node.State.ready) return nodeToReady;
-        return setReady(List.of(nodeToReady), agent, reason).get(0);
+        return db.writeTo(Node.State.ready, node, agent, Optional.of(reason));
     }
 
     /** Reserve nodes. This method does <b>not</b> lock the node repository. */
@@ -475,21 +461,27 @@ public class Nodes {
      * containers this will remove the node from node repository, otherwise the node will be moved to state ready.
      */
     public Node markNodeAvailableForNewAllocation(String hostname, Agent agent, String reason) {
-        Node node = requireNode(hostname);
-        if (node.flavor().getType() == Flavor.Type.DOCKER_CONTAINER && node.type() == NodeType.tenant) {
-            if (node.state() != Node.State.dirty)
-                illegal("Cannot make " + node  + " available for new allocation as it is not in state [dirty]");
-            return removeRecursively(node, true).get(0);
-        }
+        try (NodeMutex nodeMutex = lockAndGetRequired(hostname)) {
+            Node node = nodeMutex.node();
+            if (node.type() == NodeType.tenant) {
+                if (node.state() != Node.State.dirty)
+                    illegal("Cannot make " + node + " available for new allocation as it is not in state [dirty]");
+
+                NestedTransaction transaction = new NestedTransaction();
+                db.removeNodes(List.of(node), transaction);
+                transaction.commit();
+                return node;
+            }
 
-        if (node.state() == Node.State.ready) return node;
+            if (node.state() == Node.State.ready) return node;
 
-        Node parentHost = node.parentHostname().flatMap(this::node).orElse(node);
-        List<String> failureReasons = NodeFailer.reasonsToFailHost(parentHost);
-        if ( ! failureReasons.isEmpty())
-            illegal(node + " cannot be readied because it has hard failures: " + failureReasons);
+            Node parentHost = node.parentHostname().flatMap(this::node).orElse(node);
+            List<String> failureReasons = NodeFailer.reasonsToFailHost(parentHost);
+            if (!failureReasons.isEmpty())
+                illegal(node + " cannot be readied because it has hard failures: " + failureReasons);
 
-        return setReady(List.of(node), agent, reason).get(0);
+            return setReady(nodeMutex, agent, reason);
+        }
     }
 
     /**
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
index 26e1cd71a3d..dccfa830a8d 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockNodeRepository.java
@@ -23,6 +23,7 @@ import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.NodeMutex;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.applications.Application;
 import com.yahoo.vespa.hosted.provision.applications.Cluster;
@@ -148,7 +149,7 @@ public class MockNodeRepository extends NodeRepository {
         nodes.remove(node7);
         nodes.remove(node55);
         nodes = nodes().deallocate(nodes, Agent.system, getClass().getSimpleName());
-        nodes().setReady(nodes, Agent.system, getClass().getSimpleName());
+        nodes.forEach(node -> nodes().setReady(new NodeMutex(node, () -> {}), Agent.system, getClass().getSimpleName()));
 
         nodes().fail(node5.hostname(), Agent.system, getClass().getSimpleName());
         nodes().deallocateRecursively(node55.hostname(), Agent.system, getClass().getSimpleName());
@@ -195,7 +196,7 @@ public class MockNodeRepository extends NodeRepository {
         largeNodes.add(Node.create("node13", ipConfig(13), "host13.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).build());
         largeNodes.add(Node.create("node14", ipConfig(14), "host14.yahoo.com", resources(10, 48, 500, 1, fast, local), NodeType.tenant).build());
         nodes().addNodes(largeNodes, Agent.system);
-        nodes().setReady(largeNodes, Agent.system, getClass().getSimpleName());
+        largeNodes.forEach(node -> nodes().setReady(new NodeMutex(node, () -> {}), Agent.system, getClass().getSimpleName()));
         ApplicationId app4 = ApplicationId.from(TenantName.from("tenant4"), ApplicationName.from("application4"), InstanceName.from("instance4"));
         ClusterSpec cluster4 = ClusterSpec.request(ClusterSpec.Type.container, ClusterSpec.Id.from("id4")).vespaVersion("6.42").build();
         activate(provisioner.prepare(app4, cluster4, Capacity.from(new ClusterResources(2, 1, new NodeResources(10, 48, 500, 1)), false, true), null), app4, provisioner);