BUG-2283 Fix close order when reconfiguring config modules.
[controller.git] / opendaylight / config / config-manager / src / main / java / org / opendaylight / controller / config / manager / impl / ConfigTransactionControllerImpl.java
index 90d2cb4002098812218135b1a039548bc5db1001..37c2e2d777d83004b43e9dbd32cb3e9793c61f36 100644 (file)
@@ -7,12 +7,31 @@
  */
 package org.opendaylight.controller.config.manager.impl;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+import static java.lang.String.format;
+
+import com.google.common.collect.Lists;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.GuardedBy;
+import javax.management.DynamicMBean;
+import javax.management.InstanceAlreadyExistsException;
+import javax.management.InstanceNotFoundException;
+import javax.management.MBeanServer;
+import javax.management.ObjectName;
 import org.opendaylight.controller.config.api.DependencyResolver;
 import org.opendaylight.controller.config.api.ModuleIdentifier;
-import org.opendaylight.controller.config.api.ServiceReferenceWritableRegistry;
 import org.opendaylight.controller.config.api.ValidationException;
 import org.opendaylight.controller.config.api.jmx.ObjectNameUtil;
 import org.opendaylight.controller.config.manager.impl.dependencyresolver.DependencyResolverManager;
+import org.opendaylight.controller.config.manager.impl.dependencyresolver.ModuleInternalTransactionalInfo;
 import org.opendaylight.controller.config.manager.impl.dynamicmbean.DynamicWritableWrapper;
 import org.opendaylight.controller.config.manager.impl.dynamicmbean.ReadOnlyAtomicBoolean;
 import org.opendaylight.controller.config.manager.impl.dynamicmbean.ReadOnlyAtomicBoolean.ReadOnlyAtomicBooleanImpl;
@@ -22,28 +41,10 @@ import org.opendaylight.controller.config.manager.impl.jmx.TransactionModuleJMXR
 import org.opendaylight.controller.config.spi.Module;
 import org.opendaylight.controller.config.spi.ModuleFactory;
 import org.opendaylight.yangtools.concepts.Identifiable;
+import org.opendaylight.yangtools.yang.data.impl.codec.CodecRegistry;
 import org.osgi.framework.BundleContext;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
