Merge pull request #12859 from vespa-engine/jonmv/avoid-application-(dead)-lock

Avoid taking run lock and then application lock, to avoid deadlock

2 files changed, 13 insertions, 18 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 9b71439e01d..893956e5767 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -153,12 +153,7 @@ public class DeploymentTrigger {
                                  .parallel().map(Supplier::get).reduce(0L, Long::sum);
     }
 
-    /**
-     * Attempts to trigger the given job for the given application and returns the outcome.
-     *
-     * If the build service can not find the given job, or claims it is illegal to trigger it,
-     * the project id is removed from the application owning the job, to prevent further trigger attempts.
-     */
+    /** Attempts to trigger the given job. */
     public void trigger(Job job) {
         log.log(LogLevel.DEBUG, "Triggering " + job);
         applications().lockApplicationOrThrow(TenantAndApplicationId.from(job.applicationId()), application -> {
@@ -432,4 +427,3 @@ public class DeploymentTrigger {
     }
 
 }
-
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index 802b326156c..9a5f9b1074a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -416,16 +416,14 @@ public class JobController {
 
     /** Orders a run of the given type, or throws an IllegalStateException if that job type is already running. */
     public void start(ApplicationId id, JobType type, Versions versions, JobProfile profile) {
-        controller.applications().lockApplicationIfPresent(TenantAndApplicationId.from(id), application -> {
-            locked(id, type, __ -> {
-                Optional<Run> last = last(id, type);
-                if (last.flatMap(run -> active(run.id())).isPresent())
-                    throw new IllegalStateException("Can not start " + type + " for " + id + "; it is already running!");
-
-                RunId newId = new RunId(id, type, last.map(run -> run.id().number()).orElse(0L) + 1);
-                curator.writeLastRun(Run.initial(newId, versions, controller.clock().instant(), profile));
-                metric.jobStarted(newId.job());
-            });
+        locked(id, type, __ -> {
+            Optional<Run> last = last(id, type);
+            if (last.flatMap(run -> active(run.id())).isPresent())
+                throw new IllegalStateException("Can not start " + type + " for " + id + "; it is already running!");
+
+            RunId newId = new RunId(id, type, last.map(run -> run.id().number()).orElse(0L) + 1);
+            curator.writeLastRun(Run.initial(newId, versions, controller.clock().instant(), profile));
+            metric.jobStarted(newId.job());
         });
     }
 
@@ -439,7 +437,8 @@ public class JobController {
         });
 
         last(id, type).filter(run -> ! run.hasEnded()).ifPresent(run -> abortAndWait(run.id()));
-        locked(id, type, __ -> {
+
+        controller.applications().lockApplicationOrThrow(TenantAndApplicationId.from(id), application -> {
             controller.applications().applicationStore().putDev(id, type.zone(controller.system()), applicationPackage.zippedContent());
             start(id,
                   type,
@@ -450,7 +449,9 @@ public class JobController {
                                Optional.empty(),
                                Optional.empty()),
                   JobProfile.development);
+        });
 
+        locked(id, type, __ -> {
             runner.get().accept(last(id, type).get());
         });
     }