From 4db37f2ab0c8174d1990e2262cb466fe22352573 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Alexis=20de=20Talhou=C3=ABt?= Date: Tue, 15 Nov 2016 13:55:33 -0500 Subject: [PATCH] Bug 6969 - Memory leak during bundle tree restart MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit There are two root causes for that memory leak: - Retention on BlueprintContainer when desotrying them before "quiescing" them: Restarting a set of bundles requires cooperation between them to avoid having retention on services due to on-going call, processing. As OSGi APIs don't provide such functionality, the Quiesce APIs where created, ensuring all calls to be finished before actually shutting down the bundle. Once the set of bundles are quiesced, e.g. shut down, they can safely be destroyed. - Retention on the TopoProcessingProviderImpl due to unreleased resources: https://git.opendaylight.org/gerrit/#/c/48283/ Change-Id: I0af4a27ac7c87f1cc158313f0497158733a045a5 Signed-off-by: Alexis de Talhouët --- .../blueprint/BlueprintBundleTracker.java | 47 ++++++++-- .../BlueprintContainerRestartServiceImpl.java | 92 ++++++++++++++++--- 2 files changed, 118 insertions(+), 21 deletions(-) diff --git a/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintBundleTracker.java b/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintBundleTracker.java index 6c267ac5fe..404d78ac83 100644 --- a/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintBundleTracker.java +++ b/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintBundleTracker.java @@ -18,6 +18,7 @@ import java.util.Hashtable; import java.util.List; import org.apache.aries.blueprint.NamespaceHandler; import org.apache.aries.blueprint.services.BlueprintExtenderService; +import org.apache.aries.quiesce.participant.QuiesceParticipant; import org.apache.aries.util.AriesFrameworkUtil; import org.opendaylight.controller.blueprint.ext.OpendaylightNamespaceHandler; import org.opendaylight.controller.config.api.ConfigSystemService; @@ -54,10 +55,12 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus private static final String BLUEPRINT_FLE_PATTERN = "*.xml"; private static final long SYSTEM_BUNDLE_ID = 0; - private ServiceTracker serviceTracker; + private ServiceTracker blueprintExtenderServiceTracker; + private ServiceTracker quiesceParticipantTracker; private BundleTracker bundleTracker; private BundleContext bundleContext; private volatile BlueprintExtenderService blueprintExtenderService; + private volatile QuiesceParticipant quiesceParticipant; private volatile ServiceRegistration blueprintContainerRestartReg; private volatile BlueprintContainerRestartServiceImpl restartService; private volatile boolean shuttingDown; @@ -71,6 +74,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus public void start(BundleContext context) { LOG.info("Starting {}", getClass().getSimpleName()); + restartService = new BlueprintContainerRestartServiceImpl(); + bundleContext = context; registerBlueprintEventHandler(context); @@ -79,7 +84,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus bundleTracker = new BundleTracker<>(context, Bundle.ACTIVE, this); - serviceTracker = new ServiceTracker<>(context, BlueprintExtenderService.class.getName(), + blueprintExtenderServiceTracker = new ServiceTracker<>(context, BlueprintExtenderService.class.getName(), new ServiceTrackerCustomizer() { @Override public BlueprintExtenderService addingService( @@ -91,7 +96,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus LOG.debug("Got BlueprintExtenderService"); - restartService = new BlueprintContainerRestartServiceImpl(blueprintExtenderService); + restartService.setBlueprintExtenderService(blueprintExtenderService); + blueprintContainerRestartReg = context.registerService( BlueprintContainerRestartService.class.getName(), restartService, new Hashtable<>()); @@ -108,7 +114,33 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus BlueprintExtenderService service) { } }); - serviceTracker.open(); + blueprintExtenderServiceTracker.open(); + + quiesceParticipantTracker = new ServiceTracker<>(context, QuiesceParticipant.class.getName(), + new ServiceTrackerCustomizer() { + @Override + public QuiesceParticipant addingService( + ServiceReference reference) { + quiesceParticipant = reference.getBundle().getBundleContext().getService(reference); + + LOG.debug("Got QuiesceParticipant"); + + restartService.setQuiesceParticipant(quiesceParticipant); + + return quiesceParticipant; + } + + @Override + public void modifiedService(ServiceReference reference, + QuiesceParticipant service) { + } + + @Override + public void removedService(ServiceReference reference, + QuiesceParticipant service) { + } + }); + quiesceParticipantTracker.open(); } private void registerNamespaceHandler(BundleContext context) { @@ -131,7 +163,8 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus @Override public void stop(BundleContext context) { bundleTracker.close(); - serviceTracker.close(); + blueprintExtenderServiceTracker.close(); + quiesceParticipantTracker.close(); AriesFrameworkUtil.safeUnregisterService(eventHandlerReg); AriesFrameworkUtil.safeUnregisterService(namespaceReg); @@ -160,7 +193,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus } /** - * Implemented from BundleActivator. + * Implemented from BundleTrackerCustomizer. */ @Override public void modifiedBundle(Bundle bundle, BundleEvent event, Bundle object) { @@ -180,7 +213,7 @@ public class BlueprintBundleTracker implements BundleActivator, BundleTrackerCus } /** - * Implemented from BundleActivator. + * Implemented from BundleTrackerCustomizer. */ @Override public void removedBundle(Bundle bundle, BundleEvent event, Bundle object) { diff --git a/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintContainerRestartServiceImpl.java b/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintContainerRestartServiceImpl.java index 6a464b75d0..e58c956b5a 100644 --- a/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintContainerRestartServiceImpl.java +++ b/opendaylight/blueprint/src/main/java/org/opendaylight/controller/blueprint/BlueprintContainerRestartServiceImpl.java @@ -8,11 +8,16 @@ package org.opendaylight.controller.blueprint; import com.google.common.base.Optional; +import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import com.google.common.util.concurrent.ThreadFactoryBuilder; import java.lang.management.ManagementFactory; import java.util.AbstractMap.SimpleEntry; +import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Deque; import java.util.Dictionary; import java.util.Hashtable; import java.util.LinkedHashSet; @@ -28,6 +33,7 @@ import javax.management.InstanceNotFoundException; import javax.management.ObjectName; import javax.xml.parsers.ParserConfigurationException; import org.apache.aries.blueprint.services.BlueprintExtenderService; +import org.apache.aries.quiesce.participant.QuiesceParticipant; import org.apache.aries.util.AriesFrameworkUtil; import org.opendaylight.controller.config.api.ConfigRegistry; import org.opendaylight.controller.config.api.ConflictingVersionException; @@ -68,12 +74,18 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo private final ExecutorService restartExecutor = Executors.newSingleThreadExecutor(new ThreadFactoryBuilder() .setDaemon(true).setNameFormat("BlueprintContainerRestartService").build()); - private final BlueprintExtenderService blueprintExtenderService; - BlueprintContainerRestartServiceImpl(BlueprintExtenderService blueprintExtenderService) { + private BlueprintExtenderService blueprintExtenderService; + private QuiesceParticipant quiesceParticipant; + + void setBlueprintExtenderService(final BlueprintExtenderService blueprintExtenderService) { this.blueprintExtenderService = blueprintExtenderService; } + void setQuiesceParticipant(final QuiesceParticipant quiesceParticipant) { + this.quiesceParticipant = quiesceParticipant; + } + public void restartContainer(final Bundle bundle, final List paths) { if (restartExecutor.isShutdown()) { return; @@ -99,6 +111,9 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo } private void restartContainerAndDependentsInternal(Bundle forBundle) { + Preconditions.checkNotNull(blueprintExtenderService); + Preconditions.checkNotNull(quiesceParticipant); + // We use a LinkedHashSet to preserve insertion order as we walk the service usage hierarchy. Set containerBundlesSet = new LinkedHashSet<>(); List> configModules = new ArrayList<>(); @@ -109,11 +124,6 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo LOG.info("Restarting blueprint containers for bundle {} and its dependent bundles {}", forBundle, containerBundles.subList(1, containerBundles.size())); - // Destroy the containers in reverse order with 'forBundle' last, ie bottom-up in the service tree. - for (Bundle bundle: Lists.reverse(containerBundles)) { - blueprintExtenderService.destroyContainer(bundle, blueprintExtenderService.getContainer(bundle)); - } - // The blueprint containers are created asynchronously so we register a handler for blueprint events // that are sent when a container is complete, successful or not. The CountDownLatch tells when all // containers are complete. This is done to ensure all blueprint containers are finished before we @@ -126,14 +136,11 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo } }); - // Restart the containers top-down starting with 'forBundle'. - for (Bundle bundle: containerBundles) { - List paths = BlueprintBundleTracker.findBlueprintPaths(bundle); + final Runnable createContainerCallback = () -> createContainers(containerBundles); - LOG.info("Restarting blueprint container for bundle {} with paths {}", bundle, paths); + // Destroy the container down-top recursively and once done, restart the container top-down + destroyContainers(new ArrayDeque<>(Lists.reverse(containerBundles)), createContainerCallback); - blueprintExtenderService.createContainer(bundle, paths); - } try { containerCreationComplete.await(5, TimeUnit.MINUTES); @@ -148,6 +155,62 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo restartConfigModules(forBundle.getBundleContext(), configModules); } + /** + * Recursively quiesce and destroy the bundles one by one in order to maintain synchronicity and ordering. + * @param remainingBundlesToDestroy the list of remaining bundles to destroy. + * @param createContainerCallback a {@link Runnable} to {@code run()} when the recursive function is completed. + */ + private void destroyContainers(final Deque remainingBundlesToDestroy, + final Runnable createContainerCallback) { + + final Bundle nextBundle; + synchronized (remainingBundlesToDestroy) { + if (remainingBundlesToDestroy.isEmpty()) { + LOG.debug("All blueprint containers were quiesced and destroyed"); + createContainerCallback.run(); + return; + } + + nextBundle = remainingBundlesToDestroy.poll(); + } + + // The Quiesce capability is a like a soft-stop, clean-stop. In the case of the Blueprint extender, in flight + // service calls are allowed to finish; they're counted in and counted out, and no new calls are allowed. When + // there are no in flight service calls, the bundle is told to stop. The Blueprint bundle itself doesn't know + // this is happening which is a key design point. In the case of Blueprint, the extender ensures no new Entity + // Managers(EMs) are created. Then when all those EMs are closed the quiesce operation reports that it is + // finished. + // To properly restart the blueprint containers, first we have to quiesce the list of bundles, and once done, it + // is safe to destroy their BlueprintContainer, so no reference is retained. + // + // Mail - thread explaining Quiesce API: + // https://www.mail-archive.com/dev@aries.apache.org/msg08403.html + + // Quiesced the bundle to unregister the associated BlueprintContainer + quiesceParticipant.quiesce(bundlesQuiesced -> { + + // Destroy the container once Quiesced + Arrays.stream(bundlesQuiesced).forEach(quiescedBundle -> { + LOG.debug("Quiesced bundle {}", quiescedBundle); + blueprintExtenderService.destroyContainer( + quiescedBundle, blueprintExtenderService.getContainer(quiescedBundle)); + }); + + destroyContainers(remainingBundlesToDestroy, createContainerCallback); + + }, Collections.singletonList(nextBundle)); + } + + private void createContainers(List containerBundles) { + containerBundles.forEach(bundle -> { + List paths = BlueprintBundleTracker.findBlueprintPaths(bundle); + + LOG.info("Restarting blueprint container for bundle {} with paths {}", bundle, paths); + + blueprintExtenderService.createContainer(bundle, paths); + }); + } + private void restartConfigModules(BundleContext bundleContext, List> configModules) { if (configModules.isEmpty()) { @@ -193,7 +256,7 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo Config configMapping = configFacade.getConfigMapping(); ConfigRegistry configRegistryClient = new ConfigRegistryJMXClient(ManagementFactory.getPlatformMBeanServer()); - for (Entry entry: configModules) { + for (Entry entry : configModules) { String moduleNamespace = entry.getKey(); ModuleIdentifier moduleId = entry.getValue(); try { @@ -229,6 +292,7 @@ class BlueprintContainerRestartServiceImpl implements AutoCloseable, BlueprintCo * * @param bundle the bundle to traverse * @param containerBundles the current set of bundles containing blueprint containers + * @param configModules the current set of bundles containing config modules */ private void findDependentContainersRecursively(Bundle bundle, Set containerBundles, List> configModules) { -- 2.36.6