Merge pull request #4440 from vespa-engine/mpolden/remove-all-instances-on-delete

Delete all instances on application delete

2 files changed, 53 insertions, 24 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 fb92109c5df..44e87d258df 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
@@ -7,7 +7,6 @@ import com.yahoo.config.application.api.ValidationId;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.Environment;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.config.provision.Zone;
 import com.yahoo.config.provision.ZoneId;
 import com.yahoo.vespa.curator.Lock;
 import com.yahoo.vespa.hosted.controller.api.ActivateResult;
@@ -22,6 +21,10 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.DeploymentId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.Hostname;
 import com.yahoo.vespa.hosted.controller.api.identifiers.RevisionId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
+import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzClientFactory;
+import com.yahoo.vespa.hosted.controller.api.integration.athenz.NToken;
+import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsClient;
+import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsException;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.ConfigServerClient;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.Log;
 import com.yahoo.vespa.hosted.controller.api.integration.configserver.NoInstanceException;
@@ -42,10 +45,6 @@ import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport;
 import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.AthenzClientFactory;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.NToken;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsClient;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.ZmsException;
 import com.yahoo.vespa.hosted.controller.deployment.DeploymentTrigger;
 import com.yahoo.vespa.hosted.controller.maintenance.DeploymentExpirer;
 import com.yahoo.vespa.hosted.controller.persistence.ControllerDb;
