Modify ModuleInfoBundleTracker to track RESOLVED bundles (round 2) 74/27874/4
authorTom Pantelis <tpanteli@brocade.com>
Fri, 2 Oct 2015 21:14:46 +0000 (17:14 -0400)
committerGerrit Code Review <gerrit@opendaylight.org>
Thu, 8 Oct 2015 07:16:31 +0000 (07:16 +0000)
My first patch https://git.opendaylight.org/gerrit/#/c/27138/ didn't
do well with the feature tests due the BundleContext bust wait so it
was reverted.

I went back to my original solution to confgure the ModuleInfoBundleTracker
to track RESOLVED and ModuleFactoryBundleTracker to track ACTIVE.
Originally when I tried that I had some failure due to the ModuleFactory
not loaded yet but I don't remember exactly what. This patch seems to work
fine - I've restarted karaf several times and also ran the tsdr features tests
several times successfully. Originally I did the first patch in stable/lithium
so maybe something else has changed in master or the way I did it wasn't right.

Since the initial yang module info's are now processed synchronously when the
BundleTracker is opened, I modified the ModuleInfoBundleTracker to
ensure it doesn't propagate runtime ex's. This would disrupt the
BundleTracker and the ConfigManagerActivator - if one module had an
issue the config manager wouldn't start.

For every YangModuleInfo scraped, it registers it with the
ModuleInfoRegistry. The backing impl is RefreshingSCPModuleInfoRegistry
which causes a new SchemaContext to be created from the current yang
models (via updateService). This isn't efficient - on startup, we'll get all
YangModuleInfo's in quick succession so, optimially, it should build the
SchemaContext once after open is complete. This is what the
GlobalBundleScanningSchemaServiceImpl does.

To accomplish this, I removed the call to updateService from
RefreshingSCPModuleInfoRegistry#registerModuleInfo - it is now
specifically called by ModuleInfoBundleTracker. This means the
ModuleInfoBundleTracker now references RefreshingSCPModuleInfoRegistry
instead of the ModuleInfoRegistry interface which makes it less clean.
Any other way would require changes to the ModuleInfoRegistry interface,
which I didn't want to do, or extending the interface which I didn't think that
was worth the effort. The RefreshingSCPModuleInfoRegistry is only used by
ModuleInfoBundleTracker.

Change-Id: I20213ce8bd1dfc5109f3ef223cec8048bec92e12
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>
opendaylight/config/config-manager-facade-xml/src/main/java/org/opendaylight/controller/config/facade/xml/osgi/YangStoreActivator.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ConfigManagerActivator.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/ModuleInfoBundleTracker.java
opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/RefreshingSCPModuleInfoRegistry.java
opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/RefreshingSCPModuleInfoRegistryTest.java

