Merge pull request #15081 from vespa-engine/mpolden/reorder-locking

Take provision lock before session lock

2 files changed, 53 insertions, 26 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index 10d566910fa..42c5d80bd9b 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -65,7 +65,6 @@ import com.yahoo.vespa.config.server.tenant.Tenant;
 import com.yahoo.vespa.config.server.tenant.TenantMetaData;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.curator.Curator;
-import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.curator.stats.LockStats;
 import com.yahoo.vespa.curator.stats.ThreadLockStats;
 import com.yahoo.vespa.defaults.Defaults;
@@ -90,6 +89,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.OptionalLong;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.logging.Level;
@@ -476,7 +476,10 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         if (tenant == null) return false;
 
         TenantApplications tenantApplications = tenant.getApplicationRepo();
-        try (Lock lock = tenantApplications.lock(applicationId)) {
+        NestedTransaction transaction = new NestedTransaction();
+        Optional<ApplicationTransaction> applicationTransaction = hostProvisioner.map(provisioner -> provisioner.lock(applicationId))
+                                                                                 .map(lock -> new ApplicationTransaction(lock, transaction));
+        try (var sessionLock = tenantApplications.lock(applicationId)) {
             Optional<Long> activeSession = tenantApplications.activeSessionOf(applicationId);
             if (activeSession.isEmpty()) return false;
 
@@ -489,7 +492,6 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
                 log.log(Level.INFO, TenantRepository.logPre(applicationId) + "Active session exists, but has not been deleted properly. Trying to cleanup");
             }
 
-            NestedTransaction transaction = new NestedTransaction();
             Curator curator = tenantRepository.getCurator();
             transaction.add(new ContainerEndpointsCache(tenant.getPath(), curator).delete(applicationId)); // TODO: Not unit tested
             // Delete any application roles
@@ -501,14 +503,14 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
             transaction.add(tenantApplications.createDeleteTransaction(applicationId));
             transaction.onCommitted(() -> log.log(Level.INFO, "Deleted " + applicationId));
 
-            if (hostProvisioner.isPresent()) {
-                try (var applicationTransaction = new ApplicationTransaction(hostProvisioner.get().lock(applicationId), transaction)) {
-                    hostProvisioner.get().remove(applicationTransaction);
-                }
+            if (applicationTransaction.isPresent()) {
+                hostProvisioner.get().remove(applicationTransaction.get());
             } else {
                 transaction.commit();
             }
             return true;
+        } finally {
+            applicationTransaction.ifPresent(ApplicationTransaction::close); // Commits transaction and releases lock
         }
     }
 
@@ -719,20 +721,26 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     // ---------------- Session operations ----------------------------------------------------------------
 
