Grab a deployment lock to deploy

2 files changed, 86 insertions, 79 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 66ba44ec129..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,76 +265,77 @@ public class ApplicationController {
         if (applicationId.instance().isTester())
             throw new IllegalArgumentException("'" + applicationId + "' is a tester application!");
 
-        Version platformVersion;
-        ApplicationVersion applicationVersion;
-        ApplicationPackage applicationPackage;
-        Set<String> rotationNames = new HashSet<>();
-        Set<String> cnames = new HashSet<>();
-
-        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.
-            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);
-            }
-
-            // 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());
-            });
+        try (Lock deploymentLock = lockForDeployment(applicationId, zone)) {
+            Version platformVersion;
+            ApplicationVersion applicationVersion;
+            ApplicationPackage applicationPackage;
+            Set<String> rotationNames = new HashSet<>();
+            Set<String> cnames = new HashSet<>();
+
+            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())
-                // TODO jvenstad: Store only on submissions
-                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());
+                });
 
+                // 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 without holding the application lock.
+            options = withVersion(platformVersion, options);
+            ActivateResult result = deploy(applicationId, applicationPackage, zone, options, rotationNames, cnames);
+
+            lockOrThrow(applicationId, application ->
+                    store(application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant())));
+            return result;
         }
-
-        // Carry out deployment without holding the application lock.
-        options = withVersion(platformVersion, options);
-        ActivateResult result = deploy(applicationId, applicationPackage, zone, options, rotationNames, cnames);
-
-        lockOrThrow(applicationId, application ->
-                store(application.withNewDeployment(zone, applicationVersion, platformVersion, clock.instant())));
-        return result;
     }
 
     /** Fetches the requested application package from the artifact store(s). */
@@ -670,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..2ec5873ef58 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);
 
@@ -132,11 +133,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) {
@@ -519,6 +517,17 @@ public class CuratorDb {
         return lockPath;
     }
 
+    private Path lockPath(ApplicationId application, ZoneId zone) {
+        Path lockPath = lockRoot
+                .append(application.tenant().value())
+                .append(application.application().value())
+                .append(application.instance().value())
+                .append(zone.environment().value())
+                .append(zone.region().value());
+        curator.create(lockPath);
+        return lockPath;
+    }
+
     private Path lockPath(ApplicationId application, JobType type) {
         Path lockPath = lockRoot
                 .append(application.tenant().value())