Merge pull request #16645 from vespa-engine/mpolden/shuffle-moves

Shuffle node moves

3 files changed, 14 insertions, 7 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMover.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMover.java
index 9fe9b7db478..29dcfddd821 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMover.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeMover.java
@@ -12,6 +12,7 @@ import com.yahoo.vespa.hosted.provision.provisioning.HostCapacity;
 
 import java.time.Duration;
 import java.time.Instant;
+import java.util.Random;
 import java.util.Set;
 
 /**
@@ -25,11 +26,13 @@ public abstract class NodeMover<MOVE> extends NodeRepositoryMaintainer {
 
     private final Deployer deployer;
     private final MOVE emptyMove;
+    private final Random random;
 
     public NodeMover(Deployer deployer, NodeRepository nodeRepository, Duration interval, Metric metric, MOVE emptyMove) {
         super(nodeRepository, interval, metric);
         this.deployer = deployer;
         this.emptyMove = emptyMove;
+        this.random = new Random(nodeRepository.clock().millis());
     }
 
     /** Returns a suggested move for given node */
@@ -39,7 +42,11 @@ public abstract class NodeMover<MOVE> extends NodeRepositoryMaintainer {
     protected final MOVE findBestMove(NodeList allNodes) {
         HostCapacity capacity = new HostCapacity(allNodes, nodeRepository().resourcesCalculator());
         MOVE bestMove = emptyMove;
-        NodeList activeNodes = allNodes.nodeType(NodeType.tenant).state(Node.State.active);
+        // Shuffle nodes so we did not get stuck if the chosen move is consistently discarded. Node moves happen through
+        // a soft request to retire (preferToRetire), which node allocation can disregard
+        NodeList activeNodes = allNodes.nodeType(NodeType.tenant)
+                                       .state(Node.State.active)
+                                       .shuffle(random);
         Set<Node> spares = capacity.findSpareHosts(allNodes.asList(), nodeRepository().spareCount());
         for (Node node : activeNodes) {
             if (node.parentHostname().isEmpty()) continue;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
index 042d227f8f1..d5d221cba7c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeAllocation.java
@@ -154,7 +154,7 @@ class NodeAllocation {
         if (violatesParentHostPolicy(candidate)) return true;
         if ( ! hasCompatibleFlavor(candidate)) return true;
         if (candidate.wantToRetire()) return true;
-        if (candidate.preferToRetire() && !candidate.replacementIncreasesSkew(candidates)) return true;
+        if (candidate.preferToRetire() && candidate.replacableBy(candidates)) return true;
         if (violatesExclusivity(candidate)) return true;
         return false;
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
index 297a8ed77e6..143ae8f99ad 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java
@@ -99,18 +99,18 @@ public abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidat
     /** Returns whether this node can - as far as we know - be used to run the application workload */
     public abstract boolean isValid();
 
-    /** Returns whether replacing this with any of the reserved candidates will increase skew */
-    public boolean replacementIncreasesSkew(List<NodeCandidate> candidates) {
+    /** Returns whether this can be replaced by any of the reserved candidates */
+    public boolean replacableBy(List<NodeCandidate> candidates) {
         return candidates.stream()
                          .filter(candidate -> candidate.state() == Node.State.reserved)
-                         .allMatch(reserved -> {
-                             int switchPriority = switchPriority(reserved);
+                         .anyMatch(candidate -> {
+                             int switchPriority = candidate.switchPriority(this);
                              if (switchPriority < 0) {
                                  return true;
                              } else if (switchPriority > 0) {
                                  return false;
                              }
-                             return hostPriority(reserved) < 0;
+                             return candidate.hostPriority(this) < 0;
                          });
     }