From 31c097851e2c1160ba8c074eafe6b302551f577b Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 24 Feb 2021 13:27:11 +0100 Subject: Plug the holes in node indices for non-content nodes --- .../provision/provisioning/GroupPreparer.java | 19 ++++---- .../provision/provisioning/NodeAllocation.java | 12 ++--- .../hosted/provision/provisioning/NodeIndices.java | 57 ++++++++++++++++++++++ .../hosted/provision/provisioning/Preparer.java | 25 +++------- 4 files changed, 79 insertions(+), 34 deletions(-) create mode 100644 node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java (limited to 'node-repository') diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java index 543e452c810..0a22cc1cc58 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/GroupPreparer.java @@ -20,6 +20,7 @@ import com.yahoo.vespa.hosted.provision.provisioning.HostProvisioner.HostSharing import java.util.List; import java.util.Optional; +import java.util.function.Supplier; import java.util.stream.Collectors; /** @@ -51,15 +52,15 @@ public class GroupPreparer { * @param requestedNodes a specification of the requested nodes * @param surplusActiveNodes currently active nodes which are available to be assigned to this group. * This method will remove from this list if it finds it needs additional nodes - * @param highestIndex the current highest node index among all active nodes in this cluster. - * This method will increase this number when it allocates new nodes to the cluster. + * @param indices the next available node indices for this cluster. + * This method will consume these when it allocates new nodes to the cluster. * @return the list of nodes this cluster group will have allocated if activated */ // Note: This operation may make persisted changes to the set of reserved and inactive nodes, // but it may not change the set of active nodes, as the active nodes must stay in sync with the // active config model which is changed on activate public List prepare(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, - List surplusActiveNodes, MutableInteger highestIndex, int wantedGroups) { + List surplusActiveNodes, NodeIndices indices, int wantedGroups) { String allocateOsRequirement = allocateOsRequirementFlag .with(FetchVector.Dimension.APPLICATION_ID, application.serializedForm()) @@ -68,16 +69,16 @@ public class GroupPreparer { // Try preparing in memory without global unallocated lock. Most of the time there should be no changes and we // can return nodes previously allocated. { - MutableInteger probePrepareHighestIndex = new MutableInteger(highestIndex.get()); NodeAllocation probeAllocation = prepareAllocation(application, cluster, requestedNodes, surplusActiveNodes, - probePrepareHighestIndex, wantedGroups, PROBE_LOCK, + indices::probeNext, wantedGroups, PROBE_LOCK, allocateOsRequirement); if (probeAllocation.fulfilledAndNoChanges()) { List acceptedNodes = probeAllocation.finalNodes(); surplusActiveNodes.removeAll(acceptedNodes); - highestIndex.set(probePrepareHighestIndex.get()); + indices.commitProbe(); return acceptedNodes; } + indices.resetProbe(); } // There were some changes, so re-do the allocation with locks @@ -85,7 +86,7 @@ public class GroupPreparer { Mutex allocationLock = nodeRepository.nodes().lockUnallocated()) { NodeAllocation allocation = prepareAllocation(application, cluster, requestedNodes, surplusActiveNodes, - highestIndex, wantedGroups, allocationLock, + indices::next, wantedGroups, allocationLock, allocateOsRequirement); if (nodeRepository.zone().getCloud().dynamicProvisioning()) { @@ -133,11 +134,11 @@ public class GroupPreparer { } private NodeAllocation prepareAllocation(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, - List surplusActiveNodes, MutableInteger highestIndex, int wantedGroups, + List surplusActiveNodes, Supplier nextIndex, int wantedGroups, Mutex allocationLock, String allocateOsRequirement) { LockedNodeList allNodes = nodeRepository.nodes().list(allocationLock); NodeAllocation allocation = new NodeAllocation(allNodes, application, cluster, requestedNodes, - highestIndex, nodeRepository); + nextIndex, nodeRepository); NodePrioritizer prioritizer = new NodePrioritizer( allNodes, application, cluster, requestedNodes, wantedGroups, nodeRepository.zone().getCloud().dynamicProvisioning(), nodeRepository.nameResolver(), 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 d5d221cba7c..6f323955ac7 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 @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Collectors; /** @@ -71,18 +72,18 @@ class NodeAllocation { private final Set indexes = new HashSet<>(); /** The next membership index to assign to a new node */ - private final MutableInteger highestIndex; + private final Supplier nextIndex; private final NodeRepository nodeRepository; private final NodeResourceLimits nodeResourceLimits; NodeAllocation(NodeList allNodes, ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, - MutableInteger highestIndex, NodeRepository nodeRepository) { + Supplier nextIndex, NodeRepository nodeRepository) { this.allNodes = allNodes; this.application = application; this.cluster = cluster; this.requestedNodes = requestedNodes; - this.highestIndex = highestIndex; + this.nextIndex = nextIndex; this.nodeRepository = nodeRepository; nodeResourceLimits = new NodeResourceLimits(nodeRepository); } @@ -135,7 +136,7 @@ class NodeAllocation { continue; } candidate = candidate.allocate(application, - ClusterMembership.from(cluster, highestIndex.add(1)), + ClusterMembership.from(cluster, nextIndex.get()), requestedNodes.resources().orElse(candidate.resources()), nodeRepository.clock().instant()); if (candidate.isValid()) @@ -257,7 +258,6 @@ class NodeAllocation { } candidate = candidate.withNode(node); indexes.add(node.allocation().get().membership().index()); - highestIndex.set(Math.max(highestIndex.get(), node.allocation().get().membership().index())); nodes.put(node.hostname(), candidate); return node; } @@ -284,7 +284,7 @@ class NodeAllocation { return requestedNodes.fulfilledBy(accepted); } - /** Returns true this allocation was already fulfilled and resulted in no new changes */ + /** Returns true if this allocation was already fulfilled and resulted in no new changes */ public boolean fulfilledAndNoChanges() { return fulfilled() && reservableNodes().isEmpty() && newNodes().isEmpty(); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java new file mode 100644 index 00000000000..310e4f1e69a --- /dev/null +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java @@ -0,0 +1,57 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.provision.provisioning; + +import com.yahoo.config.provision.ClusterSpec; +import com.yahoo.vespa.hosted.provision.NodeList; + +import java.util.List; + +import static java.util.Comparator.naturalOrder; + +/** + * Tracks indices of a node cluster, and proposes the index of the next allocation. + * + * @author jonmv + */ +class NodeIndices { + + private final boolean compact; + private final List used; + + private int last; + private int probe; + + NodeIndices(NodeList nodes, boolean compact) { + this.compact = compact; + this.used = nodes.mapToList(node -> node.allocation().get().membership().index()); + this.last = compact ? -1 : used.stream().max(naturalOrder()).orElse(-1); + this.probe = last; + } + + /** Returns the next available index and commits to using it. Throws if a probe is ongoing. */ + int next() { + if (probe != last) + throw new IllegalStateException("Must commit ongoing probe before calling 'next'"); + + probeNext(); + commitProbe(); + return last; + } + + /** Returns the next available index, without committing to using it. May be called multiple times. */ + int probeNext() { + while (used.contains(++probe)); + return probe; + } + + /** Commits to using any indices returned by an ongoing probe. */ + void commitProbe() { + last = probe; + } + + /** Resets any probed state to what's currently committed. */ + void resetProbe() { + probe = last; + } + +} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java index 87b3742efb4..0e22f4f0e6b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java @@ -8,6 +8,7 @@ import com.yahoo.config.provision.OutOfCapacityException; import com.yahoo.lang.MutableInteger; import com.yahoo.vespa.flags.FlagSource; import com.yahoo.vespa.hosted.provision.Node; +import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Agent; @@ -60,13 +61,16 @@ class Preparer { private List prepareNodes(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups) { List surplusNodes = findNodesInRemovableGroups(application, cluster, wantedGroups); - MutableInteger highestIndex = new MutableInteger(findHighestIndex(application, cluster)); + NodeList nodesInCluster = nodeRepository.nodes().list(Node.State.allocatedStates().toArray(new Node.State[0])) + .owner(application) + .matching(node -> node.allocation().get().membership().cluster().satisfies(cluster)); + NodeIndices indices = new NodeIndices(nodesInCluster, ! cluster.type().isContent()); List acceptedNodes = new ArrayList<>(); for (int groupIndex = 0; groupIndex < wantedGroups; groupIndex++) { ClusterSpec clusterGroup = cluster.with(Optional.of(ClusterSpec.Group.from(groupIndex))); List accepted = groupPreparer.prepare(application, clusterGroup, requestedNodes.fraction(wantedGroups), surplusNodes, - highestIndex, wantedGroups); + indices, wantedGroups); replace(acceptedNodes, accepted); } moveToActiveGroup(surplusNodes, wantedGroups, cluster.group()); @@ -120,21 +124,4 @@ class Preparer { return list; } - /** - * Returns the highest index number of all active and failed nodes in this cluster, or -1 if there are no nodes. - * We include failed nodes to avoid reusing the index of the failed node in the case where the failed node is the - * node with the highest index. - */ - private int findHighestIndex(ApplicationId application, ClusterSpec cluster) { - int highestIndex = -1; - for (Node node : nodeRepository.nodes().list(Node.State.allocatedStates().toArray(new Node.State[0])).owner(application)) { - ClusterSpec nodeCluster = node.allocation().get().membership().cluster(); - if ( ! nodeCluster.id().equals(cluster.id())) continue; - if ( ! nodeCluster.type().equals(cluster.type())) continue; - - highestIndex = Math.max(node.allocation().get().membership().index(), highestIndex); - } - return highestIndex; - } - } -- cgit v1.2.3 From 3e898d23e511875a1a00f15275e2767bdee96c47 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 24 Feb 2021 13:27:39 +0100 Subject: Start indices at 0 in test, to avoid reverse order indcies vs host names --- .../vespa/hosted/provision/provisioning/InfraDeployerImplTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'node-repository') diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/InfraDeployerImplTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/InfraDeployerImplTest.java index f4d8bbfcb21..6f50cbf9803 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/InfraDeployerImplTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/InfraDeployerImplTest.java @@ -150,7 +150,7 @@ public class InfraDeployerImplTest { Node node = tester.addHost("id-" + id, "node-" + id, "default", nodeType); Optional nodeWithAllocation = wantedVespaVersion.map(version -> { ClusterSpec clusterSpec = application.getClusterSpecWithVersion(version).with(Optional.of(ClusterSpec.Group.from(0))); - ClusterMembership membership = ClusterMembership.from(clusterSpec, 1); + ClusterMembership membership = ClusterMembership.from(clusterSpec, 0); Allocation allocation = new Allocation(application.getApplicationId(), membership, node.resources(), Generation.initial(), false); return node.with(allocation); }); -- cgit v1.2.3 From 28bc4276f6e6ba8f4ed1f081d2cf902b33c521e3 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 24 Feb 2021 17:01:03 +0100 Subject: Check only cluster id, better doc, test to show usage --- .../vespa/hosted/provision/provisioning/NodeIndices.java | 11 +++++------ .../yahoo/vespa/hosted/provision/provisioning/Preparer.java | 9 +++++---- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'node-repository') diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java index 310e4f1e69a..da4ae2a119e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIndices.java @@ -15,15 +15,14 @@ import static java.util.Comparator.naturalOrder; */ class NodeIndices { - private final boolean compact; private final List used; private int last; private int probe; - NodeIndices(NodeList nodes, boolean compact) { - this.compact = compact; - this.used = nodes.mapToList(node -> node.allocation().get().membership().index()); + /** Pass the list of current indices in the cluster, and whether to fill gaps or not. */ + NodeIndices(List used, boolean compact) { + this.used = List.copyOf(used); this.last = compact ? -1 : used.stream().max(naturalOrder()).orElse(-1); this.probe = last; } @@ -38,13 +37,13 @@ class NodeIndices { return last; } - /** Returns the next available index, without committing to using it. May be called multiple times. */ + /** Returns the next available index, without committing to using it. Yields increasing indices when called multiple times. */ int probeNext() { while (used.contains(++probe)); return probe; } - /** Commits to using any indices returned by an ongoing probe. */ + /** Commits to using all indices returned by an ongoing probe. */ void commitProbe() { last = probe; } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java index 0e22f4f0e6b..e36554b3600 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java @@ -61,10 +61,11 @@ class Preparer { private List prepareNodes(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups) { List surplusNodes = findNodesInRemovableGroups(application, cluster, wantedGroups); - NodeList nodesInCluster = nodeRepository.nodes().list(Node.State.allocatedStates().toArray(new Node.State[0])) - .owner(application) - .matching(node -> node.allocation().get().membership().cluster().satisfies(cluster)); - NodeIndices indices = new NodeIndices(nodesInCluster, ! cluster.type().isContent()); + List usedIndices = nodeRepository.nodes().list(Node.State.allocatedStates().toArray(new Node.State[0])) + .owner(application) + .cluster(cluster.id()) + .mapToList(node -> node.allocation().get().membership().index()); + NodeIndices indices = new NodeIndices(usedIndices, ! cluster.type().isContent()); List acceptedNodes = new ArrayList<>(); for (int groupIndex = 0; groupIndex < wantedGroups; groupIndex++) { ClusterSpec clusterGroup = cluster.with(Optional.of(ClusterSpec.Group.from(groupIndex))); -- cgit v1.2.3 From 4c12b3aa80e7a08590727adcf11718b45821cb3b Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Wed, 24 Feb 2021 17:03:00 +0100 Subject: Skip redundant filter, and actually add test --- .../hosted/provision/provisioning/Preparer.java | 2 +- .../provision/provisioning/NodeIncidesTest.java | 72 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIncidesTest.java (limited to 'node-repository') diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java index e36554b3600..2eee3c3f01c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/Preparer.java @@ -61,7 +61,7 @@ class Preparer { private List prepareNodes(ApplicationId application, ClusterSpec cluster, NodeSpec requestedNodes, int wantedGroups) { List surplusNodes = findNodesInRemovableGroups(application, cluster, wantedGroups); - List usedIndices = nodeRepository.nodes().list(Node.State.allocatedStates().toArray(new Node.State[0])) + List usedIndices = nodeRepository.nodes().list() .owner(application) .cluster(cluster.id()) .mapToList(node -> node.allocation().get().membership().index()); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIncidesTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIncidesTest.java new file mode 100644 index 00000000000..eee9c2ffc01 --- /dev/null +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodeIncidesTest.java @@ -0,0 +1,72 @@ +// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.vespa.hosted.provision.provisioning; + +import org.junit.Test; + +import java.util.List; + +import static org.junit.Assert.assertEquals; + +/** + * @author jonmv + */ +public class NodeIncidesTest { + + @Test + public void testNonCompactIndices() { + NodeIndices indices = new NodeIndices(List.of(1, 3, 4), false); + assertEquals(5, indices.probeNext()); + assertEquals(6, indices.probeNext()); + + indices.resetProbe(); + assertEquals(5, indices.probeNext()); + assertEquals(6, indices.probeNext()); + + indices.commitProbe(); + assertEquals(7, indices.probeNext()); + assertEquals(8, indices.probeNext()); + + indices.resetProbe(); + assertEquals(7, indices.next()); + assertEquals(8, indices.next()); + + assertEquals(9, indices.probeNext()); + try { + indices.next(); + } + catch (IllegalStateException e) { + assertEquals("Must commit ongoing probe before calling 'next'", e.getMessage()); + } + } + + + @Test + public void testCompactIndices() { + NodeIndices indices = new NodeIndices(List.of(1, 3, 4), true); + assertEquals(0, indices.probeNext()); + assertEquals(2, indices.probeNext()); + assertEquals(5, indices.probeNext()); + assertEquals(6, indices.probeNext()); + + indices.resetProbe(); + assertEquals(0, indices.probeNext()); + assertEquals(2, indices.probeNext()); + + indices.commitProbe(); + assertEquals(5, indices.probeNext()); + assertEquals(6, indices.probeNext()); + + indices.resetProbe(); + assertEquals(5, indices.next()); + assertEquals(6, indices.next()); + + assertEquals(7, indices.probeNext()); + try { + indices.next(); + } + catch (IllegalStateException e) { + assertEquals("Must commit ongoing probe before calling 'next'", e.getMessage()); + } + } + +} -- cgit v1.2.3