Merge pull request #12520 from vespa-engine/hakonhall/always-do-status-service-cleanup

Always do status service cleanup

6 files changed, 26 insertions, 51 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZkStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZkStatusService.java
index 3bf0d886a41..fab58aaf638 100644
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZkStatusService.java
+++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZkStatusService.java
@@ -47,7 +47,6 @@ public class ZkStatusService implements StatusService {
     private final HostInfosCache hostInfosCache;
     private final Metric metric;
     private final Timer timer;
-    private final boolean doCleanup;
     private final AntiServiceMonitor antiServiceMonitor;
 
     /**
@@ -61,24 +60,21 @@ public class ZkStatusService implements StatusService {
             @Component Curator curator,
             @Component Metric metric,
             @Component Timer timer,
-            @Component FlagSource flagSource,
             @Component AntiServiceMonitor antiServiceMonitor) {
         this(curator,
                 metric,
                 timer,
                 new HostInfosCache(curator, new HostInfosServiceImpl(curator, timer)),
-                Flags.CLEANUP_STATUS_SERVICE.bindTo(flagSource).value(),
                 antiServiceMonitor);
     }
 
     /** Non-private for testing only. */
     ZkStatusService(Curator curator, Metric metric, Timer timer, HostInfosCache hostInfosCache,
-                    boolean doCleanup, AntiServiceMonitor antiServiceMonitor) {
+                    AntiServiceMonitor antiServiceMonitor) {
         this.curator = curator;
         this.metric = metric;
         this.timer = timer;
         this.hostInfosCache = hostInfosCache;
-        this.doCleanup = doCleanup;
         this.antiServiceMonitor = antiServiceMonitor;
     }
 
@@ -241,19 +237,15 @@ public class ZkStatusService implements StatusService {
      */
     @Override
     public void onApplicationActivate(ApplicationInstanceReference reference, Set<HostName> hostnames) {
-        if (doCleanup) {
-            withLockForAdminOp(reference, " was activated", () -> {
-                HostInfos hostInfos = hostInfosCache.getCachedHostInfos(reference);
-                Set<HostName> toRemove = new HashSet<>(hostInfos.getZkHostnames());
-                toRemove.removeAll(hostnames);
-                if (toRemove.size() > 0) {
-                    log.log(LogLevel.INFO, "Removing " + toRemove + " of " + reference + " from status service");
-                    hostInfosCache.removeHosts(reference, toRemove);
-                }
-            });
-        } else {
-            log.log(LogLevel.INFO, "Would have removed orphaned hosts of " + reference + " from status service");
-        }
+        withLockForAdminOp(reference, " was activated", () -> {
+            HostInfos hostInfos = hostInfosCache.getCachedHostInfos(reference);
+            Set<HostName> toRemove = new HashSet<>(hostInfos.getZkHostnames());
+            toRemove.removeAll(hostnames);
+            if (toRemove.size() > 0) {
+                log.log(LogLevel.INFO, "Removing " + toRemove + " of " + reference + " from status service");
+                hostInfosCache.removeHosts(reference, toRemove);
+            }
+        });
     }
 
     /**
@@ -267,22 +259,18 @@ public class ZkStatusService implements StatusService {
      */
     @Override
     public void onApplicationRemove(ApplicationInstanceReference reference) {
-        if (doCleanup) {
-            withLockForAdminOp(reference, " was removed", () -> {
-                log.log(LogLevel.INFO, "Removing application " + reference + " from status service");
+        withLockForAdminOp(reference, " was removed", () -> {
+            log.log(LogLevel.INFO, "Removing application " + reference + " from status service");
 
-                // /vespa/application-status-service/REFERENCE
-                curator.delete(Path.fromString(applicationInstanceSuspendedPath(reference)));
+            // /vespa/application-status-service/REFERENCE
+            curator.delete(Path.fromString(applicationInstanceSuspendedPath(reference)));
 
-                // /vespa/host-status-service/REFERENCE/hosts-allowed-down
-                curator.delete(Path.fromString(hostsAllowedDownPath(reference)));
+            // /vespa/host-status-service/REFERENCE/hosts-allowed-down
+            curator.delete(Path.fromString(hostsAllowedDownPath(reference)));
 
-                // /vespa/host-status/APPLICATION_ID
-                hostInfosCache.removeApplication(reference);
-            });
-        } else {
-            log.log(LogLevel.INFO, "Would have removed application " + reference + " from status service");
-        }
+            // /vespa/host-status/APPLICATION_ID
+            hostInfosCache.removeApplication(reference);
+        });
     }
 
     private void withLockForAdminOp(ApplicationInstanceReference reference,
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
index f49dc490fe6..b32a6aafa56 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
@@ -80,7 +80,6 @@ public class OrchestratorImplTest {
             curator,
             mock(Metric.class),
             new TestTimer(),
-            flagSource,
             new DummyAntiServiceMonitor());
 
     private ApplicationId app1;
@@ -403,7 +402,6 @@ public class OrchestratorImplTest {
                 new MockCurator(),
                 mock(Metric.class),
                 new TestTimer(),
-                flagSource,
                 new DummyAntiServiceMonitor());
 
         HostName hostName = new HostName("host.yahoo.com");
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
index 88e9d1c90fe..8162542e540 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java
@@ -63,7 +63,6 @@ class ModelTestUtils {
             new MockCurator(),
             mock(Metric.class),
             new TestTimer(),
-            flagSource,
             new DummyAntiServiceMonitor());
     private final Orchestrator orchestrator = new OrchestratorImpl(new HostedVespaPolicy(new HostedVespaClusterPolicy(), clusterControllerClientFactory, applicationApiFactory()),
                                                                    clusterControllerClientFactory,
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java
index ffb79b4edf6..0605fcb8a0a 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java
@@ -82,8 +82,7 @@ public class HostResourceTest {
     private static final ApplicationInstanceId APPLICATION_INSTANCE_ID = new ApplicationInstanceId("applicationId");
     private static final ServiceMonitor serviceMonitor = mock(ServiceMonitor.class);
     private static final StatusService EVERY_HOST_IS_UP_HOST_STATUS_SERVICE = new ZkStatusService(
-            new MockCurator(), mock(Metric.class), new TestTimer(), new InMemoryFlagSource(),
-            new DummyAntiServiceMonitor());
+            new MockCurator(), mock(Metric.class), new TestTimer(), new DummyAntiServiceMonitor());
     private static final ApplicationApiFactory applicationApiFactory = new ApplicationApiFactory(3);
 
     static {
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusService2Test.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusService2Test.java
index e8cfe15611e..d3c366f5174 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusService2Test.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusService2Test.java
@@ -37,7 +37,7 @@ public class ZkStatusService2Test {
     private final CriticalRegion criticalRegion = mock(CriticalRegion.class);
     private final AntiServiceMonitor antiServiceMonitor = mock(AntiServiceMonitor.class);
     private final ZkStatusService zkStatusService =
-            new ZkStatusService(curator, metric, timer, cache, false, antiServiceMonitor);
+            new ZkStatusService(curator, metric, timer, cache, antiServiceMonitor);
 
     private final OrchestratorContext context = mock(OrchestratorContext.class);
     private final InterProcessMutex mutex = mock(InterProcessMutex.class);
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusServiceTest.java
index 9fe910079cf..ddff782cdb1 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusServiceTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZkStatusServiceTest.java
@@ -28,6 +28,7 @@ import org.hamcrest.Description;
 import org.hamcrest.Matcher;
 import org.hamcrest.TypeSafeMatcher;
 import org.junit.After;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
@@ -73,19 +74,19 @@ public class ZkStatusServiceTest {
     private final Metric metric = mock(Metric.class);
     private final OrchestratorContext context = mock(OrchestratorContext.class);
     private final InMemoryFlagSource flagSource = new InMemoryFlagSource();
-    private final ServiceMonitor serviceMonitor = mock(ServiceMonitor.class);
     private final CriticalRegion criticalRegion = mock(CriticalRegion.class);
     private final AntiServiceMonitor antiServiceMonitor = mock(AntiServiceMonitor.class);
 
     @Captor
     private ArgumentCaptor<Map<String, String>> captor;
 
+    @Before
     public void setUp() throws Exception {
         Logger.getLogger("").setLevel(LogLevel.WARNING);
 
         testingServer = new TestingServer();
         curator = createConnectedCurator(testingServer);
-        statusService = new ZkStatusService(curator, metric, timer, flagSource, antiServiceMonitor);
+        statusService = new ZkStatusService(curator, metric, timer, antiServiceMonitor);
         when(context.getTimeLeft()).thenReturn(Duration.ofSeconds(10));
         when(context.isProbe()).thenReturn(false);
         when(timer.currentTime()).thenReturn(Instant.ofEpochMilli(1));
@@ -112,7 +113,6 @@ public class ZkStatusServiceTest {
 
     @Test
     public void host_state_for_unknown_hosts_is_no_remarks() throws Exception {
-        setUp();
         assertThat(
                 statusService.getHostInfo(TestIds.APPLICATION_INSTANCE_REFERENCE, TestIds.HOST_NAME1).status(),
                 is(HostStatus.NO_REMARKS));
@@ -120,7 +120,6 @@ public class ZkStatusServiceTest {
 
     @Test
     public void setting_host_state_is_idempotent() throws Exception {
-        setUp();
         when(timer.currentTime()).thenReturn(
                 Instant.ofEpochMilli((1)),
                 Instant.ofEpochMilli((3)),
@@ -157,9 +156,8 @@ public class ZkStatusServiceTest {
 
     @Test
     public void locks_are_exclusive() throws Exception {
-        setUp();
         ZkStatusService zkStatusService2 =
-                new ZkStatusService(curator, mock(Metric.class), new TestTimer(), flagSource, antiServiceMonitor);
+                new ZkStatusService(curator, mock(Metric.class), new TestTimer(), antiServiceMonitor);
 
         final CompletableFuture<Void> lockedSuccessfullyFuture;
         try (ApplicationLock lock = statusService
@@ -184,9 +182,8 @@ public class ZkStatusServiceTest {
 
     @Test
     public void failing_to_get_lock_closes_SessionFailRetryLoop() throws Exception {
-        setUp();
         ZkStatusService zkStatusService2 =
-                new ZkStatusService(curator, mock(Metric.class), new TestTimer(), flagSource, antiServiceMonitor);
+                new ZkStatusService(curator, mock(Metric.class), new TestTimer(), antiServiceMonitor);
 
         try (ApplicationLock lock = statusService
                 .lockApplication(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) {
@@ -257,8 +254,6 @@ public class ZkStatusServiceTest {
     @Test
     public void suspend_and_resume_application_works_and_is_symmetric() throws Exception {
 
-        setUp();
-
         // Initial state is NO_REMARK
         assertThat(
                 statusService
@@ -290,7 +285,6 @@ public class ZkStatusServiceTest {
 
     @Test
     public void suspending_two_applications_returns_two_applications() throws Exception {
-        setUp();
         Set<ApplicationInstanceReference> suspendedApps = statusService.getAllSuspendedApplications();
         assertThat(suspendedApps.size(), is(0));
 
@@ -312,9 +306,6 @@ public class ZkStatusServiceTest {
 
     @Test
     public void zookeeper_cleanup() throws Exception {
-        flagSource.withBooleanFlag(Flags.CLEANUP_STATUS_SERVICE.id(), true);
-        setUp();
-
         HostName strayHostname = new HostName("stray1.com");
 
         verify(antiServiceMonitor, times(0)).disallowDuperModelLockAcquisition(any());