index 57ebfb2..32e064a 100644 (file)
@@ -73,6 +73,14 @@ public class YangStoreActivator implements BundleActivator {
                 // TODO avoid cast
                 final YangStoreService yangStoreService = new YangStoreService(schemaContextProvider,
                     ((SchemaSourceProvider<YangTextSchemaSource>) sourceProvider));
+
+                final BindingRuntimeContext runtimeContext = (BindingRuntimeContext) reference
+                        .getProperty(BindingRuntimeContext.class.getName());
+                LOG.debug("BindingRuntimeContext retrieved as {}", runtimeContext);
+                if(runtimeContext != null) {
+                    yangStoreService.refresh(runtimeContext);
+                }
+
                 yangStoreServiceServiceRegistration = context.registerService(YangStoreService.class, yangStoreService, new Hashtable<String, Object>());
                 configRegistryLookup = new ConfigRegistryLookupThread(yangStoreService);
                 configRegistryLookup.start();
index 7966b89..c15c6c8 100644 (file)
@@ -49,7 +49,7 @@ public class ConfigManagerActivator implements BundleActivator {
             RefreshingSCPModuleInfoRegistry moduleInfoRegistryWrapper = new RefreshingSCPModuleInfoRegistry(
                     moduleInfoBackedContext, moduleInfoBackedContext, moduleInfoBackedContext, moduleInfoBackedContext, bindingContextProvider, context);
 
-            ModuleInfoBundleTracker moduleInfoBundleTracker = new ModuleInfoBundleTracker(moduleInfoRegistryWrapper);
+            ModuleInfoBundleTracker moduleInfoBundleTracker = new ModuleInfoBundleTracker(context, moduleInfoRegistryWrapper);
 
             // start config registry
             BundleContextBackedModuleFactoriesResolver bundleContextBackedModuleFactoriesResolver = new BundleContextBackedModuleFactoriesResolver(
@@ -60,13 +60,15 @@ public class ConfigManagerActivator implements BundleActivator {
             // track bundles containing factories
             BlankTransactionServiceTracker blankTransactionServiceTracker = new BlankTransactionServiceTracker(
                     configRegistry);
-            ModuleFactoryBundleTracker primaryModuleFactoryBundleTracker = new ModuleFactoryBundleTracker(
+            ModuleFactoryBundleTracker moduleFactoryTracker = new ModuleFactoryBundleTracker(
                     blankTransactionServiceTracker);
 
             // start extensible tracker
-            ExtensibleBundleTracker<?> bundleTracker = new ExtensibleBundleTracker<>(context,
-                    primaryModuleFactoryBundleTracker, moduleInfoBundleTracker);
-            bundleTracker.open();
+            ExtensibleBundleTracker<?> moduleFactoryBundleTracker = new ExtensibleBundleTracker<>(context,
+                    moduleFactoryTracker);
+            moduleFactoryBundleTracker.open();
+
+            moduleInfoBundleTracker.open();
 
             // Wrap config registry with JMX notification publishing adapter
             final JMXNotifierConfigRegistry notifyingConfigRegistry =
@@ -97,11 +99,17 @@ public class ConfigManagerActivator implements BundleActivator {
                     blankTransactionServiceTracker);
             serviceTracker.open();
 
-            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg, configRegistry, wrap(bundleTracker),
+            List<AutoCloseable> list = Arrays.asList(bindingContextProvider, clsReg, configRegistry,
+                    wrap(moduleFactoryBundleTracker), moduleInfoBundleTracker,
                     configRegReg, configRegistryJMXRegistrator, configRegistryJMXRegistratorWithNotifications, wrap(serviceTracker), moduleInfoRegistryWrapper, notifyingConfigRegistry);
             autoCloseable = OsgiRegistrationUtil.aggregate(list);
         } catch(Exception e) {
             LOG.warn("Error starting config manager", e);
+        } catch(Error e) {
+            // Log JVM Error and re-throw. The OSGi container may silently fail the bundle and not always log
+            // the exception. This has been seen on initial feature install.
+            LOG.error("Error starting config manager", e);
+            throw e;
         }
     }
 
index 6d29604..31bd5fa 100644 (file)
@@ -9,7 +9,6 @@ package org.opendaylight.controller.config.manager.impl.osgi;
 
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
 import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
@@ -90,7 +89,7 @@ public final class ExtensibleBundleTracker<T> extends BundleTracker<Future<T>> {
                     LOG.trace("AddingBundle for {} and event {} finished successfully",bundle,event);
                     return primaryTrackerRetVal;
                 } catch (Exception e) {
-                    LOG.error("Failed to add bundle ",e);
+                    LOG.error("Failed to add bundle {}", bundle, e);
                     throw e;
                 }
             }
@@ -122,8 +121,8 @@ public final class ExtensibleBundleTracker<T> extends BundleTracker<Future<T>> {
                 }
             });
             LOG.trace("Removed bundle event for {} finished successfully.",bundle);
-        } catch (InterruptedException | ExecutionException e) {
-            LOG.error("Addition of bundle failed, ", e);
+        } catch (Exception e) {
+            LOG.error("Failed to remove bundle {}", bundle, e);
         }
     }
 
index 56535e7..1006513 100644 (file)
@@ -16,11 +16,12 @@ import java.util.Collection;
 import java.util.LinkedList;
 import java.util.List;
 import org.opendaylight.yangtools.concepts.ObjectRegistration;
