Merge pull request #4360 from vespa-engine/mpolden/curator-lock-for-rotation-repository

Use curator lock when assigning rotation

10 files changed, 108 insertions, 66 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/ConfigServerClientMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
index 9228e83bbc6..bf7f19a996c 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerClientMock.java
@@ -22,7 +22,6 @@ import com.yahoo.vespa.serviceview.bindings.ApplicationView;
 import com.yahoo.vespa.serviceview.bindings.ClusterView;
 import com.yahoo.vespa.serviceview.bindings.ServiceView;
 
-import java.io.IOException;
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -43,16 +42,14 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     private final Map<ApplicationId, Boolean> applicationActivated = new HashMap<>();
     private final Map<String, EndpointStatus> endpoints = new HashMap<>();
     private final Map<URI, Version> versions = new HashMap<>();
-    private Version defaultVersion = new Version(6, 1, 0);
 
-    /** The exception to throw on the next prepare run, or null to continue normally */
+    private Version defaultVersion = new Version(6, 1, 0);
     private RuntimeException prepareException = null;
-    
-    private Optional<Version> lastPrepareVersion = Optional.empty();
+    private Version lastPrepareVersion = null;
 
     /** The version given in the previous prepare call, or empty if no call has been made */
     public Optional<Version> lastPrepareVersion() {
-        return lastPrepareVersion;
+        return Optional.ofNullable(lastPrepareVersion);
     }
 
     /** Return map of applications that may have been activated */
@@ -60,6 +57,7 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
         return Collections.unmodifiableMap(applicationActivated);
     }
 
+    /** The exception to throw on the next prepare run, or null to continue normally */
     public void throwOnNextPrepare(RuntimeException prepareException) {
         this.prepareException = prepareException;
     }
@@ -71,10 +69,16 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     public Map<URI, Version> versions() {
         return versions;
     }
+
+    /** Set the default config server version */
+    public void setDefaultVersion(Version version) {
+        this.defaultVersion = version;
+    }
     
     @Override
