Merge pull request #17061 from vespa-engine/mpolden/require-host-ip

Require primary IP addresses for ready hosts

14 files changed, 36 insertions, 27 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index 13cb998bb68..5135ae73ea1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -96,8 +96,13 @@ public final class Node implements Nodelike {
         if (state == State.active)
             requireNonEmpty(ipConfig.primary(), "Active node " + hostname + " must have at least one valid IP address");
 
+        if (state == State.ready && type.isHost()) {
+            requireNonEmpty(ipConfig.primary(), "A " + type + " must have at least one primary IP address in state " + state);
+            requireNonEmpty(ipConfig.pool().ipSet(), "A " + type + " must have a non-empty IP address pool in state " + state);
+        }
+
         if (parentHostname.isPresent()) {
-            if (!ipConfig.pool().getIpSet().isEmpty()) throw new IllegalArgumentException("A child node cannot have an IP address pool");
+            if (!ipConfig.pool().ipSet().isEmpty()) throw new IllegalArgumentException("A child node cannot have an IP address pool");
             if (modelName.isPresent()) throw new IllegalArgumentException("A child node cannot have model name set");
             if (switchHostname.isPresent()) throw new IllegalArgumentException("A child node cannot have switch hostname set");
         }
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 e4b439c8c32..2b48ae98549 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
@@ -135,12 +135,12 @@ public class CapacityChecker {
         for (var host : hosts) {
             NodeResources hostResources = host.flavor().resources();
             int occupiedIps = 0;
-            Set<String> ipPool = host.ipConfig().pool().getIpSet();
+            Set<String> ipPool = host.ipConfig().pool().ipSet();
             for (var child : nodeChildren.get(host)) {
                 hostResources = hostResources.subtract(child.resources().justNumbers());
                 occupiedIps += child.ipConfig().primary().stream().filter(ipPool::contains).count();
             }
-            availableResources.put(host, new AllocationResources(hostResources, host.ipConfig().pool().getIpSet().size() - occupiedIps));
+            availableResources.put(host, new AllocationResources(hostResources, host.ipConfig().pool().ipSet().size() - occupiedIps));
         }
 
         return availableResources;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
index d4eaaa87082..a04e305242f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/IP.java
@@ -98,12 +98,12 @@ public class IP {
 
         /** Returns a copy of this with pool set to given value */
         public Config withPool(Pool pool) {
-            return new Config(primary, pool.getIpSet(), pool.getAddressList());
+            return new Config(primary, pool.ipSet(), pool.getAddressList());
         }
 
         /** Returns a copy of this with pool set to given value */
         public Config withPrimary(Set<String> primary) {
-            return new Config(primary, pool.getIpSet(), pool.getAddressList());
+            return new Config(primary, pool.ipSet(), pool.getAddressList());
         }
 
         @Override
@@ -122,7 +122,7 @@ public class IP {
 
         @Override
         public String toString() {
-            return String.format("ip config primary=%s pool=%s", primary, pool.getIpSet());
+            return String.format("ip config primary=%s pool=%s", primary, pool.ipSet());
         }
 
         /**
@@ -139,8 +139,8 @@ public class IP {
                     var addresses = new HashSet<>(node.ipConfig().primary());
                     var otherAddresses = new HashSet<>(other.ipConfig().primary());
                     if (node.type().isHost()) { // Addresses of a host can never overlap with any other nodes
-                        addresses.addAll(node.ipConfig().pool().getIpSet());
-                        otherAddresses.addAll(other.ipConfig().pool().getIpSet());
+                        addresses.addAll(node.ipConfig().pool().ipSet());
+                        otherAddresses.addAll(other.ipConfig().pool().ipSet());
                     }
                     otherAddresses.retainAll(addresses);
                     if (!otherAddresses.isEmpty())
@@ -289,8 +289,8 @@ public class IP {
          * @param nodes a list of all nodes in the repository
          */
         public Set<String> findUnusedIpAddresses(NodeList nodes) {
-            var unusedAddresses = new LinkedHashSet<>(getIpSet());
-            nodes.matching(node -> node.ipConfig().primary().stream().anyMatch(ip -> getIpSet().contains(ip)))
+            var unusedAddresses = new LinkedHashSet<>(ipSet());
+            nodes.matching(node -> node.ipConfig().primary().stream().anyMatch(ip -> ipSet().contains(ip)))
                  .forEach(node -> unusedAddresses.removeAll(node.ipConfig().primary()));
             return Collections.unmodifiableSet(unusedAddresses);
         }
@@ -325,7 +325,8 @@ public class IP {
             return ipAddresses.protocol;
         }
 
-        public Set<String> getIpSet() {
+        /** Returns the IP addresses in this pool as a set */
+        public Set<String> ipSet() {
             return ipAddresses.asSet();
         }
 
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 66fc07b51f2..cb9fb8182d4 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
@@ -163,8 +163,6 @@ public class Nodes {
                                                    .map(node -> {
                                                        if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty)
                                                            illegal("Can not set " + node + " ready. It is not provisioned or dirty.");
-                                                       if (node.type() == NodeType.host && node.ipConfig().pool().getIpSet().isEmpty())
-                                                           illegal("Can not set host " + node + " ready. Its IP address pool is empty.");
                                                        return node.withWantToRetire(false, false, Agent.system, clock.instant());
                                                    })
                                                    .collect(Collectors.toList());
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
index 2d0ef78c9ba..f08d7aa07e1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializer.java
@@ -151,7 +151,7 @@ public class NodeSerializer {
     private void toSlime(Node node, Cursor object) {
         object.setString(hostnameKey, node.hostname());
         toSlime(node.ipConfig().primary(), object.setArray(ipAddressesKey));
-        toSlime(node.ipConfig().pool().getIpSet(), object.setArray(ipAddressPoolKey));
+        toSlime(node.ipConfig().pool().ipSet(), object.setArray(ipAddressPoolKey));
         toSlime(node.ipConfig().pool().getAddressList(), object);
         object.setString(idKey, node.id());
         node.parentHostname().ifPresent(hostname -> object.setString(parentHostnameKey, hostname));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java
index 2c61d8092b6..1b521118106 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java
@@ -171,7 +171,7 @@ class NodesResponse extends SlimeJsonResponse {
         object.setBool("wantToDeprovision", node.status().wantToDeprovision());
         toSlime(node.history(), object.setArray("history"));
         ipAddressesToSlime(node.ipConfig().primary(), object.setArray("ipAddresses"));
-        ipAddressesToSlime(node.ipConfig().pool().getIpSet(), object.setArray("additionalIpAddresses"));
+        ipAddressesToSlime(node.ipConfig().pool().ipSet(), object.setArray("additionalIpAddresses"));
         addressesToSlime(node.ipConfig().pool().getAddressList(), object);
         node.reports().toSlime(object, "reports");
         node.modelName().ifPresent(modelName -> object.setString("modelName", modelName));
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
index 5d8330da21a..305f1b5952e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java
@@ -389,12 +389,12 @@ public class DynamicProvisioningMaintainerTest {
         tester.maintainer.maintain();
 
         assertTrue("No IP addresses written as DNS updates are failing",
-                   provisioning.get().stream().allMatch(host -> host.ipConfig().pool().getIpSet().isEmpty()));
+                   provisioning.get().stream().allMatch(host -> host.ipConfig().pool().ipSet().isEmpty()));
 
         tester.hostProvisioner.without(Behaviour.failDnsUpdate);
         tester.maintainer.maintain();
         assertTrue("IP addresses written as DNS updates are succeeding",
-                   provisioning.get().stream().noneMatch(host -> host.ipConfig().pool().getIpSet().isEmpty()));
+                   provisioning.get().stream().noneMatch(host -> host.ipConfig().pool().ipSet().isEmpty()));
     }
 
     @Test
@@ -452,7 +452,7 @@ public class DynamicProvisioningMaintainerTest {
         dynamicProvisioningTester.hostProvisioner.overrideHostFlavor("default");
 
         // Initial config server hosts are provisioned manually
-        List<Node> provisionedHosts = tester.makeReadyNodes(3, "default", hostType).stream()
+        List<Node> provisionedHosts = tester.makeReadyNodes(3, "default", hostType, 1).stream()
                                             .sorted(Comparator.comparing(Node::hostname))
                                             .collect(Collectors.toList());
         tester.prepareAndActivateInfraApplication(hostApp, hostType);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
index 9801233f396..27f5b45173b 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailTester.java
@@ -251,9 +251,12 @@ public class NodeFailTester {
 
     private List<Node> createReadyNodes(int count, int startIndex, Optional<String> parentHostname, Flavor flavor, NodeType nodeType) {
         List<Node> nodes = new ArrayList<>(count);
+        int lastOctetOfPoolAddress = 0;
         for (int i = startIndex; i < startIndex + count; i++) {
             String hostname = "host" + i;
-            IP.Config ipConfig = new IP.Config(nodeRepository.nameResolver().resolveAll(hostname), Set.of());
+            Set<String> ipPool = nodeType.isHost() ? Set.of("127.0." + i + "." + (++lastOctetOfPoolAddress)) : Set.of();
+            IP.Config ipConfig = new IP.Config(nodeRepository.nameResolver().resolveAll(hostname),
+                                               ipPool);
             Node.Builder builder = Node.create("node" + i, ipConfig, hostname, flavor, nodeType);
             parentHostname.ifPresent(builder::parentHostname);
             nodes.add(builder.build());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
index 3f0b94170f6..5e8ce1ed328 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/OsUpgradeActivatorTest.java
@@ -39,11 +39,11 @@ public class OsUpgradeActivatorTest {
 
         // Create infrastructure nodes
         var configHostApplication = ApplicationId.from("hosted-vespa", "configserver-host", "default");
-        var configHostNodes = tester.makeReadyNodes(3, "default", NodeType.confighost);
+        var configHostNodes = tester.makeReadyNodes(3, "default", NodeType.confighost, 1);
         tester.prepareAndActivateInfraApplication(configHostApplication, NodeType.confighost, version0);
 
         var tenantHostApplication = ApplicationId.from("hosted-vespa", "tenant-host", "default");
-        var tenantHostNodes = tester.makeReadyNodes(3, "default", NodeType.host);
+        var tenantHostNodes = tester.makeReadyNodes(3, "default", NodeType.host, 1);
         tester.prepareAndActivateInfraApplication(tenantHostApplication, NodeType.host, version0);
 
         var allNodes = new ArrayList<>(configHostNodes);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
index 8dbfbf44604..26c6f742052 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java
@@ -255,7 +255,7 @@ public class OsVersionsTest {
     }
 
     private List<Node> provisionInfraApplication(int nodeCount, NodeType nodeType) {
-        var nodes = tester.makeReadyNodes(nodeCount, "default", nodeType);
+        var nodes = tester.makeReadyNodes(nodeCount, "default", nodeType, 1);
         tester.prepareAndActivateInfraApplication(infraApplication, nodeType);
         return nodes.stream()
                     .map(Node::hostname)
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
index 11df26e4da6..f47fb7f23be 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
@@ -29,7 +29,7 @@ public class CuratorDatabaseClientTest {
 
     @Test
     public void can_read_stored_host_information() throws Exception {
-        String zkline = "{\"hostname\":\"host1\",\"ipAddresses\":[\"127.0.0.1\"],\"openStackId\":\"7951bb9d-3989-4a60-a21c-13690637c8ea\",\"flavor\":\"default\",\"created\":1421054425159, \"type\":\"host\"}";
+        String zkline = "{\"hostname\":\"host1\",\"ipAddresses\":[\"127.0.0.1\"],\"additionalIpAddresses\":[\"127.0.0.2\"],\"openStackId\":\"7951bb9d-3989-4a60-a21c-13690637c8ea\",\"flavor\":\"default\",\"created\":1421054425159, \"type\":\"host\"}";
         curator.framework().create().creatingParentsIfNeeded().forPath("/provision/v1/ready/host1", zkline.getBytes());
 
         List<Node> allocatedNodes = zkClient.readNodes(Node.State.ready);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
index e5333ea4186..0a852bbab56 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
@@ -255,13 +255,13 @@ public class NodeSerializerTest {
                 Set.of("::1", "::2", "::3"),
                 List.of(new Address("a"), new Address("b"), new Address("c")))));
         Node copy = nodeSerializer.fromJson(node.state(), nodeSerializer.toJson(node));
-        assertEquals(node.ipConfig().pool().getIpSet(), copy.ipConfig().pool().getIpSet());
+        assertEquals(node.ipConfig().pool().ipSet(), copy.ipConfig().pool().ipSet());
         assertEquals(Set.copyOf(node.ipConfig().pool().getAddressList()), Set.copyOf(copy.ipConfig().pool().getAddressList()));
 
         // Test round-trip without address pool (handle empty pool)
         node = createNode();
         copy = nodeSerializer.fromJson(node.state(), nodeSerializer.toJson(node));
-        assertEquals(node.ipConfig().pool().getIpSet(), copy.ipConfig().pool().getIpSet());
+        assertEquals(node.ipConfig().pool().ipSet(), copy.ipConfig().pool().ipSet());
         assertEquals(Set.copyOf(node.ipConfig().pool().getAddressList()), Set.copyOf(copy.ipConfig().pool().getAddressList()));
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ContainerImagesTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ContainerImagesTest.java
index d8df7721717..7b84b7a31f3 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ContainerImagesTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ContainerImagesTest.java
@@ -42,7 +42,7 @@ public class ContainerImagesTest {
         }
 
         // Proxy host uses image used by child nodes (proxy nodes), which is overridden in this case (for preload purposes)
-        var proxyHosts = tester.makeReadyNodes(2, "default", NodeType.proxyhost);
+        var proxyHosts = tester.makeReadyNodes(2, "default", NodeType.proxyhost, 1);
         for (var host : proxyHosts) {
             assertEquals(proxyImage, tester.nodeRepository().containerImages().imageFor(host.type()));
         }
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidateTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidateTest.java
index 8aa362aa932..e94733967b7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidateTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeCandidateTest.java
@@ -5,6 +5,7 @@ import com.yahoo.config.provision.Flavor;
 import com.yahoo.config.provision.NodeResources;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.vespa.hosted.provision.Node;
+import com.yahoo.vespa.hosted.provision.node.IP;
 import org.junit.Test;
 
 import java.util.ArrayList;
@@ -145,7 +146,8 @@ public class NodeCandidateTest {
                 .parentHostname(hostname + "parent")
                 .ipConfigWithEmptyPool(Set.of("::1")).build();
         Node parent = Node.create(hostname + "parent", hostname, new Flavor(totalHostResources), Node.State.ready, NodeType.host)
-                .ipConfigWithEmptyPool(Set.of("::1")).build();
+                          .ipConfig(new IP.Config(Set.of("::1"), Set.of("::2")))
+                          .build();
         return new NodeCandidate.ConcreteNodeCandidate(node, totalHostResources.subtract(allocatedHostResources), Optional.of(parent),
                                                        false, exclusiveSwitch, false, true, false);
     }