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 <tpanteli@brocade.com>

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 45dac318fa06caa539b2daeb52f0e3c8bb31cfba..b58d2afd24b095dbecb0e2a3681cd216a49758ce 100644 (file)
--- 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.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.GuardedBy;

-import javax.annotation.concurrent.NotThreadSafe;

 import javax.annotation.concurrent.ThreadSafe;
 import javax.management.InstanceAlreadyExistsException;
 import javax.management.InstanceNotFoundException;
 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);
 @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;
 
 
     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.
      */
 
     /**
      * 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();
 
     /**
     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.
      */
      * 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<transactionName, transactionController> and purges it each time
      * its content is requested.
      */
 
     /**
      * Holds Map<transactionName, transactionController> and purges it each time
      * its content is requested.
      */

-    @GuardedBy("this")

     private final TransactionsHolder transactionsHolder = new TransactionsHolder();
 
     private final BaseJMXRegistrator baseJMXRegistrator;
     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;
 
     // 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
     // Used for finding new factory instances for default module functionality

-    @GuardedBy("this")
+    @GuardedBy("configTransactionLock")

     private List<ModuleFactory> lastListOfFactories = Collections.emptyList();
 
     private List<ModuleFactory> 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,
 
     // constructor
     public ConfigRegistryImpl(ModuleFactoriesResolver resolver,


@@ -135,7 +143,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * Create new {@link ConfigTransactionControllerImpl }
      */
     @Override
      * Create new {@link ConfigTransactionControllerImpl }
      */
     @Override

-    public synchronized ObjectName beginConfig() {
+    public ObjectName beginConfig() {

         return beginConfig(false);
     }
 
         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
      */
      * @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;
 
         versionCounter++;
         final String transactionName = "ConfigTransaction-" + version + "-" + versionCounter;
 


@@ -196,7 +239,22 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * {@inheritDoc}
      */
     @Override
      * {@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);
             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) {
 
     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
         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);
 
         return new CommitStatus(newInstances, reusedInstances,
                 recreatedInstances);


@@ -407,7 +468,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * {@inheritDoc}
      */
     @Override
      * {@inheritDoc}
      */
     @Override

-    public synchronized List<ObjectName> getOpenConfigs() {
+    public List<ObjectName> getOpenConfigs() {

         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();
         List<ObjectName> result = new ArrayList<>(transactions.size());
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();
         List<ObjectName> result = new ArrayList<>(transactions.size());


@@ -425,7 +486,11 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
      * here.
      */
     @Override
      * here.
      */
     @Override

-    public synchronized void close() {
+    public void close() {
+        if(!closed.compareAndSet(false, true)) {
+            return;
+        }
+

         // abort transactions
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();
         // abort transactions
         Map<String, Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = transactionsHolder
                 .getCurrentTransactions();


@@ -437,14 +502,15 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
                 configTransactionControllerEntry.getValue().close();
                 configTransactionController.abortConfig();
             } catch (RuntimeException e) {
                 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
             }
         }
 
         // destroy all live objects one after another in order of the dependency hierarchy, from top to bottom

-        List<DestroyedModule> destroyedModules = currentConfig
-                .getModulesToBeDestroyed();
+        List<DestroyedModule> destroyedModules = currentConfig.getModulesToBeDestroyed();
+
+        LOG.info("ConfigRegistry closing - destroying {} modules", destroyedModules.size());
+

         for (DestroyedModule destroyedModule : destroyedModules) {
             destroyedModule.close();
         }
         for (DestroyedModule destroyedModule : destroyedModules) {
             destroyedModule.close();
         }


@@ -454,6 +520,7 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
         MBeanServerFactory.releaseMBeanServer(registryMBeanServer);
         MBeanServerFactory.releaseMBeanServer(transactionsMBeanServer);
 
         MBeanServerFactory.releaseMBeanServer(registryMBeanServer);
         MBeanServerFactory.releaseMBeanServer(transactionsMBeanServer);
 

+        LOG.info("ConfigRegistry closed");

     }
 
     /**
     }
 
     /**


@@ -554,38 +621,52 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 
     // service reference functionality:
     @Override
 
     // 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
     }
 
     @Override

-    public synchronized Map<String, Map<String, ObjectName>> getServiceMapping() {
-        return readableSRRegistry.getServiceMapping();
+    public Map<String, Map<String, ObjectName>> getServiceMapping() {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.getServiceMapping();
+        }

     }
 
     @Override
     }
 
     @Override

-    public synchronized Map<String, ObjectName> lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) {
-        return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName);
+    public Map<String, ObjectName> lookupServiceReferencesByServiceInterfaceName(String serviceInterfaceQName) {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.lookupServiceReferencesByServiceInterfaceName(serviceInterfaceQName);
+        }

     }
 
     @Override
     }
 
     @Override

-    public synchronized Set<String> lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException {
-        return readableSRRegistry.lookupServiceInterfaceNames(objectName);
+    public Set<String> lookupServiceInterfaceNames(ObjectName objectName) throws InstanceNotFoundException {
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.lookupServiceInterfaceNames(objectName);
+        }

     }
 
     @Override
     }
 
     @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 {
     }
 
     @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 {
     }
 
     @Override
     public ObjectName getServiceReference(String serviceInterfaceQName, String refName) throws InstanceNotFoundException {

-        return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName);
+        synchronized(readableSRRegistryLock) {
+            return readableSRRegistry.getServiceReference(serviceInterfaceQName, refName);
+        }

     }
 
     @Override
     }
 
     @Override


@@ -605,9 +686,8 @@ public class ConfigRegistryImpl implements AutoCloseable, ConfigRegistryImplMXBe
 /**
  * Holds currently running modules
  */
 /**
  * Holds currently running modules
  */

-@NotThreadSafe

 class ConfigHolder {
 class ConfigHolder {

-    private final Map<ModuleIdentifier, ModuleInternalInfo> currentConfig = new HashMap<>();
+    private final ConcurrentMap<ModuleIdentifier, ModuleInternalInfo> currentConfig = new ConcurrentHashMap<>();

 
     /**
      * Add all modules to the internal map. Also add service instance to OSGi
 
     /**
      * Add all modules to the internal map. Also add service instance to OSGi


@@ -625,8 +705,7 @@ class ConfigHolder {
     }
 
     private void add(ModuleInternalInfo configInfo) {
     }
 
     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:"
         if (oldValue != null) {
             throw new IllegalStateException(
                     "Cannot overwrite module with same name:"


@@ -665,7 +744,6 @@ class ConfigHolder {
  * Holds Map<transactionName, transactionController> and purges it each time its
  * content is requested.
  */
  * Holds Map<transactionName, transactionController> and purges it each time its
  * content is requested.
  */

-@NotThreadSafe

 class TransactionsHolder {
     /**
      * This map keeps transaction names and
 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.
      */
      * MBeanServer transforms mbeans into another representation. Map is cleaned
      * every time current transactions are requested.
      */

-    @GuardedBy("ConfigRegistryImpl.this")
-    private final Map<String /* transactionName */,
-            Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = new HashMap<>();
+    private final ConcurrentMap<String /* transactionName */,
+            Entry<ConfigTransactionControllerInternal, ConfigTransactionLookupRegistry>> transactions = new ConcurrentHashMap<>();

 
 

-    /**
-     * Can only be called from within synchronized method.
-     */

     public void add(String transactionName,
                     ConfigTransactionControllerInternal transactionController, ConfigTransactionLookupRegistry txLookupRegistry) {
     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(
                 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.
      * 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 b945bf6965d67acf33d17795b7c558debcc3971d..ebf62665406346597baa5cfa4410cb68ef65375f 100644 (file)
--- 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
     }
 
     @Override

-    public synchronized void abortConfig() {
+    public void abortConfig() {

         transactionStatus.checkNotCommitStarted();
         transactionStatus.checkNotAborted();
         internalAbort();
         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 2e2bf969a93203c3beb2f7a126e2f3f04b57bb17..fa46cae5167925c4bf7e459d21a7dc150a1ff92f 100644 (file)
--- 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;
 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;
 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;
     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 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);
             }
                 ObjectName tx = configRegistry.beginConfig(true);
                 return configRegistry.commitConfig(tx);
             }


@@ -43,22 +48,29 @@ public class BlankTransactionServiceTracker implements ServiceTrackerCustomizer<
     }
 
     public BlankTransactionServiceTracker(final BlankTransaction blankTransaction) {
     }
 
     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
     }
 
     @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.blankTransaction = blankTx;
         this.maxAttempts = maxAttempts;

+        this.txExecutor = txExecutor;

     }
 
     @Override
     public Object addingService(ServiceReference<ModuleFactory> moduleFactoryServiceReference) {
     }
 
     @Override
     public Object addingService(ServiceReference<ModuleFactory> moduleFactoryServiceReference) {

-        blankTransaction();
+        blankTransactionAsync();

         return null;
     }
 
         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++) {
         // 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();
                     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);
                 }
             } 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 <ModuleFactory> moduleFactoryServiceReference, Object o) {
     }
 
     @Override
     public void modifiedService(ServiceReference <ModuleFactory> moduleFactoryServiceReference, Object o) {

-        blankTransaction();
+        blankTransactionAsync();

     }
 
     @Override
     public void removedService(ServiceReference<ModuleFactory> moduleFactoryServiceReference, Object o) {
     }
 
     @Override
     public void removedService(ServiceReference<ModuleFactory> moduleFactoryServiceReference, Object o) {

-        blankTransaction();
+        blankTransactionAsync();

     }
 
     @VisibleForTesting
     }
 
     @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 50ab13eb68d4fa4c2bee1cde8f1f685322c5f3e4..38aeb9a792efe18a2fec6a5398cd8e461f637213 100644 (file)
