Merge pull request #15023 from vespa-engine/bratseth/avoid-allocating-invalid-nodes

Bratseth/avoid allocating invalid nodes

2 files changed, 20 insertions, 9 deletions

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 240963a8c0d..1e98160955c 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
@@ -112,8 +112,11 @@ class NodeAllocation {
                 boolean resizeable = requestedNodes.considerRetiring() && candidate.isResizable;
                 boolean acceptToRetire = acceptToRetire(candidate);
 
-                if ((! saturated() && hasCompatibleFlavor(candidate) && requestedNodes.acceptable(candidate)) || acceptToRetire)
-                    accepted.add(acceptNode(candidate, shouldRetire(candidate), resizeable));
+                if ((! saturated() && hasCompatibleFlavor(candidate) && requestedNodes.acceptable(candidate)) || acceptToRetire) {
+                    candidate = candidate.withNode();
+                    if (candidate.isValid())
+                        accepted.add(acceptNode(candidate, shouldRetire(candidate), resizeable));
+                }
             }
             else if (! saturated() && hasCompatibleFlavor(candidate)) {
                 if ( ! nodeResourceLimits.isWithinRealLimits(candidate, cluster)) {
@@ -240,7 +243,6 @@ class NodeAllocation {
     }
 
     private Node acceptNode(NodeCandidate candidate, boolean wantToRetire, boolean resizeable) {
-        candidate = candidate.withNode();
         Node node = candidate.toNode();
 
         if (node.allocation().isPresent()) // Record the currently requested resources
@@ -356,7 +358,7 @@ class NodeAllocation {
             candidate = candidate.withNode();
             Allocation allocation = candidate.allocation().get();
             candidate = candidate.withNode(candidate.toNode().with(allocation.with(allocation.membership()
-                                .with(allocation.membership().cluster().exclusive(requestedNodes.isExclusive())))));
+                                 .with(allocation.membership().cluster().exclusive(requestedNodes.isExclusive())))));
             nodes.put(candidate.toNode().hostname(), candidate);
         }
 
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 b915053fff5..02086e2bace 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
@@ -87,7 +87,11 @@ abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidate> {
     /** Returns a copy of this with exclusive switch set to given value */
     public abstract NodeCandidate withExclusiveSwitch(boolean exclusiveSwitch);
 
-    /** Returns the node instance of this candidate, or an invalid node if it cannot be created */
+    /**
+     * Returns the node instance of this candidate, allocating it if necessary.
+     *
+     * @throws IllegalStateException if the node candidate is invalid
+     */
     public abstract Node toNode();
 
     /** Returns whether this node can - as far as we know - be used to run the application workload */
@@ -358,10 +362,12 @@ abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidate> {
             Optional<IP.Allocation> allocation;
             try {
                 allocation = parent.get().ipConfig().pool().findAllocation(allNodes, nodeRepository.nameResolver());
-                if (allocation.isEmpty()) return new InvalidNodeCandidate(resources, freeParentCapacity, parent.get());
+                if (allocation.isEmpty()) return new InvalidNodeCandidate(resources, freeParentCapacity, parent.get(),
+                                                                          "No IP addresses available on parent host");
             } catch (Exception e) {
                 log.warning("Failed allocating IP address on " + parent.get() +": " + Exceptions.toMessageString(e));
-                return new InvalidNodeCandidate(resources, freeParentCapacity, parent.get());
+                return new InvalidNodeCandidate(resources, freeParentCapacity, parent.get(),
+                                                "Failed when allocating IP address on host");
             }
 
             Node node = Node.createDockerNode(allocation.get().addresses(),
@@ -409,10 +415,13 @@ abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidate> {
     static class InvalidNodeCandidate extends NodeCandidate {
 
         private final NodeResources resources;
+        private final String invalidReason;
 
-        private InvalidNodeCandidate(NodeResources resources, NodeResources freeParentCapacity, Node parent) {
+        private InvalidNodeCandidate(NodeResources resources, NodeResources freeParentCapacity, Node parent,
+                                     String invalidReason) {
             super(freeParentCapacity, Optional.of(parent), false, false, false, true, false);
             this.resources = resources;
+            this.invalidReason = invalidReason;
         }
 
         @Override
@@ -453,7 +462,7 @@ abstract class NodeCandidate implements Nodelike, Comparable<NodeCandidate> {
 
         @Override
         public Node toNode() {
-            throw new IllegalStateException("Candidate node on " + parent.get() + " is invalid");
+            throw new IllegalStateException("Candidate node on " + parent.get() + " is invalid: " + invalidReason);
         }
 
         @Override