Refactor deletion of local session

* Move delete method to SessionRepository
* Move test to ApplicationRepositoryTest

6 files changed, 106 insertions, 117 deletions

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 54d06deeaeb..4feaf51c3e0 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
@@ -19,7 +19,6 @@ import com.yahoo.vespa.config.server.ReloadHandler;
 import com.yahoo.vespa.config.server.ReloadListener;
 import com.yahoo.vespa.config.server.RequestHandler;
 import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs;
-import com.yahoo.vespa.config.server.host.HostRegistries;
 import com.yahoo.vespa.config.server.host.HostRegistry;
 import com.yahoo.vespa.config.server.host.HostValidator;
 import com.yahoo.vespa.config.server.monitoring.MetricUpdater;
@@ -432,4 +431,5 @@ public class TenantApplications implements RequestHandler, ReloadHandler, HostVa
         return hostRegistry.getKeyForHost(hostname);
     }
 
+    public TenantFileSystemDirs getTenantFileSystemDirs() { return tenantFileSystemDirs; }
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java
index b01ddb04d48..73e7b36c381 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/LocalSession.java
@@ -5,16 +5,11 @@ import com.yahoo.config.application.api.ApplicationFile;
 import com.yahoo.config.application.api.ApplicationMetaData;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.io.IOUtils;
 import com.yahoo.path.Path;
-import com.yahoo.transaction.AbstractTransaction;
-import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.config.server.TimeoutBudget;
 import com.yahoo.vespa.config.server.application.TenantApplications;
 
-import java.io.File;
-
 /**
  * A LocalSession is a session that has been created locally on this configserver. A local session can be edited and
  * prepared. Deleting a local session will ensure that the local filesystem state and global zookeeper state is
@@ -28,7 +23,6 @@ public class LocalSession extends Session {
 
     protected final ApplicationPackage applicationPackage;
     private final TenantApplications applicationRepo;
-    private final File serverDBSessionDir;
 
     /**
      * Creates a session. This involves loading the application, validating it and distributing it.
@@ -36,10 +30,9 @@ public class LocalSession extends Session {
      * @param sessionId The session id for this session.
      */
     public LocalSession(TenantName tenant, long sessionId, ApplicationPackage applicationPackage,
-                        SessionZooKeeperClient sessionZooKeeperClient, File serverDBSessionDir,
+                        SessionZooKeeperClient sessionZooKeeperClient,
                         TenantApplications applicationRepo) {
         super(tenant, sessionId, sessionZooKeeperClient);
-        this.serverDBSessionDir = serverDBSessionDir;
         this.applicationPackage = applicationPackage;
         this.applicationRepo = applicationRepo;
     }
@@ -78,12 +71,6 @@ public class LocalSession extends Session {
         return applicationPackage.getMetaData().getPreviousActiveGeneration();
     }
 
-    /** Add transactions to delete this session to the given nested transaction */
-    public void delete(NestedTransaction transaction) {
-        transaction.add(sessionZooKeeperClient.deleteTransaction(), FileTransaction.class);
-        transaction.add(FileTransaction.from(FileOperations.delete(serverDBSessionDir.getAbsolutePath())));
-    }
-
     public void waitUntilActivated(TimeoutBudget timeoutBudget) {
         sessionZooKeeperClient.getActiveWaiter().awaitCompletion(timeoutBudget.timeLeft());
     }
