Merge pull request #8452 from vespa-engine/jvenstad/deploy-without-application-lock

Avoid holding the application lock while deploying — just when writing

2 files changed, 79 insertions, 92 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 7316a859382..30c99288d51 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -265,72 +265,75 @@ public class ApplicationController {
         if (applicationId.instance().isTester())
             throw new IllegalArgumentException("'" + applicationId + "' is a tester application!");
 
-        try (Lock lock = lockForDeployment(applicationId)) {
-            LockedApplication application = get(applicationId)
-                    .map(app -> new LockedApplication(app, lock))
-                    .orElseGet(() -> new LockedApplication(createApplication(applicationId, Optional.empty()), lock));
-
-            boolean manuallyDeployed = options.deployDirectly || zone.environment().isManuallyDeployed();
-            boolean preferOldestVersion = options.deployCurrentVersion;
-
-            // Determine versions to use.
+        try (Lock deploymentLock = lockForDeployment(applicationId, zone)) {
             Version platformVersion;
             ApplicationVersion applicationVersion;
             ApplicationPackage applicationPackage;
-            if (manuallyDeployed) {
-                applicationVersion = applicationVersionFromDeployer.orElse(ApplicationVersion.unknown);
-                applicationPackage = applicationPackageFromDeployer.orElseThrow(
-                        () -> new IllegalArgumentException("Application package must be given when deploying to " + zone));
-                platformVersion = options.vespaVersion.map(Version::new).orElse(applicationPackage.deploymentSpec().majorVersion()
-                                                                                                  .flatMap(this::lastCompatibleVersion)
-                                                                                                  .orElse(controller.systemVersion()));
-            }
-            else {
-                JobType jobType = JobType.from(controller.system(), zone)
-                                         .orElseThrow(() -> new IllegalArgumentException("No job is known for " + zone + "."));
-                Optional<JobStatus> job = Optional.ofNullable(application.get().deploymentJobs().jobStatus().get(jobType));
-                if (    ! job.isPresent()
-                     || ! job.get().lastTriggered().isPresent()
-                     ||   job.get().lastCompleted().isPresent() && job.get().lastCompleted().get().at().isAfter(job.get().lastTriggered().get().at()))
-                    return unexpectedDeployment(applicationId, zone);
-                JobRun triggered = job.get().lastTriggered().get();
-                platformVersion = preferOldestVersion
-                        ? triggered.sourcePlatform().orElse(triggered.platform())
-                        : triggered.platform();
-                applicationVersion = preferOldestVersion
-                        ? triggered.sourceApplication().orElse(triggered.application())
-                        : triggered.application();
-
-                applicationPackage = getApplicationPackage(application.get(), applicationVersion);
-                validateRun(application.get(), zone, platformVersion, applicationVersion);
-            }
+            Set<String> rotationNames = new HashSet<>();
+            Set<String> cnames = new HashSet<>();
 
-            // TODO: Remove this when all packages are validated upon submission, as in ApplicationApiHandler.submit(...).
-            verifyApplicationIdentityConfiguration(applicationId.tenant(), applicationPackage, deployingIdentity);
+            try (Lock lock = lock(applicationId)) {
+                LockedApplication application = get(applicationId)
+                        .map(app -> new LockedApplication(app, lock))
+                        .orElseGet(() -> new LockedApplication(createApplication(applicationId, Optional.empty()), lock));
+
+                boolean manuallyDeployed = options.deployDirectly || zone.environment().isManuallyDeployed();
+                boolean preferOldestVersion = options.deployCurrentVersion;
+
+                // Determine versions to use.
+                if (manuallyDeployed) {
+                    applicationVersion = applicationVersionFromDeployer.orElse(ApplicationVersion.unknown);
+                    applicationPackage = applicationPackageFromDeployer.orElseThrow(
+                            () -> new IllegalArgumentException("Application package must be given when deploying to " + zone));
+                    platformVersion = options.vespaVersion.map(Version::new).orElse(applicationPackage.deploymentSpec().majorVersion()
+                                                                                                      .flatMap(this::lastCompatibleVersion)
+                                                                                                      .orElse(controller.systemVersion()));
+                }
+                else {
+                    JobType jobType = JobType.from(controller.system(), zone)
+                                             .orElseThrow(() -> new IllegalArgumentException("No job is known for " + zone + "."));
+                    Optional<JobStatus> job = Optional.ofNullable(application.get().deploymentJobs().jobStatus().get(jobType));
+                    if (   ! job.isPresent()
+                        || ! job.get().lastTriggered().isPresent()
+                        ||   job.get().lastCompleted().isPresent() && job.get().lastCompleted().get().at().isAfter(job.get().lastTriggered().get().at()))
+                        return unexpectedDeployment(applicationId, zone);
+                    JobRun triggered = job.get().lastTriggered().get();
+                    platformVersion = preferOldestVersion ? triggered.sourcePlatform().orElse(triggered.platform())
+                                                          : triggered.platform();
+                    applicationVersion = preferOldestVersion ? triggered.sourceApplication().orElse(triggered.application())
+                                                             : triggered.application();
+
+                    applicationPackage = getApplicationPackage(application.get(), applicationVersion);
+                    validateRun(application.get(), zone, platformVersion, applicationVersion);
+                }
 
-            // Update application with information from application package
-            if ( ! preferOldestVersion &&
-                 ! application.get().deploymentJobs().deployedInternally() &&
-                 ! zone.environment().isManuallyDeployed())
-                application = storeWithUpdatedConfig(application, applicationPackage);
+                // TODO: Remove this when all packages are validated upon submission, as in ApplicationApiHandler.submit(...).
+                verifyApplicationIdentityConfiguration(applicationId.tenant(), applicationPackage, deployingIdentity);
+
+                // Assign global rotation
+                application = withRotation(application, zone);
+                Application app = application.get();
+                app.globalDnsName(controller.system()).ifPresent(applicationRotation -> {
+                    rotationNames.add(app.rotation().orElseThrow(() -> new RuntimeException("Global Dns assigned, but no rotation id present")).asString());
+                    cnames.add(applicationRotation.dnsName());
+                    cnames.add(applicationRotation.secureDnsName());
+                    cnames.add(applicationRotation.oathDnsName());
+                });
 
-            // Assign global rotation
-            application = withRotation(application, zone);
-            Set<String> rotationNames = new HashSet<>();
-            Set<String> cnames = new HashSet<>();
-            Application app = application.get();
-            app.globalDnsName(controller.system()).ifPresent(applicationRotation -> {
-                rotationNames.add(app.rotation().orElseThrow(() -> new RuntimeException("Global Dns assigned, but no rotation id present")).asString());
-                cnames.add(applicationRotation.dnsName());
-                cnames.add(applicationRotation.secureDnsName());
-                cnames.add(applicationRotation.oathDnsName());
-            });
+                // Update application with information from application package
+                if (   ! preferOldestVersion
+                    && ! application.get().deploymentJobs().deployedInternally()
+                    && ! zone.environment().isManuallyDeployed())
+                    // TODO jvenstad: Store only on submissions
+                    storeWithUpdatedConfig(application, applicationPackage);
+            } // Release application lock while doing the deployment, which is a lengthy task.
 
-            // Carry out deployment
+            // Carry out deployment without holding the application lock.
             options = withVersion(platformVersion, options);
             ActivateResult result = deploy(applicationId, applicationPackage, zone, options, rotationNames, cnames);
-            application = application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant());
-            store(application);
+
+            lockOrThrow(applicationId, application ->
+                    store(application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant())));
             return result;
         }
     }
