More tests

3 files changed, 87 insertions, 5 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
index 1c078dbd0c4..4d21a92d10c 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/CapacityChecker.java
@@ -6,6 +6,7 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeRepository;
 import com.yahoo.vespa.hosted.provision.node.Allocation;
+import com.yahoo.vespa.hosted.provision.provisioning.NodeResourceComparator;
 
 import java.util.*;
 import java.util.function.Function;
@@ -95,7 +96,8 @@ public class CapacityChecker {
         if (hosts.size() == 0) return Optional.empty();
 
         List<Node> parentRemovalPriorityList = heuristic.entrySet().stream()
-                                                        .sorted(Comparator.comparingInt(Map.Entry::getValue))
+                                                        .sorted(this::hostMitigationOrder)
+//                                                        .sorted(Comparator.comparingInt(Map.Entry::getValue))
                                                         .map(Map.Entry::getKey)
                                                         .collect(Collectors.toList());
 
@@ -113,6 +115,13 @@ public class CapacityChecker {
         throw new IllegalStateException("No path to failure found. This should be impossible!");
     }
 
+    private int hostMitigationOrder(Map.Entry<Node, Integer> entry1, Map.Entry<Node, Integer> entry2) {
+        int result = Integer.compare(entry1.getValue(), entry2.getValue());
+        if (result != 0) return result;
+        // Mitigate the largest hosts first
+        return NodeResourceComparator.defaultOrder().compare(entry2.getKey().resources(), entry1.getKey().resources());
+    }
+
     private Map<String, Node> constructHostnameToNodeMap(List<Node> nodes) {
         return nodes.stream().collect(Collectors.toMap(Node::hostname, n -> n));
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainer.java
index a396bbda5c9..4f17a0f624b 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainer.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.provision.maintenance;
 
 import com.yahoo.config.provision.Deployer;
 import com.yahoo.config.provision.NodeResources;
-import com.yahoo.config.provision.NodeType;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.NodeList;
@@ -92,6 +91,7 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
     }
 
     private Move moveTowardsSpareFor(Node node) {
+        System.out.println("Trying to find mitigation for " + node);
         NodeList allNodes = nodeRepository().list();
         // Allocation will assign the two most empty nodes as "spares", which will not be allocated on
         // unless needed for node failing. Our goal here is to make room on these spares for the given node
@@ -108,6 +108,7 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
                 shortestMitigation = mitigation;
         }
         if (shortestMitigation == null || shortestMitigation.isEmpty()) return Move.empty();
+        System.out.println("Shortest mitigation to create spare for " + node + ":\n  " + shortestMitigation);
         return shortestMitigation.get(0);
     }
 
@@ -137,11 +138,12 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
             if (movesLeft == 0) return null;
 
             List<Move> shortest = null;
