From 5bb1d9666d9c6b7a5702a9e0e683dffd4dc20c9e Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Tue, 21 Nov 2017 12:05:58 +0100 Subject: No longer file issues when no production deployments --- .../vespa/hosted/controller/LockedApplication.java | 2 +- .../maintenance/ApplicationOwnershipConfirmer.java | 28 +++++++------- .../controller/deployment/DeploymentTester.java | 14 ++++++- .../ApplicationOwnershipConfirmerTest.java | 43 +++++++++++++++------- 4 files changed, 57 insertions(+), 30 deletions(-) (limited to 'controller-server') 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 782e97b12ee..5053de020c0 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 @@ -159,7 +159,7 @@ public class LockedApplication extends Application { public LockedApplication withOwnershipIssueId(IssueId issueId) { return new LockedApplication(new Application(id(), deploymentSpec(), validationOverrides(), deployments(), deploymentJobs(), deploying(), - hasOutstandingChange(), Optional.of(issueId), metrics()), lock); + hasOutstandingChange(), Optional.ofNullable(issueId), metrics()), lock); } public LockedApplication with(MetricsService.ApplicationMetrics metrics) { diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java index 0e2d35cdd82..a4c2390b6f7 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ApplicationOwnershipConfirmer.java @@ -41,19 +41,21 @@ public class ApplicationOwnershipConfirmer extends Maintainer { /** File an ownership issue with the owners of all applications we know about. */ private void confirmApplicationOwnerships() { - for (Application application : controller().applications().asList()) { - try { - Tenant tenant = ownerOf(application.id()); - Optional ourIssueId = application.ownershipIssueId(); - ourIssueId = tenant.tenantType() == TenantType.USER - ? ownershipIssues.confirmOwnership(ourIssueId, application.id(), userFor(tenant)) - : ownershipIssues.confirmOwnership(ourIssueId, application.id(), propertyIdFor(tenant)); - ourIssueId.ifPresent(issueId -> store(issueId, application.id())); - } - catch (RuntimeException e) { // Catch errors due to wrong data in the controller, or issues client timeout. - log.log(Level.WARNING, "Exception caught when attempting to file an issue for " + application.id(), e); - } - } + for (Application application : controller().applications().asList()) + if (application.productionDeployments().isEmpty()) + store(null, application.id()); + else + try { + Tenant tenant = ownerOf(application.id()); + Optional ourIssueId = application.ownershipIssueId(); + ourIssueId = tenant.tenantType() == TenantType.USER + ? ownershipIssues.confirmOwnership(ourIssueId, application.id(), userFor(tenant)) + : ownershipIssues.confirmOwnership(ourIssueId, application.id(), propertyIdFor(tenant)); + ourIssueId.ifPresent(issueId -> store(issueId, application.id())); + } + catch (RuntimeException e) { // Catch errors due to wrong data in the controller, or issues client timeout. + log.log(Level.WARNING, "Exception caught when attempting to file an issue for " + application.id(), e); + } } /** Escalate ownership issues which have not been closed before a defined amount of time has passed. */ 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 23033fbc4f8..2b0e953c12c 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 @@ -119,8 +119,13 @@ public class DeploymentTester { /** Simulate the full lifecycle of an application deployment as declared in given application package */ public Application createAndDeploy(String applicationName, int projectId, ApplicationPackage applicationPackage) { - tester.createTenant("tenant1", "domain1", 1L); - Application application = tester.createApplication(new TenantId("tenant1"), applicationName, "default", projectId); + TenantId tenantId = tester.createTenant("tenant1", "domain1", 1L); + return createAndDeploy(tenantId, applicationName, projectId, applicationPackage); + } + + /** Simulate the full lifecycle of an application deployment as declared in given application package */ + public Application createAndDeploy(TenantId tenantId, String applicationName, int projectId, ApplicationPackage applicationPackage) { + Application application = tester.createApplication(tenantId, applicationName, "default", projectId); deployCompletely(application, applicationPackage); return applications().require(application.id()); } @@ -130,6 +135,11 @@ public class DeploymentTester { return createAndDeploy(applicationName, projectId, applicationPackage(upgradePolicy)); } + /** Simulate the full lifecycle of an application deployment to prod.us-west-1 with the given upgrade policy */ + public Application createAndDeploy(TenantId tenantId, String applicationName, int projectId, String upgradePolicy) { + return createAndDeploy(tenantId, applicationName, projectId, applicationPackage(upgradePolicy)); + } + /** Complete an ongoing deployment */ public void deployCompletely(String applicationName) { deployCompletely(applications().require(ApplicationId.from("tenant1", applicationName, "default")), 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 68a1aab0f07..f5be882fcb8 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 @@ -1,6 +1,9 @@ package com.yahoo.vespa.hosted.controller.maintenance; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.Environment; +import com.yahoo.config.provision.RegionName; +import com.yahoo.config.provision.Zone; import com.yahoo.vespa.hosted.controller.Application; import com.yahoo.vespa.hosted.controller.ControllerTester; import com.yahoo.vespa.hosted.controller.api.Tenant; @@ -9,6 +12,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId; import com.yahoo.vespa.hosted.controller.api.integration.organization.IssueId; import com.yahoo.vespa.hosted.controller.api.integration.organization.OwnershipIssues; import com.yahoo.vespa.hosted.controller.api.integration.organization.User; +import com.yahoo.vespa.hosted.controller.deployment.DeploymentTester; import com.yahoo.vespa.hosted.controller.persistence.MockCuratorDb; import org.junit.Before; import org.junit.Test; @@ -28,26 +32,25 @@ public class ApplicationOwnershipConfirmerTest { private MockOwnershipIssues issues; private ApplicationOwnershipConfirmer confirmer; - private ControllerTester tester; + private DeploymentTester tester; @Before public void setup() { - tester = new ControllerTester(); + tester = new DeploymentTester(); issues = new MockOwnershipIssues(); confirmer = new ApplicationOwnershipConfirmer(tester.controller(), Duration.ofDays(1), new JobControl(new MockCuratorDb()), issues); } @Test public void testConfirmation() { - TenantId property = tester.createTenant("tenant", "domain", 1L); - ApplicationId propertyAppId = tester.createApplication(property, "application", "default", 1).id(); - Supplier propertyApp = () -> tester.controller().applications().require(propertyAppId); + TenantId property = tester.controllerTester().createTenant("property", "domain", 1L); + tester.createAndDeploy(property, "application", 1, "default"); + Supplier propertyApp = () -> tester.controller().applications().require(ApplicationId.from("property", "application", "default")); TenantId user = new TenantId("by-user"); - tester.controller().tenants().addTenant(Tenant.createUserTenant(new TenantId("by-user")), Optional.empty()); - assertTrue(tester.controller().tenants().tenant(user).isPresent()); - ApplicationId userAppId = tester.createApplication(user, "application", "default", 1).id(); - Supplier userApp = () -> tester.controller().applications().require(userAppId); + tester.controller().tenants().addTenant(Tenant.createUserTenant(user), Optional.empty()); + tester.createAndDeploy(user, "application", 2, "default"); + Supplier userApp = () -> tester.controller().applications().require(ApplicationId.from("by-user", "application", "default")); assertFalse("No issue is initially stored for a new application.", propertyApp.get().ownershipIssueId().isPresent()); assertFalse("No issue is initially stored for a new application.", userApp.get().ownershipIssueId().isPresent()); @@ -59,8 +62,8 @@ public class ApplicationOwnershipConfirmerTest { confirmer.maintain(); confirmer.maintain(); - assertEquals("Confirmation issue has been filed for property owned application.", propertyApp.get().ownershipIssueId(), issueId); - assertEquals("Confirmation issue has been filed for user owned application.", userApp.get().ownershipIssueId(), issueId); + assertEquals("Confirmation issue has been filed for property owned application.", issueId, propertyApp.get().ownershipIssueId()); + assertEquals("Confirmation issue has been filed for user owned application.", issueId, userApp.get().ownershipIssueId()); assertTrue("Both applications have had their responses ensured.", issues.escalatedForProperty && issues.escalatedForUser); // No new issue is created, so return empty now. @@ -68,15 +71,27 @@ public class ApplicationOwnershipConfirmerTest { confirmer.maintain(); confirmer.maintain(); - assertEquals("Confirmation issue reference is not updated when no issue id is returned.", propertyApp.get().ownershipIssueId(), issueId); + assertEquals("Confirmation issue reference is not updated when no issue id is returned.", issueId, propertyApp.get().ownershipIssueId()); + assertEquals("Confirmation issue reference is not updated when no issue id is returned.", issueId, userApp.get().ownershipIssueId()); - // Time has passed, and a new confirmation issue is in order. + // The user deletes its production deployment — 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()); + assertTrue("No production deployments are listed for user.", userApp.get().productionDeployments().isEmpty()); + confirmer.maintain(); + confirmer.maintain(); + + assertEquals("Confirmation issue has been forgotten for application without production deployments.", Optional.empty(), userApp.get().ownershipIssueId()); + + // Time has passed, and a new confirmation issue is in order for the property which is still in production. Optional issueId2 = Optional.of(IssueId.from("2")); issues.response = issueId2; confirmer.maintain(); confirmer.maintain(); - assertEquals("A new confirmation issue id is stored when something is returned to the maintainer.", propertyApp.get().ownershipIssueId(), issueId2); + assertEquals("A new confirmation issue id is stored when something is returned to the maintainer.", issueId2, propertyApp.get().ownershipIssueId()); + assertEquals("Confirmation issue for application without production deployments has not been filed.", Optional.empty(), userApp.get().ownershipIssueId()); + } private class MockOwnershipIssues implements OwnershipIssues { -- cgit v1.2.3