-import org.opendaylight.yangtools.sal.binding.generator.api.ModuleInfoRegistry;
 import org.opendaylight.yangtools.yang.binding.YangModelBindingProvider;
 import org.opendaylight.yangtools.yang.binding.YangModuleInfo;
 import org.osgi.framework.Bundle;
+import org.osgi.framework.BundleContext;
 import org.osgi.framework.BundleEvent;
+import org.osgi.util.tracker.BundleTracker;
 import org.osgi.util.tracker.BundleTrackerCustomizer;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -28,17 +29,39 @@ import org.slf4j.LoggerFactory;
 /**
  * Tracks bundles and attempts to retrieve YangModuleInfo, which is then fed into ModuleInfoRegistry
  */
-public final class ModuleInfoBundleTracker implements BundleTrackerCustomizer<Collection<ObjectRegistration<YangModuleInfo>>> {
+public final class ModuleInfoBundleTracker implements AutoCloseable,
+        BundleTrackerCustomizer<Collection<ObjectRegistration<YangModuleInfo>>> {
 
     private static final Logger LOG = LoggerFactory.getLogger(ModuleInfoBundleTracker.class);
 
     public static final String MODULE_INFO_PROVIDER_PATH_PREFIX = "META-INF/services/";
 
 
-    private final ModuleInfoRegistry moduleInfoRegistry;
+    private final RefreshingSCPModuleInfoRegistry moduleInfoRegistry;
+    private final BundleTracker<Collection<ObjectRegistration<YangModuleInfo>>> bundleTracker;
+    private boolean starting;
 
-    public ModuleInfoBundleTracker(ModuleInfoRegistry moduleInfoRegistry) {
+    public ModuleInfoBundleTracker(BundleContext context, RefreshingSCPModuleInfoRegistry moduleInfoRegistry) {
         this.moduleInfoRegistry = moduleInfoRegistry;
+        bundleTracker = new BundleTracker<>(context, Bundle.RESOLVED | Bundle.STARTING |
+                Bundle.STOPPING | Bundle.ACTIVE, this);
+    }
+
+    public void open() {
+        LOG.debug("ModuleInfoBundleTracker open starting");
+
+        starting = true;
+        bundleTracker.open();
+
+        starting = false;
+        moduleInfoRegistry.updateService();
+
+        LOG.debug("ModuleInfoBundleTracker open complete");
+    }
+
+    @Override
+    public void close() {
+        bundleTracker.close();
     }
 
     @Override
@@ -56,9 +79,14 @@ public final class ModuleInfoBundleTracker implements BundleTrackerCustomizer<Co
                 YangModuleInfo moduleInfo = retrieveModuleInfo(moduleInfoName, bundle);
                 registrations.add(moduleInfoRegistry.registerModuleInfo(moduleInfo));
             }
+
+            if(!starting) {
+                moduleInfoRegistry.updateService();
+            }
         } catch (IOException e) {
-            LOG.error("Error while reading {}", resource, e);
-            throw new RuntimeException(e);
+            LOG.error("Error while reading {} from bundle {}", resource, bundle, e);
+        } catch (RuntimeException e) {
+            LOG.error("Failed to process {} for bundle {}", resource, bundle, e);
         }
 
         LOG.trace("Got following registrations {}", registrations);
@@ -79,7 +107,7 @@ public final class ModuleInfoBundleTracker implements BundleTrackerCustomizer<Co
             try {
                 reg.close();
             } catch (Exception e) {
-                throw new RuntimeException("Unable to unregister YangModuleInfo " + reg.getInstance(), e);
+                LOG.error("Unable to unregister YangModuleInfo {}", reg.getInstance(), e);
             }
         }
     }
@@ -104,13 +132,11 @@ public final class ModuleInfoBundleTracker implements BundleTrackerCustomizer<Co
                     moduleInfoClass, bundle, e);
             throw new IllegalStateException(errorMessage, e);
         }
+
         try{
             return instance.getModuleInfo();
-        } catch (NoClassDefFoundError e) {
-
-
-            LOG.error("Error while executing getModuleInfo on {}", instance, e);
-            throw e;
+        } catch (NoClassDefFoundError | ExceptionInInitializerError e) {
+            throw new IllegalStateException("Error while executing getModuleInfo on " + instance, e);
         }
     }
 
