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();