Use curator lock when assigning rotation

With multi controller deployments can happen on any controller, so lock must be
taken cluster-wide when assigning rotation.

5 files changed, 75 insertions, 35 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 a489b0f4e91..9767ae57bf0 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
@@ -48,6 +48,7 @@ import com.yahoo.vespa.hosted.controller.maintenance.DeploymentExpirer;
 import com.yahoo.vespa.hosted.controller.persistence.ControllerDb;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.controller.rotation.Rotation;
+import com.yahoo.vespa.hosted.controller.rotation.RotationLock;
 import com.yahoo.vespa.hosted.controller.rotation.RotationRepository;
 import com.yahoo.vespa.hosted.rotation.config.RotationsConfig;
 import com.yahoo.yolean.Exceptions;
@@ -96,8 +97,6 @@ public class ApplicationController {
 
     private final DeploymentTrigger deploymentTrigger;
 
-    private final Object monitor = new Object();
-    
     ApplicationController(Controller controller, ControllerDb db, CuratorDb curator,
                           AthenzClientFactory zmsClientFactory, RotationsConfig rotationsConfig,
                           NameService nameService, ConfigServerClient configserverClient,
@@ -111,7 +110,7 @@ public class ApplicationController {
         this.routingGenerator = routingGenerator;
         this.clock = clock;
 
-        this.rotationRepository = new RotationRepository(rotationsConfig, this);
+        this.rotationRepository = new RotationRepository(rotationsConfig, this, curator);
         this.deploymentTrigger = new DeploymentTrigger(controller, curator, clock);
 
         for (Application application : db.listApplications()) {
@@ -330,15 +329,13 @@ public class ApplicationController {
                                                        " the current version " + existingDeployment.version());
             }
 
-            // Synchronize rotation assignment. Rotation can only be considered assigned once application has been
-            // persisted.
             Optional<Rotation> rotation;
-            synchronized (monitor) {
-                rotation = getRotation(application, zone);
+            try (RotationLock rotationLock = rotationRepository.lock()) {
+                rotation = getRotation(application, zone, rotationLock);
                 if (rotation.isPresent()) {
                     application = application.with(rotation.get().id());
                     store(application); // store assigned rotation even if deployment fails
-                    registerRotationInDns(application.rotation().get(), rotation.get());
+                    registerRotationInDns(rotation.get(), application.rotation().get().dnsName());
                 }
             }
 
@@ -453,14 +450,13 @@ public class ApplicationController {
     }
 
     /** Register a DNS name for rotation */
-    private void registerRotationInDns(ApplicationRotation applicationRotation, Rotation rotation) {
-        String dnsName = applicationRotation.dnsName();
+    private void registerRotationInDns(Rotation rotation, String dnsName) {
         try {
             Optional<Record> record = nameService.findRecord(Record.Type.CNAME, dnsName);
             if (!record.isPresent()) {
-                RecordId recordId = nameService.createCname(dnsName, rotation.name());
-                log.info("Registered mapping with record ID " + recordId.asString() + ": " +
-                         dnsName + " -> " + rotation.name());
+                RecordId id = nameService.createCname(dnsName, rotation.name());
+                log.info("Registered mapping with record ID " + id.asString() + ": " + dnsName + " -> "
+                         + rotation.name());
             }
         } catch (RuntimeException e) {
             log.log(Level.WARNING, "Failed to register CNAME", e);
@@ -468,12 +464,12 @@ public class ApplicationController {
     }
 
     /** Get an available rotation, if deploying to a production zone and a service ID is specified */
-    private Optional<Rotation> getRotation(Application application, Zone zone) {
+    private Optional<Rotation> getRotation(Application application, Zone zone, RotationLock lock) {
         if (zone.environment() != Environment.prod ||
             !application.deploymentSpec().globalServiceId().isPresent()) {
             return Optional.empty();
         }
-        return Optional.of(rotationRepository.getRotation(application));
+        return Optional.of(rotationRepository.getRotation(application, lock));
     }
 
     /** Returns the endpoints of the deployment, or empty if obtaining them failed */
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 95db401d15b..a3bb191fc38 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
@@ -39,6 +39,8 @@ public class CuratorDb {
 
     private static final Path root = Path.fromString("/controller/v1");
 
+    private static final Path lockRoot = root.append("locks");
+
     private static final Duration defaultLockTimeout = Duration.ofMinutes(5);
 
     private final StringSetSerializer stringSetSerializer = new StringSetSerializer();
@@ -67,6 +69,10 @@ public class CuratorDb {
         return lock(lockPath(id), timeout);
     }
 
+    public Lock lockRotations() {
+        return lock(lockRoot.append("rotations"), defaultLockTimeout);
+    }
+
     /** Create a reentrant lock */
     private Lock lock(Path path, Duration timeout) {
         Lock lock = locks.computeIfAbsent(path, (pathArg) -> new Lock(pathArg.getAbsolute(), curator));
@@ -75,18 +81,18 @@ public class CuratorDb {
     }
 
     public Lock lockInactiveJobs() {
-        return lock(root.append("locks").append("inactiveJobsLock"), defaultLockTimeout);
+        return lock(lockRoot.append("inactiveJobsLock"), defaultLockTimeout);
     }
 
     public Lock lockJobQueues() {
-        return lock(root.append("locks").append("jobQueuesLock"), defaultLockTimeout);
+        return lock(lockRoot.append("jobQueuesLock"), defaultLockTimeout);
     }
 
     public Lock lockMaintenanceJob(String jobName) {
         // Use a short timeout such that if maintenance jobs are started at about the same time on different nodes
         // and the maintenance job takes a long time to complete, only one of the nodes will run the job
         // in each maintenance interval
-        return lock(root.append("locks").append("maintenanceJobLocks").append(jobName), Duration.ofSeconds(1));
+        return lock(lockRoot.append("maintenanceJobLocks").append(jobName), Duration.ofSeconds(1));
     }
 
     public Lock lockProvisionState(String provisionStateId) {
@@ -94,11 +100,11 @@ public class CuratorDb {
     }
 
     public Lock lockVespaServerPool() {
-        return lock(root.append("locks").append("vespaServerPoolLock"), Duration.ofSeconds(1));
+        return lock(lockRoot.append("vespaServerPoolLock"), Duration.ofSeconds(1));
     }
 
     public Lock lockOpenStackServerPool() {
-        return lock(root.append("locks").append("openStackServerPoolLock"), Duration.ofSeconds(1));
+        return lock(lockRoot.append("openStackServerPoolLock"), Duration.ofSeconds(1));
     }
 
     // -------------- Read and write --------------------------------------------------
@@ -223,14 +229,14 @@ public class CuratorDb {
     // -------------- Paths --------------------------------------------------
 
     private Path lockPath(TenantId tenant) {
-        Path lockPath = root.append("locks")
+        Path lockPath = lockRoot
                 .append(tenant.id());
         curator.create(lockPath);
         return lockPath;
     }
 
     private Path lockPath(ApplicationId application) {
-        Path lockPath = root.append("locks")
+        Path lockPath = lockRoot
                 .append(application.tenant().value())
                 .append(application.application().value())
                 .append(application.instance().value());
@@ -239,7 +245,7 @@ public class CuratorDb {
     }
 
     private Path lockPath(String provisionId) {
-        Path lockPath = root.append("locks")
+        Path lockPath = lockRoot
                 .append(provisionStatePath())
                 .append(provisionId);
         curator.create(lockPath);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationLock.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationLock.java
new file mode 100644
index 00000000000..508df263837
--- /dev/null
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationLock.java
@@ -0,0 +1,25 @@
+// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.hosted.controller.rotation;
+
+import com.yahoo.vespa.curator.Lock;
+
+import java.util.Objects;
+
+/**
+ * A lock for the rotation repository. This is a type-safe wrapper for a curator lock.
+ *
+ * @author mpolden
+ */
+public class RotationLock implements AutoCloseable {
+
+    private final Lock lock;
+
+    RotationLock(Lock lock) {
+        this.lock = Objects.requireNonNull(lock, "lock cannot be null");
+    }
+
+    @Override
+    public void close() {
+        lock.close();
+    }
+}
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
index 70836232417..e70d177a641 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/rotation/RotationRepository.java
@@ -5,6 +5,7 @@ import com.yahoo.config.application.api.DeploymentSpec;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.vespa.hosted.controller.Application;
 import com.yahoo.vespa.hosted.controller.ApplicationController;
+import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 import com.yahoo.vespa.hosted.rotation.config.RotationsConfig;
 
 import java.util.ArrayList;
@@ -32,10 +33,17 @@ public class RotationRepository {
 
     private final Map<RotationId, Rotation> allRotations;
     private final ApplicationController applications;
+    private final CuratorDb curator;
 
-    public RotationRepository(RotationsConfig rotationsConfig, ApplicationController applications) {
+    public RotationRepository(RotationsConfig rotationsConfig, ApplicationController applications, CuratorDb curator) {
         this.allRotations = from(rotationsConfig);
         this.applications = applications;
+        this.curator = curator;
+    }
+
+    /** Acquire a exclusive lock for this */
+    public RotationLock lock() {
+        return new RotationLock(curator.lockRotations());
     }
 
     /**
@@ -44,9 +52,10 @@ public class RotationRepository {
      * If a rotation is already assigned to the application, that rotation will be returned.
      * If no rotation is assigned, return an available rotation. The caller is responsible for assigning the rotation.
      *
-     * @param application The application to get a rotation for
+     * @param application The application requesting a rotation
+     * @param lock Lock which must be acquired by the caller
      */
-    public Rotation getRotation(Application application) {
+    public Rotation getRotation(Application application, @SuppressWarnings("unused") RotationLock lock) {
         if (application.rotation().isPresent()) {
             return allRotations.get(application.rotation().get().id());
         }
@@ -54,10 +63,10 @@ public class RotationRepository {
             throw new IllegalArgumentException("global-service-id is not set in deployment spec");
         }
         long productionZones = application.deploymentSpec().zones().stream()
-                                              .filter(zone -> zone.deploysTo(Environment.prod))
-                                              // Global rotations don't work for nodes in corp network
-                                              .filter(zone -> !isCorp(zone))
-                                              .count();
+                                          .filter(zone -> zone.deploysTo(Environment.prod))
+                                          // Global rotations don't work for nodes in corp network
+                                          .filter(zone -> !isCorp(zone))
+                                          .count();
         if (productionZones < 2) {
             throw new IllegalArgumentException("global-service-id is set but less than 2 prod zones are defined");
         }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java
index 8fc41b8daa6..c259ae0ca60 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/rotation/RotationTest.java
@@ -67,8 +67,10 @@ public class RotationTest {
         assertEquals(expected.id(), application.rotation().get().id());
         assertEquals(URI.create("http://app1.tenant1.global.vespa.yahooapis.com:4080/"),
                      application.rotation().get().url());
-        Rotation rotation = repository.getRotation(tester.applications().require(application.id()));
-        assertEquals(expected, rotation);
+        try (RotationLock lock = repository.lock()) {
+            Rotation rotation = repository.getRotation(tester.applications().require(application.id()), lock);
+            assertEquals(expected, rotation);
+        }
 
         // Deploying once more assigns same rotation
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
@@ -90,9 +92,11 @@ public class RotationTest {
         tester.deployCompletely(application, applicationPackage);
         application = tester.applications().require(application.id());
 
-        Rotation rotation = repository.getRotation(application);
-        Rotation assignedRotation = new Rotation(new RotationId("foo-1"), "foo-1.com");
-        assertEquals(assignedRotation, rotation);
+        try (RotationLock lock = repository.lock()) {
+            Rotation rotation = repository.getRotation(application, lock);
+            Rotation assignedRotation = new Rotation(new RotationId("foo-1"), "foo-1.com");
+            assertEquals(assignedRotation, rotation);
+        }
     }