Make application remove transactional

9 files changed, 47 insertions, 16 deletions

diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Provisioner.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Provisioner.java
index 0ec00d965b2..bf8773858a2 100644
--- a/config-provisioning/src/main/java/com/yahoo/config/provision/Provisioner.java
+++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Provisioner.java
@@ -39,8 +39,25 @@ public interface Provisioner {
      * Notifies provisioner that an application has been removed.
      *
      * @param application The {@link ApplicationId} that was removed.
-     */    
-    void removed(ApplicationId application);
+     * @deprecated use remove(transaction, application) instead
+     */
+    @Deprecated
+    default void removed(ApplicationId application) {
+        throw new IllegalStateException("Unexpected use of deprecated method");
+    }
+
+    /**
+     * Transactionally remove this application.
+     * This default implementation delegates to removed(application), i.e performs the removal non-transactional.
+     * 
+     * @param application
+     */
+    // TODO: Remove the default implementation in this when
+    //       no applications are on a version before 5.17
+    @SuppressWarnings("deprecation")
+    default void remove(NestedTransaction transaction, ApplicationId application) {
+        removed(application);
+    }
 
     /**
      * Requests a restart of the services of the given application
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployer.java
index 3cc652cb617..5efc4521061 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployer.java
@@ -88,6 +88,7 @@ public class Deployer implements com.yahoo.config.provision.Deployer {
      * Removes a previously deployed application
      * 
      * @return true if the application was found and removed, false if it was not present
+     * @throws RuntimeException if the remove transaction fails. This method is exception safe.
      */
     public boolean remove(ApplicationId applicationId) {
         Optional<Tenant> owner = Optional.ofNullable(tenants.tenantsCopy().get(applicationId.tenant()));
@@ -110,11 +111,11 @@ public class Deployer implements com.yahoo.config.provision.Deployer {
 
         transaction.add(applicationRepo.deleteApplication(applicationId));
 
+        if (hostProvisioner.isPresent())
+            hostProvisioner.get().remove(transaction, applicationId);
+        transaction.onCommitted(() -> log.log(LogLevel.INFO, "Deleted " + applicationId));
         transaction.commit();
 
-        if (hostProvisioner.isPresent())
-            hostProvisioner.get().removed(applicationId);
-        log.log(LogLevel.INFO, "Deleted " + applicationId);
         return true;
     }
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
index fbd4f791f83..8876c26c9c8 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/deploy/HostedDeployTest.java
@@ -90,7 +90,7 @@ public class HostedDeployTest extends TestWithTenant {
         }
 
         @Override
-        public void removed(ApplicationId application) {
+        public void remove(NestedTransaction transaction, ApplicationId application) {
             // noop
         }
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
index 659a435ca6d..eb312636225 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/SessionActiveHandlerTest.java
@@ -188,7 +188,7 @@ public class SessionActiveHandlerTest extends SessionActiveHandlerTestBase {
         }
 
         @Override
-        public void removed(ApplicationId application) {
+        public void remove(NestedTransaction transaction, ApplicationId application) {
             removed = true;
             lastApplicationId = application;
         }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
index 63163bce9d1..0533538b016 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeRepository.java
@@ -163,9 +163,11 @@ public class NodeRepository extends AbstractComponent {
         }
     }
 
-    public void deactivate(ApplicationId application) {
+    public void deactivate(ApplicationId application, NestedTransaction transaction) {
         try (Mutex lock = lock(application)) {
-            zkClient.writeTo(Node.State.inactive, zkClient.getNodes(application, Node.State.reserved, Node.State.active));
+            zkClient.writeTo(Node.State.inactive, 
+                             zkClient.getNodes(application, Node.State.reserved, Node.State.active), 
+                             transaction);
         }
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
index e2ff947de80..5924e3fcb18 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodeRepositoryProvisioner.java
@@ -87,8 +87,8 @@ public class NodeRepositoryProvisioner implements Provisioner {
     }
 
     @Override
-    public void removed(ApplicationId application) {
-        nodeRepository.deactivate(application);
+    public void remove(NestedTransaction transaction, ApplicationId application) {
+        nodeRepository.deactivate(application, transaction);
     }
 
     private List<HostSpec> asSortedHosts(List<Node> nodes) {
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
index 0c853e4b1dd..390e3fe3569 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/InactiveAndFailedExpirerTest.java
@@ -68,7 +68,9 @@ public class InactiveAndFailedExpirerTest {
         provisioner.activate(transaction, applicationId, asHosts(nodes));
         transaction.commit();
         assertEquals(2, nodeRepository.getNodes(Node.Type.tenant, Node.State.active).size());
-        nodeRepository.deactivate(applicationId);
+        NestedTransaction deactivateTransaction = new NestedTransaction();
+        nodeRepository.deactivate(applicationId, deactivateTransaction);
+        deactivateTransaction.commit();
         assertEquals(2, nodeRepository.getNodes(Node.Type.tenant, Node.State.inactive).size());
 
         // Inactive times out
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionTest.java
index 2c80ece4aab..3348177f750 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionTest.java
@@ -12,6 +12,7 @@ import com.yahoo.config.provision.HostSpec;
 import com.yahoo.config.provision.OutOfCapacityException;
 import com.yahoo.config.provision.RegionName;
 import com.yahoo.config.provision.Zone;
+import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.hosted.provision.Node;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -75,7 +76,9 @@ public class ProvisionTest {
         assertEquals(5, tester.getNodes(application1, Node.State.inactive).size());
 
         // delete app
-        tester.provisioner().removed(application1);
+        NestedTransaction removeTransaction = new NestedTransaction();
+        tester.provisioner().remove(removeTransaction, application1);
+        removeTransaction.commit();
         assertEquals(tester.toHostNames(state1.allHosts), tester.toHostNames(tester.nodeRepository().getNodes(application1, Node.State.inactive)));
         assertEquals(0, tester.getNodes(application1, Node.State.active).size());
 
@@ -168,7 +171,9 @@ public class ProvisionTest {
         SystemState state7 = prepare(application1, 8, 2, 2, 2, "default", tester);
 
         // delete app
-        tester.provisioner().removed(application1);
+        NestedTransaction removeTransaction = new NestedTransaction();
+        tester.provisioner().remove(removeTransaction, application1);
+        removeTransaction.commit();
         assertEquals(0, tester.getNodes(application1, Node.State.active).size());
         assertEquals(0, tester.getNodes(application1, Node.State.reserved).size());
     }
@@ -370,7 +375,9 @@ public class ProvisionTest {
         SystemState state = prepare(application, 2, 2, 3, 3, "default", tester);
 
         // Simulate expiry
-        tester.nodeRepository().deactivate(application);
+        NestedTransaction deactivateTransaction = new NestedTransaction();
+        tester.nodeRepository().deactivate(application, deactivateTransaction);
+        deactivateTransaction.commit();
 
         try {
             tester.activate(application, state.allHosts);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/legacy/ProvisionResourceTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/legacy/ProvisionResourceTest.java
index 4acd748c3d0..8e98d185d0b 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/legacy/ProvisionResourceTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/legacy/ProvisionResourceTest.java
@@ -109,7 +109,9 @@ public class ProvisionResourceTest {
     public void test_recycle_deallocated() {
         createNodesInRepository(2, 0);
         assignNode(application, 2);
-        nodeRepository.deactivate(application);
+        NestedTransaction deactivateTransaction = new NestedTransaction();
+        nodeRepository.deactivate(application, deactivateTransaction);
+        deactivateTransaction.commit();
         List<Node> nodes = nodeRepository.deallocate(nodeRepository.getNodes(application, Node.State.inactive));
         assertEquals(0, nodeRepository.getNodes(Node.Type.tenant, Node.State.ready).size());
         assertEquals(2, nodeRepository.getNodes(Node.Type.tenant, Node.State.dirty).size());