Simplify since we don't have node locks

1 files changed, 25 insertions, 21 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Rebalancer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Rebalancer.java
index 0d105cd5fd8..ffc4461af51 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Rebalancer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Rebalancer.java
@@ -113,14 +113,18 @@ public class Rebalancer extends Maintainer {
         return bestMove;
     }
 
-    private boolean markWantToRetire(Node node, boolean wantToRetire, Mutex lock) {
-        Optional<Node> nodeToMove = nodeRepository().getNode(node.hostname());
-        if (nodeToMove.isEmpty()) return false;
-        if (nodeToMove.get().state() != Node.State.active) return false;
+    /** Returns true only if this operation changes the state of the wantToRetire flag */
+    private boolean markWantToRetire(Node node, boolean wantToRetire) {
+        try (Mutex lock = nodeRepository().lock(node)) {
+            Optional<Node> nodeToMove = nodeRepository().getNode(node.hostname());
+            if (nodeToMove.isEmpty()) return false;
+            if (nodeToMove.get().state() != Node.State.active) return false;
+
+            if (node.status().wantToRetire() == wantToRetire) return false;
 
-        if (node.status().wantToRetire() != wantToRetire)
             nodeRepository().write(nodeToMove.get().withWantToRetire(wantToRetire, Agent.system, clock.instant()), lock);
-        return true;
+            return true;
+        }
     }
 
     /**
@@ -133,21 +137,21 @@ public class Rebalancer extends Maintainer {
         ApplicationId application = move.node.allocation().get().owner();
         try (MaintenanceDeployment deployment = new MaintenanceDeployment(application, deployer, nodeRepository())) {
             if ( ! deployment.isValid()) return false;
-            try (Mutex lock = nodeRepository().lock(move.node)) {
-                boolean couldMarkRetired = markWantToRetire(move.node, true, lock);
-                if ( ! couldMarkRetired) return false;
-                try {
-                    if ( ! deployment.prepare()) return false;
-                    if (nodeRepository().getNodes(application, Node.State.reserved).stream().noneMatch(node -> node.hasParent(move.toHost.hostname())))
-                        return false; // Deployment is not moving the from node to the target we identified for some reason
-                    if ( ! deployment.activate()) return false;
-
-                    log.info("Rebalancer redeployed " + application + " to " + move);
-                    return true;
-                }
-                finally {
-                    markWantToRetire(move.node, false, lock); // In case this failed, noop otherwise
-                }
+
+            boolean couldMarkRetiredNow = markWantToRetire(move.node, true);
+            if ( ! couldMarkRetiredNow) return false;
+
+            try {
+                if ( ! deployment.prepare()) return false;
+                if (nodeRepository().getNodes(application, Node.State.reserved).stream().noneMatch(node -> node.hasParent(move.toHost.hostname())))
+                    return false; // Deployment is not moving the from node to the target we identified for some reason
+                if ( ! deployment.activate()) return false;
+
+                log.info("Rebalancer redeployed " + application + " to " + move);
+                return true;
+            }
+            finally {
+                markWantToRetire(move.node, false); // Necessary if this failed, no-op otherwise
             }
         }
     }