Merge pull request #14095 from vespa-engine/revert-14094-revert-14092-hmusum/throw-instead-of-returning-null

Reapply "Throw exception instead of returning null when reading application id"

10 files changed, 72 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 b8508922b78..ebeb7e7e377 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
@@ -760,9 +760,9 @@ public class ApplicationRepository implements com.yahoo.config.provision.Deploye
         Set<ApplicationId> applicationIds = new HashSet<>();
         sessionsPerTenant.values()
                 .forEach(sessionList -> sessionList.stream()
-                        .map(Session::getApplicationId)
-                        .filter(Objects::nonNull)
-                        .forEach(applicationIds::add));
+                        .map(Session::getOptionalApplicationId)
+                        .filter(Optional::isPresent)
+                        .forEach(appId -> applicationIds.add(appId.get())));
 
         Map<ApplicationId, Long> activeSessions = new HashMap<>();
         applicationIds.forEach(applicationId -> {
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java b/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java
index 0aab83d5a6a..cbea6d99dd2 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/ConfigServerBootstrap.java
@@ -105,7 +105,11 @@ public class ConfigServerBootstrap extends AbstractComponent implements Runnable
         initializing(vipStatusMode);
 
         // Run maintainers that cleans up zookeeper and disk usage before bootstrapping
-        configServerMaintenance.ifPresent(ConfigServerMaintenance::runBeforeBootstrap);
+        try {
+            configServerMaintenance.ifPresent(ConfigServerMaintenance::runBeforeBootstrap);
+        } catch (Exception e) {
+            log.log(Level.INFO, "Running maintainers before bootstrap failed, continuing with bootstrap", e);
+        }
 
         switch (mode) {
             case BOOTSTRAP_IN_SEPARATE_THREAD:
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
index 8c2e6027691..11ce659625d 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/deploy/Deployment.java
@@ -108,17 +108,18 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
     @Override
     public void prepare() {
         if (prepared) return;
-        try (ActionTimer timer = applicationRepository.timerFor(session.getApplicationId(), "deployment.prepareMillis")) {
+        ApplicationId applicationId = session.getApplicationId();
+        try (ActionTimer timer = applicationRepository.timerFor(applicationId, "deployment.prepareMillis")) {
             TimeoutBudget timeoutBudget = new TimeoutBudget(clock, timeout);
 
-            PrepareParams.Builder params = new PrepareParams.Builder().applicationId(session.getApplicationId())
+            PrepareParams.Builder params = new PrepareParams.Builder().applicationId(applicationId)
                     .timeoutBudget(timeoutBudget)
                     .ignoreValidationErrors(!validate)
                     .vespaVersion(version.toString())
                     .isBootstrap(isBootstrap);
             dockerImageRepository.ifPresent(params::dockerImageRepository);
             athenzDomain.ifPresent(params::athenzDomain);
-            Optional<ApplicationSet> activeApplicationSet = applicationRepository.getCurrentActiveApplicationSet(tenant, session.getApplicationId());
+            Optional<ApplicationSet> activeApplicationSet = applicationRepository.getCurrentActiveApplicationSet(tenant, applicationId);
             tenant.getSessionRepository().prepareLocalSession(session, logger, params.build(), activeApplicationSet,
                                                               tenant.getPath(), clock.instant());
             this.prepared = true;
@@ -131,11 +132,10 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
         if ( ! prepared)
             prepare();
 
-        try (ActionTimer timer = applicationRepository.timerFor(session.getApplicationId(), "deployment.activateMillis")) {
+        validateSessionStatus(session);
+        ApplicationId applicationId = session.getApplicationId();
+        try (ActionTimer timer = applicationRepository.timerFor(applicationId, "deployment.activateMillis")) {
             TimeoutBudget timeoutBudget = new TimeoutBudget(clock, timeout);
-            validateSessionStatus(session);
-            ApplicationId applicationId = session.getApplicationId();
-
             if ( ! timeoutBudget.hasTimeLeft()) throw new RuntimeException("Timeout exceeded when trying to activate '" + applicationId + "'");
 
             RemoteSession previousActiveSession;
@@ -148,7 +148,7 @@ public class Deployment implements com.yahoo.config.provision.Deployment {
                 throw e;
             }
             catch (Exception e) {
-                throw new InternalServerException("Error activating application", e);
+                throw new InternalServerException("Error when activating '" + applicationId + "'", e);
             }
 
             waiter.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 642ac33ab09..36cac87a326 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
@@ -60,7 +60,7 @@ public class RemoteSession extends Session {
         // Read hosts allocated on the config server instance which created this
         Optional<AllocatedHosts> allocatedHosts = applicationPackage.getAllocatedHosts();
 
-        return ApplicationSet.fromList(applicationLoader.buildModels(sessionZooKeeperClient.readApplicationId(),
+        return ApplicationSet.fromList(applicationLoader.buildModels(getApplicationId(),
                                                                      sessionZooKeeperClient.readDockerImageRepository(),
                                                                      sessionZooKeeperClient.readVespaVersion(),
                                                                      applicationPackage,
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 d401669b8d6..b3e35e955de 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
@@ -134,7 +134,20 @@ public abstract class Session implements Comparable<Session>  {
         sessionZooKeeperClient.writeAthenzDomain(athenzDomain);
     }
 
-    public ApplicationId getApplicationId() { return sessionZooKeeperClient.readApplicationId(); }
+    /** Returns application id read from ZooKeeper. Will throw RuntimeException if not found */
+    public ApplicationId getApplicationId() {
+        return sessionZooKeeperClient.readApplicationId()
+                .orElseThrow(() -> new RuntimeException("Unable to read application id for session " + sessionId));
+    }
+
+    /** Returns application id read from ZooKeeper. Will return Optional.empty() if not found */
+    public Optional<ApplicationId> getOptionalApplicationId() {
+        try {
+            return Optional.of(getApplicationId());
+        } catch (RuntimeException e) {
+            return Optional.empty();
+        }
+    }
 
     public FileReference getApplicationPackageReference() {return sessionZooKeeperClient.readApplicationPackageReference(); }
 
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 6c4ef469be6..cce79412cfe 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
@@ -181,8 +181,9 @@ public class SessionRepository {
                     deleteLocalSession(candidate);
                 } else if (createTime.plus(Duration.ofDays(1)).isBefore(clock.instant())) {
                     //  Sessions with state ACTIVATE, but which are not actually active
-                    ApplicationId applicationId = candidate.getApplicationId();
-                    Long activeSession = activeSessions.get(applicationId);
+                    Optional<ApplicationId> applicationId = candidate.getOptionalApplicationId();
+                    if (applicationId.isEmpty()) continue;
+                    Long activeSession = activeSessions.get(applicationId.get());
                     if (activeSession == null || activeSession != candidate.getSessionId()) {
                         deleteLocalSession(candidate);
                         log.log(Level.INFO, "Deleted inactive session " + candidate.getSessionId() + " created " +
@@ -622,7 +623,8 @@ public class SessionRepository {
                 log.log(Level.INFO, "File reference for session id " + sessionId + ": " + fileReference + " not found in " + fileDirectory);
                 return Optional.empty();
             }
-            ApplicationId applicationId = sessionZKClient.readApplicationId();
+            ApplicationId applicationId = sessionZKClient.readApplicationId()
+                    .orElseThrow(() -> new RuntimeException("Could not find application id for session " + sessionId));
             log.log(Level.INFO, "Creating local session for tenant '" + tenantName + "' with session id " + sessionId);
             LocalSession localSession = createLocalSession(sessionDir, applicationId, sessionId);
             addLocalSession(localSession);
@@ -696,7 +698,7 @@ public class SessionRepository {
 
     public Transaction createActivateTransaction(Session session) {
         Transaction transaction = createSetStatusTransaction(session, Session.Status.ACTIVATE);
-        transaction.add(applicationRepo.createPutTransaction(session.sessionZooKeeperClient.readApplicationId(), session.getSessionId()).operations());
+        transaction.add(applicationRepo.createPutTransaction(session.getApplicationId(), session.getSessionId()).operations());
         return transaction;
     }
 
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
index b0236e4e4e4..bbf72067b00 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClient.java
@@ -11,7 +11,6 @@ import com.yahoo.config.provision.AllocatedHosts;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.AthenzDomain;
 import com.yahoo.config.provision.DockerImage;
-import com.yahoo.config.provision.NodeFlavors;
 import com.yahoo.path.Path;
 import com.yahoo.text.Utf8;
 import com.yahoo.transaction.Transaction;
@@ -141,9 +140,11 @@ public class SessionZooKeeperClient {
         configCurator.putData(applicationIdPath(), id.serializedForm());
     }
 
-    public ApplicationId readApplicationId() {
+    public Optional<ApplicationId> readApplicationId() {
         String idString = configCurator.getData(applicationIdPath());
-        return idString == null ? null : ApplicationId.fromSerializedForm(idString);
+        return (idString == null)
+                ? Optional.empty()
+                : Optional.of(ApplicationId.fromSerializedForm(idString));
     }
 
     void writeApplicationPackageReference(FileReference applicationPackageReference) {
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 99530041088..c2e394fc450 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
@@ -9,6 +9,7 @@ import com.yahoo.config.application.api.ApplicationMetaData;
 import com.yahoo.config.model.NullConfigModelRegistry;
 import com.yahoo.config.model.api.ApplicationRoles;
 import com.yahoo.config.model.application.provider.BaseDeployLogger;
+import com.yahoo.config.model.application.provider.FilesApplicationPackage;
 import com.yahoo.config.provision.AllocatedHosts;
 import com.yahoo.config.provision.ApplicationId;
 import com.yahoo.config.provision.ApplicationName;
@@ -41,11 +42,13 @@ import com.yahoo.vespa.config.server.session.PrepareParams;
 import com.yahoo.vespa.config.server.session.RemoteSession;
 import com.yahoo.vespa.config.server.session.Session;
 import com.yahoo.vespa.config.server.session.SessionRepository;
+import com.yahoo.vespa.config.server.session.SessionZooKeeperClient;
 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.config.util.ConfigUtils;
 import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.flags.FlagSource;
@@ -61,6 +64,7 @@ import org.junit.rules.TemporaryFolder;
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -105,6 +109,7 @@ public class ApplicationRepositoryTest {
     private SessionHandlerTest.MockProvisioner  provisioner;
     private OrchestratorMock orchestrator;
     private TimeoutBudget timeoutBudget;
+    private Curator curator;
     private ConfigCurator configCurator;
 
     @Rule
@@ -119,7 +124,7 @@ public class ApplicationRepositoryTest {
     }
 
     public void setup(FlagSource flagSource) throws IOException {
-        Curator curator = new MockCurator();
+        curator = new MockCurator();
         configCurator = ConfigCurator.create(curator);
         ConfigserverConfig configserverConfig = new ConfigserverConfig.Builder()
                 .payloadCompressionType(ConfigserverConfig.PayloadCompressionType.Enum.UNCOMPRESSED)
@@ -344,9 +349,10 @@ public class ApplicationRepositoryTest {
 
     @Test
     public void testDeletingInactiveSessions() throws IOException {
+        File serverdb = temporaryFolder.newFolder("serverdb");
         ConfigserverConfig configserverConfig =
                 new ConfigserverConfig(new ConfigserverConfig.Builder()
-                                               .configServerDBDir(temporaryFolder.newFolder("serverdb").getAbsolutePath())
+                                               .configServerDBDir(serverdb.getAbsolutePath())
                                                .configDefinitionsDir(temporaryFolder.newFolder("configdefinitions").getAbsolutePath())
                                                .sessionLifetime(60));
         DeployTester tester = new DeployTester(configserverConfig, clock);
@@ -384,6 +390,8 @@ public class ApplicationRepositoryTest {
         // There should be no expired remote sessions in the common case
         assertEquals(0, tester.applicationRepository().deleteExpiredRemoteSessions(clock, Duration.ofSeconds(0)));
 
+        assertEquals(1, sessionRepository.getLocalSessions().size());
+
         // Deploy, but do not activate
         Optional<com.yahoo.config.provision.Deployment> deployment4 = tester.redeployFromLocalActive();
         assertTrue(deployment4.isPresent());
@@ -393,6 +401,24 @@ public class ApplicationRepositoryTest {
         sessionRepository.deleteLocalSession(localSession);
         assertEquals(1, sessionRepository.getLocalSessions().size());
 
+        // Create a local session without any data in zookeeper (corner case seen in production occasionally)
+        // and check that expiring local sessions still work
+        int sessionId = 6;
+        Files.createDirectory(new TenantFileSystemDirs(serverdb, tenant1).getUserApplicationDir(sessionId).toPath());
+        LocalSession localSession2 = new LocalSession(tenant1,
+                                                      sessionId,
+                                                      FilesApplicationPackage.fromFile(testApp),
+                                                      new SessionZooKeeperClient(curator,
+                                                                                 configCurator,
+                                                                                 sessionRepository.getSessionPath(sessionId),
+                                                                                 ConfigUtils.getCanonicalHostName()));
+        sessionRepository.addLocalSession(localSession2);
+        assertEquals(2, sessionRepository.getLocalSessions().size());
+
+        // Check that trying to expire local session when there exists a local session with no zookeeper data works
+        tester.applicationRepository().deleteExpiredLocalSessions();
+        assertEquals(1, sessionRepository.getLocalSessions().size());
+
         // Check that trying to expire when there are no active sessions works
         tester.applicationRepository().deleteExpiredLocalSessions();
     }
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
index 1fa7ceed755..ee8f00f6bcf 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionPreparerTest.java
@@ -201,7 +201,7 @@ public class SessionPreparerTest {
         PrepareParams params = new PrepareParams.Builder().applicationId(origId).build();
         prepare(testApp, params);
         assertTrue(configCurator.exists(sessionsPath.append(SessionZooKeeperClient.APPLICATION_ID_PATH).getAbsolute()));
-        assertThat(createSessionZooKeeperClient().readApplicationId(), is(origId));
+        assertThat(createSessionZooKeeperClient().readApplicationId().get(), is(origId));
     }
 
     @Test
diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClientTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClientTest.java
index fd0e34b9814..2fc4c0f456f 100644
--- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClientTest.java
+++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionZooKeeperClientTest.java
@@ -108,8 +108,8 @@ public class SessionZooKeeperClientTest {
         SessionZooKeeperClient zkc = createSessionZKClient(sessionId);
         String path = "/" + sessionId + "/" + SessionZooKeeperClient.APPLICATION_ID_PATH;
         configCurator.putData(path, idString);
-        ApplicationId zkId = zkc.readApplicationId();
-        assertThat(zkId.serializedForm(), is(expectedIdString));
+        ApplicationId applicationId = zkc.readApplicationId().get();
+        assertThat(applicationId.serializedForm(), is(expectedIdString));
     }
 
     private SessionZooKeeperClient createSessionZKClient(String sessionId) {