Merge pull request #6391 from vespa-engine/hmusum/delete-expired-remote-sessions

Delete expired remote sessions

7 files changed, 76 insertions, 21 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 770b4c75488..b14fe5bcac9 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
@@ -558,10 +558,20 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return session.getSessionId();
     }
 
-    public void deleteOldSessions() {
+    public void deleteExpiredLocalSessions() {
         listApplications().forEach(app -> tenantRepository.getTenant(app.tenant()).getLocalSessionRepo().purgeOldSessions());
     }
 
+    public int deleteExpiredRemoteSessions(Duration expiryTime) {
+        return listApplications()
+                .stream()
+                .map(app -> tenantRepository.getTenant(app.tenant()).getRemoteSessionRepo()
+                        // TODO: Delete in all zones
+                        .deleteExpiredSessions(expiryTime, zone().system() == SystemName.cd))
+                .mapToInt(i -> i)
+                .sum();
+    }
+
     // ---------------- Tenant operations ----------------------------------------------------------------
 
     public Set<TenantName> deleteUnusedTenants(Duration ttlForUnusedTenant, Instant now) {
@@ -605,7 +615,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return getLocalSession(tenant, sessionId).getMetaData();
     }
 
-    ConfigserverConfig configserverConfig() {
+    public ConfigserverConfig configserverConfig() {
         return configserverConfig;
     }
 
@@ -765,7 +775,7 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         return deployLog;
     }
 
-    private Zone zone() {
+    public Zone zone() {
         return new Zone(SystemName.from(configserverConfig.system()),
                         Environment.from(configserverConfig.environment()),
                         RegionName.from(configserverConfig.region()));
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/SessionsMaintainer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/SessionsMaintainer.java
index 53f2553161f..a0cbf4e4845 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/SessionsMaintainer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/maintenance/SessionsMaintainer.java
@@ -14,13 +14,22 @@ import java.time.Duration;
  * @author hmusum
  */
 public class SessionsMaintainer extends Maintainer {
+    private final boolean hostedVespa;
 
     SessionsMaintainer(ApplicationRepository applicationRepository, Curator curator, Duration interval) {
         super(applicationRepository, curator, interval);
+        this.hostedVespa = applicationRepository.configserverConfig().hostedVespa();
     }
 
     @Override
     protected void maintain() {
-        applicationRepository.deleteOldSessions();
+        applicationRepository.deleteExpiredLocalSessions();
+
+        // Expired remote sessions are not expected to exist, they should have been deleted when
+        // a deployment happened or when the application was deleted. We still see them from time to time,
+        // probably due to some race or another bug
+        Duration expiryTime = Duration.ofDays(30);
+        if (hostedVespa)
+            applicationRepository.deleteExpiredRemoteSessions(expiryTime);
     }
 }
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 70ea20d5b4d..0f9f8b72de1 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
@@ -134,11 +134,6 @@ public class LocalSession extends Session implements Comparable<LocalSession> {
         return lhsId.compareTo(rhsId);
     }
 
-    // in seconds
-    public long getCreateTime() {
-        return zooKeeperClient.readCreateTime();
-    }
-
     public void waitUntilActivated(TimeoutBudget timeoutBudget) {
         zooKeeperClient.getActiveWaiter().awaitCompletion(timeoutBudget.timeLeft());
     }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java
index c47d5791a1b..dbccba395a2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSession.java
@@ -132,4 +132,9 @@ public class RemoteSession extends Session {
         }
     }
 
+    public void delete() {
+        Transaction transaction = zooKeeperClient.deleteTransaction();
+        transaction.commit();
+    }
+
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
index d16d5a17518..8cd4977de65 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/RemoteSessionRepo.java
@@ -1,6 +1,8 @@
 // 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 java.time.Duration;
+import java.time.Instant;
 import java.util.*;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -48,6 +50,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
     private final RemoteSessionFactory remoteSessionFactory;
     private final Map<Long, RemoteSessionStateWatcher> sessionStateWatchers = new HashMap<>();
     private final ReloadHandler reloadHandler;
+    private final TenantName tenantName;
     private final MetricUpdater metrics;
     private final Curator.DirectoryCache directoryCache;
     private final TenantApplications applicationRepo;
@@ -56,20 +59,21 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
      * @param curator              a {@link Curator} instance.
      * @param remoteSessionFactory a {@link com.yahoo.vespa.config.server.session.RemoteSessionFactory}
      * @param reloadHandler        a {@link com.yahoo.vespa.config.server.ReloadHandler}
-     * @param tenant               a {@link TenantName} instance.
+     * @param tenantName           a {@link TenantName} instance.
      * @param applicationRepo      a {@link TenantApplications} instance.
      */
     public RemoteSessionRepo(Curator curator,
                              RemoteSessionFactory remoteSessionFactory,
                              ReloadHandler reloadHandler,
-                             TenantName tenant,
+                             TenantName tenantName,
                              TenantApplications applicationRepo,
                              MetricUpdater metricUpdater) {
         this.curator = curator;
-        this.sessionsPath = TenantRepository.getSessionsPath(tenant);
+        this.sessionsPath = TenantRepository.getSessionsPath(tenantName);
         this.applicationRepo = applicationRepo;
         this.remoteSessionFactory = remoteSessionFactory;
         this.reloadHandler = reloadHandler;
+        this.tenantName = tenantName;
         this.metrics = metricUpdater;
         initializeSessions();
         this.directoryCache = curator.createDirectoryCache(sessionsPath.getAbsolute(), false, false, pathChildrenExecutor);
@@ -82,12 +86,37 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
         this.curator = null;
         this.remoteSessionFactory = null;
         this.reloadHandler = null;
+        this.tenantName = tenantName;
         this.sessionsPath = TenantRepository.getSessionsPath(tenantName);
         this.metrics = null;
         this.directoryCache = null;
         this.applicationRepo = null;
     }
 
+    public List<Long> getSessions() {
+        return getSessionList(curator.getChildren(sessionsPath));
+    }
+
+    public int deleteExpiredSessions(Duration expiryTime, boolean deleteFromZooKeeper) {
+        int deleted = 0;
+        for (long sessionId : getSessions()) {
+            RemoteSession session = getSession(sessionId);
+            Instant created = Instant.ofEpochSecond(session.getCreateTime());
+            if (session.getStatus() == Session.Status.DEACTIVATE && sessionHasExpired(created, expiryTime)) {
+                log.log(LogLevel.INFO, "Remote session " + sessionId + " for " + tenantName + " has expired");
+                if (deleteFromZooKeeper) {
+                    session.delete();
+                    deleted++;
+                }
+            }
+        }
+        return deleted;
+    }
+
+    private boolean sessionHasExpired(Instant created, Duration expiryTime) {
+        return (created.plus(expiryTime).isBefore(Instant.now()));
+    }
+
     private void loadActiveSession(RemoteSession session) {
         tryReload(session.ensureApplicationLoaded(), session.logPre());
     }
@@ -113,7 +142,7 @@ public class RemoteSessionRepo extends SessionRepo<RemoteSession> implements Nod
 
     // TODO: Add sessions in parallel
     private void initializeSessions() throws NumberFormatException {
-        getSessionList(curator.getChildren(sessionsPath)).forEach(this::sessionAdded);
+        getSessions().forEach(this::sessionAdded);
     }
 
     private synchronized void sessionsChanged() throws NumberFormatException {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
index e98931b0573..64ecc510fe9 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/Session.java
@@ -67,4 +67,9 @@ public abstract class Session {
         return TenantRepository.logPre(getTenant());
     }
 
+    // in seconds
+    public long getCreateTime() {
+        return zooKeeperClient.readCreateTime();
+    }
+
 }
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 b0320dad88b..6148c276088 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
@@ -245,12 +245,11 @@ public class ApplicationRepositoryTest {
     @Test
     public void testDeletingInactiveSessions() {
         ManualClock clock = new ManualClock(Instant.now());
-        ConfigserverConfig configserverConfig = new ConfigserverConfig(new ConfigserverConfig.Builder()
-                                                                               .configServerDBDir(Files.createTempDir()
-                                                                                                          .getAbsolutePath())
-                                                                               .configDefinitionsDir(Files.createTempDir()
-                                                                                                             .getAbsolutePath())
-                                                                               .sessionLifetime(60));
+        ConfigserverConfig configserverConfig =
+                new ConfigserverConfig(new ConfigserverConfig.Builder()
+                                               .configServerDBDir(Files.createTempDir().getAbsolutePath())
+                                               .configDefinitionsDir(Files.createTempDir().getAbsolutePath())
+                                               .sessionLifetime(60));
         DeployTester tester = new DeployTester(configserverConfig, clock);
         tester.deployApp("src/test/apps/app", "myapp", Instant.now()); // session 2 (numbering starts at 2)
 
@@ -274,11 +273,14 @@ public class ApplicationRepositoryTest {
 
         clock.advance(Duration.ofHours(1)); // longer than session lifetime
 
-        // All sessions except 3 should be removed after the call to deleteOldSessions
-        tester.applicationRepository().deleteOldSessions();
+        // All sessions except 3 should be removed after the call to deleteExpiredLocalSessions
+        tester.applicationRepository().deleteExpiredLocalSessions();
         final Collection<LocalSession> sessions = tester.tenant().getLocalSessionRepo().listSessions();
         assertEquals(1, sessions.size());
         assertEquals(3, new ArrayList<>(sessions).get(0).getSessionId());
+
+        // There should be no expired remote sessions in the common case
+        assertEquals(0, applicationRepository.deleteExpiredRemoteSessions(Duration.ofSeconds(0)));
     }
 
     private PrepareResult prepareAndActivateApp(File application) throws IOException {