From 98eebc5f7ea07dfab4db2f631a0f904554015182 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Mon, 13 Aug 2018 13:22:55 +0200 Subject: Clean up deactivation code --- .../vespa/hosted/controller/ApplicationController.java | 4 ++-- .../hosted/controller/deployment/InternalStepRunner.java | 16 +++------------- .../hosted/controller/deployment/JobController.java | 3 +-- .../hosted/controller/maintenance/DeploymentExpirer.java | 2 +- .../restapi/application/ApplicationApiHandler.java | 2 +- .../yahoo/vespa/hosted/controller/ControllerTest.java | 4 ++-- .../hosted/controller/deployment/DeploymentTester.java | 3 +-- .../maintenance/ApplicationOwnershipConfirmerTest.java | 4 ++-- .../hosted/controller/maintenance/DnsMaintainerTest.java | 4 ++-- .../controller/restapi/deployment/DeploymentApiTest.java | 3 +-- 10 files changed, 16 insertions(+), 29 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 6e1940931e7..5e775ea6cd3 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 @@ -579,8 +579,8 @@ public class ApplicationController { } /** Deactivate application in the given zone */ - public void deactivate(Application application, ZoneId zone) { - lockOrThrow(application.id(), lockedApplication -> store(deactivate(lockedApplication, zone))); + public void deactivate(ApplicationId application, ZoneId zone) { + lockOrThrow(application, lockedApplication -> store(deactivate(lockedApplication, zone))); } /** diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index a0e273dc4c8..7241d35eaf8 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -346,23 +346,13 @@ public class InternalStepRunner implements StepRunner { private Status deactivateReal(RunId id, ByteArrayLogger logger) { logger.log("Deactivating deployment of " + id.application() + " in " + zone(id.type()) + " ..."); - Status status = deactivate(id.application(), id.type()); - if (status == succeeded) - controller.applications().lockOrThrow(id.application(), application -> - controller.applications().store(application.withoutDeploymentIn(zone(id.type())))); - return status; + controller.applications().deactivate(id.application(), id.type().zone(controller.system())); + return succeeded; } private Status deactivateTester(RunId id, ByteArrayLogger logger) { logger.log("Deactivating tester of " + id.application() + " in " + zone(id.type()) + " ..."); - return deactivate(testerOf(id.application()), id.type()); - } - - private Status deactivate(ApplicationId id, JobType type) { - try { - controller.configServer().deactivate(new DeploymentId(id, zone(type))); - } - catch (NoInstanceException e) { } + controller.jobController().deactivateTester(id.application(), id.type()); return succeeded; } 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 4a14d116c1c..c966492259f 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 @@ -242,13 +242,12 @@ public class JobController { }); } - // TODO jvenstad: Urgh, clean this up somehow? public void deactivateTester(ApplicationId id, JobType type) { try { controller.configServer().deactivate(new DeploymentId(testerOf(id), type.zone(controller.system()))); } catch (NoInstanceException ignored) { - // ok; already gone + // Already gone -- great! } } diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java index 423f7f50aed..9133c8980ec 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentExpirer.java @@ -42,7 +42,7 @@ public class DeploymentExpirer extends Maintainer { if (hasExpired(controller().zoneRegistry(), deployment, clock.instant())) { try { - controller().applications().deactivate(application, deployment.zone()); + controller().applications().deactivate(application.id(), deployment.zone()); } catch (Exception e) { log.log(Level.WARNING, "Could not expire " + deployment + " of " + application + ": " + Exceptions.toMessageString(e) + ". Retrying in " + diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java index c079fb31e99..34d2257c5f4 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java @@ -820,7 +820,7 @@ public class ApplicationApiHandler extends LoggingRequestHandler { Application application = controller.applications().require(ApplicationId.from(tenantName, applicationName, instanceName)); // Attempt to deactivate application even if the deployment is not known by the controller - controller.applications().deactivate(application, ZoneId.from(environment, region)); + controller.applications().deactivate(application.id(), ZoneId.from(environment, region)); // TODO: Change to return JSON return new StringResponse("Deactivated " + path(TenantResource.API_PATH, tenantName, 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 85c7ea0af1e..635e0c1fb26 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 @@ -305,8 +305,8 @@ public class ControllerTest { .build(); tester.jobCompletion(component).application(app1).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(app1, applicationPackage, true, systemTest); - tester.applications().deactivate(app1, ZoneId.from(Environment.test, RegionName.from("us-east-1"))); - tester.applications().deactivate(app1, ZoneId.from(Environment.staging, RegionName.from("us-east-3"))); + tester.applications().deactivate(app1.id(), ZoneId.from(Environment.test, RegionName.from("us-east-1"))); + tester.applications().deactivate(app1.id(), ZoneId.from(Environment.staging, RegionName.from("us-east-3"))); tester.applications().deleteApplication(app1.id(), Optional.of(new NToken("ntoken"))); try (RotationLock lock = tester.applications().rotationRepository().lock()) { assertTrue("Rotation is unassigned", diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index 75a6d3d72d9..44af8edb96e 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -30,7 +30,6 @@ import java.time.Duration; import java.util.List; import java.util.Optional; import java.util.UUID; -import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -267,7 +266,7 @@ public class DeploymentTester { } // Deactivate test deployments after deploy. This replicates the behaviour of the tenant pipeline if (job.isTest()) { - controller().applications().deactivate(application, job.zone(controller().system())); + controller().applications().deactivate(application.id(), job.zone(controller().system())); } jobCompletion(job).application(application).success(success).submit(); } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java index 703d65c8f9d..555fdb338e8 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmerTest.java @@ -73,8 +73,8 @@ public class ApplicationOwnershipConfirmerTest { // The user deletes all production deployments — see that the issue is forgotten. assertEquals("Confirmation issue for user is sitll open.", issueId, userApp.get().ownershipIssueId()); - tester.controller().applications().deactivate(userApp.get(), userApp.get().productionDeployments().keySet().stream().findAny().get()); - tester.controller().applications().deactivate(userApp.get(), userApp.get().productionDeployments().keySet().stream().findAny().get()); + tester.controller().applications().deactivate(userApp.get().id(), userApp.get().productionDeployments().keySet().stream().findAny().get()); + tester.controller().applications().deactivate(userApp.get().id(), userApp.get().productionDeployments().keySet().stream().findAny().get()); assertTrue("No production deployments are listed for user.", userApp.get().productionDeployments().isEmpty()); confirmer.maintain(); confirmer.maintain(); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java index 1bedb29ec97..b950e969300 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/DnsMaintainerTest.java @@ -86,8 +86,8 @@ public class DnsMaintainerTest { tester.jobCompletion(component).application(application).nextBuildNumber().uploadArtifact(applicationPackage).submit(); tester.deployAndNotify(application, applicationPackage, true, systemTest); - tester.applications().deactivate(application, ZoneId.from(Environment.test, RegionName.from("us-east-1"))); - tester.applications().deactivate(application, ZoneId.from(Environment.staging, RegionName.from("us-east-3"))); + tester.applications().deactivate(application.id(), ZoneId.from(Environment.test, RegionName.from("us-east-1"))); + tester.applications().deactivate(application.id(), ZoneId.from(Environment.staging, RegionName.from("us-east-3"))); tester.applications().deleteApplication(application.id(), Optional.of(new NToken("ntoken"))); // DnsMaintainer removes records diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java index 1ed2af1f7b9..c284b120e33 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiTest.java @@ -61,8 +61,7 @@ public class DeploymentApiTest extends ControllerContainerTest { // Deploy once so that job information is stored, then remove the deployment deployCompletely(applicationWithoutDeployment, applicationPackage, 3L, true); - tester.controller().applications().deactivate(applicationWithoutDeployment, - ZoneId.from("prod", "corp-us-east-1")); + tester.controller().applications().deactivate(applicationWithoutDeployment.id(), ZoneId.from("prod", "corp-us-east-1")); // New version released version = Version.fromString("5.1"); -- cgit v1.2.3