Merge pull request #18802 from vespa-engine/mpolden/fix-capacity-check

Always account for available IP addresses when checking capacity

6 files changed, 65 insertions, 36 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 29dcfddd821..54e9886d2b7 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
@@ -56,7 +56,7 @@ public abstract class NodeMover<MOVE> extends NodeRepositoryMaintainer {
             for (Node toHost : allNodes.matching(nodeRepository().nodes()::canAllocateTenantNodeTo)) {
                 if (toHost.hostname().equals(node.parentHostname().get())) continue;
                 if (spares.contains(toHost)) continue; // Do not offer spares as a valid move as they are reserved for replacement of failed nodes
-                if ( ! capacity.freeCapacityOf(toHost).satisfies(node.resources())) continue;
+                if ( ! capacity.hasCapacity(toHost, node.resources())) continue;
 
                 MOVE suggestedMove = suggestedMove(node, allNodes.parentOf(node).get(), toHost, allNodes);
                 bestMove = bestMoveOf(bestMove, suggestedMove);
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 7bb748c92c9..50bb213ad73 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
@@ -69,20 +69,20 @@ public class Rebalancer extends NodeMover<Rebalancer.Move> {
         int hostCount = 0;
         for (Node host : allNodes.nodeType(NodeType.host).state(Node.State.active)) {
             hostCount++;
-            totalSkew += Node.skew(host.flavor().resources(), capacity.freeCapacityOf(host));
+            totalSkew += Node.skew(host.flavor().resources(), capacity.unusedCapacityOf(host));
         }
         metric.set("hostedVespa.docker.skew", totalSkew/hostCount, null);
     }
 
     private double skewReductionByRemoving(Node node, Node fromHost, HostCapacity capacity) {
-        NodeResources freeHostCapacity = capacity.freeCapacityOf(fromHost);
+        NodeResources freeHostCapacity = capacity.unusedCapacityOf(fromHost);
         double skewBefore = Node.skew(fromHost.flavor().resources(), freeHostCapacity);
         double skewAfter = Node.skew(fromHost.flavor().resources(), freeHostCapacity.add(node.flavor().resources().justNumbers()));
         return skewBefore - skewAfter;
     }
 
     private double skewReductionByAdding(Node node, Node toHost, HostCapacity capacity) {
-        NodeResources freeHostCapacity = capacity.freeCapacityOf(toHost);
+        NodeResources freeHostCapacity = capacity.unusedCapacityOf(toHost);
         double skewBefore = Node.skew(toHost.flavor().resources(), freeHostCapacity);
         double skewAfter = Node.skew(toHost.flavor().resources(), freeHostCapacity.subtract(node.resources().justNumbers()));
         return skewBefore - skewAfter;
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 0589571e9d8..2447503515a 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
@@ -220,7 +220,7 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
                 return null;
 
             if ( ! host.resources().satisfies(node.resources())) return null;
-            NodeResources freeCapacity = freeCapacityWith(movesMade, host);
+            NodeResources freeCapacity = availableCapacityWith(movesMade, host);
             if (freeCapacity.satisfies(node.resources())) return List.of();
 
             List<Move> shortest = null;
@@ -291,8 +291,8 @@ public class SpareCapacityMaintainer extends NodeRepositoryMaintainer {
             return list;
         }
 
-        private NodeResources freeCapacityWith(List<Move> moves, Node host) {
-            NodeResources resources = hostCapacity.freeCapacityOf(host);
+        private NodeResources availableCapacityWith(List<Move> moves, Node host) {
+            NodeResources resources = hostCapacity.availableCapacityOf(host);
             for (Move move : moves) {
                 if ( ! move.toHost().equals(host)) continue;
                 resources = resources.subtract(move.node().resources());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacity.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacity.java
index 58a7aa2b189..9109f9fcf72 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacity.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacity.java
@@ -62,7 +62,7 @@ public class HostCapacity {
     }
 
     private int compareWithoutInactive(Node a, Node b) {
-        int result = compare(freeCapacityOf(b, true), freeCapacityOf(a, true));
+        int result = compare(availableCapacityOf(b, true, false), availableCapacityOf(a, true, false));
         if (result != 0) return result;
 
         // If resources are equal we want to assign to the one with the most IP addresses free
@@ -73,33 +73,41 @@ public class HostCapacity {
         return NodeResourceComparator.defaultOrder().compare(a, b);
     }
 
-    /** Returns whether host has room for requested capacity */
-    boolean hasCapacity(Node host, NodeResources requestedCapacity) {
-        return freeCapacityOf(host, false).satisfies(requestedCapacity) && freeIps(host) > 0;
+    /** Returns whether host can allocate a node with requested capacity */
+    public boolean hasCapacity(Node host, NodeResources requestedCapacity) {
+        return availableCapacityOf(host).satisfies(requestedCapacity);
     }
 
     /** Returns the number of available IP addresses on given host */
     int freeIps(Node host) {
         if (host.type() == NodeType.host) {
             return host.ipConfig().pool().eventuallyUnusedAddressCount(allNodes);
-        } else {
-            return host.ipConfig().pool().findUnusedIpAddresses(allNodes).size();
         }
+        return host.ipConfig().pool().findUnusedIpAddresses(allNodes).size();
+    }
+
+    /** Returns the capacity of given host that is both free and usable */
+    public NodeResources availableCapacityOf(Node host) {
+        return availableCapacityOf(host, false, true);
     }
 
     /**
-     * Calculate the remaining capacity of a host.
+     * Calculate the unused capacity of given host.
+     *
+     * Note that unlike {@link this#availableCapacityOf(Node)}, this only considers resources and returns any unused
+     * capacity even if the host does not have available IP addresses.
      *
-     * @param host the host to find free capacity of.
-     * @return a default (empty) capacity if not host, otherwise the free/unallocated/rest capacity
+     * @param host the host to find free capacity of
+     * @return a default (empty) capacity if not host, otherwise the free capacity
      */
-    public NodeResources freeCapacityOf(Node host) {
-        return freeCapacityOf(host, false);
+    public NodeResources unusedCapacityOf(Node host) {
+        return availableCapacityOf(host, false, false);
     }
 
-    NodeResources freeCapacityOf(Node host, boolean excludeInactive) {
+    private NodeResources availableCapacityOf(Node host, boolean excludeInactive, boolean requireIps) {
         // Only hosts have free capacity
-        if ( ! host.type().canRun(NodeType.tenant)) return new NodeResources(0, 0, 0, 0);
+        if ( ! host.type().canRun(NodeType.tenant)) return NodeResources.zero();
+        if (   requireIps && freeIps(host) == 0) return NodeResources.zero();
 
         NodeResources hostResources = hostResourcesCalculator.advertisedResourcesOf(host.flavor());
         return allNodes.childrenOf(host).asList().stream()
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
index 6568046991b..bfc848d4031 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
@@ -94,8 +94,10 @@ public class NodePrioritizer {
     /** Returns the list of nodes sorted by {@link NodeCandidate#compareTo(NodeCandidate)} */
     private List<NodeCandidate> prioritize() {
         // Group candidates by their switch hostname
-        Map<Optional<String>, List<NodeCandidate>> candidatesBySwitch = this.nodes.stream()
-                .collect(Collectors.groupingBy(candidate -> candidate.parent.orElseGet(candidate::toNode).switchHostname()));
+        Map<String, List<NodeCandidate>> candidatesBySwitch = this.nodes.stream()
+                .collect(Collectors.groupingBy(candidate -> candidate.parent.orElseGet(candidate::toNode)
+                                                                            .switchHostname()
+                                                                            .orElse("")));
         // Mark lower priority nodes on shared switch as non-exclusive
         List<NodeCandidate> nodes = new ArrayList<>(this.nodes.size());
         for (var clusterSwitch : candidatesBySwitch.keySet()) {
@@ -142,7 +144,7 @@ public class NodePrioritizer {
             if ( ! capacity.hasCapacity(host, requestedNodes.resources().get())) continue;
             if ( ! allNodes.childrenOf(host).owner(application).cluster(clusterSpec.id()).isEmpty()) continue;
             nodes.add(NodeCandidate.createNewChild(requestedNodes.resources().get(),
-                                                   capacity.freeCapacityOf(host, false),
+                                                   capacity.availableCapacityOf(host),
                                                    host,
                                                    spareHosts.contains(host),
                                                    allNodes,
@@ -179,14 +181,14 @@ public class NodePrioritizer {
         Optional<Node> parent = allNodes.parentOf(node);
         if (parent.isPresent()) {
             return NodeCandidate.createChild(node,
-                                             capacity.freeCapacityOf(parent.get(), false),
+                                             capacity.availableCapacityOf(parent.get()),
                                              parent.get(),
                                              spareHosts.contains(parent.get()),
                                              isSurplus,
                                              false,
                                              parent.get().exclusiveToApplicationId().isEmpty()
                                              && requestedNodes.canResize(node.resources(),
-                                                                         capacity.freeCapacityOf(parent.get(), false),
+                                                                         capacity.availableCapacityOf(parent.get()),
                                                                          topologyChange,
                                                                          currentClusterSize));
         }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacityTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacityTest.java
index 995d25043f3..d03a238f64f 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacityTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/HostCapacityTest.java
@@ -34,7 +34,10 @@ public class HostCapacityTest {
     private final HostResourcesCalculator hostResourcesCalculator = mock(HostResourcesCalculator.class);
     private HostCapacity capacity;
     private List<Node> nodes;
-    private Node host1, host2, host3;
+    private Node host1;
+    private Node host2;
+    private Node host3;
+    private Node host4;
     private final NodeResources resources0 = new NodeResources(1, 30, 20, 1.5);
     private final NodeResources resources1 = new NodeResources(2, 40, 40, 0.5);
 
@@ -45,10 +48,11 @@ public class HostCapacityTest {
         // Create flavors
         NodeFlavors nodeFlavors = FlavorConfigBuilder.createDummies("host", "docker", "docker2");
 
-        // Create three hosts
+        // Create hosts
         host1 = Node.create("host1", IP.Config.of(Set.of("::1"), createIps(2, 4), List.of()), "host1", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build();
         host2 = Node.create("host2", IP.Config.of(Set.of("::11"), createIps(12, 3), List.of()), "host2", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build();
-        host3 = Node.create("host3", IP.Config.of(Set.of("::21"), createIps(22, 1), List.of()), "host3", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build();
+        host3 = Node.create("host3", IP.Config.of(Set.of("::21"), createIps(22, 2), List.of()), "host3", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build();
+        host4 = Node.create("host3", IP.Config.of(Set.of("::21"), createIps(50, 0), List.of()), "host4", nodeFlavors.getFlavorOrThrow("host"), NodeType.host).build();
 
         // Add two containers to host1
         var nodeA = Node.reserve(Set.of("::2"), "nodeA", "host1", resources0, NodeType.tenant).build();
@@ -72,8 +76,10 @@ public class HostCapacityTest {
         assertTrue(capacity.hasCapacity(host1, resources1));
         assertTrue(capacity.hasCapacity(host2, resources0));
         assertTrue(capacity.hasCapacity(host2, resources1));
-        assertFalse(capacity.hasCapacity(host3, resources0));  // No ip available
-        assertFalse(capacity.hasCapacity(host3, resources1)); // No ip available
+        assertTrue(capacity.hasCapacity(host3, resources0));
+        assertTrue(capacity.hasCapacity(host3, resources1));
+        assertFalse(capacity.hasCapacity(host4, resources0));  // No IPs available
+        assertFalse(capacity.hasCapacity(host4, resources1)); // No IPs available
 
         // Add a new node to host1 to deplete the memory resource
         Node nodeF = Node.reserve(Set.of("::6"), "nodeF", "host1", resources0, NodeType.tenant).build();
@@ -87,15 +93,18 @@ public class HostCapacityTest {
     public void freeIPs() {
         assertEquals(2, capacity.freeIps(host1));
         assertEquals(1, capacity.freeIps(host2));
-        assertEquals(0, capacity.freeIps(host3));
+        assertEquals(1, capacity.freeIps(host3));
+        assertEquals(0, capacity.freeIps(host4));
     }
 
     @Test
-    public void freeCapacityOf() {
+    public void unusedCapacityOf() {
         assertEquals(new NodeResources(5, 40, 80, 2, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
-                     capacity.freeCapacityOf(host1, false));
+                     capacity.unusedCapacityOf(host1));
         assertEquals(new NodeResources(5, 60, 80, 4.5, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
-                     capacity.freeCapacityOf(host3, false));
+                     capacity.unusedCapacityOf(host3));
+        assertEquals(new NodeResources(7, 100, 120, 5, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
+                     capacity.unusedCapacityOf(host4));
 
         doAnswer(invocation -> {
             NodeResources totalHostResources = ((Flavor) invocation.getArguments()[0]).resources();
@@ -103,9 +112,19 @@ public class HostCapacityTest {
         }).when(hostResourcesCalculator).advertisedResourcesOf(any());
 
         assertEquals(new NodeResources(4, 38, 77, 1.5, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
-                     capacity.freeCapacityOf(host1, false));
+                     capacity.unusedCapacityOf(host1));
         assertEquals(new NodeResources(4, 58, 77, 4, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
-                     capacity.freeCapacityOf(host3, false));
+                     capacity.unusedCapacityOf(host3));
+    }
+
+    @Test
+    public void availableCapacityOf() {
+        assertEquals(new NodeResources(5, 40, 80, 2, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
+                     capacity.availableCapacityOf(host1));
+        assertEquals(new NodeResources(5, 60, 80, 4.5, NodeResources.DiskSpeed.fast, NodeResources.StorageType.remote),
+                     capacity.availableCapacityOf(host3));
+        assertEquals(NodeResources.zero(),
+                     capacity.availableCapacityOf(host4));
     }
 
     @Test