Revert "Hold config lock when modifying load balancers"

4 files changed, 24 insertions, 68 deletions

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 1b5b5469302..6f7be09aa0e 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
@@ -5,6 +5,7 @@ import com.google.inject.Inject;
 import com.yahoo.collections.ListMap;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.component.Version;
+import com.yahoo.vespa.hosted.provision.Node.State;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.DockerImage;
 import com.yahoo.config.provision.Flavor;
@@ -16,7 +17,8 @@ import com.yahoo.config.provisioning.NodeRepositoryConfig;
 import com.yahoo.transaction.Mutex;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.curator.Curator;
-import com.yahoo.vespa.hosted.provision.Node.State;
+import com.yahoo.vespa.flags.FlagSource;
+import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.provision.applications.Applications;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancer;
 import com.yahoo.vespa.hosted.provision.lb.LoadBalancerId;
@@ -128,9 +130,6 @@ public class NodeRepository extends AbstractComponent {
 
         // read and write all nodes to make sure they are stored in the latest version of the serialized format
         for (State state : State.values())
-            // TODO(mpolden): Add per-node locking. In its current state this may collide with other callers making
-            //                node state changes. Example: A redeployment on another config server which moves a node
-            //                to another state while this is constructed.
             db.writeTo(state, db.getNodes(state), Agent.system, Optional.empty());
     }
 
