diff options
author | Martin Polden <mpolden@mpolden.no> | 2017-12-05 18:09:22 +0100 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2017-12-05 18:14:07 +0100 |
commit | c2961c66705780d8819329debfce3487a792ad86 (patch) | |
tree | d37f7de2f60c0013d256639e933e50dcf188245d | |
parent | 62339f63d53c0faad58880642643f6d386e85bc4 (diff) |
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); + } } |