@@ -96,61 +83,4 @@ public class LocalSession extends Session {
 
     public ApplicationPackage getApplicationPackage() { return applicationPackage; }
 
-    // The rest of this class should be moved elsewhere ...
-    
-    private static class FileTransaction extends AbstractTransaction {
-        
-        public static FileTransaction from(FileOperation operation) {
-            FileTransaction transaction = new FileTransaction();
-            transaction.add(operation);
-            return transaction;
-        }
-
-        @Override
-        public void prepare() { }
-
-        @Override
-        public void commit() {
-            for (Operation operation : operations())
-                ((FileOperation)operation).commit();
-        }
-
-    }
-    
-    /** Factory for file operations */
-    private static class FileOperations {
-        
-        /** Creates an operation which recursively deletes the given path */
-        public static DeleteOperation delete(String pathToDelete) {
-            return new DeleteOperation(pathToDelete);
-        }
-        
-    }
-    
-    private interface FileOperation extends Transaction.Operation {
-
-        void commit();
-        
-    }
-
-    /** 
-     * Recursively deletes this path and everything below. 
-     * Succeeds with no action if the path does not exist.
-     */
-    private static class DeleteOperation implements FileOperation {
-
-        private final String pathToDelete;
-        
-        DeleteOperation(String pathToDelete) {
-            this.pathToDelete = pathToDelete;
-        }
-        
-        @Override
-        public void commit() {
-            // TODO: Check delete access in prepare()
-            IOUtils.recursiveDeleteDir(new File(pathToDelete));
-        }
-
-    }
-
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
index 2a61c621224..c6e33ed39ef 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionRepository.java
@@ -13,7 +13,9 @@ import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.io.IOUtils;
 import com.yahoo.path.Path;
+import com.yahoo.transaction.AbstractTransaction;
 import com.yahoo.transaction.NestedTransaction;
+import com.yahoo.transaction.Transaction;
 import com.yahoo.vespa.config.server.GlobalComponentRegistry;
 import com.yahoo.vespa.config.server.ReloadHandler;
 import com.yahoo.vespa.config.server.TimeoutBudget;
@@ -210,10 +212,18 @@ public class SessionRepository {
         if (watcher != null)  watcher.close();
         localSessionCache.removeSession(sessionId);
         NestedTransaction transaction = new NestedTransaction();
-        session.delete(transaction);
+        deleteLocalSession(session, transaction);
         transaction.commit();
     }
 
+    /** Add transactions to delete this session to the given nested transaction */
+    public void deleteLocalSession(LocalSession session, NestedTransaction transaction) {
+        long sessionId = session.getSessionId();
+        SessionZooKeeperClient sessionZooKeeperClient = createSessionZooKeeperClient(sessionId);
+        transaction.add(sessionZooKeeperClient.deleteTransaction(), FileTransaction.class);
+        transaction.add(FileTransaction.from(FileOperations.delete(getSessionAppDir(sessionId).getAbsolutePath())));
+    }
+
     public void close() {
         deleteAllSessions();
         tenantFileSystemDirs.delete();
@@ -424,8 +434,7 @@ public class SessionRepository {
         SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
         sessionZKClient.createNewSession(clock.instant());
         Curator.CompletionWaiter waiter = sessionZKClient.getUploadWaiter();
-        LocalSession session = new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient,
-                                                getSessionAppDir(sessionId), applicationRepo);
+        LocalSession session = new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient, applicationRepo);
         waiter.awaitCompletion(timeoutBudget.timeLeft());
         return session;
     }
@@ -483,8 +492,7 @@ public class SessionRepository {
             ApplicationPackage applicationPackage = createApplicationPackage(applicationFile, applicationId,
                                                                              sessionId, currentlyActiveSessionId, false);
             SessionZooKeeperClient sessionZooKeeperClient = createSessionZooKeeperClient(sessionId);
-            return new LocalSession(tenantName, sessionId, applicationPackage, sessionZooKeeperClient,
-                                    getSessionAppDir(sessionId), applicationRepo);
+            return new LocalSession(tenantName, sessionId, applicationPackage, sessionZooKeeperClient, applicationRepo);
         } catch (Exception e) {
             throw new RuntimeException("Error creating session " + sessionId, e);
         }
@@ -512,8 +520,7 @@ public class SessionRepository {
         File sessionDir = getAndValidateExistingSessionAppDir(sessionId);
         ApplicationPackage applicationPackage = FilesApplicationPackage.fromFile(sessionDir);
         SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId);
