Merge pull request #14718 from vespa-engine/mpolden/double-locking

Take both application locks during transition

3 files changed, 30 insertions, 38 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 e8a4620b520..48ca63b4d9e 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
@@ -156,7 +156,7 @@ public class NodeRepository extends AbstractComponent {
         // Flag is read once here as it shouldn't not change at runtime
         this.useConfigServerLock = Flags.USE_CONFIG_SERVER_LOCK.bindTo(flagSource).value();
         long nodeObjectCacheSize = Flags.NODE_OBJECT_CACHE_SIZE.bindTo(flagSource).value();
-        this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, useCuratorClientCache, useConfigServerLock, nodeObjectCacheSize);
+        this.db = new CuratorDatabaseClient(flavors, curator, clock, zone, useCuratorClientCache, nodeObjectCacheSize);
         this.zone = zone;
         this.clock = clock;
         this.flavors = flavors;
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 160ab86591a..7a3267604fb 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
@@ -11,6 +11,7 @@ import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.path.Path;
+import com.yahoo.transaction.Mutex;
 import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.curator.Curator;
@@ -77,16 +78,14 @@ public class CuratorDatabaseClient {
     private final Clock clock;
     private final Zone zone;
     private final CuratorCounter provisionIndexCounter;
-    private final boolean logStackTracesOnLockTimeout;
 
-    public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, boolean useCache, boolean logStackTracesOnLockTimeout,
+    public CuratorDatabaseClient(NodeFlavors flavors, Curator curator, Clock clock, Zone zone, boolean useCache,
                                  long nodeObjectCacheSize) {
         this.nodeSerializer = new NodeSerializer(flavors, nodeObjectCacheSize);
         this.zone = zone;
         this.db = new CuratorDatabase(curator, root, useCache);
         this.clock = clock;
         this.provisionIndexCounter = new CuratorCounter(curator, root.append("provisionIndexCounter").getAbsolute());
-        this.logStackTracesOnLockTimeout = logStackTracesOnLockTimeout;
         initZK();
     }
 
@@ -392,31 +391,33 @@ public class CuratorDatabaseClient {
      * transaction. The config server then commits (writes) the transaction which may include operations that modify
      * data in paths owned by this class.
      */
-    public Lock lock(ApplicationId application, Duration timeout) {
+    // TODO(mpolden): Simplify once we are down to one application lock
+    public Mutex lock(ApplicationId application, Duration timeout) {
+        Mutex legacyLock;
+        Mutex lock;
+        // Take the application lock (same as config server). This is likely held at this point, but is re-entrant.
         try {
-            return db.lock(lockPath(application), timeout);
+            lock = db.lock(lockPath(application), timeout);
         } catch (UncheckedTimeoutException e) {
-            if (logStackTracesOnLockTimeout) {
-                log.log(Level.WARNING, "Logging stack trace from all threads due to lock timeout");
-                Map<Thread, StackTraceElement[]> stackTraces = Thread.getAllStackTraces();
-                for (Map.Entry<Thread, StackTraceElement[]> kv : stackTraces.entrySet()) {
-                    StringBuilder sb = new StringBuilder();
-                    sb.append("Thread '")
-                      .append(kv.getKey().getName())
-                      .append("'\n");
-                    for (var stackTraceElement : kv.getValue()) {
-                        sb.append("\tat ")
-                          .append(stackTraceElement)
-                          .append("\n");
-                    }
-                    log.log(Level.WARNING, sb.toString());
-                }
-            }
             throw new ApplicationLockException(e);
         }
+        // Take the legacy node-repository lock
+        try {
+            legacyLock = db.lock(legacyLockPath(application), timeout);
+        } catch (UncheckedTimeoutException e) {
+            lock.close();
+            throw new ApplicationLockException(e);
+        }
+        return () -> {
+            try {
+                legacyLock.close();
+            } finally {
+                lock.close();
+            }
+        };
     }
 
-    public Lock lock(ApplicationId application) {
+    public Mutex lock(ApplicationId application) {
         return lock(application, defaultLockTimeout);
     }
 
@@ -424,7 +425,7 @@ public class CuratorDatabaseClient {
 
     public List<ApplicationId> readApplicationIds() {
         return db.getChildren(applicationsPath).stream()
-                 .map(path -> ApplicationId.fromSerializedForm(path))
+                 .map(ApplicationId::fromSerializedForm)
                  .collect(Collectors.toList());
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
index c7535f04c4f..b60364d903e 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/CuratorDatabaseClientTest.java
@@ -8,7 +8,6 @@ import com.yahoo.config.provision.NodeType;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.Zone;
 import com.yahoo.vespa.curator.Curator;
-import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.hosted.provision.Node;
 import com.yahoo.vespa.hosted.provision.provisioning.FlavorConfigBuilder;
@@ -26,7 +25,7 @@ public class CuratorDatabaseClientTest {
 
     private final Curator curator = new MockCurator();
     private final CuratorDatabaseClient zkClient = new CuratorDatabaseClient(
-            FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), Zone.defaultZone(), true, false, 1000);
+            FlavorConfigBuilder.createDummies("default"), curator, Clock.systemUTC(), Zone.defaultZone(), true, 1000);
 
     @Test
     public void can_read_stored_host_information() throws Exception {
@@ -41,22 +40,14 @@ public class CuratorDatabaseClientTest {
     @Test
     public void locks_can_be_acquired_and_released() {
         ApplicationId app = ApplicationId.from(TenantName.from("testTenant"), ApplicationName.from("testApp"), InstanceName.from("testInstance"));
-
-        try (Lock mutex1 = zkClient.lock(app)) {
-            mutex1.toString(); // reference to avoid warning
+        try (var ignored = zkClient.lock(app)) {
             throw new RuntimeException();
+        } catch (RuntimeException expected) {
         }
-        catch (RuntimeException expected) {
+        try (var ignored = zkClient.lock(app)) {
         }
-
-        try (Lock mutex2 = zkClient.lock(app)) {
-            mutex2.toString(); // reference to avoid warning
+        try (var ignored = zkClient.lock(app)) {
         }
-
-        try (Lock mutex3 = zkClient.lock(app)) {
-            mutex3.toString(); // reference to avoid warning
-        }
-
     }
 
  }