From cb6f8dee6387515ff50bca132644afedb878ed75 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Fri, 21 Jul 2023 10:52:33 +0200 Subject: Ignore sessions in state NEW when initializing --- .../config/server/session/SessionRepository.java | 35 +++++++++++++--------- .../server/session/SessionRepositoryTest.java | 20 +++++++++++++ 2 files changed, 41 insertions(+), 14 deletions(-) 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 f82aa405380..65ac68ac234 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 @@ -182,7 +182,7 @@ public class SessionRepository { loadSessions(executor); } - // For testing + // non-private for testing void loadSessions(ExecutorService executor) { loadRemoteSessions(executor); try { @@ -410,14 +410,15 @@ public class SessionRepository { } private void loadRemoteSessions(ExecutorService executor) throws NumberFormatException { - Map> futures = new HashMap<>(); + Map> futures = new HashMap<>(); for (long sessionId : getRemoteSessionsFromZooKeeper()) { - futures.put(sessionId, executor.submit(() -> sessionAdded(sessionId))); + futures.put(sessionId, executor.submit(() -> addSession(sessionId, true))); } futures.forEach((sessionId, future) -> { try { - future.get(); - log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded"); + boolean loaded = future.get(); + if (loaded) + log.log(Level.FINE, () -> "Remote session " + sessionId + " loaded"); } catch (ExecutionException | InterruptedException e) { throw new RuntimeException("Could not load remote session " + sessionId, e); } @@ -425,26 +426,32 @@ public class SessionRepository { } /** - * A session for which we don't have a watcher, i.e. hitherto unknown to us. + * Add session if it is not already added and is not in a state where it should be ignored. * * @param sessionId session id for the new session + * @return true if added, false otherwise */ - public void sessionAdded(long sessionId) { - if (hasStatusDeleted(sessionId)) return; + public boolean addSession(long sessionId, boolean initializing) { + if (sessionShouldBeIgnored(sessionId, initializing)) return false; log.log(Level.FINE, () -> "Adding remote session " + sessionId); Session session = createRemoteSession(sessionId); - if (session.getStatus() == Session.Status.NEW) { - log.log(Level.FINE, () -> session.logPre() + "Confirming upload for session " + sessionId); + if (session.getStatus() == Session.Status.NEW) confirmUpload(session); - } createLocalSessionFromDistributedApplicationPackage(sessionId); + return true; } - private boolean hasStatusDeleted(long sessionId) { + private boolean sessionShouldBeIgnored(long sessionId, boolean initializing) { + var ignorableStates = new HashSet<>(Set.of(Session.Status.DELETE)); + // Also ignore sessions in NEW state when initializing, they are missing application package + // and other session data, usually because deployment failed + if (initializing) + ignorableStates.add(Session.Status.NEW); + SessionZooKeeperClient sessionZKClient = createSessionZooKeeperClient(sessionId); RemoteSession session = new RemoteSession(tenantName, sessionId, sessionZKClient); - return session.getStatus() == Session.Status.DELETE; + return ignorableStates.contains(session.getStatus()); } void activate(long sessionId) { @@ -960,7 +967,7 @@ public class SessionRepository { private void checkForAddedSessions(List sessions) { for (Long sessionId : sessions) if (remoteSessionCache.get(sessionId) == null) - sessionAdded(sessionId); + addSession(sessionId, false); } public Transaction createActivateTransaction(Session session) { diff --git a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java index 07d3aac5a52..fec4c096c7b 100644 --- a/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java +++ b/configserver/src/test/java/com/yahoo/vespa/config/server/session/SessionRepositoryTest.java @@ -83,6 +83,10 @@ public class SessionRepositoryTest { private void setup(ModelFactoryRegistry modelFactoryRegistry) throws Exception { curator = new MockCurator(); + setup(modelFactoryRegistry, curator); + } + + private void setup(ModelFactoryRegistry modelFactoryRegistry, MockCurator curator) throws Exception { File configserverDbDir = temporaryFolder.newFolder().getAbsoluteFile(); ConfigserverConfig configserverConfig = new ConfigserverConfig.Builder() .configServerDBDir(configserverDbDir.getAbsolutePath()) @@ -190,6 +194,22 @@ public class SessionRepositoryTest { assertTrue(sessionRepository.getRemoteSessionsFromZooKeeper().isEmpty()); } + // If reading a session that originally failed deployment we should just skip it + @Test + public void testThatSessionThatFailedDeploymentIsIgnored() throws Exception { + setup(); + + // Create a new session, which will have status NEW, which is what will be the state if e.g. + // a deployment failed when building the config model + createSession(3, false); + + // setup() will load remote sessions, session 3 should not be loaded, since it is in state NEW + setup(new ModelFactoryRegistry(List.of(VespaModelFactory.createTestFactory())), curator); + assertTrue(sessionRepository.getRemoteSessions().isEmpty()); + Path sessionPath = TenantRepository.getSessionsPath(tenantName).append(3 + ""); + assertTrue(curator.exists(sessionPath)); + } + @Test public void require_that_new_invalid_application_throws_exception() throws Exception { MockModelFactory failingFactory = new MockModelFactory(); -- cgit v1.2.3