From: Tom Pantelis Date: Thu, 24 Mar 2016 15:53:39 +0000 (-0400) Subject: Improve config system shutdown X-Git-Tag: release/boron~243 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=c164056dbd60d7df05285ed098b74e85994071d5 Improve config system shutdown On shutdown, the ModuleFactoryBundleTracker is invoked for every removed bundle which executes the "blank" transaction which in turn calls ConfigRegistryImpl#beginConfig. This method is synchronized and may block if a config commit is in progress (which might be waiting on capabilities or a ModuleFactory). The ModuleFactoryBundleTracker is invoked synchronously so this can block karaf shut down. It can affect single feature tests in particular which start and stop karaf quickly. I'm not exactly sure what the blank tx is for - I think its purpose is to indirectly cause the deactivated bundle's module(s) to be destroyed. On shutdown, the ConfigManagerActivator stop method calls ConfigRegistryImpl#close which destroys all the modules so we really don't need the blank tx to run on every bundle on shutdown. On shutdown, karaf first sends a STOPPING event for the system bundle (id 0). So I added the ConfigManagerActivator as a bundle listener and when the system bundle 0 transitions to STOPPING, it calls ConfigRegistryImpl#close. I also added a closed flag in ConfigRegistryImpl which is checked by beginConfig and commitConfig. In order to make this work right, I had to change the synchronization in ConfigRegistryImpl, which really was over-synchronized on "this". I removed synchronized from the beginConfig and close methods to break the locking between the two and I made ConfigHolder and TransactionsHolder classes thread-safe by using ConcurrentHashMap which allows close to access them safely w/o synchronization. If the closed flag is set, beginConfig returns a no-op ObjectName, otherwise it calls the synchronized beginConfigSafe method. In commitConfig, if closed is set or the ON is the no-op ObjectName, it returns an an empty CommitStatus. I also changed from synchronizing "this" to using a Lock to take advantage of the tryLock functionality. As an added measure of safety against prolonged blocking, if beginConfig is called with the blank tx flag set, it tries to acquire the lock for 5 sec. If it fails it returns the no-op ObjectName. After the modules are destroyed by ConfigRegistryImpl#close. the container will proceed to stop the rest of the bundles and the ModuleFactoryBundleTracker will initiate blank transactions but they'll be no-ops and won't block. The ModuleFactoryBundleTracker was also changed to only initiate a blank transaction for bundles that contain a ModuleFactory (via Boolean state stored with the BundleTracker). With these changes, karaf shuts down noticeably faster. The BlankTransactionServiceTracker also initiates a blank transaction when services are added and removed. Since these are synchronous calls we really shouldn't block the OSGi container. So I added a single-threaded executor to process the blank transactions for the service changes. The blank transaction on bundle removed has to be done synchronously as it may still need access to the BundleContext which becomes invalid after the bundle removed event. Change-Id: I5482caa4182dcc64df9b6bafefac9b8f2d505d3e Signed-off-by: Tom Pantelis --- diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java index 45dac318fa..b58d2afd24 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigRegistryImpl.java @@ -18,8 +18,13 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import javax.annotation.concurrent.GuardedBy; -import javax.annotation.concurrent.NotThreadSafe; import javax.annotation.concurrent.ThreadSafe; import javax.management.InstanceAlreadyExistsException; import javax.management.InstanceNotFoundException; @@ -60,22 +65,20 @@ import org.slf4j.LoggerFactory; @ThreadSafe public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBean { private static final Logger LOG = LoggerFactory.getLogger(ConfigRegistryImpl.class); + private static final ObjectName NOOP_TX_NAME = ObjectNameUtil.createTransactionControllerON("noop"); private final ModuleFactoriesResolver resolver; private final MBeanServer configMBeanServer; private final BindingContextProvider bindingContextProvider; - @GuardedBy("this") - private long version = 0; - @GuardedBy("this") - private long versionCounter = 0; + private volatile long version = 0; + private volatile long versionCounter = 0; /** * Contains current configuration in form of {moduleName:{instanceName,read * only module}} for copying state to new transaction. Each running module * is present just once, no matter how many interfaces it exposes. */ - @GuardedBy("this") private final ConfigHolder currentConfig = new ConfigHolder(); /** @@ -83,14 +86,12 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * validation but failed in second phase of commit. In this case the server * is unstable and its state is undefined. */ - @GuardedBy("this") - private boolean isHealthy = true; + private volatile boolean isHealthy = true; /** * Holds Map and purges it each time * its content is requested. */ - @GuardedBy("this") private final TransactionsHolder transactionsHolder = new TransactionsHolder(); private final BaseJMXRegistrator baseJMXRegistrator; @@ -102,12 +103,19 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe // internal jmx server shared by all transactions private final MBeanServer transactionsMBeanServer; + private final AtomicBoolean closed = new AtomicBoolean(); + + private final Object readableSRRegistryLock = new Object(); + + private final Lock configTransactionLock = new ReentrantLock(); + // Used for finding new factory instances for default module functionality - @GuardedBy("this") + @GuardedBy("configTransactionLock") private List lastListOfFactories = Collections.emptyList(); - @GuardedBy("this") // switched in every 2ndPC - private CloseableServiceReferenceReadableRegistry readableSRRegistry = ServiceReferenceRegistryImpl.createInitialSRLookupRegistry(); + @GuardedBy("readableSRRegistryLock") // switched in every 2ndPC + private CloseableServiceReferenceReadableRegistry readableSRRegistry = + ServiceReferenceRegistryImpl.createInitialSRLookupRegistry(); // constructor public ConfigRegistryImpl(ModuleFactoriesResolver resolver, @@ -135,7 +143,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * Create new {@link ConfigTransactionControllerImpl } */ @Override - public synchronized ObjectName beginConfig() { + public ObjectName beginConfig() { return beginConfig(false); } @@ -143,11 +151,46 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * @param blankTransaction true if this transaction is created automatically by * org.opendaylight.controller.config.manager.impl.osgi.BlankTransactionServiceTracker */ - public synchronized ObjectName beginConfig(boolean blankTransaction) { - return beginConfigInternal(blankTransaction).getControllerObjectName(); + public ObjectName beginConfig(boolean blankTransaction) { + // If we're closed or in the process of closing then all modules are destroyed or being destroyed + // so there's no point in trying to acquire the lock and beginning an actual transaction. Also we want + // to avoid trying to lock as it may block the shutdown process if there is an outstanding transaction + // attempting to be committed. + // + // We could throw an exception here to indicate this but that's not part of the original API contract + // and callers may not be expecting an unchecked exception. Therefore we return a special transaction + // handle that signifies a "no-op". + if(closed.get()) { + return NOOP_TX_NAME; + } + + if(blankTransaction) { + try { + // For a "blank" transaction, we'll try to obtain the config lock but "blank" transactions + // are initiated via OSGi events so we don't want to block indefinitely or for a long period + // of time. + if(!configTransactionLock.tryLock(5, TimeUnit.SECONDS)) { + LOG.debug("Timed out trying to obtain configTransactionLock"); + return NOOP_TX_NAME; + } + } catch(InterruptedException e) { + LOG.debug("Interrupted trying to obtain configTransactionLock", e); + Thread.currentThread().interrupt(); + return NOOP_TX_NAME; + } + } else { + configTransactionLock.lock(); + } + + try { + return beginConfigSafe(blankTransaction).getControllerObjectName(); + } finally { + configTransactionLock.unlock(); + } } - private synchronized ConfigTransactionControllerInternal beginConfigInternal(boolean blankTransaction) { + @GuardedBy("configTransactionLock") + private ConfigTransactionControllerInternal beginConfigSafe(boolean blankTransaction) { versionCounter++; final String transactionName = "ConfigTransaction-" + version + "-" + versionCounter; @@ -196,7 +239,22 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * {@inheritDoc} */ @Override - public synchronized CommitStatus commitConfig(ObjectName transactionControllerON) + public CommitStatus commitConfig(ObjectName transactionControllerON) + throws ConflictingVersionException, ValidationException { + if(transactionControllerON == NOOP_TX_NAME || closed.get()) { + return new CommitStatus(Collections.emptyList(), Collections.emptyList(), Collections.emptyList()); + } + + configTransactionLock.lock(); + try { + return commitConfigSafe(transactionControllerON); + } finally { + configTransactionLock.unlock(); + } + } + + @GuardedBy("configTransactionLock") + private CommitStatus commitConfigSafe(ObjectName transactionControllerON) throws ConflictingVersionException, ValidationException { final String transactionName = ObjectNameUtil .getTransactionName(transactionControllerON); @@ -237,6 +295,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe } } + @GuardedBy("configTransactionLock") private CommitStatus secondPhaseCommit(ConfigTransactionControllerInternal configTransactionController, CommitInfo commitInfo, ConfigTransactionLookupRegistry txLookupRegistry) { @@ -395,9 +454,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe version = configTransactionController.getVersion(); // switch readable Service Reference Registry - this.readableSRRegistry.close(); - this.readableSRRegistry = ServiceReferenceRegistryImpl.createSRReadableRegistry( - configTransactionController.getWritableRegistry(), this, baseJMXRegistrator); + synchronized(readableSRRegistryLock) { + readableSRRegistry.close(); + readableSRRegistry = ServiceReferenceRegistryImpl.createSRReadableRegistry( + configTransactionController.getWritableRegistry(), this, baseJMXRegistrator); + } return new CommitStatus(newInstances, reusedInstances, recreatedInstances); @@ -407,7 +468,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * {@inheritDoc} */ @Override - public synchronized List getOpenConfigs() { + public List getOpenConfigs() { Map> transactions = transactionsHolder .getCurrentTransactions(); List result = new ArrayList<>(transactions.size()); @@ -425,7 +486,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe * here. */ @Override - public synchronized void close() { + public void close() { + if(!closed.compareAndSet(false, true)) { + return; + } + // abort transactions Map> transactions = transactionsHolder .getCurrentTransactions(); @@ -437,14 +502,15 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe configTransactionControllerEntry.getValue().close(); configTransactionController.abortConfig(); } catch (RuntimeException e) { - LOG.warn("Ignoring exception while aborting {}", - configTransactionController, e); + LOG.debug("Ignoring exception while aborting {}", configTransactionController, e); } } // destroy all live objects one after another in order of the dependency hierarchy, from top to bottom - List destroyedModules = currentConfig - .getModulesToBeDestroyed(); + List destroyedModules = currentConfig.getModulesToBeDestroyed(); + + LOG.info("ConfigRegistry closing - destroying {} modules", destroyedModules.size()); + for (DestroyedModule destroyedModule : destroyedModules) { destroyedModule.close(); } @@ -454,6 +520,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe MBeanServerFactory.releaseMBeanServer(registryMBeanServer); MBeanServerFactory.releaseMBeanServer(transactionsMBeanServer); + LOG.info("ConfigRegistry closed"); } /** @@ -554,38 +621,52 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe // service reference functionality: @Override - public synchronized ObjectName lookupConfigBeanByServiceInterfaceName(String serviceInterfaceQName, String refName) { - return readableSRRegistry.lookupConfigBeanByServiceInterfaceName(serviceInterfaceQName, refName); + public ObjectName lookupConfigBeanByServiceInterfaceName(String serviceInterfaceQName, String refName) { + synchronized(readableSRRegistryLock) { + return readableSRRegistry.lookupConfigBeanByServiceInterfaceName(serviceInterfaceQName, refName); + } } @Override - public synchronized Map> getServiceMapping() { - return readableSRRegistry.getServiceMapping(); + public Map> getServiceMapping() { + synchronized(readableSRRegistryLock) { + return readableSRRegistry.getServiceMapping(); + } } @Override - public synchronized Map lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) { - return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName); + public Map lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) { + synchronized(readableSRRegistryLock) { + return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName); + } } @Override - public synchronized Set lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException { - return readableSRRegistry.lookupServiceInterfaceNames(objectName); + public Set lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException { + synchronized(readableSRRegistryLock) { + return readableSRRegistry.lookupServiceInterfaceNames(objectName); + } } @Override - public synchronized String getServiceInterfaceName(String namespace, String localName) { - return readableSRRegistry.getServiceInterfaceName(namespace, localName); + public String getServiceInterfaceName(String namespace, String localName) { + synchronized(readableSRRegistryLock) { + return readableSRRegistry.getServiceInterfaceName(namespace, localName); + } } @Override public void checkServiceReferenceExists(ObjectName objectName) throws InstanceNotFoundException { - readableSRRegistry.checkServiceReferenceExists(objectName); + synchronized(readableSRRegistryLock) { + readableSRRegistry.checkServiceReferenceExists(objectName); + } } @Override public ObjectName getServiceReference(String serviceInterfaceQName, String refName) throws InstanceNotFoundException { - return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName); + synchronized(readableSRRegistryLock) { + return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName); + } } @Override @@ -605,9 +686,8 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe /** * Holds currently running modules */ -@NotThreadSafe class ConfigHolder { - private final Map currentConfig = new HashMap<>(); + private final ConcurrentMap currentConfig = new ConcurrentHashMap<>(); /** * Add all modules to the internal map. Also add service instance to OSGi @@ -625,8 +705,7 @@ class ConfigHolder { } private void add(ModuleInternalInfo configInfo) { - ModuleInternalInfo oldValue = currentConfig.put(configInfo.getIdentifier(), - configInfo); + ModuleInternalInfo oldValue = currentConfig.putIfAbsent(configInfo.getIdentifier(), configInfo); if (oldValue != null) { throw new IllegalStateException( "Cannot overwrite module with same name:" @@ -665,7 +744,6 @@ class ConfigHolder { * Holds Map and purges it each time its * content is requested. */ -@NotThreadSafe class TransactionsHolder { /** * This map keeps transaction names and @@ -673,16 +751,12 @@ class TransactionsHolder { * MBeanServer transforms mbeans into another representation. Map is cleaned * every time current transactions are requested. */ - @GuardedBy("ConfigRegistryImpl.this") - private final Map> transactions = new HashMap<>(); + private final ConcurrentMap> transactions = new ConcurrentHashMap<>(); - /** - * Can only be called from within synchronized method. - */ public void add(String transactionName, ConfigTransactionControllerInternal transactionController, ConfigTransactionLookupRegistry txLookupRegistry) { - Object oldValue = transactions.put(transactionName, + Object oldValue = transactions.putIfAbsent(transactionName, Maps.immutableEntry(transactionController, txLookupRegistry)); if (oldValue != null) { throw new IllegalStateException( @@ -691,8 +765,7 @@ class TransactionsHolder { } /** - * Purges closed transactions from transactions map. Can only be called from - * within synchronized method. Calling this method more than once within the + * Purges closed transactions from transactions map. Calling this method more than once within the * method can modify the resulting map that was obtained in previous calls. * * @return current view on transactions map. diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java index b945bf6965..ebf6266540 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/ConfigTransactionControllerImpl.java @@ -437,7 +437,7 @@ class ConfigTransactionControllerImpl implements } @Override - public synchronized void abortConfig() { + public void abortConfig() { transactionStatus.checkNotCommitStarted(); transactionStatus.checkNotAborted(); internalAbort(); diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java index 2e2bf969a9..fa46cae516 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTracker.java @@ -8,6 +8,9 @@ package org.opendaylight.controller.config.manager.impl.osgi; import com.google.common.annotations.VisibleForTesting; +import com.google.common.util.concurrent.ThreadFactoryBuilder; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import javax.management.ObjectName; import org.opendaylight.controller.config.api.ConflictingVersionException; import org.opendaylight.controller.config.api.ValidationException; @@ -30,12 +33,14 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer< public static final int DEFAULT_MAX_ATTEMPTS = 10; private final BlankTransaction blankTransaction; - private int maxAttempts; + private final ExecutorService txExecutor; + private final int maxAttempts; public BlankTransactionServiceTracker(final ConfigRegistryImpl configRegistry) { this(new BlankTransaction() { @Override - public CommitStatus hit() throws ValidationException, ConflictingVersionException { + public CommitStatus hit() + throws ValidationException, ConflictingVersionException { ObjectName tx = configRegistry.beginConfig(true); return configRegistry.commitConfig(tx); } @@ -43,22 +48,29 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer< } public BlankTransactionServiceTracker(final BlankTransaction blankTransaction) { - this(blankTransaction, DEFAULT_MAX_ATTEMPTS); + this(blankTransaction, DEFAULT_MAX_ATTEMPTS, Executors.newSingleThreadExecutor(new ThreadFactoryBuilder() + .setNameFormat("config-blank-txn-%d").build())); } @VisibleForTesting - BlankTransactionServiceTracker(final BlankTransaction blankTx, final int maxAttempts) { + BlankTransactionServiceTracker(final BlankTransaction blankTx, final int maxAttempts, + final ExecutorService txExecutor) { this.blankTransaction = blankTx; this.maxAttempts = maxAttempts; + this.txExecutor = txExecutor; } @Override public Object addingService(ServiceReference moduleFactoryServiceReference) { - blankTransaction(); + blankTransactionAsync(); return null; } - synchronized void blankTransaction() { + private void blankTransactionAsync() { + txExecutor.execute(() -> { blankTransactionSync(); }); + } + + void blankTransactionSync() { // race condition check: config-persister might push new configuration while server is starting up. ConflictingVersionException lastException = null; for (int i = 0; i < maxAttempts; i++) { @@ -73,25 +85,26 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer< Thread.sleep(1000); } catch (InterruptedException interruptedException) { Thread.currentThread().interrupt(); - throw new IllegalStateException(interruptedException); + LOG.debug("blankTransactionSync was interrupted"); + return; } } catch (ValidationException e) { LOG.error("Validation exception while running blank transaction indicates programming error", e); - throw new RuntimeException("Validation exception while running blank transaction indicates programming error", e); } } - throw new RuntimeException("Maximal number of attempts reached and still cannot get optimistic lock from " + - "config manager",lastException); + + LOG.error("Maximal number of attempts reached and still cannot get optimistic lock from config manager", + lastException); } @Override public void modifiedService(ServiceReference moduleFactoryServiceReference, Object o) { - blankTransaction(); + blankTransactionAsync(); } @Override public void removedService(ServiceReference moduleFactoryServiceReference, Object o) { - blankTransaction(); + blankTransactionAsync(); } @VisibleForTesting diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java index 50ab13eb68..38aeb9a792 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java @@ -31,19 +31,25 @@ import org.opendaylight.yangtools.yang.binding.YangModuleInfo; import org.osgi.framework.Bundle; import org.osgi.framework.BundleActivator; import org.osgi.framework.BundleContext; +import org.osgi.framework.BundleEvent; +import org.osgi.framework.SynchronousBundleListener; import org.osgi.util.tracker.BundleTracker; import org.osgi.util.tracker.ServiceTracker; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class ConfigManagerActivator implements BundleActivator { +public class ConfigManagerActivator implements BundleActivator, SynchronousBundleListener { private static final Logger LOG = LoggerFactory.getLogger(ConfigManagerActivator.class); + private static final long SYSTEM_BUNDLE_ID = 0; + private final MBeanServer configMBeanServer = ManagementFactory.getPlatformMBeanServer(); private AutoCloseable autoCloseable; + private ConfigRegistryImpl configRegistry; + @Override public void start(final BundleContext context) { try { @@ -59,7 +65,7 @@ public class ConfigManagerActivator implements BundleActivator { // start config registry BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver( context); - ConfigRegistryImpl configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer, + configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer, bindingContextProvider); // track bundles containing factories @@ -117,10 +123,12 @@ public class ConfigManagerActivator implements BundleActivator { blankTransactionServiceTracker); serviceTracker.open(); - List list = Arrays.asList(bindingContextProvider, clsReg, configRegistry, + List list = Arrays.asList(bindingContextProvider, clsReg, wrap(moduleFactoryBundleTracker), moduleInfoBundleTracker, configRegReg, configRegistryJMXRegistrator, configRegistryJMXRegistratorWithNotifications, wrap(serviceTracker), moduleInfoRegistryWrapper, notifyingConfigRegistry); autoCloseable = OsgiRegistrationUtil.aggregate(list); + + context.addBundleListener(this); } catch(Exception e) { LOG.warn("Error starting config manager", e); } catch(Error e) { @@ -133,6 +141,20 @@ public class ConfigManagerActivator implements BundleActivator { @Override public void stop(final BundleContext context) throws Exception { + context.removeBundleListener(this); autoCloseable.close(); } + + @Override + public void bundleChanged(BundleEvent event) { + if(configRegistry == null) { + return; + } + + // If the system bundle (id 0) is stopping close the ConfigRegistry so it destroys all modules. On + // shutdown the system bundle is stopped first. + if(event.getBundle().getBundleId() == SYSTEM_BUNDLE_ID && event.getType() == BundleEvent.STOPPING) { + configRegistry.close(); + } + } } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java index cd72a73ecf..15e6255215 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ModuleFactoryBundleTracker.java @@ -30,7 +30,7 @@ import org.slf4j.LoggerFactory; * the services are unregistered automatically. * Code based on http://www.toedter.com/blog/?p=236 */ -public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer { +public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer { private final BlankTransactionServiceTracker blankTransactionServiceTracker; private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class); @@ -39,7 +39,7 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizeremptyList(), Collections.emptyList(), Collections.emptyList())).when(blankTx).hit(); - tracker = new BlankTransactionServiceTracker(blankTx); + doReturn(new CommitStatus(Collections.emptyList(), Collections.emptyList(), + Collections.emptyList())).when(blankTx).hit(); + tracker = new BlankTransactionServiceTracker(blankTx, 10, MoreExecutors.newDirectExecutorService()); } @Test @@ -56,33 +53,21 @@ public class BlankTransactionServiceTrackerTest { IllegalArgumentException argumentException = new IllegalArgumentException(); ValidationException validationException = ValidationException.createForSingleException(new ModuleIdentifier("m", "i"), argumentException); doThrow(validationException).when(blankTx).hit(); - try { - tracker.addingService(getMockServiceReference()); - } catch (Exception e) { - verify(blankTx, times(1)).hit(); - assertNotNull(e.getCause()); - assertSame(validationException, e.getCause()); - return; - } - fail("Exception should have occurred for validation exception"); + tracker.addingService(getMockServiceReference()); + verify(blankTx, times(10)).hit(); } @Test public void testConflictingException() throws Exception { int maxAttempts = 2; - tracker = new BlankTransactionServiceTracker(blankTx, maxAttempts); + tracker = new BlankTransactionServiceTracker(blankTx, maxAttempts, MoreExecutors.newDirectExecutorService()); final ConflictingVersionException ex = new ConflictingVersionException(); doThrow(ex).when(blankTx).hit(); - try { - tracker.addingService(getMockServiceReference()); - } catch (Exception e) { - verify(blankTx, times(maxAttempts)).hit(); - return; - } - fail("Exception should have occurred for conflicting exception"); + tracker.addingService(getMockServiceReference()); + verify(blankTx, times(maxAttempts)).hit(); } private ServiceReference getMockServiceReference() { diff --git a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java index d5df5349a6..66c43c2650 100644 --- a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java +++ b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/osgi/ConfigPersisterActivator.java @@ -133,7 +133,7 @@ public class ConfigPersisterActivator implements BundleActivator { LOG.warn("Unable to process configs as BundleContext is null"); } } catch (InterruptedException e) { - LOG.info("ConfigPusher thread stopped",e); + LOG.info("ConfigPusher thread stopped"); } LOG.info("Configuration Persister initialization completed."); }