From: Tom Pantelis Date: Fri, 2 Oct 2015 21:14:46 +0000 (-0400) Subject: Modify ModuleInfoBundleTracker to track RESOLVED bundles (round 2) X-Git-Tag: release/beryllium~245 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=de1885b0d6abdabf6074d251332f1919bcdb9ac6 Modify ModuleInfoBundleTracker to track RESOLVED bundles (round 2) 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 --- diff --git a/opendaylight/config/config-manager-facade-xml/src/main/java/org/opendaylight/controller/config/facade/xml/osgi/YangStoreActivator.java b/opendaylight/config/config-manager-facade-xml/src/main/java/org/opendaylight/controller/config/facade/xml/osgi/YangStoreActivator.java index 57ebfb2b10..32e064a27a 100644 --- a/opendaylight/config/config-manager-facade-xml/src/main/java/org/opendaylight/controller/config/facade/xml/osgi/YangStoreActivator.java +++ b/opendaylight/config/config-manager-facade-xml/src/main/java/org/opendaylight/controller/config/facade/xml/osgi/YangStoreActivator.java @@ -73,6 +73,14 @@ public class YangStoreActivator implements BundleActivator { // TODO avoid cast final YangStoreService yangStoreService = new YangStoreService(schemaContextProvider, ((SchemaSourceProvider) 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()); configRegistryLookup = new ConfigRegistryLookupThread(yangStoreService); configRegistryLookup.start(); 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 7966b8983a..c15c6c8220 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 @@ -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 list = Arrays.asList(bindingContextProvider, clsReg, configRegistry, wrap(bundleTracker), + List 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; } } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java index 6d29604f3d..31bd5fa25e 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/ExtensibleBundleTracker.java @@ -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 extends BundleTracker> { 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 extends BundleTracker> { } }); 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); } } diff --git a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/ModuleInfoBundleTracker.java b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/ModuleInfoBundleTracker.java index 56535e797b..1006513fe9 100644 --- a/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/ModuleInfoBundleTracker.java +++ b/opendaylight/config/config-manager/src/main/java/org/opendaylight/controller/config/manager/impl/osgi/mapping/ModuleInfoBundleTracker.java @@ -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>> { +public final class ModuleInfoBundleTracker implements AutoCloseable, + BundleTrackerCustomizer>> { 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>> 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 osgiReg; + private volatile ServiceRegistration 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()); } - private void updateService() { - bindingContextProvider.update(classLoadingStrat, schemaContextProvider); - osgiReg.setProperties(new Hashtable() {{ - 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() {{ + 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 registerModuleInfo(YangModuleInfo yangModuleInfo) { ObjectRegistration 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 { diff --git a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/RefreshingSCPModuleInfoRegistryTest.java b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/RefreshingSCPModuleInfoRegistryTest.java index 8613eeba6b..565e9bb3a5 100644 --- a/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/RefreshingSCPModuleInfoRegistryTest.java +++ b/opendaylight/config/config-manager/src/test/java/org/opendaylight/controller/config/manager/impl/osgi/RefreshingSCPModuleInfoRegistryTest.java @@ -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);