--- 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.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;
 
 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 Logger LOG = LoggerFactory.getLogger(ConfigManagerActivator.class);
 

+    private static final long SYSTEM_BUNDLE_ID = 0;
+

     private final MBeanServer configMBeanServer = ManagementFactory.getPlatformMBeanServer();
 
     private AutoCloseable autoCloseable;
 
     private final MBeanServer configMBeanServer = ManagementFactory.getPlatformMBeanServer();
 
     private AutoCloseable autoCloseable;
 

+    private ConfigRegistryImpl configRegistry;
+

     @Override
     public void start(final BundleContext context) {
         try {
     @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);
             // start config registry
             BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver(
                     context);

-            ConfigRegistryImpl configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer,
+            configRegistry = new ConfigRegistryImpl(bundleContextBackedModuleFactoriesResolver, configMBeanServer,

                     bindingContextProvider);
 
             // track bundles containing factories
                     bindingContextProvider);
 
             // track bundles containing factories


@@ -117,10 +123,12 @@ public class ConfigManagerActivator implements BundleActivator {
                     blankTransactionServiceTracker);
             serviceTracker.open();
 
                     blankTransactionServiceTracker);
             serviceTracker.open();
 

-            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg, configRegistry,
+            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg,

                     wrap(moduleFactoryBundleTracker), moduleInfoBundleTracker,
                     configRegReg, configRegistryJMXRegistrator, configRegistryJMXRegistratorWithNotifications, wrap(serviceTracker), moduleInfoRegistryWrapper, notifyingConfigRegistry);
             autoCloseable = OsgiRegistrationUtil.aggregate(list);
                     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) {
         } 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 {
 
     @Override
     public void stop(final BundleContext context) throws Exception {

+        context.removeBundleListener(this);

         autoCloseable.close();
     }
         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 cd72a73ecf3455365c35ae1a74b71d62654f9763..15e62552152cb341e1467a6d14e2a25c168b9a9e 100644 (file)
--- 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
  */
  * the services are unregistered automatically.
  * Code based on http://www.toedter.com/blog/?p=236
  */

-public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Object> {
+public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Boolean> {

     private final BlankTransactionServiceTracker blankTransactionServiceTracker;
     private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class);
 
     private final BlankTransactionServiceTracker blankTransactionServiceTracker;
     private static final Logger LOG = LoggerFactory.getLogger(ModuleFactoryBundleTracker.class);
 


@@ -39,7 +39,7 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Objec
     }
 
     @Override
     }
 
     @Override

