Merge pull request #15359 from vespa-engine/hmusum/handle-session-not-existing-when-deleting-an-app

Handle session not existing when deleting app

3 files changed, 53 insertions, 4 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
index ef45b1aa0dd..32b1a847134 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ApplicationRepository.java
@@ -486,8 +486,12 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
             Optional<Long> activeSession = tenantApplications.activeSessionOf(applicationId);
             if (activeSession.isEmpty()) return false;
 
-            Session session = getRemoteSession(tenant, activeSession.get());
-            tenant.getSessionRepository().createSetStatusTransaction(session, Session.Status.DELETE).commit();
+            try {
+                Session session = getRemoteSession(tenant, activeSession.get());
+                transaction.add(tenant.getSessionRepository().createSetStatusTransaction(session, Session.Status.DELETE));
+            } catch (NotFoundException e) {
+                log.log(Level.INFO, TenantRepository.logPre(applicationId) + "Active session exists, but has not been deleted properly. Trying to cleanup");
+            }
 
             Curator curator = tenantRepository.getCurator();
             transaction.add(new ContainerEndpointsCache(tenant.getPath(), curator).delete(applicationId)); // TODO: Not unit tested
@@ -592,7 +596,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return tenantRepository.getTenant(applicationId.tenant());
     }
 
-    private Application getApplication(ApplicationId applicationId) {
+    Application getApplication(ApplicationId applicationId) {
         return getApplication(applicationId, Optional.empty());
     }
 
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
index d153c63b537..0cab49a9f00 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/ApplicationRepositoryTest.java
@@ -77,6 +77,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author hmusum
@@ -354,6 +355,43 @@ public class ApplicationRepositoryTest {
 
             assertTrue(applicationRepository.delete(applicationId()));
         }
+
+        // If delete fails, a retry should work if the failure is transient and zookeeper state should be constistent
+        {
+            PrepareResult result = deployApp(testApp);
+            long sessionId = result.sessionId();
+            assertNotNull(sessionRepository.getRemoteSession(sessionId));
+            assertNotNull(applicationRepository.getActiveSession(applicationId()));
+            assertEquals(sessionId, applicationRepository.getActiveSession(applicationId()).getSessionId());
+            assertNotNull(applicationRepository.getApplication(applicationId()));
+
+            provisioner.failureOnRemove(true);
+            try {
+                applicationRepository.delete(applicationId());
+                fail("Should fail with RuntimeException");
+            } catch (RuntimeException e) {
+                // ignore
+            }
+            assertNotNull(sessionRepository.getRemoteSession(sessionId));
+            assertNotNull(applicationRepository.getActiveSession(applicationId()));
+            assertEquals(sessionId, applicationRepository.getActiveSession(applicationId()).getSessionId());
+
+            // Delete should work when there is no failure anymore
+            provisioner.failureOnRemove(false);
+            assertTrue(applicationRepository.delete(applicationId()));
+
+            // Session should be in state DELETE
+            String sessionNode = sessionRepository.getSessionPath(sessionId).getAbsolute();
+            assertEquals(Session.Status.DELETE.name(), configCurator.getData(sessionNode + "/sessionState"));
+            assertNotNull(sessionRepository.getRemoteSession(sessionId)); // session still exists
+            assertNull(applicationRepository.getActiveSession(applicationId())); // but it is not active
+            try {
+                applicationRepository.getApplication(applicationId());
+                fail("Should fail with NotFoundException, application should not exist");
+            } catch (NotFoundException e) {
+                // ignore
+            }
+        }
     }
 
     @Test
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/MockProvisioner.java b/configserver/src/test/java/com/yahoo/vespa/config/server/MockProvisioner.java
index b47a81284c0..e828aa1fc6c 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/MockProvisioner.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/MockProvisioner.java
@@ -13,7 +13,6 @@ import com.yahoo.config.provision.ProvisionLock;
 import com.yahoo.config.provision.ProvisionLogger;
 import com.yahoo.config.provision.Provisioner;
 import com.yahoo.config.provision.exception.LoadBalancerServiceException;
-import com.yahoo.transaction.NestedTransaction;
 
 import java.util.Collection;
 import java.util.List;
@@ -31,6 +30,7 @@ public class MockProvisioner implements Provisioner {
     private HostFilter lastRestartFilter;
 
     private boolean transientFailureOnPrepare = false;
+    private boolean failureOnRemove = false;
     private HostProvisioner hostProvisioner = null;
 
     public MockProvisioner hostProvisioner(HostProvisioner hostProvisioner) {
@@ -43,6 +43,10 @@ public class MockProvisioner implements Provisioner {
         return this;
     }
 
+    public void failureOnRemove(boolean failureOnRemove) {
+        this.failureOnRemove = failureOnRemove;
+    }
+
     @Override
     public List<HostSpec> prepare(ApplicationId applicationId, ClusterSpec cluster, Capacity capacity, ProvisionLogger logger) {
         if (hostProvisioner != null) {
@@ -63,6 +67,9 @@ public class MockProvisioner implements Provisioner {
 
     @Override
     public void remove(ApplicationTransaction transaction) {
+        if (failureOnRemove)
+            throw new IllegalStateException("Unable to remove " + transaction.application());
+
         removed = true;
         lastApplicationId = transaction.application();
     }