From f0ca34c31c48cfcfe0817b53d7d564339fa2564e Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Fri, 23 Oct 2020 18:49:19 +0200 Subject: Reject invalid nodes also if allocated This will never happen now, but I think it's clearer to show that explicitly in the code. --- .../vespa/hosted/provision/provisioning/NodeAllocation.java | 10 ++++++---- .../vespa/hosted/provision/provisioning/NodeCandidate.java | 13 +++++++++---- 2 files changed, 15 insertions(+), 8 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..a8457eef374 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 @@ -358,10 +358,12 @@ abstract class NodeCandidate implements Nodelike, Comparable { Optional 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 +411,13 @@ abstract class NodeCandidate implements Nodelike, Comparable { 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 +458,7 @@ abstract class NodeCandidate implements Nodelike, Comparable { @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 -- cgit v1.2.3 From b91259635e9af7d80bb6225d8c7310552667e51d Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Fri, 23 Oct 2020 19:06:38 +0200 Subject: Clarify consequences of calling toNode --- .../yahoo/vespa/hosted/provision/provisioning/NodeCandidate.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 a8457eef374..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 { /** 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 */ -- cgit v1.2.3