-    public Object addingBundle(Bundle bundle, BundleEvent event) {
+    public Boolean addingBundle(Bundle bundle, BundleEvent event) {

         URL resource = bundle.getEntry("META-INF/services/" + ModuleFactory.class.getName());
         LOG.trace("Got addingBundle event of bundle {}, resource {}, event {}",
                 bundle, resource, event);
         URL resource = bundle.getEntry("META-INF/services/" + ModuleFactory.class.getName());
         LOG.trace("Got addingBundle event of bundle {}, resource {}, event {}",
                 bundle, resource, event);


@@ -48,23 +48,28 @@ public class ModuleFactoryBundleTracker implements BundleTrackerCustomizer<Objec
                 for (String factoryClassName : Resources.readLines(resource, Charsets.UTF_8)) {
                     registerFactory(factoryClassName, bundle);
                 }
                 for (String factoryClassName : Resources.readLines(resource, Charsets.UTF_8)) {
                     registerFactory(factoryClassName, bundle);
                 }

+
+                return Boolean.TRUE;

             } catch (IOException e) {
                 LOG.error("Error while reading {}", resource, e);
                 throw new RuntimeException(e);
             }
         }
             } catch (IOException e) {
                 LOG.error("Error while reading {}", resource, e);
                 throw new RuntimeException(e);
             }
         }

