diff options
author | Martin Polden <mpolden@mpolden.no> | 2019-02-05 16:26:54 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-02-05 16:26:54 +0100 |
commit | d45246af7c8aabcc5a56a19e36d5f3374a47a408 (patch) | |
tree | bdaf87b17b648ec5708026b4d238e8ddfd3dd9ce | |
parent | a3579ec19f39d5f311b7335d79e618e4b77dd949 (diff) | |
parent | 90ab3ed4b08437f062c95e650ab4df9a49514a9a (diff) |
Merge pull request #8387 from vespa-engine/bratseth/minor-noderepo-cleanup
Nonfunctional changes only
12 files changed, 34 insertions, 31 deletions
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java index bc89d0dfcf5..30f08d13772 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java @@ -49,7 +49,7 @@ public class ComplexAttributeFieldsValidator extends Validator { if (!unsupportedFields.isEmpty()) { throw new IllegalArgumentException( String.format("For cluster '%s', search '%s': The following complex fields do not support using struct field attributes: %s. " + - "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types", + "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types", clusterName, search.getName(), unsupportedFields)); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java index c74ba6abe0f..e48d8a11c27 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java @@ -252,7 +252,7 @@ public class NodeRepository extends AbstractComponent { default: throw new IllegalArgumentException( String.format("Don't know how to create ACL for node [hostname=%s type=%s]", - node.hostname(), node.type())); + node.hostname(), node.type())); } return new NodeAcl(node, trustedNodes, trustedNetworks, trustedPorts); @@ -303,7 +303,9 @@ public class NodeRepository extends AbstractComponent { } /** Adds a list of newly created docker container nodes to the node repository as <i>reserved</i> nodes */ - public List<Node> addDockerNodes(List<Node> nodes) { + // NOTE: This can only be called while holding the allocation lock, and that lock must have been held since + // the nodes list was computed + public List<Node> addDockerNodes(List<Node> nodes, Mutex allocationLock) { for (Node node : nodes) { if (!node.flavor().getType().equals(Flavor.Type.DOCKER_CONTAINER)) { throw new IllegalArgumentException("Cannot add " + node.hostname() + ": This is not a docker node"); @@ -314,12 +316,10 @@ public class NodeRepository extends AbstractComponent { Optional<Node> existing = getNode(node.hostname()); if (existing.isPresent()) throw new IllegalArgumentException("Cannot add " + node.hostname() + ": A node with this name already exists (" + - existing.get() + ", " + existing.get().history() + "). Node to be added: " + - node + ", " + node.history()); - } - try (Mutex lock = lockUnallocated()) { - return db.addNodesInState(nodes, Node.State.reserved); + existing.get() + ", " + existing.get().history() + "). Node to be added: " + + node + ", " + node.history()); } + return db.addNodesInState(nodes, Node.State.reserved); } /** Adds a list of (newly created) nodes to the node repository as <i>provisioned</i> nodes */ @@ -329,14 +329,14 @@ public class NodeRepository extends AbstractComponent { if (existing.isPresent()) throw new IllegalArgumentException("Cannot add " + node.hostname() + ": A node with this name already exists"); } - try (Mutex lock = lockUnallocated()) { + try (Mutex lock = lockAllocation()) { return db.addNodes(nodes); } } /** Sets a list of nodes ready and returns the nodes in the ready state */ public List<Node> setReady(List<Node> nodes, Agent agent, String reason) { - try (Mutex lock = lockUnallocated()) { + try (Mutex lock = lockAllocation()) { List<Node> nodesWithResetFields = nodes.stream() .map(node -> { if (node.state() != Node.State.provisioned && node.state() != Node.State.dirty) @@ -554,7 +554,7 @@ public class NodeRepository extends AbstractComponent { } private List<Node> removeRecursively(Node node, boolean force) { - try (Mutex lock = lockUnallocated()) { + try (Mutex lock = lockAllocation()) { List<Node> removed = new ArrayList<>(); if (node.type().isDockerHost()) { @@ -665,7 +665,7 @@ public class NodeRepository extends AbstractComponent { // perform operation while holding locks List<Node> resultingNodes = new ArrayList<>(); - try (Mutex lock = lockUnallocated()) { + try (Mutex lock = lockAllocation()) { for (Node node : unallocatedNodes) resultingNodes.add(action.apply(node)); } @@ -690,12 +690,12 @@ public class NodeRepository extends AbstractComponent { /** Create a lock with a timeout which provides exclusive rights to making changes to the given application */ public Mutex lock(ApplicationId application, Duration timeout) { return db.lock(application, timeout); } - /** Create a lock which provides exclusive rights to changing the set of ready nodes */ - public Mutex lockUnallocated() { return db.lockInactive(); } + /** Create a lock which provides exclusive rights to allocating nodes */ + public Mutex lockAllocation() { return db.lockInactive(); } /** Acquires the appropriate lock for this node */ private Mutex lock(Node node) { - return node.allocation().isPresent() ? lock(node.allocation().get().owner()) : lockUnallocated(); + return node.allocation().isPresent() ? lock(node.allocation().get().owner()) : lockAllocation(); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostDeprovisionMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostDeprovisionMaintainer.java index 1e4016f2112..d38738ff019 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostDeprovisionMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostDeprovisionMaintainer.java @@ -33,7 +33,7 @@ public class HostDeprovisionMaintainer extends Maintainer { @Override protected void maintain() { - try (Mutex lock = nodeRepository().lockUnallocated()) { + try (Mutex lock = nodeRepository().lockAllocation()) { NodeList nodes = nodeRepository().list(); for (Node node : candidates(nodes)) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostProvisionMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostProvisionMaintainer.java index 0479f20ba37..5acdc56ffda 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostProvisionMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/HostProvisionMaintainer.java @@ -36,7 +36,7 @@ public class HostProvisionMaintainer extends Maintainer { @Override protected void maintain() { - try (Mutex lock = nodeRepository().lockUnallocated()) { + try (Mutex lock = nodeRepository().lockAllocation()) { NodeList nodes = nodeRepository().list(); candidates(nodes).forEach((host, children) -> { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java index 80f6f5ccea5..6c430351a88 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java @@ -95,7 +95,7 @@ public class NodeFailer extends Maintainer { int throttledNodeFailures = 0; // Ready nodes - try (Mutex lock = nodeRepository().lockUnallocated()) { + try (Mutex lock = nodeRepository().lockAllocation()) { updateNodeLivenessEventsForReadyNodes(); for (Map.Entry<Node, String> entry : getReadyNodesByFailureReason().entrySet()) { diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java index 149e013687f..b170d4eb66e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeRetirer.java @@ -67,7 +67,7 @@ public class NodeRetirer extends Maintainer { * Returns true iff all there are no unallocated nodes that match the retirement policy */ boolean retireUnallocated() { - try (Mutex lock = nodeRepository().lockUnallocated()) { + try (Mutex lock = nodeRepository().lockAllocation()) { List<Node> allNodes = nodeRepository().getNodes(NodeType.tenant); Map<Flavor, Map<Node.State, Long>> numSpareNodesByFlavorByState = getNumberOfNodesByFlavorByNodeState(allNodes); flavorSpareChecker.updateReadyAndActiveCountsByFlavor(numSpareNodesByFlavorByState); 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 1b438ef2bd6..544585c43ed 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 @@ -85,7 +85,8 @@ public class IP { return allocation; } - /** Find all unused addresses in this pool + /** + * Finds all unused addresses in this pool * * @param nodes All nodes in the 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 eb3be9e6d80..8d6edbd7b9b 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 @@ -44,8 +44,8 @@ public class GroupPreparer { List<Node> surplusActiveNodes, MutableInteger highestIndex, int spareCount) { try (Mutex lock = nodeRepository.lock(application)) { - // Lock ready pool to ensure that ready nodes are not simultaneously grabbed by others - try (Mutex readyLock = nodeRepository.lockUnallocated()) { + // Lock ready pool to ensure that the same nodes are not simultaneously allocated by others + try (Mutex allocationLock = nodeRepository.lockAllocation()) { // Create a prioritized set of nodes NodePrioritizer prioritizer = new NodePrioritizer(nodeRepository.getNodes(), @@ -58,7 +58,7 @@ public class GroupPreparer { prioritizer.addApplicationNodes(); prioritizer.addSurplusNodes(surplusActiveNodes); prioritizer.addReadyNodes(); - prioritizer.addNewDockerNodes(); + prioritizer.addNewDockerNodes(allocationLock); // Allocate from the prioritized list NodeAllocation allocation = new NodeAllocation(application, cluster, requestedNodes, highestIndex, nodeRepository); @@ -73,7 +73,7 @@ public class GroupPreparer { // Carry out and return allocation nodeRepository.reserve(allocation.reservableNodes()); - nodeRepository.addDockerNodes(allocation.newNodes()); + nodeRepository.addDockerNodes(allocation.newNodes(), allocationLock); surplusActiveNodes.removeAll(allocation.surplusNodes()); return allocation.finalNodes(surplusActiveNodes); } 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 990cc575178..e2293578f4a 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 @@ -215,19 +215,19 @@ class NodeAllocation { if ( ! node.state().equals(Node.State.active)) { // reactivated node - make sure its not retired node = node.unretire(); - prioritizableNode.node= node; + prioritizableNode.node = node; } acceptedOfRequestedFlavor++; } else { ++wasRetiredJustNow; // Retire nodes which are of an unwanted flavor, retired flavor or have an overlapping parent host node = node.retire(nodeRepository.clock().instant()); - prioritizableNode.node= node; + prioritizableNode.node = node; } if ( ! node.allocation().get().membership().cluster().equals(cluster)) { // group may be different node = setCluster(cluster, node); - prioritizableNode.node= node; + prioritizableNode.node = node; } indexes.add(node.allocation().get().membership().index()); highestIndex.set(Math.max(highestIndex.get(), node.allocation().get().membership().index())); 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 7798d44a645..0646e23b141 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 @@ -6,6 +6,7 @@ import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Flavor; import com.yahoo.config.provision.NodeType; import com.yahoo.log.LogLevel; +import com.yahoo.transaction.Mutex; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; import com.yahoo.vespa.hosted.provision.node.IP; @@ -115,7 +116,8 @@ public class NodePrioritizer { /** * Add a node on each docker host with enough capacity for the requested flavor */ - void addNewDockerNodes() { + // NOTE: This must only be called while holding the allocation lock. + void addNewDockerNodes(Mutex allocationLock) { if (!isDocker) return; DockerHostCapacity capacity = new DockerHostCapacity(allNodes); ResourceCapacity wantedResourceCapacity = ResourceCapacity.of(getFlavor(requestedNodes)); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java index 6b45352adba..1e502439aeb 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java @@ -129,11 +129,11 @@ public class MetricsReporterTest { Node container1 = Node.createDockerNode(Collections.singleton("::2"), Collections.emptySet(), "container1", "dockerHost", nodeFlavors.getFlavorOrThrow("docker"), NodeType.tenant); container1 = container1.with(allocation(Optional.of("app1")).get()); - nodeRepository.addDockerNodes(Collections.singletonList(container1)); + nodeRepository.addDockerNodes(Collections.singletonList(container1), nodeRepository.lockAllocation()); Node container2 = Node.createDockerNode(Collections.singleton("::3"), Collections.emptySet(), "container2", "dockerHost", nodeFlavors.getFlavorOrThrow("docker2"), NodeType.tenant); container2 = container2.with(allocation(Optional.of("app2")).get()); - nodeRepository.addDockerNodes(Collections.singletonList(container2)); + nodeRepository.addDockerNodes(Collections.singletonList(container2), nodeRepository.lockAllocation()); Orchestrator orchestrator = mock(Orchestrator.class); ServiceMonitor serviceMonitor = mock(ServiceMonitor.class); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java index 23e45e06a47..51c32c8f85a 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java @@ -176,7 +176,7 @@ public class NodeIdentifierTest { String environment = ZONE.environment().value(); NodeRepositoryTester nodeRepositoryDummy = new NodeRepositoryTester(); Node node = createNode(clusterId, clusterIndex, tenant, application); - nodeRepositoryDummy.nodeRepository().addDockerNodes(singletonList(node)); + nodeRepositoryDummy.nodeRepository().addDockerNodes(singletonList(node), nodeRepositoryDummy.nodeRepository().lockAllocation()); Pkcs10Csr csr = Pkcs10CsrBuilder .fromKeypair(new X500Principal("CN=" + TENANT_DOCKER_CONTAINER_IDENTITY), KEYPAIR, SHA256_WITH_ECDSA) .build(); |