@@ -764,7 +763,6 @@ public class NodeRepository extends AbstractComponent {
     public Zone zone() { return zone; }
 
     /** Create a lock which provides exclusive rights to making changes to the given application */
-    // TODO(mpolden): Make this delegate to CuratorDatabaseClient#lockConfig instead
     public Mutex lock(ApplicationId application) { return db.lock(application); }
 
     /** Create a lock with a timeout which provides exclusive rights to making changes to the given application */
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
index 7beb717ea8b..e2b70608d58 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/LoadBalancerExpirer.java
@@ -120,13 +120,11 @@ public class LoadBalancerExpirer extends Maintainer {
     /** Apply operation to all load balancers that exist in given state, while holding lock */
     private void withLoadBalancersIn(LoadBalancer.State state, Consumer<LoadBalancer> operation) {
         for (var id : db.readLoadBalancerIds()) {
-            try (var lock = db.lockConfig(id.application())) {
-                try (var legacyLock = db.lockLoadBalancers(id.application())) {
-                    var loadBalancer = db.readLoadBalancer(id);
-                    if (loadBalancer.isEmpty()) continue;              // Load balancer was removed during loop
-                    if (loadBalancer.get().state() != state) continue; // Wrong state
-                    operation.accept(loadBalancer.get());
-                }
+            try (var lock = db.lockLoadBalancers(id.application())) {
+                var loadBalancer = db.readLoadBalancer(id);
+                if (loadBalancer.isEmpty()) continue;              // Load balancer was removed during loop
+                if (loadBalancer.get().state() != state) continue; // Wrong state
+                operation.accept(loadBalancer.get());
             }
         }
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
index 6036cc2366f..8ecdb0cbb1f 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClient.java
@@ -62,7 +62,6 @@ public class CuratorDatabaseClient {
 
     private static final Path root = Path.fromString("/provision/v1");
     private static final Path lockRoot = root.append("locks");
-    private static final Path configLockRoot = Path.fromString("/config/v2/locks/");
     private static final Path loadBalancersRoot = root.append("loadBalancers");
     private static final Duration defaultLockTimeout = Duration.ofMinutes(2);
 
@@ -313,7 +312,7 @@ public class CuratorDatabaseClient {
         return root.append(toDir(nodeState)).append(nodeName);
     }
 
-    /** Creates and returns the path to the lock for this application */
+    /** Creates an returns the path to the lock for this application */
     private Path lockPath(ApplicationId application) {
         Path lockPath =
                 lockRoot
@@ -324,14 +323,6 @@ public class CuratorDatabaseClient {
         return lockPath;
     }
 
-    /** Creates and returns the path to the config server lock for this application */
-    private Path configLockPath(ApplicationId application) {
-        // This must match the lock path used by com.yahoo.vespa.config.server.application.TenantApplications
-        Path lockPath = configLockRoot.append(application.tenant().value()).append(application.serializedForm());
-        curatorDatabase.create(lockPath);
-        return lockPath;
-    }
-
     private String toDir(Node.State state) {
         switch (state) {
             case active: return "allocated"; // legacy name
@@ -367,28 +358,6 @@ public class CuratorDatabaseClient {
         }
     }
 
-    /**
-     * Acquires the single cluster-global, re-entrant config lock for given application. Note that this is the same lock
-     * that configserver itself takes when modifying applications.
-     *
-     * This lock must be taken when writes to paths owned by this class may happen on both the configserver and
-     * node-repository side. This behaviour is obviously wrong, but since we pass a NestedTransaction across the
-     * configserver and node-repository boundary, the ownership semantics of the transaction (and its operations)
-     * becomes unclear.
-     *
-     * Example of when to use: The config server creates a new transaction and passes the transaction to
-     * {@link com.yahoo.vespa.hosted.provision.provisioning.NodeRepositoryProvisioner}, which appends operations to the
-     * transaction. The config server then commits (writes) the transaction which may include operations that modify
-     * data in paths owned by this class.
-     */
-    public Lock lockConfig(ApplicationId application) {
-        try {
-            return lock(configLockPath(application), defaultLockTimeout);
-        } catch (UncheckedTimeoutException e) {
-            throw new ApplicationLockException(e);
-        }
-    }
-
     private Lock lock(Path path, Duration timeout) {
         return curatorDatabase.lock(path, timeout);
     }
@@ -556,7 +525,6 @@ public class CuratorDatabaseClient {
         transaction.commit();
     }
 
-    // TODO(mpolden): Remove this and usages after April 2020
     public Lock lockLoadBalancers(ApplicationId application) {
         return lock(lockRoot.append("loadBalancersLock2").append(application.serializedForm()), defaultLockTimeout);
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
index b694a7c3cd4..1d3fe114e23 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/LoadBalancerProvisioner.java
@@ -52,11 +52,9 @@ public class LoadBalancerProvisioner {
         this.service = service;
         // Read and write all load balancers to make sure they are stored in the latest version of the serialization format
         for (var id : db.readLoadBalancerIds()) {
-            try (var lock = db.lockConfig(id.application())) {
-                try (var legacyLock = db.lockLoadBalancers(id.application())) {
-                    var loadBalancer = db.readLoadBalancer(id);
-                    loadBalancer.ifPresent(db::writeLoadBalancer);
-                }
+            try (var lock = db.lockLoadBalancers(id.application())) {
+                var loadBalancer = db.readLoadBalancer(id);
+                loadBalancer.ifPresent(db::writeLoadBalancer);
             }
         }
     }
@@ -75,10 +73,8 @@ public class LoadBalancerProvisioner {
         if (requestedNodes.type() != NodeType.tenant) return; // Nothing to provision for this node type
         if (!cluster.type().isContainer()) return; // Nothing to provision for this cluster type
         if (application.instance().isTester()) return; // Do not provision for tester instances
-        try (var lock = db.lockConfig(application)) {
-            try (var legacyLock = db.lockLoadBalancers(application)) {
-                provision(application, effectiveId(cluster), false, lock);
-            }
+        try (var lock = db.lockLoadBalancers(application)) {
+            provision(application, effectiveId(cluster), false, lock);
         }
     }
 
@@ -94,17 +90,15 @@ public class LoadBalancerProvisioner {
      */
     public void activate(ApplicationId application, Set<ClusterSpec> clusters,
                          @SuppressWarnings("unused") Mutex applicationLock, NestedTransaction transaction) {
-        try (var lock = db.lockConfig(application)) {
-            try (var legacyLock = db.lockLoadBalancers(application)) {
-                var containerClusters = containerClustersOf(clusters);
-                for (var clusterId : containerClusters) {
-                    // Provision again to ensure that load balancer instance is re-configured with correct nodes
-                    provision(application, clusterId, true, legacyLock);
-                }
-                // Deactivate any surplus load balancers, i.e. load balancers for clusters that have been removed
-                var surplusLoadBalancers = surplusLoadBalancersOf(application, containerClusters);
-                deactivate(surplusLoadBalancers, transaction);
+        try (var lock = db.lockLoadBalancers(application)) {
+            var containerClusters = containerClustersOf(clusters);
+            for (var clusterId : containerClusters) {
+                // Provision again to ensure that load balancer instance is re-configured with correct nodes
+                provision(application, clusterId, true, lock);
             }
+            // Deactivate any surplus load balancers, i.e. load balancers for clusters that have been removed
+            var surplusLoadBalancers = surplusLoadBalancersOf(application, containerClusters);
+            deactivate(surplusLoadBalancers, transaction);
         }
     }
 
@@ -114,10 +108,8 @@ public class LoadBalancerProvisioner {
      */
     public void deactivate(ApplicationId application, NestedTransaction transaction) {
         try (var applicationLock = nodeRepository.lock(application)) {
-            try (var lock = db.lockConfig(application)) {
-                try (var legacyLock = db.lockLoadBalancers(application)) {
-                    deactivate(nodeRepository.loadBalancers(application).asList(), transaction);
-                }
+            try (var lock = db.lockLoadBalancers(application)) {
+                deactivate(nodeRepository.loadBalancers(application).asList(), transaction);
             }
         }
     }