@@ -271,10 +270,11 @@ public class ApplicationController {
     public ActivateResult deployApplication(ApplicationId applicationId, ZoneId zone,
                                             ApplicationPackage applicationPackage, DeployOptions options) {
         try (Lock lock = lock(applicationId)) {
-            // TODO: Shouldn't this go through the above method? Seems you can cheat the checks here ... ?
-            LockedApplication application = get(applicationId).map(application1 -> new LockedApplication(application1, lock)).orElse(new LockedApplication(
-                    new Application(applicationId), lock)
-                                                                                                                                    );
+            // Not ideal, but since we create on missing and return a result computed inside the lock,
+            // the lock-with-action methods cannot be used
+            LockedApplication application = get(applicationId)
+                    .map(app -> new LockedApplication(app, lock))
+                    .orElseGet(() -> new LockedApplication(new Application(applicationId), lock));
 
             // Determine what we are doing
             Version version;
@@ -505,31 +505,43 @@ public class ApplicationController {
     }
 
     /**
-     * Deletes the application with this id
+     * Deletes the the given application. All known instances of the applications will be deleted,
+     * including PR instances.
      * 
      * @throws IllegalArgumentException if the application has deployments or the caller is not authorized
-     * @throws NotExistsException if the application does not exist
+     * @throws NotExistsException if no instances of the application exist
      */
-    public void deleteApplication(ApplicationId id, Optional<NToken> token) {
-        if ( ! controller.applications().get(id).isPresent())
-            throw new NotExistsException("Could not delete application '" + id + "': Application not found");
+    public void deleteApplication(ApplicationId applicationId, Optional<NToken> token) {
+        // Find all instances of the application
+        List<ApplicationId> instances = controller.applications().asList(applicationId.tenant())
+                                                  .stream()
+                                                  .map(Application::id)
+                                                  .filter(id -> id.application().equals(applicationId.application()) &&
+                                                                id.tenant().equals(applicationId.tenant()))
+                                                  .collect(Collectors.toList());
+        if (instances.isEmpty()) {
+            throw new NotExistsException("Could not delete application '" + applicationId + "': Application not found");
+        }
 
-        lockOrThrow(id, application -> {
+        // TODO: Make this one transaction when database is moved to ZooKeeper
+        instances.forEach(id -> lockOrThrow(id, application -> {
             if ( ! application.deployments().isEmpty())
                 throw new IllegalArgumentException("Could not delete '" + application + "': It has active deployments");
-            
+
             Tenant tenant = controller.tenants().tenant(new TenantId(id.tenant().value())).get();
             if (tenant.isAthensTenant() && ! token.isPresent())
                 throw new IllegalArgumentException("Could not delete '" + application + "': No NToken provided");
 
-            // NB: Next 2 lines should have been one transaction
-            if (tenant.isAthensTenant())
+            // Only delete in Athenz once
+            if (id.instance().isDefault() && tenant.isAthensTenant()) {
                 zmsClientFactory.createZmsClientWithAuthorizedServiceToken(token.get())
-                        .deleteApplication(tenant.getAthensDomain().get(), new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value()));
+                                .deleteApplication(tenant.getAthensDomain().get(),
+                                                   new com.yahoo.vespa.hosted.controller.api.identifiers.ApplicationId(id.application().value()));
+            }
             db.deleteApplication(id);
 
             log.info("Deleted " + application);
-        });
+        }));
     }
 
     /** 
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 4a78c97750f..58c74c9d6d2 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
@@ -10,7 +10,6 @@ import com.yahoo.config.provision.InstanceName;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.config.provision.Zone;
 import com.yahoo.config.provision.ZoneId;
 import com.yahoo.vespa.config.SlimeUtils;
 import com.yahoo.vespa.hosted.controller.api.Tenant;
@@ -23,6 +22,7 @@ import com.yahoo.vespa.hosted.controller.api.identifiers.PropertyId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.TenantId;
 import com.yahoo.vespa.hosted.controller.api.identifiers.UserGroup;
 import com.yahoo.vespa.hosted.controller.api.integration.BuildService.BuildJob;
+import com.yahoo.vespa.hosted.controller.api.integration.athenz.NToken;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.Record;
 import com.yahoo.vespa.hosted.controller.api.integration.dns.RecordName;
 import com.yahoo.vespa.hosted.controller.application.ApplicationPackage;
@@ -32,7 +32,6 @@ import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
 import com.yahoo.vespa.hosted.controller.application.JobStatus;
-import com.yahoo.vespa.hosted.controller.api.integration.athenz.NToken;
 import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock;
 import com.yahoo.vespa.hosted.controller.deployment.ApplicationPackageBuilder;
 import com.yahoo.vespa.hosted.controller.deployment.BuildSystem;
@@ -299,10 +298,14 @@ public class ControllerTest {
 
         // staging deployment
         long app1ProjectId = 22;
-        ApplicationId app1 = tester.createAndDeploy("tenant1",  "domain1", "application1", Environment.staging, app1ProjectId).id();
+        ApplicationId app1 = tester.createAndDeploy("tenant1",  "domain1",
+                                                    "application1", Environment.staging,
+                                                    app1ProjectId).id();
         
         // pull-request deployment - uses different instance id
-        ApplicationId app1pr = tester.createAndDeploy("tenant1",  "domain1", "application1", "default-pr1", Environment.staging, app1ProjectId, null).id();
+        ApplicationId app1pr = tester.createAndDeploy("tenant1",  "domain1",
+                                                      "application1", "default-pr1",
+                                                      Environment.staging, app1ProjectId, null).id();
 
         assertTrue(applications.get(app1).isPresent());
         assertEquals(app1, applications.get(app1).get().id());
@@ -317,6 +320,20 @@ public class ControllerTest {
         assertEquals(app1, applications.get(app1).get().id());
         assertTrue(applications.get(app1pr).isPresent());
         assertEquals(app1pr, applications.get(app1pr).get().id());
+
+        // Deleting application also removes PR instance
+        ApplicationId app2 = tester.createAndDeploy("tenant1",  "domain1",
+                                                    "application2", Environment.staging,
+                                                    33).id();
+        tester.controller().applications().deleteApplication(app1, Optional.of(new NToken("ntoken")));
+        assertEquals("All instances deleted", 0,
+                     tester.controller().applications().asList(app1.tenant()).stream()
+                                                    .filter(app -> app.id().application().equals(app1.application()))
+                                                    .count());
+        assertEquals("Other application survives", 1,
+                     tester.controller().applications().asList(app1.tenant()).stream()
+                           .filter(app -> app.id().application().equals(app2.application()))
+                           .count());
     }
     
     @Test