Bug 4560: Improve config system logging for debuggability

Manually cherry-picked from
https://git.opendaylight.org/gerrit/#/c/28985 as the files have moved in
master.

Also the code has changed slightly in master, specifically the
ConfigPusherImplTest no longer uses a Thread uncaught exception handler
for verification. However it does rely on exceptions thrown from the
ConfigPusherImpl so, to keep the same behavior, I added a
propagateExceptions flag to ConfigPusherImpl#process. The
ConfigPersisterActivator production code passes false so unchecked
exceptions aren't handled as uncaught exceptions.

Change-Id: Iabc22030abc22cf11a1476986ba3d3366021b4fb
Signed-off-by: Tom Pantelis <tpanteli@brocade.com>

diff --git a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java
index 77f355e0b25c27530c0d36e20bf6e46c5ebfd04b..90f71ca51aa1fc338fdae21aaacdff0a665196ca 100644 (file)
--- a/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java
+++ b/opendaylight/config/config-persister-impl/src/main/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImpl.java
@@ -69,13 +69,15 @@ public class ConfigPusherImpl implements ConfigPusher {
         this.facade = facade;
     }
 
         this.facade = facade;
     }
 

-    public void process(List<AutoCloseable> autoCloseables, MBeanServerConnection platformMBeanServer, Persister persisterAggregator) throws InterruptedException {
-        while(true) {
-            processSingle(autoCloseables, platformMBeanServer, persisterAggregator);
+    public void process(List<AutoCloseable> autoCloseables, MBeanServerConnection platformMBeanServer,
+            Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException {
+        while(processSingle(autoCloseables, platformMBeanServer, persisterAggregator, propagateExceptions)) {
+            ;

         }
     }
 
         }
     }
 

-    void processSingle(final List<AutoCloseable> autoCloseables, final MBeanServerConnection platformMBeanServer, final Persister persisterAggregator) throws InterruptedException {
+    boolean processSingle(final List<AutoCloseable> autoCloseables, final MBeanServerConnection platformMBeanServer,
+            final Persister persisterAggregator, boolean propagateExceptions) throws InterruptedException {

         final List<? extends ConfigSnapshotHolder> configs = queue.take();
         try {
             internalPushConfigs(configs);
         final List<? extends ConfigSnapshotHolder> configs = queue.take();
         try {
             internalPushConfigs(configs);


@@ -90,10 +92,22 @@ public class ConfigPusherImpl implements ConfigPusher {
             }
 
             LOG.debug("ConfigPusher has pushed configs {}", configs);
             }
 
             LOG.debug("ConfigPusher has pushed configs {}", configs);

-        } catch (DocumentedException e) {
-            LOG.error("Error pushing configs {}",configs);
-            throw new IllegalStateException(e);
+        } catch (Exception e) {
+            // Exceptions are logged to error downstream
+            LOG.debug("Failed to push some of configs: {}", configs, e);
+
+            if(propagateExceptions) {
+                if(e instanceof RuntimeException) {
+                    throw (RuntimeException)e;
+                } else {
+                    throw new IllegalStateException(e);
+                }
+            } else {
+                return false;
+            }

         }
         }

+
+        return true;

     }
 
     @Override
     }
 
     @Override


@@ -109,15 +123,21 @@ public class ConfigPusherImpl implements ConfigPusher {
         // start pushing snapshots
         for (ConfigSnapshotHolder configSnapshotHolder : configs) {
             if (configSnapshotHolder != null) {
         // start pushing snapshots
         for (ConfigSnapshotHolder configSnapshotHolder : configs) {
             if (configSnapshotHolder != null) {

+                LOG.info("Pushing configuration snapshot {}", configSnapshotHolder);

                 boolean pushResult = false;
                 try {
                     pushResult = pushConfigWithConflictingVersionRetries(configSnapshotHolder);
                 } catch (ConfigSnapshotFailureException e) {
                 boolean pushResult = false;
                 try {
                     pushResult = pushConfigWithConflictingVersionRetries(configSnapshotHolder);
                 } catch (ConfigSnapshotFailureException e) {

-                    LOG.warn("Failed to apply configuration snapshot: {}. Config snapshot is not semantically correct and will be IGNORED. " +
+                    LOG.error("Failed to apply configuration snapshot: {}. Config snapshot is not semantically correct and will be IGNORED. " +

                             "for detailed information see enclosed exception.", e.getConfigIdForReporting(), e);
                     throw new IllegalStateException("Failed to apply configuration snapshot " + e.getConfigIdForReporting(), e);
                             "for detailed information see enclosed exception.", e.getConfigIdForReporting(), e);
                     throw new IllegalStateException("Failed to apply configuration snapshot " + e.getConfigIdForReporting(), e);

+                }  catch (Exception e) {
+                    String msg = String.format("Failed to apply configuration snapshot: %s", configSnapshotHolder);
+                    LOG.error(msg, e);
+                    throw new IllegalStateException(msg, e);

                 }
                 }

-                LOG.debug("Config snapshot pushed successfully: {}, result: {}", configSnapshotHolder, result);
+
+                LOG.info("Successfully pushed configuration snapshot {}", configSnapshotHolder);

                 result.put(configSnapshotHolder, pushResult);
             }
         }
                 result.put(configSnapshotHolder, pushResult);
             }
         }

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 ef53ecd8f242899b65ceccbd4a602578e0782022..5f25d6c3b2c27cd1f3b0d9dd2a0144eb250f8e38 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
@@ -128,7 +128,7 @@ public class ConfigPersisterActivator implements BundleActivator {
                         }
                         if(context != null) {
                             registration = context.registerService(ConfigPusher.class.getName(), configPusher, null);
                         }
                         if(context != null) {
                             registration = context.registerService(ConfigPusher.class.getName(), configPusher, null);

-                            configPusher.process(autoCloseables, platformMBeanServer, persisterAggregator);
+                            configPusher.process(autoCloseables, platformMBeanServer, persisterAggregator, false);

                         } else {
                             LOG.warn("Unable to process configs as BundleContext is null");
                         }
                         } else {
                             LOG.warn("Unable to process configs as BundleContext is null");
                         }