-import javax.annotation.Nullable;
-import javax.annotation.concurrent.GuardedBy;
-import javax.management.DynamicMBean;
-import javax.management.InstanceAlreadyExistsException;
-import javax.management.InstanceNotFoundException;
-import javax.management.MBeanServer;
-import javax.management.ObjectName;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Map.Entry;
-import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
-
-import static java.lang.String.format;
-
 /**
  * This is a JMX bean representing current transaction. It contains
  * transaction identifier, unique version and parent version for
@@ -52,8 +53,8 @@ import static java.lang.String.format;
 class ConfigTransactionControllerImpl implements
         ConfigTransactionControllerInternal,
         ConfigTransactionControllerImplMXBean,
-        Identifiable<TransactionIdentifier>{
-    private static final Logger logger = LoggerFactory.getLogger(ConfigTransactionControllerImpl.class);
+        Identifiable<TransactionIdentifier> {
+    private static final Logger LOG = LoggerFactory.getLogger(ConfigTransactionControllerImpl.class);
 
     private final ConfigTransactionLookupRegistry txLookupRegistry;
     private final ObjectName controllerON;
@@ -79,14 +80,13 @@ class ConfigTransactionControllerImpl implements
     private final boolean blankTransaction;
 
     @GuardedBy("this")
-    private final ServiceReferenceWritableRegistry writableSRRegistry;
+    private final SearchableServiceReferenceWritableRegistry writableSRRegistry;
 
     public ConfigTransactionControllerImpl(ConfigTransactionLookupRegistry txLookupRegistry,
-                                           long parentVersion, long currentVersion,
-                                           Map<String, Map.Entry<ModuleFactory, BundleContext>> currentlyRegisteredFactories,
+                                           long parentVersion, CodecRegistry codecRegistry, long currentVersion,
+                                           Map<String, Entry<ModuleFactory, BundleContext>> currentlyRegisteredFactories,
                                            MBeanServer transactionsMBeanServer, MBeanServer configMBeanServer,
-                                           boolean blankTransaction, ServiceReferenceWritableRegistry writableSRRegistry) {
-
+                                           boolean blankTransaction, SearchableServiceReferenceWritableRegistry  writableSRRegistry) {
         this.txLookupRegistry = txLookupRegistry;
         String transactionName = txLookupRegistry.getTransactionIdentifier().getName();
         this.controllerON = ObjectNameUtil.createTransactionControllerON(transactionName);
@@ -95,7 +95,8 @@ class ConfigTransactionControllerImpl implements
         this.currentlyRegisteredFactories = currentlyRegisteredFactories;
         this.factoriesHolder = new HierarchicalConfigMBeanFactoriesHolder(currentlyRegisteredFactories);
         this.transactionStatus = new TransactionStatus();
-        this.dependencyResolverManager = new DependencyResolverManager(transactionName, transactionStatus, writableSRRegistry);
+        this.dependencyResolverManager = new DependencyResolverManager(txLookupRegistry.getTransactionIdentifier(),
+                transactionStatus, writableSRRegistry, codecRegistry, transactionsMBeanServer);
         this.transactionsMBeanServer = transactionsMBeanServer;
         this.configMBeanServer = configMBeanServer;
         this.blankTransaction = blankTransaction;
@@ -124,26 +125,28 @@ class ConfigTransactionControllerImpl implements
 
         List<ModuleFactory> toBeAdded = new ArrayList<>();
         List<ModuleFactory> toBeRemoved = new ArrayList<>();
-        for(ModuleFactory moduleFactory: factoriesHolder.getModuleFactories()) {
-            if (oldSet.contains(moduleFactory) == false){
+        for (ModuleFactory moduleFactory : factoriesHolder.getModuleFactories()) {
+            if (oldSet.contains(moduleFactory) == false) {
                 toBeAdded.add(moduleFactory);
             }
         }
-        for(ModuleFactory moduleFactory: lastListOfFactories){
+        for (ModuleFactory moduleFactory : lastListOfFactories) {
             if (newSet.contains(moduleFactory) == false) {
                 toBeRemoved.add(moduleFactory);
             }
         }
         // add default modules
         for (ModuleFactory moduleFactory : toBeAdded) {
+            BundleContext bundleContext = getModuleFactoryBundleContext(moduleFactory.getImplementationName());
             Set<? extends Module> defaultModules = moduleFactory.getDefaultModules(dependencyResolverManager,
-                    getModuleFactoryBundleContext(moduleFactory.getImplementationName()));
+                    bundleContext);
             for (Module module : defaultModules) {
                 // ensure default module to be registered to jmx even if its module factory does not use dependencyResolverFactory
                 DependencyResolver dependencyResolver = dependencyResolverManager.getOrCreate(module.getIdentifier());
                 try {
                     boolean defaultBean = true;
-                    putConfigBeanToJMXAndInternalMaps(module.getIdentifier(), module, moduleFactory, null, dependencyResolver, defaultBean);
+                    putConfigBeanToJMXAndInternalMaps(module.getIdentifier(), module, moduleFactory, null,
+                            dependencyResolver, defaultBean, bundleContext);
                 } catch (InstanceAlreadyExistsException e) {
                     throw new IllegalStateException(e);
                 }
@@ -151,7 +154,7 @@ class ConfigTransactionControllerImpl implements
         }
 
         // remove modules belonging to removed factories
-        for(ModuleFactory removedFactory: toBeRemoved){
+        for (ModuleFactory removedFactory : toBeRemoved) {
             List<ModuleIdentifier> modulesOfRemovedFactory = dependencyResolverManager.findAllByFactory(removedFactory);
             for (ModuleIdentifier name : modulesOfRemovedFactory) {
                 destroyModule(name);
@@ -160,21 +163,26 @@ class ConfigTransactionControllerImpl implements
     }
 
 
-    private synchronized void copyExistingModule(
-            ModuleInternalInfo oldConfigBeanInfo)
-            throws InstanceAlreadyExistsException {
+    private synchronized void copyExistingModule(ModuleInternalInfo oldConfigBeanInfo) throws InstanceAlreadyExistsException {
+
         transactionStatus.checkNotCommitStarted();
         transactionStatus.checkNotAborted();
         ModuleIdentifier moduleIdentifier = oldConfigBeanInfo.getIdentifier();
         dependencyResolverManager.assertNotExists(moduleIdentifier);
 
-        ModuleFactory moduleFactory = factoriesHolder
-                .findByModuleName(moduleIdentifier.getFactoryName());
+        ModuleFactory moduleFactory;
+        BundleContext bc;
+        try {
+            moduleFactory = factoriesHolder.findByModuleName(moduleIdentifier.getFactoryName());
+            bc = getModuleFactoryBundleContext(moduleFactory.getImplementationName());
+        } catch (InstanceNotFoundException e) {
+            throw new IllegalStateException(e);
+        }
 
         Module module;
         DependencyResolver dependencyResolver = dependencyResolverManager.getOrCreate(moduleIdentifier);
         try {
-            BundleContext bc = getModuleFactoryBundleContext(moduleFactory.getImplementationName());
+
             module = moduleFactory.createModule(
                     moduleIdentifier.getInstanceName(), dependencyResolver,
                     oldConfigBeanInfo.getReadableModule(), bc);
@@ -184,12 +192,12 @@ class ConfigTransactionControllerImpl implements
                     oldConfigBeanInfo, moduleFactory), e);
         }
         putConfigBeanToJMXAndInternalMaps(moduleIdentifier, module, moduleFactory, oldConfigBeanInfo, dependencyResolver,
-                oldConfigBeanInfo.isDefaultBean());
+                oldConfigBeanInfo.isDefaultBean(), bc);
     }
 
     @Override
     public synchronized ObjectName createModule(String factoryName,
-            String instanceName) throws InstanceAlreadyExistsException {
+                                                String instanceName) throws InstanceAlreadyExistsException {
 
         transactionStatus.checkNotCommitStarted();
         transactionStatus.checkNotAborted();
@@ -197,27 +205,34 @@ class ConfigTransactionControllerImpl implements
         dependencyResolverManager.assertNotExists(moduleIdentifier);
 
         // find factory
-        ModuleFactory moduleFactory = factoriesHolder.findByModuleName(factoryName);
+        ModuleFactory moduleFactory;
+        try {
+            moduleFactory = factoriesHolder.findByModuleName(factoryName);
+        } catch (InstanceNotFoundException e) {
+            throw new IllegalArgumentException(e);
+        }
         DependencyResolver dependencyResolver = dependencyResolverManager.getOrCreate(moduleIdentifier);
+        BundleContext bundleContext = getModuleFactoryBundleContext(moduleFactory.getImplementationName());
         Module module = moduleFactory.createModule(instanceName, dependencyResolver,
-                getModuleFactoryBundleContext(moduleFactory.getImplementationName()));
+                bundleContext);
         boolean defaultBean = false;
         return putConfigBeanToJMXAndInternalMaps(moduleIdentifier, module,
-                moduleFactory, null, dependencyResolver, defaultBean);
+                moduleFactory, null, dependencyResolver, defaultBean, bundleContext);
     }
 
     private synchronized ObjectName putConfigBeanToJMXAndInternalMaps(
             ModuleIdentifier moduleIdentifier, Module module,
             ModuleFactory moduleFactory,
-            @Nullable ModuleInternalInfo maybeOldConfigBeanInfo, DependencyResolver dependencyResolver, boolean isDefaultBean)
+            @Nullable ModuleInternalInfo maybeOldConfigBeanInfo, DependencyResolver dependencyResolver,
+            boolean isDefaultBean, BundleContext bundleContext)
             throws InstanceAlreadyExistsException {
 
-        logger.debug("Adding module {} to transaction {}", moduleIdentifier, this);
-        if (moduleIdentifier.equals(module.getIdentifier())==false) {
+        LOG.debug("Adding module {} to transaction {}", moduleIdentifier, this);
+        if (moduleIdentifier.equals(module.getIdentifier()) == false) {
             throw new IllegalStateException("Incorrect name reported by module. Expected "
-             + moduleIdentifier + ", got " + module.getIdentifier());
+                    + moduleIdentifier + ", got " + module.getIdentifier());
         }
-        if (dependencyResolver.getIdentifier().equals(moduleIdentifier) == false ) {
+        if (dependencyResolver.getIdentifier().equals(moduleIdentifier) == false) {
             throw new IllegalStateException("Incorrect name reported by dependency resolver. Expected "
                     + moduleIdentifier + ", got " + dependencyResolver.getIdentifier());
         }
@@ -231,11 +246,10 @@ class ConfigTransactionControllerImpl implements
         // put wrapper to jmx
         TransactionModuleJMXRegistration transactionModuleJMXRegistration = getTxModuleJMXRegistrator()
                 .registerMBean(writableDynamicWrapper, writableON);
-        ModuleInternalTransactionalInfo moduleInternalTransactionalInfo = new ModuleInternalTransactionalInfo(
-                moduleIdentifier, module, moduleFactory,
-                maybeOldConfigBeanInfo, transactionModuleJMXRegistration, isDefaultBean);
 
-        dependencyResolverManager.put(moduleInternalTransactionalInfo);
+        dependencyResolverManager.put(
+                moduleIdentifier, module, moduleFactory,
+                maybeOldConfigBeanInfo, transactionModuleJMXRegistration, isDefaultBean, bundleContext);
         return writableON;
     }
 
@@ -258,22 +272,21 @@ class ConfigTransactionControllerImpl implements
     }
 
     private synchronized void destroyModule(ModuleIdentifier moduleIdentifier) {
-        logger.debug("Destroying module {} in transaction {}", moduleIdentifier, this);
+        LOG.debug("Destroying module {} in transaction {}", moduleIdentifier, this);
         transactionStatus.checkNotAborted();
 
-        if (blankTransaction == false) {
-            ModuleInternalTransactionalInfo found =
-                    dependencyResolverManager.findModuleInternalTransactionalInfo(moduleIdentifier);
-            if (found.isDefaultBean()) {
-                logger.warn("Warning: removing default bean. This will be forbidden in next version of config-subsystem");
-            }
+        ModuleInternalTransactionalInfo found = dependencyResolverManager.findModuleInternalTransactionalInfo(moduleIdentifier);
+        if (blankTransaction == false &&
+                found.isDefaultBean()) {
+            LOG.warn("Warning: removing default bean. This will be forbidden in next version of config-subsystem");
         }
         // first remove refNames, it checks for objectname existence
+
         try {
             writableSRRegistry.removeServiceReferences(
-                    ObjectNameUtil.createTransactionModuleON(getTransactionName(),moduleIdentifier));
+                    ObjectNameUtil.createTransactionModuleON(getTransactionName(), moduleIdentifier));
         } catch (InstanceNotFoundException e) {
-            logger.error("Possible code error: cannot find {} in {}", moduleIdentifier, writableSRRegistry);
+            LOG.error("Possible code error: cannot find {} in {}", moduleIdentifier, writableSRRegistry);
             throw new IllegalStateException("Possible code error: cannot find " + moduleIdentifier, e);
         }
 
@@ -294,19 +307,20 @@ class ConfigTransactionControllerImpl implements
 
     @Override
     public synchronized void validateConfig() throws ValidationException {
-        if (configBeanModificationDisabled.get())
+        if (configBeanModificationDisabled.get()) {
             throw new IllegalStateException("Cannot start validation");
+        }
         configBeanModificationDisabled.set(true);
         try {
-            validate_noLocks();
+            validateNoLocks();
         } finally {
             configBeanModificationDisabled.set(false);
         }
     }
 
-    private void validate_noLocks() throws ValidationException {
+    private void validateNoLocks() throws ValidationException {
         transactionStatus.checkNotAborted();
-        logger.trace("Validating transaction {}", getTransactionIdentifier());
+        LOG.trace("Validating transaction {}", getTransactionIdentifier());
         // call validate()
         List<ValidationException> collectedExceptions = new ArrayList<>();
         for (Entry<ModuleIdentifier, Module> entry : dependencyResolverManager
@@ -316,17 +330,17 @@ class ConfigTransactionControllerImpl implements
             try {
                 module.validate();
             } catch (Exception e) {
-                logger.warn("Validation exception in {}", getTransactionName(),
+                LOG.warn("Validation exception in {}", getTransactionName(),
                         e);
                 collectedExceptions.add(ValidationException
                         .createForSingleException(name, e));
             }
         }
-        if (collectedExceptions.size() > 0) {
+        if (!collectedExceptions.isEmpty()) {
             throw ValidationException
                     .createFromCollectedValidationExceptions(collectedExceptions);
         }
-        logger.trace("Validated transaction {}", getTransactionIdentifier());
+        LOG.trace("Validated transaction {}", getTransactionIdentifier());
     }
 
     /**
@@ -343,9 +357,9 @@ class ConfigTransactionControllerImpl implements
         transactionStatus.checkNotCommitStarted();
         configBeanModificationDisabled.set(true);
         try {
-            validate_noLocks();
+            validateNoLocks();
         } catch (ValidationException e) {
-            logger.trace("Commit failed on validation");
+            LOG.trace("Commit failed on validation");
             configBeanModificationDisabled.set(false); // recoverable error
             throw e;
         }
@@ -368,35 +382,36 @@ class ConfigTransactionControllerImpl implements
                             + "to obtain a lock");
         }
 
-        logger.trace("Committing transaction {}", getTransactionIdentifier());
+        LOG.trace("Committing transaction {}", getTransactionIdentifier());
+
+        Map<ModuleIdentifier, Module> allModules = dependencyResolverManager.getAllModules();
+
+        // call getInstance() on all Modules from top to bottom (from source to target of the dependency relation)
+        // The source of a dependency closes itself and calls getInstance recursively on the dependencies (in case of reconfiguration)
+        // This makes close() calls from top to bottom while createInstance() calls are performed bottom to top
+        List<ModuleIdentifier> sortedModuleIdentifiers = Lists.reverse(dependencyResolverManager.getSortedModuleIdentifiers());
+        for (ModuleIdentifier moduleIdentifier : sortedModuleIdentifiers) {
+            Module module = allModules.get(moduleIdentifier);
 
-        // call getInstance()
-        for (Entry<ModuleIdentifier, Module> entry : dependencyResolverManager
-                .getAllModules().entrySet()) {
-            Module module = entry.getValue();
-            ModuleIdentifier name = entry.getKey();
             try {
-                logger.debug("About to commit {} in transaction {}",
-                        name, getTransactionIdentifier());
-                module.getInstance();
+                LOG.debug("About to commit {} in transaction {}",
+                        moduleIdentifier, getTransactionIdentifier());
+                AutoCloseable instance = module.getInstance();
+                checkNotNull(instance, "Instance is null:{} in transaction {}", moduleIdentifier, getTransactionIdentifier());
             } catch (Exception e) {
-                logger.error("Commit failed on {} in transaction {}", name,
+                LOG.error("Commit failed on {} in transaction {}", moduleIdentifier,
                         getTransactionIdentifier(), e);
                 internalAbort();
-                throw new RuntimeException(
+                throw new IllegalStateException(
                         format("Error - getInstance() failed for %s in transaction %s",
-                                name, getTransactionIdentifier()), e);
+                                moduleIdentifier, getTransactionIdentifier()), e);
             }
         }
 
-        // count dependency order
-
-        logger.trace("Committed configuration {}", getTransactionIdentifier());
+        LOG.trace("Committed configuration {}", getTransactionIdentifier());
         transactionStatus.setCommitted();
-        // unregister this and all modules from jmx
-        close();
 
-        return dependencyResolverManager.getSortedModuleIdentifiers();
+        return sortedModuleIdentifiers;
     }
 
     @Override
@@ -407,12 +422,13 @@ class ConfigTransactionControllerImpl implements
     }
 
     private void internalAbort() {
+        LOG.trace("Aborting {}", this);
         transactionStatus.setAborted();
         close();
     }
 
     public void close() {
-        //FIXME: should not close object that was retrieved in constructor, a wrapper object should do that perhaps
+        dependencyResolverManager.close();
         txLookupRegistry.close();
     }
 
@@ -567,10 +583,11 @@ class ConfigTransactionControllerImpl implements
     }
 
     @Override
-    public ServiceReferenceWritableRegistry getWritableRegistry() {
+    public SearchableServiceReferenceWritableRegistry  getWritableRegistry() {
         return writableSRRegistry;
     }
 
+    @Override
     public TransactionIdentifier getTransactionIdentifier() {
         return txLookupRegistry.getTransactionIdentifier();
     }