-        return new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient,
-                                getSessionAppDir(sessionId), applicationRepo);
+        return new LocalSession(tenantName, sessionId, applicationPackage, sessionZKClient, applicationRepo);
     }
 
     /**
@@ -585,4 +592,59 @@ public class SessionRepository {
         return getSessions().toString();
     }
 
+    private static class FileTransaction extends AbstractTransaction {
+
+        public static FileTransaction from(FileOperation operation) {
+            FileTransaction transaction = new FileTransaction();
+            transaction.add(operation);
+            return transaction;
+        }
+
+        @Override
+        public void prepare() { }
+
+        @Override
+        public void commit() {
+            for (Operation operation : operations())
+                ((FileOperation)operation).commit();
+        }
+
+    }
+
+    /** Factory for file operations */
+    private static class FileOperations {
+
+        /** Creates an operation which recursively deletes the given path */
+        public static DeleteOperation delete(String pathToDelete) {
+            return new DeleteOperation(pathToDelete);
+        }
+
+    }
+
+    private interface FileOperation extends Transaction.Operation {
+
+        void commit();
+
+    }
+
+    /**
+     * Recursively deletes this path and everything below.
+     * Succeeds with no action if the path does not exist.
+     */
+    private static class DeleteOperation implements FileOperation {
+
+        private final String pathToDelete;
+
+        DeleteOperation(String pathToDelete) {
+            this.pathToDelete = pathToDelete;
+        }
+
+        @Override
+        public void commit() {
+            // TODO: Check delete access in prepare()
+            IOUtils.recursiveDeleteDir(new File(pathToDelete));
+        }
+
+    }
+
 }
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 2c07d50b470..6039b91d2ec 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
@@ -33,6 +33,7 @@ import com.yahoo.vespa.config.protocol.VespaVersion;
 import com.yahoo.vespa.config.server.application.OrchestratorMock;
 import com.yahoo.vespa.config.server.application.TenantApplications;
 import com.yahoo.vespa.config.server.deploy.DeployTester;
+import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs;
 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;
@@ -45,6 +46,7 @@ import com.yahoo.vespa.config.server.session.SilentDeployLogger;
 import com.yahoo.vespa.config.server.tenant.ApplicationRolesStore;
 import com.yahoo.vespa.config.server.tenant.Tenant;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