diff --git a/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java b/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java
index 3583e32e2626ca83464e8f7e54f69cac76f3b4bb..8bfb3d73b975cb743ab4e540ce9878f029fa268a 100644 (file)
--- a/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java
+++ b/opendaylight/config/config-persister-impl/src/test/java/org/opendaylight/controller/config/persist/impl/ConfigPusherImplTest.java
@@ -8,7 +8,6 @@
 package org.opendaylight.controller.config.persist.impl;
 
 import static org.junit.Assert.assertEquals;
 package org.opendaylight.controller.config.persist.impl;
 
 import static org.junit.Assert.assertEquals;

-import static org.junit.Assert.assertNotNull;

 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;


@@ -20,7 +19,7 @@ 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.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;

-
+import com.google.common.base.Throwables;

 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import java.lang.management.ManagementFactory;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import java.lang.management.ManagementFactory;


@@ -92,11 +91,12 @@ public class ConfigPusherImplTest {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {

-            configPusher.process(Lists.<AutoCloseable>newArrayList(), ManagementFactory.getPlatformMBeanServer(), mockedAggregator);
+            configPusher.process(Lists.<AutoCloseable>newArrayList(), ManagementFactory.getPlatformMBeanServer(),
+                    mockedAggregator, true);

         } catch(IllegalStateException e) {
         } catch(IllegalStateException e) {

-            assertNotNull(e.getCause());
-            assertTrue(e.getCause() instanceof ConfigPusherImpl.NotEnoughCapabilitiesException);
-            final Set<String> missingCaps = ((ConfigPusherImpl.NotEnoughCapabilitiesException) e.getCause()).getMissingCaps();
+            Throwable cause = Throwables.getRootCause(e);
+            assertTrue(cause instanceof ConfigPusherImpl.NotEnoughCapabilitiesException);
+            final Set<String> missingCaps = ((ConfigPusherImpl.NotEnoughCapabilitiesException) cause).getMissingCaps();

             assertEquals(missingCaps.size(), 1);
             assertEquals(missingCaps.iterator().next(), "required-cap");
             return;
             assertEquals(missingCaps.size(), 1);
             assertEquals(missingCaps.iterator().next(), "required-cap");
             return;


@@ -121,7 +121,7 @@ public class ConfigPusherImplTest {
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 0, 0);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 0, 0);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));

-        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);

 
         verify(facade).executeConfigExecution(cfgExec);
         verify(facade).commitSilentTransaction();
 
         verify(facade).executeConfigExecution(cfgExec);
         verify(facade).commitSilentTransaction();


@@ -144,10 +144,10 @@ public class ConfigPusherImplTest {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         try {

-            configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+            configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);

         } catch (IllegalStateException e) {
         } catch (IllegalStateException e) {

-            assertNotNull(e.getCause());
-            assertTrue(e.getCause() instanceof ConflictingVersionException);
+            Throwable cause = Throwables.getRootCause(e);
+            assertTrue(cause instanceof ConflictingVersionException);

             return;
         }
 
             return;
         }
 


@@ -174,7 +174,7 @@ public class ConfigPusherImplTest {
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 5000, 5000);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));
         final ConfigPusherImpl configPusher = new ConfigPusherImpl(facadeFactory, 5000, 5000);
 
         configPusher.pushConfigs(Collections.singletonList(mockedConfigSnapshot));

-        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator);
+        configPusher.processSingle(Lists.<AutoCloseable>newArrayList(), mBeanServer, mockedAggregator, true);

 
         verify(facade, times(3)).executeConfigExecution(cfgExec);
         verify(facade, times(3)).commitSilentTransaction();
 
         verify(facade, times(3)).executeConfigExecution(cfgExec);
         verify(facade, times(3)).commitSilentTransaction();