-    public CompletionWaiter activate(Session session, Session previousActiveSession, ApplicationId applicationId, boolean force) {
-        CompletionWaiter waiter = session.getSessionZooKeeperClient().createActiveWaiter();
+    public Activation activate(Session session, ApplicationId applicationId, Tenant tenant, boolean force) {
         NestedTransaction transaction = new NestedTransaction();
-        transaction.add(deactivateCurrentActivateNew(previousActiveSession, session, force));
-        if (hostProvisioner.isPresent()) {
-            try (var applicationTransaction = new ApplicationTransaction(hostProvisioner.get().lock(applicationId), transaction)) {
+        Optional<ApplicationTransaction> applicationTransaction = hostProvisioner.map(provisioner -> provisioner.lock(applicationId))
+                                                                                 .map(lock -> new ApplicationTransaction(lock, transaction));
+        try (var sessionLock = tenant.getApplicationRepo().lock(applicationId)) {
+            Session activeSession = getActiveSession(applicationId);
+            CompletionWaiter waiter = session.getSessionZooKeeperClient().createActiveWaiter();
+
+            transaction.add(deactivateCurrentActivateNew(activeSession, session, force));
+            if (applicationTransaction.isPresent()) {
                 hostProvisioner.get().activate(session.getAllocatedHosts().getHosts(),
                                                new ActivationContext(session.getSessionId()),
-                                               applicationTransaction);
+                                               applicationTransaction.get());
+            } else {
+                transaction.commit();
             }
-        } else {
-            transaction.commit();
+            return new Activation(waiter, activeSession);
+        } finally {
+            applicationTransaction.ifPresent(ApplicationTransaction::close); // Commits transaction and releases lock
         }
-        return waiter;
     }
 
     /**
@@ -1072,4 +1080,27 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
 
     }
 
+    public static class Activation {
+
+        private final CompletionWaiter waiter;
+        private final OptionalLong sourceSessionId;
+
+        public Activation(CompletionWaiter waiter, Session sourceSession) {
+            this.waiter = waiter;
+            this.sourceSessionId = sourceSession == null
+                    ? OptionalLong.empty()
+                    : OptionalLong.of(sourceSession.getSessionId());
+        }
+
+        public void awaitCompletion(Duration timeout) {
+            waiter.awaitCompletion(timeout);
+        }
+
+        /** The session ID this activation was based on, if any */
+        public OptionalLong sourceSessionId() {
+            return sourceSessionId;
+        }
+
+    }
+
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
index 7d58682947f..bf2d265dd16 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
@@ -10,6 +10,7 @@ import com.yahoo.config.provision.HostFilter;
 import com.yahoo.config.provision.Provisioner;
 import com.yahoo.vespa.config.server.ApplicationRepository;
 import com.yahoo.vespa.config.server.ApplicationRepository.ActionTimer;
+import com.yahoo.vespa.config.server.ApplicationRepository.Activation;
 import com.yahoo.vespa.config.server.TimeoutBudget;
 import com.yahoo.vespa.config.server.configchange.ConfigChangeActions;
 import com.yahoo.vespa.config.server.configchange.RestartActions;
@@ -17,7 +18,6 @@ import com.yahoo.vespa.config.server.http.InternalServerException;
 import com.yahoo.vespa.config.server.session.PrepareParams;
 import com.yahoo.vespa.config.server.session.Session;
 import com.yahoo.vespa.config.server.tenant.Tenant;
-import com.yahoo.vespa.curator.Lock;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -27,8 +27,6 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
-import static com.yahoo.vespa.curator.Curator.CompletionWaiter;
-
 /**
  * The process of deploying an application.
  * Deployments are created by an {@link ApplicationRepository}.
@@ -111,11 +109,9 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
             TimeoutBudget timeoutBudget = params.getTimeoutBudget();
             timeoutBudget.assertNotTimedOut(() -> "Timeout exceeded when trying to activate '" + applicationId + "'");
 
-            Session previousActiveSession;
-            CompletionWaiter waiter;
-            try (Lock lock = tenant.getApplicationRepo().lock(applicationId)) {
-                previousActiveSession = applicationRepository.getActiveSession(applicationId);
-                waiter = applicationRepository.activate(session, previousActiveSession, applicationId, params.force());
+            Activation activation;
+            try {
+                activation = applicationRepository.activate(session, applicationId, tenant, params.force());
             }
             catch (RuntimeException e) {
                 throw e;
@@ -124,11 +120,11 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
                 throw new InternalServerException("Error when activating '" + applicationId + "'", e);
             }
 
-            waiter.awaitCompletion(timeoutBudget.timeLeft());
+            activation.awaitCompletion(timeoutBudget.timeLeft());
             log.log(Level.INFO, session.logPre() + "Session " + session.getSessionId() + " activated successfully using " +
                                 provisioner.map(provisioner -> provisioner.getClass().getSimpleName()).orElse("no host provisioner") +
                                 ". Config generation " + session.getMetaData().getGeneration() +
-                                (previousActiveSession != null ? ". Based on session " + previousActiveSession.getSessionId() : "") +
+                                activation.sourceSessionId().stream().mapToObj(id -> ". Based on session " + id).findFirst().orElse("") +
                                 ". File references: " + applicationRepository.getFileReferences(applicationId));
 
             if (configChangeActions != null && provisioner.isPresent()) {