index 8bc1a58..5321dde 100644 (file)
@@ -19,11 +19,14 @@ import org.opendaylight.yangtools.yang.model.repo.api.YangTextSchemaSource;
 import org.opendaylight.yangtools.yang.model.repo.spi.SchemaSourceProvider;
 import org.osgi.framework.BundleContext;
 import org.osgi.framework.ServiceRegistration;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Update SchemaContext service in Service Registry each time new YangModuleInfo is added or removed.
  */
 public class RefreshingSCPModuleInfoRegistry implements ModuleInfoRegistry, AutoCloseable {
+    private static final Logger LOG = LoggerFactory.getLogger(RefreshingSCPModuleInfoRegistry.class);
 
     private final ModuleInfoRegistry moduleInfoRegistry;
     private final SchemaContextProvider schemaContextProvider;
@@ -31,7 +34,7 @@ public class RefreshingSCPModuleInfoRegistry implements ModuleInfoRegistry, Auto
     private final BindingContextProvider bindingContextProvider;
     private final ClassLoadingStrategy classLoadingStrat;
 
-    private final ServiceRegistration<SchemaContextProvider> osgiReg;
+    private volatile ServiceRegistration<SchemaContextProvider> osgiReg;
 
     public RefreshingSCPModuleInfoRegistry(final ModuleInfoRegistry moduleInfoRegistry,
         final SchemaContextProvider schemaContextProvider, final ClassLoadingStrategy classLoadingStrat,
@@ -47,26 +50,35 @@ public class RefreshingSCPModuleInfoRegistry implements ModuleInfoRegistry, Auto
             .registerService(SchemaContextProvider.class, schemaContextProvider, new Hashtable<String, String>());
     }
 
-    private void updateService() {
-        bindingContextProvider.update(classLoadingStrat, schemaContextProvider);
-        osgiReg.setProperties(new Hashtable<String, Object>() {{
-                put(BindingRuntimeContext.class.getName(), bindingContextProvider.getBindingContext());
-                put(SchemaSourceProvider.class.getName(), sourceProvider);
+    public void updateService() {
+        if(osgiReg != null) {
+            try {
+                bindingContextProvider.update(classLoadingStrat, schemaContextProvider);
+                osgiReg.setProperties(new Hashtable<String, Object>() {{
+                        put(BindingRuntimeContext.class.getName(), bindingContextProvider.getBindingContext());
+                        put(SchemaSourceProvider.class.getName(), sourceProvider);
+                    }}); // send modifiedService event
+            } catch (RuntimeException e) {
+                // The ModuleInfoBackedContext throws a RuntimeException if it can't create the schema context.
+                LOG.warn("Error updating the BindingContextProvider", e);
             }
-        }); // send modifiedService event
+        }
     }
 
     @Override
     public ObjectRegistration<YangModuleInfo> registerModuleInfo(YangModuleInfo yangModuleInfo) {
         ObjectRegistration<YangModuleInfo> yangModuleInfoObjectRegistration = moduleInfoRegistry.registerModuleInfo(yangModuleInfo);
         ObjectRegistrationWrapper wrapper = new ObjectRegistrationWrapper(yangModuleInfoObjectRegistration);
-        updateService();
         return wrapper;
     }
 
     @Override
     public void close() throws Exception {
-        osgiReg.unregister();
+        if(osgiReg != null) {
+            osgiReg.unregister();
+        }
+
+        osgiReg = null;
     }
 
     private class ObjectRegistrationWrapper implements ObjectRegistration<YangModuleInfo> {
index 8613eeb..565e9bb 100644 (file)
@@ -13,7 +13,6 @@ import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
-
 import java.util.Dictionary;
 import org.junit.Before;
 import org.junit.Test;
@@ -70,6 +69,7 @@ public class RefreshingSCPModuleInfoRegistryTest {
         doReturn(ymi).when(reg).registerModuleInfo(modInfo);
 
         scpreg.registerModuleInfo(modInfo);
+        scpreg.updateService();
 
         verify(codecRegistryProvider).update(classLoadingStrat, prov);