+import com.yahoo.vespa.config.server.zookeeper.ConfigCurator;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.flags.FlagSource;
@@ -106,6 +108,7 @@ public class ApplicationRepositoryTest {
     private SessionHandlerTest.MockProvisioner  provisioner;
     private OrchestratorMock orchestrator;
     private TimeoutBudget timeoutBudget;
+    private ConfigCurator configCurator;
 
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
@@ -124,6 +127,7 @@ public class ApplicationRepositoryTest {
 
     public void setup(FlagSource flagSource) throws IOException {
         Curator curator = new MockCurator();
+        configCurator = ConfigCurator.create(curator);
         TestComponentRegistry componentRegistry = new TestComponentRegistry.Builder()
                 .curator(curator)
                 .configServerConfig(new ConfigserverConfig.Builder()
@@ -300,15 +304,21 @@ public class ApplicationRepositoryTest {
 
     @Test
     public void delete() {
+        TenantName tenantName = applicationId().tenant();
+        Tenant tenant = tenantRepository.getTenant(tenantName);
         {
             PrepareResult result = deployApp(testApp);
             long sessionId = result.sessionId();
-            Tenant tenant = tenantRepository.getTenant(applicationId().tenant());
             LocalSession applicationData = tenant.getSessionRepository().getSession(sessionId);
             assertNotNull(applicationData);
             assertNotNull(applicationData.getApplicationId());
             assertNotNull(tenant.getSessionRepo().getSession(sessionId));
             assertNotNull(applicationRepository.getActiveSession(applicationId()));
+            String sessionNode = TenantRepository.getSessionsPath(tenantName).append(String.valueOf(sessionId)).getAbsolute();
+            assertTrue(configCurator.exists(sessionNode));
+            TenantFileSystemDirs tenantFileSystemDirs = tenant.getApplicationRepo().getTenantFileSystemDirs();
+            File sessionFile = new File(tenantFileSystemDirs.sessionsPath(), String.valueOf(sessionId));
+            assertTrue(sessionFile.exists());
 
             // Delete app and verify that it has been deleted from repos and provisioner
             assertTrue(applicationRepository.delete(applicationId()));
@@ -318,6 +328,8 @@ public class ApplicationRepositoryTest {
             assertTrue(provisioner.removed);
             assertEquals(tenant.getName(), provisioner.lastApplicationId.tenant());
             assertEquals(applicationId(), provisioner.lastApplicationId);
+            assertFalse(configCurator.exists(sessionNode));
+            assertFalse(sessionFile.exists());
 
             assertFalse(applicationRepository.delete(applicationId()));
         }
@@ -354,7 +366,6 @@ public class ApplicationRepositoryTest {
             // A new delete should cleanup and be successful
             RemoteSession activeSession = applicationRepository.getActiveSession(applicationId());
             assertNull(activeSession);
-            Tenant tenant = tenantRepository.getTenant(applicationId().tenant());
             assertNull(tenant.getSessionRepo().getSession(prepareResult.sessionId()));
 
             assertTrue(applicationRepository.delete(applicationId()));
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/http/SessionHandlerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/http/SessionHandlerTest.java
index be368692aba..0b9a780d9e1 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/http/SessionHandlerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/http/SessionHandlerTest.java
@@ -86,7 +86,7 @@ public class SessionHandlerTest {
         private ApplicationId applicationId;
 
         public MockLocalSession(long sessionId, ApplicationPackage app) {
-            super(TenantName.defaultName(), sessionId, app, new MockSessionZKClient(app), null, null);
+            super(TenantName.defaultName(), sessionId, app, new MockSessionZKClient(app), null);
         }
 
         public MockLocalSession(long sessionId, ApplicationPackage app, ApplicationId applicationId) {
@@ -123,9 +123,6 @@ public class SessionHandlerTest {
             return createTime;
         }
 
-        @Override
-        public void delete(NestedTransaction transaction) {  }
-
     }
 
     public enum Cmd {
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java
index 9d84d5a7f28..c1377ae439b 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/LocalSessionTest.java
@@ -1,6 +1,7 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config.server.session;
 
+import com.yahoo.cloud.config.ConfigserverConfig;
 import com.yahoo.component.Version;
 import com.yahoo.config.application.api.ApplicationFile;
 import com.yahoo.config.model.application.provider.BaseDeployLogger;
@@ -11,26 +12,25 @@ import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.path.Path;
 import com.yahoo.slime.Slime;
-import com.yahoo.transaction.NestedTransaction;
 import com.yahoo.vespa.config.server.TestComponentRegistry;
 import com.yahoo.vespa.config.server.application.TenantApplications;
 import com.yahoo.vespa.config.server.deploy.DeployHandlerLogger;
-import com.yahoo.vespa.config.server.deploy.TenantFileSystemDirs;
 import com.yahoo.vespa.config.server.deploy.ZooKeeperClient;
 import com.yahoo.vespa.config.server.tenant.TenantRepository;
 import com.yahoo.vespa.config.server.zookeeper.ConfigCurator;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 
 import java.io.File;
-import java.nio.file.Files;
+import java.io.IOException;
 import java.time.Instant;
 import java.util.Collections;
 import java.util.Optional;
 
-import static com.yahoo.yolean.Exceptions.uncheck;
 import static org.hamcrest.core.Is.is;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
@@ -45,19 +45,26 @@ public class LocalSessionTest {
     private static final TenantName tenantName = TenantName.from("test_tenant");
     private static final Path tenantPath = Path.createRoot();
 
-    TenantRepository tenantRepository;
+    private TenantRepository tenantRepository;
     private Curator curator;
     private ConfigCurator configCurator;
-    private TenantFileSystemDirs tenantFileSystemDirs;
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
     @Before
-    public void setupTest() {
-        tenantRepository = new TenantRepository(new TestComponentRegistry.Builder().build(), false);
-        tenantRepository.addTenant(tenantName);
+    public void setupTest() throws IOException {
         curator = new MockCurator();
+        TestComponentRegistry componentRegistry = new TestComponentRegistry.Builder()
+                .curator(curator)
+                .configServerConfig(new ConfigserverConfig.Builder()
+                                            .configDefinitionsDir(temporaryFolder.newFolder().getAbsolutePath())
+                                            .configServerDBDir(temporaryFolder.newFolder().getAbsolutePath())
+                                            .build())
+                .build();
+        tenantRepository = new TenantRepository(componentRegistry, false);
+        tenantRepository.addTenant(tenantName);
         configCurator = ConfigCurator.create(curator);
-        tenantFileSystemDirs = new TenantFileSystemDirs(uncheck(() -> Files.createTempDirectory("serverdb")).toFile(),
-                                                        tenantName);
     }
 
     @Test
@@ -99,20 +106,6 @@ public class LocalSessionTest {
         assertFalse(f2.exists());
     }
 
-    @Test
-    public void require_that_session_can_be_deleted() throws Exception {
-        TenantName tenantName = TenantName.defaultName();
-        LocalSession session = createSession(tenantName, 3);
-        String sessionNode = TenantRepository.getSessionsPath(tenantName).append(String.valueOf(3)).getAbsolute();
-        assertTrue(configCurator.exists(sessionNode));
-        assertTrue(new File(tenantFileSystemDirs.sessionsPath(), "3").exists());
-        NestedTransaction transaction = new NestedTransaction();
-        session.delete(transaction);
-        transaction.commit();
-        assertFalse(configCurator.exists(sessionNode));
-        assertFalse(new File(tenantFileSystemDirs.sessionsPath(), "3").exists());
-    }
-
     @Test(expected = IllegalStateException.class)
     public void require_that_no_provision_info_throws_exception() throws Exception {
         createSession(TenantName.defaultName(), 3).getAllocatedHosts();
@@ -132,13 +125,9 @@ public class LocalSessionTest {
             zkClient.write(allocatedHosts.get());
         }
         zkClient.write(Collections.singletonMap(new Version(0, 0, 0), new MockFileRegistry()));
-        File sessionDir = new File(tenantFileSystemDirs.sessionsPath(), String.valueOf(sessionId));
-        sessionDir.createNewFile();
-        TenantApplications applications = TenantApplications.create(
-                new TestComponentRegistry.Builder().curator(curator).build(), tenant);
+        TenantApplications applications = tenantRepository.getTenant(tenantName).getApplicationRepo();
         applications.createApplication(zkc.readApplicationId());
-        return new LocalSession(tenant, sessionId, FilesApplicationPackage.fromFile(testApp),
-                                zkc, sessionDir, applications);
+        return new LocalSession(tenant, sessionId, FilesApplicationPackage.fromFile(testApp), zkc, applications);
     }
 
     private void doPrepare(LocalSession session) {
@@ -152,7 +141,7 @@ public class LocalSessionTest {
 
     private DeployHandlerLogger getLogger() {
         return new DeployHandlerLogger(new Slime().get(), false,
-                                       new ApplicationId.Builder().tenant("testtenant").applicationName("testapp").build());
+                                       new ApplicationId.Builder().tenant(tenantName).applicationName("testapp").build());
     }
 
 }