-        return bundle;
+
+        return Boolean.FALSE;

     }
 
     @Override
     }
 
     @Override

-    public void modifiedBundle(Bundle bundle, BundleEvent event, Object object) {
+    public void modifiedBundle(Bundle bundle, BundleEvent event, Boolean hasFactory) {

         // NOOP
     }
 
     @Override
         // NOOP
     }
 
     @Override

-    public void removedBundle(Bundle bundle, BundleEvent event, Object object) {
-        // workaround for service tracker not getting removed service event
-        blankTransactionServiceTracker.blankTransaction();
+    public void removedBundle(Bundle bundle, BundleEvent event, Boolean hasFactory) {
+        if(hasFactory) {
+            // workaround for service tracker not getting removed service event
+            blankTransactionServiceTracker.blankTransactionSync();
+        }

     }
 
     @VisibleForTesting
     }
 
     @VisibleForTesting

diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTrackerTest.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTrackerTest.java
index 4d342a60ade2d1259fab1363a55dba431ec200cf..47b6d100c975de02bd5583e5f6b47aa99556bcad 100644 (file)
--- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTrackerTest.java
+++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/BlankTransactionServiceTrackerTest.java
@@ -8,15 +8,12 @@
 
 package org.opendaylight.controller.config.manager.impl.osgi;
 
 
 package org.opendaylight.controller.config.manager.impl.osgi;
 

-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.fail;

 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;

-
+import com.google.common.util.concurrent.MoreExecutors;

 import java.util.Collections;
 import javax.management.ObjectName;
 import org.junit.Before;
 import java.util.Collections;
 import javax.management.ObjectName;
 import org.junit.Before;


@@ -31,7 +28,6 @@ import org.opendaylight.controller.config.spi.ModuleFactory;
 import org.osgi.framework.ServiceReference;
 
 public class BlankTransactionServiceTrackerTest {
 import org.osgi.framework.ServiceReference;
 
 public class BlankTransactionServiceTrackerTest {

-

     @Mock
     private BlankTransactionServiceTracker.BlankTransaction blankTx;
     private BlankTransactionServiceTracker tracker;
     @Mock
     private BlankTransactionServiceTracker.BlankTransaction blankTx;
     private BlankTransactionServiceTracker tracker;


@@ -39,8 +35,9 @@ public class BlankTransactionServiceTrackerTest {
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);
     @Before
     public void setUp() throws Exception {
         MockitoAnnotations.initMocks(this);

-        doReturn(new CommitStatus(Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList())).when(blankTx).hit();
-        tracker = new BlankTransactionServiceTracker(blankTx);
+        doReturn(new CommitStatus(Collections.<ObjectName>emptyList(), Collections.<ObjectName>emptyList(),
+                Collections.<ObjectName>emptyList())).when(blankTx).hit();
+        tracker = new BlankTransactionServiceTracker(blankTx, 10, MoreExecutors.newDirectExecutorService());

     }
 
     @Test
     }
 
     @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();
         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;
     }
 
     @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();
 
         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<ModuleFactory> getMockServiceReference() {
     }
 
     private ServiceReference<ModuleFactory> 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 d5df5349a64e82f349c740649359d39fb77e1c25..66c43c2650e53a6e5bcf771946ec708d1c031588 100644 (file)
--- 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.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.");
                 }
                     }
                     LOG.info("Configuration Persister initialization completed.");
                 }