Return internal server error of internal delete fails

5 files changed, 56 insertions, 26 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 465344a6d76..43b78212827 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
@@ -39,6 +39,7 @@ import com.yahoo.vespa.config.server.configchange.RestartActions;
 import com.yahoo.vespa.config.server.deploy.DeployHandlerLogger;
 import com.yahoo.vespa.config.server.deploy.Deployment;
 import com.yahoo.vespa.config.server.deploy.InfraDeployerProvider;
+import com.yahoo.vespa.config.server.http.InternalServerException;
 import com.yahoo.vespa.config.server.http.LogRetriever;
 import com.yahoo.vespa.config.server.http.SimpleHttpFetcher;
 import com.yahoo.vespa.config.server.http.v2.MetricsResponse;
@@ -325,8 +326,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         Deployment deployment = deployFromPreparedSession(localSession, tenant, timeoutBudget.timeLeft());
         deployment.setIgnoreSessionStaleFailure(ignoreSessionStaleFailure);
         deployment.activate();
-        ApplicationId applicationId = localSession.getApplicationId();
-        return applicationId;
+        return localSession.getApplicationId();
     }
 
     private Deployment deployFromPreparedSession(LocalSession session, Tenant tenant, Duration timeout) {
@@ -341,7 +341,17 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
      * @return true if the application was found and deleted, false if it was not present
      * @throws RuntimeException if the delete transaction fails. This method is exception safe.
      */
-    public boolean delete(ApplicationId applicationId) {
+    boolean delete(ApplicationId applicationId) {
+        return delete(applicationId, Duration.ofSeconds(60));
+    }
+
+    /**
+     * Deletes an application
+     *
+     * @return true if the application was found and deleted, false if it was not present
+     * @throws RuntimeException if the delete transaction fails. This method is exception safe.
+     */
+    public boolean delete(ApplicationId applicationId, Duration waitTime) {
         Tenant tenant = tenantRepository.getTenant(applicationId.tenant());
         if (tenant == null) return false;
 
@@ -349,23 +359,23 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         try (Lock lock = tenantApplications.lock(applicationId)) {
             if ( ! tenantApplications.exists(applicationId)) return false;
 
+            Optional<Long> activeSession = tenantApplications.activeSessionOf(applicationId);
+            if (activeSession.isEmpty()) return false;
+
             // Deleting an application is done by deleting the remote session and waiting
             // until the config server where the deployment happened picks it up and deletes
             // the local session
-            boolean sessionDeleted = tenantApplications.activeSessionOf(applicationId).map(sessionId -> {
-                RemoteSession remoteSession = getRemoteSession(tenant, sessionId);
-                remoteSession.createDeleteTransaction().commit();
-                log.log(LogLevel.INFO, TenantRepository.logPre(applicationId) + "Waiting for session " + sessionId + " to be deleted");
-                // TODO: Add support for timeout in request
-                Duration waitTime = Duration.ofSeconds(60);
-                if (localSessionHasBeenDeleted(applicationId, sessionId, waitTime)) {
-                    log.log(LogLevel.INFO, TenantRepository.logPre(applicationId) + "Session " + sessionId + " deleted");
-                    return true;
-                } else {
-                    log.log(LogLevel.ERROR, TenantRepository.logPre(applicationId) + "Session " + sessionId + " was not deleted (waited " + waitTime + ")");
-                    return false;
-                }
-            }).orElse(true);
+            long sessionId = activeSession.get();
+            RemoteSession remoteSession = getRemoteSession(tenant, sessionId);
+            remoteSession.createDeleteTransaction().commit();
+            log.log(LogLevel.INFO, TenantRepository.logPre(applicationId) + "Waiting for session " + sessionId + " to be deleted");
+
+
+            if ( ! waitTime.isZero() && localSessionHasBeenDeleted(applicationId, sessionId, waitTime)) {
+                log.log(LogLevel.INFO, TenantRepository.logPre(applicationId) + "Session " + sessionId + " deleted");
+            } else {
+                throw new InternalServerException("Session " + sessionId + " was not deleted (waited " + waitTime + ")");
+            }
 
             NestedTransaction transaction = new NestedTransaction();
             transaction.add(new ContainerEndpointsCache(tenant.getPath(), tenant.getCurator()).delete(applicationId)); // TODO: Not unit tested
@@ -376,7 +386,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
             hostProvisioner.ifPresent(provisioner -> provisioner.remove(transaction, applicationId));
             transaction.onCommitted(() -> log.log(LogLevel.INFO, "Deleted " + applicationId));
             transaction.commit();
-            return sessionDeleted;
+            return true;
         }
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
index 5b2f6cab3c4..7ec0f49ed60 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/application/TenantApplications.java
@@ -2,7 +2,6 @@
 package com.yahoo.vespa.config.server.application;
 
 import com.yahoo.concurrent.StripedExecutor;
-import com.yahoo.concurrent.ThreadFactoryFactory;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.log.LogLevel;
@@ -24,12 +23,10 @@ import java.time.Duration;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.OptionalLong;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
index 1e80ef2d883..babfdfa575d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandler.java
@@ -66,9 +66,10 @@ public class ApplicationHandler extends HttpHandler {
     @Override
     public HttpResponse handleDELETE(HttpRequest request) {
         ApplicationId applicationId = getApplicationIdFromRequest(request);
-        boolean deleted = applicationRepository.delete(applicationId);
+        // TODO: Add support for timeout in request
+        boolean deleted = applicationRepository.delete(applicationId, Duration.ofSeconds(60));
         if ( ! deleted)
-            return HttpErrorResponse.notFoundError("Unable to delete " + applicationId + ": Not found");
+            return HttpErrorResponse.notFoundError("Unable to delete " + applicationId.toFullString() + ": Not found");
         return new DeleteApplicationResponse(Response.Status.OK, applicationId);
     }
 
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 f25b41bd121..b7eea3eb7fd 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
@@ -17,7 +17,7 @@ import com.yahoo.test.ManualClock;
 import com.yahoo.text.Utf8;
 import com.yahoo.vespa.config.server.application.OrchestratorMock;
 import com.yahoo.vespa.config.server.deploy.DeployTester;
-import com.yahoo.vespa.config.server.http.LogRetriever;
+import com.yahoo.vespa.config.server.http.InternalServerException;
 import com.yahoo.vespa.config.server.http.SessionHandlerTest;
 import com.yahoo.vespa.config.server.http.v2.PrepareResult;
 import com.yahoo.vespa.config.server.session.LocalSession;
@@ -34,8 +34,6 @@ import org.junit.rules.TemporaryFolder;
 
 import java.io.File;
 import java.io.IOException;
-import java.io.OutputStream;
-import java.nio.charset.StandardCharsets;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
@@ -51,6 +49,7 @@ import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author hmusum
@@ -277,6 +276,16 @@ public class ApplicationRepositoryTest {
 
             assertTrue(applicationRepository.delete(applicationId()));
         }
+
+        {
+            try {
+                deployApp(testApp);
+                applicationRepository.delete(applicationId(), Duration.ZERO);
+                fail("Should have gotten an exception");
+            } catch (InternalServerException e) {
+                assertEquals("Session 5 was not deleted (waited PT0S)", e.getMessage());
+            }
+        }
     }
 
     @Test
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
index 22ce0db89e2..87f4363050b 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/v2/ApplicationHandlerTest.java
@@ -123,6 +123,15 @@ public class ApplicationHandlerTest {
     }
 
     @Test
+    public void testDeleteNonExistent() throws Exception {
+        deleteAndAssertResponse(applicationId,
+                                Zone.defaultZone(),
+                                Response.Status.NOT_FOUND,
+                                HttpErrorResponse.errorCodes.NOT_FOUND,
+                                "Unable to delete mytenant.default.default: Not found");
+    }
+
+    @Test
     public void testGet() throws Exception {
         long sessionId = applicationRepository.deploy(testApp, prepareParams(applicationId)).sessionId();
         assertApplicationGeneration(applicationId, Zone.defaultZone(), sessionId, true);
@@ -234,6 +243,10 @@ public class ApplicationHandlerTest {
         deleteAndAssertResponse(toUrlPath(applicationId, zone, fullAppIdInUrl), expectedStatus, errorCode, expectedResponse, com.yahoo.jdisc.http.HttpRequest.Method.DELETE);
     }
 
+    private void deleteAndAssertResponse(ApplicationId applicationId, Zone zone, int expectedStatus, HttpErrorResponse.errorCodes errorCode, String expectedResponse) throws IOException {
+        deleteAndAssertResponse(toUrlPath(applicationId, zone, true), expectedStatus, errorCode, expectedResponse, com.yahoo.jdisc.http.HttpRequest.Method.DELETE);
+    }
+
     private void deleteAndAssertResponse(String url, int expectedStatus, HttpErrorResponse.errorCodes errorCode, String expectedResponse, com.yahoo.jdisc.http.HttpRequest.Method method) throws IOException {
         ApplicationHandler handler = createApplicationHandler();
         HttpResponse response = handler.handle(HttpRequest.createTestRequest(url, method));