diff options
author | Bjørn Christian Seime <bjorncs@verizonmedia.com> | 2022-01-27 16:36:23 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-01-27 16:36:23 +0100 |
commit | 1452e6339dfcbd9168c780966d4cf33ce6a71cf1 (patch) | |
tree | 068c7bc47b48b74ce708c6e95859a8e692c8467c | |
parent | c2f1c6bec91ffe1d5d29df14a899acf179827d9d (diff) | |
parent | cc9334d13a5df7aed420413f56a0a25c0c12eeb3 (diff) |
Merge pull request #20949 from vespa-engine/bjorncs/jdisc-pre-shutdown
Bjorncs/jdisc pre shutdown [run-systemtest]
5 files changed, 113 insertions, 66 deletions
diff --git a/container-disc/pom.xml b/container-disc/pom.xml index 0d602e9e730..1c3115900de 100644 --- a/container-disc/pom.xml +++ b/container-disc/pom.xml @@ -21,6 +21,16 @@ <scope>test</scope> </dependency> <dependency> + <groupId>org.junit.jupiter</groupId> + <artifactId>junit-jupiter</artifactId> + <scope>test</scope> + </dependency> + <dependency> + <groupId>org.junit.vintage</groupId> + <artifactId>junit-vintage-engine</artifactId> + <scope>test</scope> + </dependency> + <dependency> <groupId>com.yahoo.vespa</groupId> <artifactId>testutil</artifactId> <version>${project.version}</version> diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java index d686bb9a3dd..e31200a4e59 100644 --- a/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/ConfiguredApplication.java @@ -8,7 +8,6 @@ import com.google.inject.Injector; import com.yahoo.cloud.config.SlobroksConfig; import com.yahoo.component.Vtag; import com.yahoo.component.provider.ComponentRegistry; -import com.yahoo.concurrent.DaemonThreadFactory; import com.yahoo.config.ConfigInstance; import com.yahoo.config.subscription.ConfigInterruptedException; import com.yahoo.container.Container; @@ -43,7 +42,6 @@ import com.yahoo.log.LogSetup; import com.yahoo.messagebus.network.rpc.SlobrokConfigSubscriber; import com.yahoo.net.HostName; import com.yahoo.vespa.config.ConfigKey; -import com.yahoo.vespa.defaults.Defaults; import com.yahoo.yolean.Exceptions; import com.yahoo.yolean.UncheckedInterruptedException; @@ -55,9 +53,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.WeakHashMap; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ScheduledThreadPoolExecutor; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.Phaser; import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; import java.util.logging.Logger; @@ -71,7 +67,6 @@ public final class ConfiguredApplication implements Application { private static final Logger log = Logger.getLogger(ConfiguredApplication.class.getName()); private static final Set<ClientProvider> startedClients = Collections.newSetFromMap(new WeakHashMap<>()); - static final String SANITIZE_FILENAME = "[/,;]"; private static final Set<ServerProvider> startedServers = Collections.newSetFromMap(new IdentityHashMap<>()); private final SubscriberFactory subscriberFactory; @@ -86,6 +81,7 @@ public final class ConfiguredApplication implements Application { // to config to make sure that container will be registered in slobrok (by {@link com.yahoo.jrt.slobrok.api.Register}) // if slobrok config changes (typically slobroks moving to other nodes) private final Optional<SlobrokConfigSubscriber> slobrokConfigSubscriber; + private final ShutdownDeadline shutdownDeadline; //TODO: FilterChainRepository should instead always be set up in the model. private final FilterChainRepository defaultFilterChainRepository = @@ -95,8 +91,8 @@ public final class ConfiguredApplication implements Application { new ComponentRegistry<>(), new ComponentRegistry<>()); private final OsgiFramework restrictedOsgiFramework; + private final Phaser nonTerminatedContainerTracker = new Phaser(1); private HandlersConfigurerDi configurer; - private ScheduledThreadPoolExecutor shutdownDeadlineExecutor; private Thread reconfigurerThread; private Thread portWatcher; private QrConfig qrConfig; @@ -136,6 +132,7 @@ public final class ConfiguredApplication implements Application { ? Optional.of(new SlobrokConfigSubscriber(configId)) : Optional.empty(); this.restrictedOsgiFramework = new DisableOsgiFramework(new RestrictedBundleContext(osgiFramework.bundleContext())); + this.shutdownDeadline = new ShutdownDeadline(configId); } @Override @@ -256,8 +253,7 @@ public final class ConfiguredApplication implements Application { configurer.getComponent(ApplicationContext.class).discBindingsConfig); installServerProviders(builder); - DeactivatedContainer deactivated = activator.activateContainer(builder); - if (deactivated != null) deactivated.notifyTermination(cleanupTask); + activateContainer(builder, cleanupTask); startClients(); startAndStopServers(); @@ -267,6 +263,20 @@ public final class ConfiguredApplication implements Application { metric.set("application_generation", configurer.generation(), metric.createContext(Map.of())); } + private void activateContainer(ContainerBuilder builder, Runnable onPreviousContainerTermination) { + DeactivatedContainer deactivated = activator.activateContainer(builder); + if (deactivated != null) { + nonTerminatedContainerTracker.register(); + deactivated.notifyTermination(() -> { + try { + onPreviousContainerTermination.run(); + } finally { + nonTerminatedContainerTracker.arriveAndDeregister(); + } + }); + } + } + private ContainerBuilder createBuilderWithGuiceBindings() { ContainerBuilder builder = activator.newContainerBuilder(); setupGuiceBindings(builder.guiceModules()); @@ -371,9 +381,8 @@ public final class ConfiguredApplication implements Application { @Override public void stop() { - startShutdownDeadlineExecutor(); + shutdownDeadline.schedule((long)(shutdownTimeoutS.get() * 1000), dumpHeapOnShutdownTimeout.get()); shutdownReconfigurerThread(); - log.info("Stop: Closing servers"); for (ServerProvider server : Container.get().getServerProviderRegistry().allComponents()) { if (startedServers.contains(server)) { @@ -382,24 +391,19 @@ public final class ConfiguredApplication implements Application { } log.info("Stop: Shutting container down"); - CountDownLatch latch = new CountDownLatch(1); - activator.activateContainer(null) - .notifyTermination(() -> { - configurer.shutdown(); - slobrokConfigSubscriber.ifPresent(SlobrokConfigSubscriber::shutdown); - Container.get().shutdown(); - unregisterInSlobrok(); - LogSetup.cleanup(); - log.info("Stop: Finished"); - latch.countDown(); - }); - try { - latch.await(); - } catch (InterruptedException e) { - throw new UncheckedInterruptedException("Failed to wait for container deactivation to complete", e, true); - } + activateContainer(null, () -> { + configurer.shutdown(); + slobrokConfigSubscriber.ifPresent(SlobrokConfigSubscriber::shutdown); + Container.get().shutdown(); + unregisterInSlobrok(); + LogSetup.cleanup(); + log.info("Stop: Finished"); + }); + nonTerminatedContainerTracker.arriveAndAwaitAdvance(); } + // TODO Do more graceful shutdown of reconfigurer thread. The interrupt may leave the container in state where + // graceful shutdown is impossible or may hang. private void shutdownReconfigurerThread() { if (reconfigurerThread == null) return; reconfigurerThread.interrupt(); @@ -418,31 +422,8 @@ public final class ConfiguredApplication implements Application { @Override public void destroy() { - if (shutdownDeadlineExecutor != null) { //stop() is not called when exception happens during start - shutdownDeadlineExecutor.shutdownNow(); - } - } - - static String santizeFileName(String s) { - return s.trim() - .replace('\\', '.') - .replaceAll(SANITIZE_FILENAME, "."); - } - - // Workaround for ApplicationLoader.stop not being able to shutdown - private void startShutdownDeadlineExecutor() { - shutdownDeadlineExecutor = new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("Shutdown deadline timer")); - shutdownDeadlineExecutor.setExecuteExistingDelayedTasksAfterShutdownPolicy(false); - long delayMillis = (long)(shutdownTimeoutS.get() * 1000.0); - shutdownDeadlineExecutor.schedule(() -> { - if (dumpHeapOnShutdownTimeout.get()) { - String heapDumpName = Defaults.getDefaults().underVespaHome("var/crash/java_pid.") + santizeFileName(configId) + "." + ProcessHandle.current().pid() + ".hprof"; - com.yahoo.protect.Process.dumpHeap(heapDumpName, true); - } - com.yahoo.protect.Process.logAndDie( - "Timed out waiting for application shutdown. Please check that all your request handlers " + - "drain their request content channels.", true); - }, delayMillis, TimeUnit.MILLISECONDS); + shutdownDeadline.cancel(); + log.info("Destroy: completed"); } private static void addHandlerBindings(ContainerBuilder builder, diff --git a/container-disc/src/main/java/com/yahoo/container/jdisc/ShutdownDeadline.java b/container-disc/src/main/java/com/yahoo/container/jdisc/ShutdownDeadline.java new file mode 100644 index 00000000000..15af216c210 --- /dev/null +++ b/container-disc/src/main/java/com/yahoo/container/jdisc/ShutdownDeadline.java @@ -0,0 +1,52 @@ +// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +package com.yahoo.container.jdisc; + +import com.yahoo.concurrent.DaemonThreadFactory; +import com.yahoo.vespa.defaults.Defaults; + +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.TimeUnit; + +import static com.yahoo.protect.Process.dumpHeap; +import static com.yahoo.protect.Process.logAndDie; + +/** + * Kills the JVM if the application is unable to shutdown before deadline. + * + * @author bjorncs + */ +class ShutdownDeadline implements AutoCloseable { + + private final String configId; + private final ScheduledThreadPoolExecutor executor; + + ShutdownDeadline(String configId) { + this.configId = configId; + executor = new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("Shutdown deadline timer")); + executor.setExecuteExistingDelayedTasksAfterShutdownPolicy(false); + } + + ShutdownDeadline schedule(long millis, boolean heapdumpOnShutdown) { + executor.schedule(() -> onDeadline(heapdumpOnShutdown), millis, TimeUnit.MILLISECONDS); + return this; + } + + void cancel() { executor.shutdownNow(); } + @Override public void close() { cancel(); } + + private void onDeadline(boolean heapdumpOnShutdown) { + if (heapdumpOnShutdown) dumpHeap(heapdumpFilename(), true); + logAndDie("Timed out waiting for application shutdown. Please check that all your request handlers " + + "drain their request content channels.", true); + } + + private String heapdumpFilename() { + return Defaults.getDefaults().underVespaHome("var/crash/java_pid.") + sanitizeFileName(configId) + "." + + ProcessHandle.current().pid() + ".hprof"; + } + + static String sanitizeFileName(String s) { + return s.trim().replace('\\', '.').replaceAll("[/,;]", "."); + } + +} diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/ConfiguredApplicationTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/ConfiguredApplicationTest.java deleted file mode 100644 index c8cf5c0ce63..00000000000 --- a/container-disc/src/test/java/com/yahoo/container/jdisc/ConfiguredApplicationTest.java +++ /dev/null @@ -1,14 +0,0 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.container.jdisc; - -import org.junit.Test; - -import static org.junit.Assert.assertEquals; - -public class ConfiguredApplicationTest { - @Test - public void testConfigId2FileName() { - assertEquals("admin.metrics.2088223-v6-1.ostk.bm2.prod.ne1.yahoo.com", ConfiguredApplication.santizeFileName("admin/metrics/2088223-v6-1.ostk.bm2.prod.ne1.yahoo.com")); - assertEquals("admin.standalone.cluster-controllers.1", ConfiguredApplication.santizeFileName("admin/standalone/cluster-controllers/1 ")); - } -} diff --git a/container-disc/src/test/java/com/yahoo/container/jdisc/ShutdownDeadlineTest.java b/container-disc/src/test/java/com/yahoo/container/jdisc/ShutdownDeadlineTest.java new file mode 100644 index 00000000000..f2733d61f97 --- /dev/null +++ b/container-disc/src/test/java/com/yahoo/container/jdisc/ShutdownDeadlineTest.java @@ -0,0 +1,18 @@ +package com.yahoo.container.jdisc;// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + + +import org.junit.jupiter.api.Test; + +import static com.yahoo.container.jdisc.ShutdownDeadline.sanitizeFileName; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * @author bjorncs + */ +class ShutdownDeadlineTest { + @Test + void testConfigId2FileName() { + assertEquals("admin.metrics.2088223-v6-1.ostk.bm2.prod.ne1.yahoo.com", sanitizeFileName("admin/metrics/2088223-v6-1.ostk.bm2.prod.ne1.yahoo.com")); + assertEquals("admin.standalone.cluster-controllers.1", sanitizeFileName("admin/standalone/cluster-controllers/1 ")); + } +} |