-            for (var i = Subsets(hostCapacity.allNodes().childrenOf(host), movesLeft); i.hasNext(); ) {
+            for (var i = subsets(hostCapacity.allNodes().childrenOf(host), movesLeft); i.hasNext(); ) {
                 List<Node> childrenToMove = i.next();
                 if ( ! addResourcesOf(childrenToMove, freeCapacity).satisfies(node.resources())) continue;
                 List<Move> moves = move(childrenToMove, host, hosts, movesMade, movesLeft);
                 if (moves == null) continue;
+
                 if (shortest == null || moves.size() < shortest.size())
                     shortest = moves;
             }
@@ -165,6 +167,7 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
         private List<Move> move(Node node, Node host, List<Node> hosts, List<Move> movesMade, int movesLeft) {
             List<Move> shortest = null;
             for (Node target : hosts) {
+                if (target.equals(host)) continue;
                 List<Move> childMoves = makeRoomFor(node, target, hosts, movesMade, movesLeft - 1);
                 if (childMoves == null) continue;
                 if (shortest == null || shortest.size() > childMoves.size() + 1) {
@@ -181,7 +184,7 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
             return resources;
         }
 
-        private Iterator<List<Node>> Subsets(NodeList nodes, int maxLength) {
+        private Iterator<List<Node>> subsets(NodeList nodes, int maxLength) {
             return new SubsetIterator(nodes.asList(), maxLength);
         }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java
index 7d7b3910cdb..8ee8565410e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/SpareCapacityMaintainerTest.java
@@ -95,7 +95,7 @@ public class SpareCapacityMaintainerTest {
     }
 
     @Test
-    public void testMoveIsNeededToHaveSpares() {
+    public void testMoveIsNeeded() {
         // Moving application id 1 and 2 to the same nodes frees up spares for application 0
         var tester = new SpareCapacityMaintainerTester();
         tester.addHosts(6, new NodeResources(10, 100, 1000, 1));
@@ -114,6 +114,76 @@ public class SpareCapacityMaintainerTest {
         assertEquals(1, tester.metric.values.get("spareHostCapacity"));
     }
 
+    @Test
+    public void testMultipleMovesAreNeeded() {
+        // Moving application id 1 and 2 to the same nodes frees up spares for application 0
+        // so that it can be moved from size 12 to size 10 hosts, clearing up spare room for the size 12 application
+        var tester = new SpareCapacityMaintainerTester();
+        tester.addHosts(4, new NodeResources(12, 120, 1200, 1.2));
+        tester.addHosts(4, new NodeResources(10, 100, 1000, 1));
+        tester.addNodes(0, 2, new NodeResources(10, 100, 1000, 1.0), 0);
+        tester.addNodes(1, 2, new NodeResources(12, 120, 1200, 1.2), 2);
+        tester.addNodes(2, 2, new NodeResources(5, 50, 500, 0.5), 4);
+        tester.addNodes(3, 2, new NodeResources(5, 50, 500, 0.5), 6);
+        tester.maintainer.maintain();
+        assertEquals(1, tester.deployer.redeployments);
+        assertEquals(1, tester.nodeRepository.list().retired().size());
+        assertEquals(1, tester.metric.values.get("spareHostCapacity"));
+    }
+
+    @Test
+    public void testMultipleNodesMustMoveFromOneHost() {
+        // By moving the 4 small nodes from host 2 we free up sufficient space on the third host to act as a spare for
+        // application 0
+        var tester = new SpareCapacityMaintainerTester();
+
+        tester.addHosts(2, new NodeResources(10, 100, 1000, 1));
+        tester.addNodes(0, 2, new NodeResources(10, 100, 1000, 1.0), 0);
+
+        tester.addHosts(1, new NodeResources(16, 160, 1600, 1.6));
+        tester.addNodes(1, 1, new NodeResources(1, 10, 100, 0.1), 2);
+        tester.addNodes(2, 1, new NodeResources(1, 10, 100, 0.1), 2);
+        tester.addNodes(3, 1, new NodeResources(1, 10, 100, 0.1), 2);
+        tester.addNodes(4, 1, new NodeResources(1, 10, 100, 0.1), 2);
+        tester.addNodes(5, 1, new NodeResources(2, 20, 200, 2.0), 2);
+        tester.addNodes(6, 1, new NodeResources(2, 20, 200, 2.0), 2);
+        tester.addNodes(7, 1, new NodeResources(2, 20, 200, 2.0), 2);
+
+        tester.addHosts(5, new NodeResources(2, 20, 200, 2.0));
+
+        tester.maintainer.maintain();
+        assertEquals(1, tester.deployer.redeployments);
+        assertEquals(1, tester.nodeRepository.list().retired().size());
+        assertEquals(1, tester.metric.values.get("spareHostCapacity"));
+    }
+
+    @Test
+    public void testTooManyMovesAreNeeded() {
+        // 6 nodes must move to the next host, which is more than the max limit
+        var tester = new SpareCapacityMaintainerTester();
+
+        tester.addHosts(2, new NodeResources(10, 100, 1000, 1));
+        tester.addHosts(1, new NodeResources(9, 90, 900, 0.9));
+        tester.addHosts(1, new NodeResources(8, 80, 800, 0.8));
+        tester.addHosts(1, new NodeResources(7, 70, 700, 0.7));
+        tester.addHosts(1, new NodeResources(6, 60, 600, 0.6));
+        tester.addHosts(1, new NodeResources(5, 50, 500, 0.5));
+        tester.addHosts(1, new NodeResources(4, 40, 400, 0.4));
+
+        tester.addNodes(0, 1, new NodeResources(10, 100, 1000, 1.0), 0);
+        tester.addNodes(1, 1, new NodeResources( 9, 90, 900, 0.9), 1);
+        tester.addNodes(2, 1, new NodeResources( 8, 80, 800, 0.8), 2);
+        tester.addNodes(3, 1, new NodeResources( 7, 70, 700, 0.7), 3);
+        tester.addNodes(4, 1, new NodeResources( 6, 60, 600, 0.6), 4);
+        tester.addNodes(5, 1, new NodeResources( 5, 50, 500, 0.5), 5);
+        tester.addNodes(6, 1, new NodeResources( 4, 40, 400, 0.4), 6);
+
+        tester.maintainer.maintain();
+        assertEquals(0, tester.deployer.redeployments);
+        assertEquals(0, tester.nodeRepository.list().retired().size());
+        assertEquals(0, tester.metric.values.get("spareHostCapacity"));
+    }
+
     private static class SpareCapacityMaintainerTester {
 
         NodeRepository nodeRepository;