@@ -668,13 +671,10 @@ public class ApplicationController {
     }
 
     /**
-     * Returns a lock which provides exclusive rights to changing this application, with a timeout that
-     * is suitable for deployments.
-     * Any operation which stores an application need to first acquire this lock, then read, modify
-     * and store the application, and finally release (close) the lock.
+     * Returns a lock which provides exclusive rights to deploying this application to the given zone.
      */
-    private Lock lockForDeployment(ApplicationId application) {
-        return curator.lockForDeployment(application);
+    private Lock lockForDeployment(ApplicationId application, ZoneId zone) {
+        return curator.lockForDeployment(application, zone);
     }
 
     /** Verify that each of the production zones listed in the deployment spec exist in this system. */
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index e9920c9f1c6..0f43aaece7f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -16,6 +16,7 @@ import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId;
+import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneId;
 import com.yahoo.vespa.hosted.controller.application.RoutingPolicy;
 import com.yahoo.vespa.hosted.controller.deployment.Run;
 import com.yahoo.vespa.hosted.controller.deployment.Step;
@@ -63,7 +64,7 @@ import static java.util.stream.Collectors.collectingAndThen;
 public class CuratorDb {
 
     private static final Logger log = Logger.getLogger(CuratorDb.class.getName());
-    private static final Duration deployLockTimeout = Duration.ofMinutes(20);
+    private static final Duration deployLockTimeout = Duration.ofMinutes(30);
     private static final Duration defaultLockTimeout = Duration.ofMinutes(5);
     private static final Duration defaultTryLockTimeout = Duration.ofSeconds(1);
 
@@ -119,6 +120,7 @@ public class CuratorDb {
 
     /** Creates a reentrant lock */
     private Lock lock(Path path, Duration timeout) {
+        curator.create(path);
         Lock lock = locks.computeIfAbsent(path, (pathArg) -> new Lock(pathArg.getAbsolute(), curator));
         lock.acquire(timeout);
         return lock;
@@ -132,11 +134,8 @@ public class CuratorDb {
         return lock(lockPath(id), defaultLockTimeout.multipliedBy(2));
     }
 
-    // Timeout should be higher than the time deployment takes, since there might be deployments wanting
-    // to run in parallel, too low timeout in that case has been seen to lead to deployments not
-    // getting the lock before it times out
-    public Lock lockForDeployment(ApplicationId id) {
-        return lock(lockPath(id), deployLockTimeout);
+    public Lock lockForDeployment(ApplicationId id, ZoneId zone) {
+        return lock(lockPath(id, zone), deployLockTimeout);
     }
 
     public Lock lock(ApplicationId id, JobType type) {
@@ -504,48 +503,36 @@ public class CuratorDb {
     // -------------- Paths ---------------------------------------------------
 
     private Path lockPath(TenantName tenant) {
-        Path lockPath = lockRoot
+        return lockRoot
                 .append(tenant.value());
-        curator.create(lockPath);
-        return lockPath;
     }
 
     private Path lockPath(ApplicationId application) {
-        Path lockPath = lockRoot
-                .append(application.tenant().value())
+        return lockPath(application.tenant())
                 .append(application.application().value())
                 .append(application.instance().value());
-        curator.create(lockPath);
-        return lockPath;
+    }
+
+    private Path lockPath(ApplicationId application, ZoneId zone) {
+        return lockPath(application)
+                .append(zone.environment().value())
+                .append(zone.region().value());
     }
 
     private Path lockPath(ApplicationId application, JobType type) {
-        Path lockPath = lockRoot
-                .append(application.tenant().value())
-                .append(application.application().value())
-                .append(application.instance().value())
+        return lockPath(application)
                 .append(type.jobName());
-        curator.create(lockPath);
-        return lockPath;
     }
 
     private Path lockPath(ApplicationId application, JobType type, Step step) {
-        Path lockPath = lockRoot
-                .append(application.tenant().value())
-                .append(application.application().value())
-                .append(application.instance().value())
-                .append(type.jobName())
+        return lockPath(application, type)
                 .append(step.name());
-        curator.create(lockPath);
-        return lockPath;
     }
 
     private Path lockPath(String provisionId) {
-        Path lockPath = lockRoot
+        return lockRoot
                 .append(provisionStatePath())
                 .append(provisionId);
-        curator.create(lockPath);
-        return lockPath;
     }
 
     private static Path inactiveJobsPath() {