-    public PreparedApplication prepare(DeploymentId deployment, DeployOptions deployOptions, Set<String> rotationCnames, Set<Rotation> rotations, byte[] content) {
-        lastPrepareVersion = deployOptions.vespaVersion.map(Version::new);
+    public PreparedApplication prepare(DeploymentId deployment, DeployOptions deployOptions, Set<String> rotationCnames,
+                                       Set<Rotation> rotations, byte[] content) {
+        lastPrepareVersion = deployOptions.vespaVersion.map(Version::new).orElse(null);
         if (prepareException != null) {
             RuntimeException prepareException = this.prepareException;
             this.prepareException = null;
@@ -108,23 +112,20 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
             public PrepareResponse prepareResponse() {
                 PrepareResponse prepareResponse = new PrepareResponse();
                 prepareResponse.message = "foo";
-                prepareResponse.configChangeActions = new ConfigChangeActions(Collections.emptyList(), Collections.emptyList());
+                prepareResponse.configChangeActions = new ConfigChangeActions(Collections.emptyList(),
+                                                                              Collections.emptyList());
                 prepareResponse.tenant = new TenantId("tenant");
                 return prepareResponse;
             }
         };
     }
-
-    /** Set the default config server version */
-    public void setDefaultVersion(Version version) { this.defaultVersion = version; }
     
     @Override
     public List<String> getNodeQueryHost(DeploymentId deployment, String type) {
         if (applicationInstances.containsKey(deployment.applicationId())) {
             return Collections.singletonList(applicationInstances.get(deployment.applicationId()));
-        } else {
-            return Collections.emptyList();
         }
+        return Collections.emptyList();
     }
 
     @Override
@@ -151,7 +152,8 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
 
     // Returns a canned example response
     @Override
-    public ApplicationView getApplicationView(String tenantName, String applicationName, String instanceName, String environment, String region) {
+    public ApplicationView getApplicationView(String tenantName, String applicationName, String instanceName,
+                                              String environment, String region) {
         ApplicationView applicationView = new ApplicationView();
         ClusterView cluster = new ClusterView();
         cluster.name = "cluster1";
@@ -172,7 +174,8 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
 
     // Returns a canned example response
     @Override
-    public Map<?,?> getServiceApiResponse(String tenantName, String applicationName, String instanceName, String environment, String region, String serviceName, String restPath) {
+    public Map<?,?> getServiceApiResponse(String tenantName, String applicationName, String instanceName,
+                                          String environment, String region, String serviceName, String restPath) {
         Map<String,List<?>> root = new HashMap<>();
         List<Map<?,?>> resources = new ArrayList<>();
         Map<String,String> resource = new HashMap<>();
@@ -199,7 +202,7 @@ public class ConfigServerClientMock extends AbstractComponent implements ConfigS
     }
 
     @Override
-    public NodeList getNodeList(DeploymentId deployment) throws IOException {
+    public NodeList getNodeList(DeploymentId deployment) {
         NodeList list = new NodeList();
         list.nodes = new ArrayList<>();
         NodeList.Node hostA = new NodeList.Node();
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerProxyMock.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerProxyMock.java
index cc915d4d9a1..02b33e4640a 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerProxyMock.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ConfigServerProxyMock.java
@@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.controller;
 import com.yahoo.component.AbstractComponent;
 import com.yahoo.container.jdisc.HttpResponse;
 import com.yahoo.vespa.hosted.controller.proxy.ConfigServerRestExecutor;
-import com.yahoo.vespa.hosted.controller.proxy.ProxyException;
 import com.yahoo.vespa.hosted.controller.proxy.ProxyRequest;
 import com.yahoo.vespa.hosted.controller.restapi.StringResponse;
 
@@ -21,7 +20,7 @@ public class ConfigServerProxyMock extends AbstractComponent implements ConfigSe
     private volatile String requestBody = null;
 
     @Override
-    public HttpResponse handle(ProxyRequest proxyRequest) throws ProxyException {
+    public HttpResponse handle(ProxyRequest proxyRequest) {
         lastReceived = proxyRequest;
         // Copy request body as the input stream is drained once the request completes
         requestBody = asString(proxyRequest.getData());
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializerTest.java
index 189b3a97a80..c668bde0d40 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/VersionStatusSerializerTest.java
@@ -21,7 +21,7 @@ import static org.junit.Assert.assertEquals;
 public class VersionStatusSerializerTest {
 
     @Test
-    public void testSerialization() throws Exception {
+    public void testSerialization() {
         List<VespaVersion> vespaVersions = new ArrayList<>();
         DeploymentStatistics statistics = new DeploymentStatistics(
                 Version.fromString("5.0"),
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
index fb0adcd3152..9f5de1e460b 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerControllerTester.java
@@ -45,18 +45,16 @@ import java.util.Optional;
 public class ContainerControllerTester {
 
     private final ContainerTester containerTester;
-    private final Controller controller;
     private final Upgrader upgrader;
 
     public ContainerControllerTester(JDisc container, String responseFilePath) {
         containerTester = new ContainerTester(container, responseFilePath);
-        controller = (Controller)container.components().getComponent("com.yahoo.vespa.hosted.controller.Controller");
         CuratorDb curatorDb = new MockCuratorDb();
         curatorDb.writeUpgradesPerMinute(100);
-        upgrader = new Upgrader(controller, Duration.ofDays(1), new JobControl(curatorDb), curatorDb);
+        upgrader = new Upgrader(controller(), Duration.ofDays(1), new JobControl(curatorDb), curatorDb);
     }
 
-    public Controller controller() { return controller; }
+    public Controller controller() { return containerTester.controller(); }
 
     public Upgrader upgrader() { return upgrader; }
 
@@ -70,18 +68,18 @@ public class ContainerControllerTester {
 
     public Application createApplication(String athensDomain, String tenant, String application) {
         AthenzDomain domain1 = addTenantAthenzDomain(athensDomain, "mytenant");
-        controller.tenants().addTenant(Tenant.createAthensTenant(new TenantId(tenant), domain1,
+        controller().tenants().addTenant(Tenant.createAthensTenant(new TenantId(tenant), domain1,
                                                                  new Property("property1"),
                                                                  Optional.of(new PropertyId("1234"))),
                                        Optional.of(TestIdentities.userNToken));
         ApplicationId app = ApplicationId.from(tenant, application, "default");
-        return controller.applications().createApplication(app, Optional.of(TestIdentities.userNToken));
+        return controller().applications().createApplication(app, Optional.of(TestIdentities.userNToken));
     }
 
     public Application deploy(Application application, ApplicationPackage applicationPackage, Zone zone, long projectId) {
         ScrewdriverId app1ScrewdriverId = new ScrewdriverId(String.valueOf(projectId));
         GitRevision app1RevisionId = new GitRevision(new GitRepository("repo"), new GitBranch("master"), new GitCommit("commit1"));
-        controller.applications().deployApplication(application.id(),
+        controller().applications().deployApplication(application.id(),
                                                     zone,
                                                     applicationPackage,
                                                     new DeployOptions(Optional.of(new ScrewdriverBuildJob(app1ScrewdriverId, app1RevisionId)), Optional.empty(), false, false));
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
index c0e8b48f821..95810e90cdb 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/ContainerTester.java
@@ -43,14 +43,16 @@ public class ContainerTester {
     
     public JDisc container() { return container; }
 
+    public Controller controller() {
+        return (Controller) container.components().getComponent(Controller.class.getName());
+    }
+
     public void updateSystemVersion() {
-        Controller controller = (Controller)container.components().getComponent("com.yahoo.vespa.hosted.controller.Controller");
-        controller.updateVersionStatus(VersionStatus.compute(controller));
+        controller().updateVersionStatus(VersionStatus.compute(controller()));
     }
 
     public void updateSystemVersion(Version version) {
-        Controller controller = (Controller)container.components().getComponent("com.yahoo.vespa.hosted.controller.Controller");
-        controller.updateVersionStatus(VersionStatus.compute(controller, version));
+        controller().updateVersionStatus(VersionStatus.compute(controller(), version));
     }
 
     public void assertResponse(Supplier<Request> request, File responseFile) throws IOException {
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);
+        }
     }