diff options
10 files changed, 52 insertions, 98 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 12c0c5fd36b..565c92f3a28 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 @@ -64,7 +64,6 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Consumer; -import java.util.function.Function; import java.util.logging.Level; import java.util.logging.Logger; import java.util.stream.Collectors; @@ -123,8 +122,8 @@ public class ApplicationController { /** Returns an locked application with the given id that be updated and stored */ - public Optional<LockedApplication> get(ApplicationId id, Lock lock) { - return db.getApplication(id).map(application -> new LockedApplication(application, lock)); + private Optional<LockedApplication> get(ApplicationId id, Lock lock) { + return get(id).map(application -> new LockedApplication(application, lock)); } /** @@ -136,16 +135,6 @@ public class ApplicationController { return get(id).orElseThrow(() -> new IllegalArgumentException(id + " not found")); } - /** - * Returns a locked application that be updated and stored - * - * @throws IllegalArgumentException if it does not exist - * - */ - public LockedApplication require(ApplicationId id, Lock lock) { - return get(id, lock).orElseThrow(() -> new IllegalArgumentException(id + " not found")); - } - /** Returns a snapshot of all applications */ public List<Application> asList() { return db.listApplications(); @@ -156,16 +145,6 @@ public class ApplicationController { return db.listApplications(new TenantId(tenant.value())); } - /** Returns an immutable list with the ids of all known applications. */ - public List<ApplicationId> idList() { - return ImmutableList.copyOf(asList().stream().map(Application::id)::iterator); - } - - /** Returns an immutable list with the ids of all applications of the tenant. */ - public List<ApplicationId> idList(TenantName tenant) { - return ImmutableList.copyOf(asList(tenant).stream().map(Application::id)::iterator); - } - /** * Set the rotations marked as 'global' either 'in' or 'out of' service. * @@ -565,7 +544,7 @@ public class ApplicationController { */ public void lockedOrThrow(ApplicationId applicationId, Consumer<LockedApplication> actions) { try (Lock lock = lock(applicationId)) { - actions.accept(require(applicationId, lock)); + actions.accept(get(applicationId, lock).orElseThrow(() -> new IllegalArgumentException(applicationId + " not found"))); } } @@ -608,20 +587,17 @@ public class ApplicationController { private void deactivate(Application application, Zone zone, Optional<Deployment> deployment, boolean requireThatDeploymentHasExpired) { - try (Lock lock = lock(application.id())) { - LockedApplication lockedApplication = controller.applications().require(application.id(), lock); - if (deployment.isPresent() && requireThatDeploymentHasExpired && - ! DeploymentExpirer.hasExpired(controller.zoneRegistry(), deployment.get(), clock.instant())) { - return; - } - lockedApplication = deactivate(lockedApplication, zone); - store(lockedApplication); - } + if (requireThatDeploymentHasExpired && deployment.isPresent() + && ! DeploymentExpirer.hasExpired(controller.zoneRegistry(), deployment.get(), clock.instant())) + return; + + lockedOrThrow(application.id(), lockedApplication -> + store(deactivate(lockedApplication, zone))); } - /** + /** * Deactivates a locked application without storing it - * + * * @return the application with the deployment in the given zone removed */ private LockedApplication deactivate(LockedApplication application, Zone zone) { @@ -637,19 +613,19 @@ public class ApplicationController { public DeploymentTrigger deploymentTrigger() { return deploymentTrigger; } private ApplicationId dashToUnderscore(ApplicationId id) { - return ApplicationId.from(id.tenant().value(), + return ApplicationId.from(id.tenant().value(), id.application().value().replaceAll("-", "_"), id.instance().value()); } - + public ConfigServerClient configserverClient() { return configserverClient; } - - /** + + /** * Returns a lock which provides exclusive rights to changing this application. * 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. */ - public Lock lock(ApplicationId application) { + Lock lock(ApplicationId application) { return curator.lock(application, Duration.ofMinutes(10)); } @@ -661,18 +637,18 @@ public class ApplicationController { } private static final class ApplicationRotation { - + private final ImmutableSet<String> cnames; private final ImmutableSet<Rotation> rotations; - + public ApplicationRotation(Set<String> cnames, Set<Rotation> rotations) { this.cnames = ImmutableSet.copyOf(cnames); this.rotations = ImmutableSet.copyOf(rotations); } - + public Set<String> cnames() { return cnames; } public Set<Rotation> rotations() { return rotations; } - + } - + } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java index 5899c95015f..a39a549fc28 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java @@ -53,7 +53,7 @@ public class LockedApplication extends Application { return new LockedApplication(new Builder(this).with(deploymentJobs().withProjectId(projectId))); } - public LockedApplication with(IssueId issueId) { + public LockedApplication withDeploymentIssueId(IssueId issueId) { return new LockedApplication(new Builder(this).with(deploymentJobs().with(issueId))); } 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 6246459441b..1eee727214b 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 @@ -238,12 +238,10 @@ public class DeploymentTrigger { * @param applicationId the application to trigger */ public void cancelChange(ApplicationId applicationId) { - try (Lock lock = applications().lock(applicationId)) { - LockedApplication application = applications().require(applicationId, lock); + applications().lockedOrThrow(applicationId, application -> { buildSystem.removeJobs(application.id()); - application = application.withDeploying(Optional.empty()); - applications().store(application); - } + applications().store(application.withDeploying(Optional.empty())); + }); } //--- End of methods which triggers deployment jobs ---------------------------- diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java index 91376bd9e80..ae6ba364d25 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java @@ -131,11 +131,8 @@ public class DeploymentIssueReporter extends Maintainer { } private void store(ApplicationId id, IssueId issueId) { - try (Lock lock = controller().applications().lock(id)) { - controller().applications().get(id, lock).ifPresent( - application -> controller().applications().store(application.withDeploymentIssueId(issueId)) - ); - } + controller().applications().lockedIfPresent(id, application -> + controller().applications().store(application.withDeploymentIssueId(issueId))); } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java index 0141179a942..13eb5075f34 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java @@ -34,11 +34,8 @@ public class DeploymentMetricsMaintainer extends Maintainer { boolean hasWarned = false; for (Application application : ApplicationList.from(controller().applications().asList()).notPullRequest().asList()) { try { - try (Lock lock = controller().applications().lock(application.id())) { - controller().applications().get(application.id(), lock) - .ifPresent(lockedApplication -> controller().applications().store( - lockedApplication.with(controller().metricsService().getApplicationMetrics(application.id())))); - } + controller().applications().lockedIfPresent(application.id(), lockedApplication -> + controller().applications().store(lockedApplication.with(controller().metricsService().getApplicationMetrics(application.id())))); for (Deployment deployment : application.deployments().values()) { MetricsService.DeploymentMetrics deploymentMetrics = controller().metricsService() @@ -49,11 +46,8 @@ public class DeploymentMetricsMaintainer extends Maintainer { deploymentMetrics.queryLatencyMillis(), deploymentMetrics.writeLatencyMillis()); - try (Lock lock = controller().applications().lock(application.id())) { - controller().applications().get(application.id(), lock) - .ifPresent(lockedApplication -> controller().applications().store( - lockedApplication.with(deployment.zone(), appMetrics))); - } + controller().applications().lockedIfPresent(application.id(), lockedApplication -> + controller().applications().store(lockedApplication.with(deployment.zone(), appMetrics))); } } catch (UncheckedIOException e) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java index a92e2533092..e350b98adb9 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java @@ -103,13 +103,13 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler { } private HttpResponse trigger(HttpRequest request, String tenantName, String applicationName) { + JobType jobType = Optional.of(asString(request.getData())) + .filter(s -> !s.isEmpty()) + .map(JobType::fromJobName) + .orElse(JobType.component); + ApplicationId applicationId = ApplicationId.from(tenantName, applicationName, "default"); - try (Lock lock = controller.applications().lock(applicationId)) { - LockedApplication application = controller.applications().require(applicationId, lock); - JobType jobType = Optional.of(asString(request.getData())) - .filter(s -> !s.isEmpty()) - .map(JobType::fromJobName) - .orElse(JobType.component); + controller.applications().lockedOrThrow(applicationId, application -> { // Since this is a manual operation we likely want it to trigger as soon as possible so we add it at to the // front of the queue application = controller.applications().deploymentTrigger().triggerAllowParallel( @@ -117,12 +117,12 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler { "Triggered from screwdriver/v1" ); controller.applications().store(application); + }); - Slime slime = new Slime(); - Cursor cursor = slime.setObject(); - cursor.setString("message", "Triggered " + jobType.jobName() + " for " + applicationId); - return new SlimeJsonResponse(slime); - } + Slime slime = new Slime(); + Cursor cursor = slime.setObject(); + cursor.setString("message", "Triggered " + jobType.jobName() + " for " + applicationId); + return new SlimeJsonResponse(slime); } private HttpResponse vespaVersion() { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java index 0738f9a7c00..d0c1fd95427 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java @@ -552,11 +552,7 @@ public class ControllerTest { // Load test data data ApplicationSerializer serializer = new ApplicationSerializer(); byte[] json = Files.readAllBytes(Paths.get("src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/canary-with-stale-data.json")); - Slime slime = SlimeUtils.jsonToSlime(json); - Application application = serializer.fromSlime(slime); - try (Lock lock = tester.controller().applications().lock(application.id())) { - tester.controller().applications().store(new LockedApplication(application, lock)); - } + Application application = tester.controllerTester().createApplication(SlimeUtils.jsonToSlime(json)); ApplicationPackage applicationPackage = new ApplicationPackageBuilder() .upgradePolicy("canary") diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java index 4e3c15ea1a4..8f9c22f8b81 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java @@ -181,12 +181,9 @@ public final class ControllerTester { public Application createApplication(TenantId tenant, String applicationName, String instanceName, long projectId) { ApplicationId applicationId = applicationId(tenant.id(), applicationName, instanceName); controller().applications().createApplication(applicationId, Optional.of(TestIdentities.userNToken)); - try (Lock lock = controller().applications().lock(applicationId)) { - LockedApplication lockedApplication = controller().applications().require(applicationId, lock) - .withProjectId(projectId); - controller().applications().store(lockedApplication); - return lockedApplication; - } + controller().applications().lockedOrThrow(applicationId, lockedApplication -> + controller().applications().store(lockedApplication.withProjectId(projectId))); + return controller().applications().require(applicationId); } public void deploy(Application application, Zone zone) { diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java index fd23ee76b73..1638cbb2e95 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java @@ -306,11 +306,10 @@ public class ApplicationApiTest extends ControllerContainerTest { } private void addIssues(ContainerControllerTester tester, ApplicationId id) { - try (Lock lock = tester.controller().applications().lock(id)) { - tester.controller().applications().store(tester.controller().applications().require(id, lock) - .withDeploymentIssueId(IssueId.from("123")) - .withOwnershipIssueId(IssueId.from("321"))); - } + tester.controller().applications().lockedOrThrow(id, application -> + tester.controller().applications().store(application + .withDeploymentIssueId(IssueId.from("123")) + .withOwnershipIssueId(IssueId.from("321")))); } @Test diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java index ee8ac5e3b8e..e6b3eacd44e 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java @@ -148,11 +148,8 @@ public class ScrewdriverApiTest extends ControllerContainerTest { tester.containerTester().updateSystemVersion(); Application app = tester.createApplication(); - try (Lock lock = tester.controller().applications().lock(app.id())) { - tester.controller().applications().store( - tester.controller().applications().require(app.id(), lock).withProjectId(1) - ); - } + tester.controller().applications().lockedOrThrow(app.id(), application -> + tester.controller().applications().store(application.withProjectId(1))); // Unknown application assertResponse(new Request("http://localhost:8080/screwdriver/v1/trigger/tenant/foo/application/bar", |