Always use transaction when removing nodes

3 files changed, 27 insertions, 24 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 7eb1744aa23..266855002ea 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
@@ -166,8 +166,10 @@ public class Nodes {
 
                 nodesToAdd.add(node);
             }
-            List<Node> resultingNodes = db.addNodesInState(IP.Config.verify(nodesToAdd, list(lock)), Node.State.provisioned, agent);
-            db.removeNodes(nodesToRemove);
+            NestedTransaction transaction = new NestedTransaction();
+            List<Node> resultingNodes = db.addNodesInState(IP.Config.verify(nodesToAdd, list(lock)), Node.State.provisioned, agent, transaction);
+            db.removeNodes(nodesToRemove, transaction);
+            transaction.commit();
             return resultingNodes;
         }
     }
@@ -481,19 +483,20 @@ public class Nodes {
     public List<Node> removeRecursively(Node node, boolean force) {
         try (Mutex lock = lockUnallocated()) {
             requireRemovable(node, false, force);
-            if (!node.type().isHost()) {
-                List<Node> removed = List.of(node);
-                db.removeNodes(removed);
-                return removed;
-            }
             NestedTransaction transaction = new NestedTransaction();
-            List<Node> removed = removeChildren(node, force, transaction);
-            if (zone.getCloud().dynamicProvisioning()) {
-                db.removeNodes(List.of(node), transaction);
+            final List<Node> removed;
+            if (!node.type().isHost()) {
+                removed = List.of(node);
+                db.removeNodes(removed, transaction);
             } else {
-                move(node.hostname(), Node.State.deprovisioned, Agent.system, false, Optional.empty(), transaction);
+                removed = removeChildren(node, force, transaction);
+                if (zone.getCloud().dynamicProvisioning()) {
+                    db.removeNodes(List.of(node), transaction);
+                } else {
+                    move(node.hostname(), Node.State.deprovisioned, Agent.system, false, Optional.empty(), transaction);
+                }
+                removed.add(node);
             }
-            removed.add(node);
             transaction.commit();
             return removed;
         }
@@ -503,7 +506,9 @@ public class Nodes {
     public void forget(Node node) {
         if (node.state() != Node.State.deprovisioned)
             throw new IllegalArgumentException(node + " must be deprovisioned before it can be forgotten");
-        db.removeNodes(List.of(node));
+        NestedTransaction transaction = new NestedTransaction();
+        db.removeNodes(List.of(node), transaction);
+        transaction.commit();
     }
 
     private List<Node> removeChildren(Node node, boolean force, NestedTransaction transaction) {
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 4899527597c..d1f881f8b7a 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
@@ -110,8 +110,7 @@ public class CuratorDatabaseClient {
     }
 
     /** Adds a set of nodes. Rollbacks/fails transaction if any node is not in the expected state. */
-    public List<Node> addNodesInState(List<Node> nodes, Node.State expectedState, Agent agent) {
-        NestedTransaction transaction = new NestedTransaction();
+    public List<Node> addNodesInState(List<Node> nodes, Node.State expectedState, Agent agent, NestedTransaction transaction) {
         CuratorTransaction curatorTransaction = db.newCuratorTransactionIn(transaction);
         for (Node node : nodes) {
             if (node.state() != expectedState)
@@ -120,7 +119,6 @@ public class CuratorDatabaseClient {
             node = node.with(node.history().recordStateTransition(null, expectedState, agent, clock.instant()));
             curatorTransaction.add(CuratorOperations.create(toPath(node).getAbsolute(), nodeSerializer.toJson(node)));
         }
-        transaction.commit();
 
         for (Node node : nodes)
             log.log(Level.INFO, "Added " + node);
@@ -128,17 +126,14 @@ public class CuratorDatabaseClient {
         return nodes;
     }
 
-    /**
-     * Removes multiple nodes in a single transaction.
-     *
-     * @param nodes list of the nodes to remove
-     */
-    public void removeNodes(List<Node> nodes) {
+    public List<Node> addNodesInState(List<Node> nodes, Node.State expectedState, Agent agent) {
         NestedTransaction transaction = new NestedTransaction();
-        removeNodes(nodes, transaction);
+        List<Node> writtenNodes = addNodesInState(nodes, expectedState, agent, transaction);
         transaction.commit();
+        return writtenNodes;
     }
 
+    /** Removes given nodes in transaction */
     public void removeNodes(List<Node> nodes, NestedTransaction transaction) {
         for (Node node : nodes) {
             Path path = toPath(node.state(), node.hostname());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
index eb3bdff484d..bdc3bdfd816 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisionerTest.java
@@ -11,6 +11,7 @@ import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.exception.LoadBalancerServiceException;
+import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.flags.InMemoryFlagSource;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
@@ -175,7 +176,9 @@ public class LoadBalancerProvisionerTest {
 
         // Application is removed, nodes are deleted and load balancer is deactivated
         tester.remove(app1);
-        tester.nodeRepository().database().removeNodes(tester.nodeRepository().nodes().list().nodeType(NodeType.tenant).asList());
+        NestedTransaction transaction = new NestedTransaction();
+        tester.nodeRepository().database().removeNodes(tester.nodeRepository().nodes().list().nodeType(NodeType.tenant).asList(), transaction);
+        transaction.commit();
         assertTrue("Nodes are deleted", tester.nodeRepository().nodes().list().nodeType(NodeType.tenant).isEmpty());
         assertSame("Load balancer is deactivated", LoadBalancer.State.inactive, lb.get().state());