diff options
author | Valerij Fredriksen <freva@users.noreply.github.com> | 2020-03-21 09:13:33 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-03-21 09:13:33 +0100 |
commit | de5ac882cd07205b1aaaa045763694ff718b614f (patch) | |
tree | 06e2b40f8f2b01e04549b8891fbd60027cd85c02 /orchestrator | |
parent | a5912471ffcaad78e83b950b8ad2bb16bb9c2b41 (diff) | |
parent | 583437679933b94f58467b40ead155f3db325c65 (diff) |
Merge pull request #12520 from vespa-engine/hakonhall/always-do-status-service-cleanup
Always do status service cleanup
Diffstat (limited to 'orchestrator')
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()); |