Exception handling tuning for OFChannelHandler

* alwasy log stack trace at debug level (even if we just log the message
  at error)
* don't wrap other exceptions in SwitchStateException. We don't log that
  with stack trace
* IOFSwitch.setDebugCounterService now throws the correct exception

src/main/java/net/floodlightcontroller/core/IOFSwitch.java

@@ -26,8 +26,8 @@ import java.util.Map;
 import java.util.concurrent.Future;
 import net.floodlightcontroller.core.IFloodlightProviderService.Role;
 import net.floodlightcontroller.core.internal.Controller;
-import net.floodlightcontroller.core.module.FloodlightModuleException;
 import net.floodlightcontroller.debugcounter.IDebugCounterService;
+import net.floodlightcontroller.debugcounter.IDebugCounterService.CounterException;
 import net.floodlightcontroller.threadpool.IThreadPoolService;
 import net.floodlightcontroller.util.OrderedCollection;
 
@@ -160,10 +160,10 @@ public interface IOFSwitch {
      * Set debug counter service for per-switch counters
      * Called immediately after instantiation
      * @param debugCounters
-     * @throws FloodlightModuleException
+     * @throws CounterException
      */
     public void setDebugCounterService(IDebugCounterService debugCounters)
-            throws FloodlightModuleException;
+            throws CounterException;
 
     /**
      * Set the netty Channel this switch instance is associated with

src/main/java/net/floodlightcontroller/core/OFSwitchBase.java

@@ -40,7 +40,6 @@ import net.floodlightcontroller.core.annotations.LogMessageDocs;
 import net.floodlightcontroller.core.internal.Controller;
 import net.floodlightcontroller.core.internal.OFFeaturesReplyFuture;
 import net.floodlightcontroller.core.internal.OFStatisticsFuture;
-import net.floodlightcontroller.core.module.FloodlightModuleException;
 import net.floodlightcontroller.core.util.AppCookie;
 import net.floodlightcontroller.core.web.serializers.DPIDSerializer;
 import net.floodlightcontroller.debugcounter.IDebugCounter;
@@ -1067,7 +1066,7 @@ public abstract class OFSwitchBase implements IOFSwitch {
     @Override
     @JsonIgnore
     public void setDebugCounterService(IDebugCounterService debugCounters)
-            throws FloodlightModuleException {
+            throws CounterException {
         this.debugCounters = debugCounters;
         registerOverloadCounters();
     }
@@ -1399,7 +1398,7 @@ public abstract class OFSwitchBase implements IOFSwitch {
                 currentRate, this);
     }
 
-    private void registerOverloadCounters() throws FloodlightModuleException {
+    private void registerOverloadCounters() throws CounterException {
         if (debugCountersRegistered) {
             return;
         }
@@ -1408,33 +1407,29 @@ public abstract class OFSwitchBase implements IOFSwitch {
             debugCounters = new NullDebugCounter();
             debugCountersRegistered = true;
         }
-        try {
-            // every level of the hierarchical counter has to be registered
-            // even if they are not used
-            ctrSwitch = debugCounters.registerCounter(
-                                       PACKAGE , stringId,
-                                       "Counter for this switch",
-                                       CounterType.ALWAYS_COUNT);
-            ctrSwitchPktin = debugCounters.registerCounter(
-                                       PACKAGE, stringId + "/pktin",
-                                       "Packet in counter for this switch",
-                                       CounterType.ALWAYS_COUNT);
-            ctrSwitchWrite = debugCounters.registerCounter(
-                                       PACKAGE, stringId + "/write",
-                                       "Write counter for this switch",
-                                       CounterType.ALWAYS_COUNT);
-            ctrSwitchPktinDrops = debugCounters.registerCounter(
-                                       PACKAGE, stringId + "/pktin/drops",
-                                       "Packet in throttle drop count",
-                                       CounterType.ALWAYS_COUNT);
-            ctrSwitchWriteDrops = debugCounters.registerCounter(
-                                       PACKAGE, stringId + "/write/drops",
-                                       "Switch write throttle drop count",
-                                       CounterType.ALWAYS_COUNT);
-        } catch (CounterException e) {
-            throw new FloodlightModuleException(e.getMessage());
-        }
-    }
+        // every level of the hierarchical counter has to be registered
+        // even if they are not used
+        ctrSwitch = debugCounters.registerCounter(
+                                   PACKAGE , stringId,
+                                   "Counter for this switch",
+                                   CounterType.ALWAYS_COUNT);
+        ctrSwitchPktin = debugCounters.registerCounter(
+                                   PACKAGE, stringId + "/pktin",
+                                   "Packet in counter for this switch",
+                                   CounterType.ALWAYS_COUNT);
+        ctrSwitchWrite = debugCounters.registerCounter(
+                                   PACKAGE, stringId + "/write",
+                                   "Write counter for this switch",
+                                   CounterType.ALWAYS_COUNT);
+        ctrSwitchPktinDrops = debugCounters.registerCounter(
+                                   PACKAGE, stringId + "/pktin/drops",
+                                   "Packet in throttle drop count",
+                                   CounterType.ALWAYS_COUNT);
+        ctrSwitchWriteDrops = debugCounters.registerCounter(
+                                   PACKAGE, stringId + "/write/drops",
+                                   "Switch write throttle drop count",
+                                   CounterType.ALWAYS_COUNT);
+}
 
     /**
      * Check if we have sampled this mac in the last second.

src/main/java/net/floodlightcontroller/core/internal/Controller.java

@@ -328,6 +328,7 @@ public class Controller implements IFloodlightProviderService,
         public IDebugCounter roleReplyTimeout;
         public IDebugCounter roleReplyReceived; // expected RoleReply received
         public IDebugCounter roleReplyErrorUnsupported;
+        public IDebugCounter switchCounterRegistrationFailed;
 
         void createCounters(IDebugCounterService debugCounters) throws CounterException {
             setRoleEqual =
@@ -709,6 +710,14 @@ public class Controller implements IFloodlightProviderService,
                             "request indicating that the switch does not " +
                             "support roles.",
                             CounterType.ALWAYS_COUNT);
+
+            switchCounterRegistrationFailed =
+                debugCounters.registerCounter(prefix,
+                            "switch-counter-registration-failed",
+                            "Number of times the controller failed to " +
+                            "register per-switch debug counters",
+                            CounterType.ALWAYS_COUNT,
+                            IDebugCounterService.CTR_MDATA_WARN);
         }
     }
 

src/main/java/net/floodlightcontroller/core/internal/OFChannelHandler.java

@@ -16,6 +16,7 @@ import net.floodlightcontroller.core.IOFSwitch.PortChangeEvent;
 import net.floodlightcontroller.core.annotations.LogMessageDoc;
 import net.floodlightcontroller.core.annotations.LogMessageDocs;
 import net.floodlightcontroller.core.internal.Controller.Counters;
+import net.floodlightcontroller.debugcounter.IDebugCounterService.CounterException;
 import net.floodlightcontroller.storage.IResultSet;
 import net.floodlightcontroller.storage.StorageException;
 import net.floodlightcontroller.util.LoadMonitor;
@@ -632,43 +633,43 @@ class OFChannelHandler
             @Override
             void processOFStatisticsReply(OFChannelHandler h,
                                           OFStatisticsReply m) {
+                // Read description, if it has been updated
+                OFDescriptionStatistics description =
+                        new OFDescriptionStatistics();
+                ChannelBuffer data =
+                        ChannelBuffers.buffer(description.getLength());
+                OFStatistics f = m.getFirstStatistics();
+                f.writeTo(data);
+                description.readFrom(data);
+                h.sw = h.controller.getOFSwitchInstance(description);
+                // set switch information
+                // set features reply and channel first so we a DPID and
+                // channel info.
+                h.sw.setFeaturesReply(h.featuresReply);
+                h.sw.setConnected(true);
+                h.sw.setChannel(h.channel);
+                h.sw.setFloodlightProvider(h.controller);
+                h.sw.setThreadPoolService(h.controller.getThreadPoolService());
                 try {
-                    // Read description, if it has been updated
-                    OFDescriptionStatistics description =
-                            new OFDescriptionStatistics();
-                    ChannelBuffer data =
-                            ChannelBuffers.buffer(description.getLength());
-                    OFStatistics f = m.getFirstStatistics();
-                    f.writeTo(data);
-                    description.readFrom(data);
-                    h.sw = h.controller.getOFSwitchInstance(description);
-                    // set switch information
-                    // set features reply and channel first so we a DPID and
-                    // channel info.
-                    h.sw.setFeaturesReply(h.featuresReply);
-                    h.sw.setConnected(true);
-                    h.sw.setChannel(h.channel);
-                    h.sw.setFloodlightProvider(h.controller);
-                    h.sw.setThreadPoolService(h.controller.getThreadPoolService());
                     h.sw.setDebugCounterService(h.controller.getDebugCounter());
-                    h.sw.setAccessFlowPriority(h.controller.getAccessFlowPriority());
-                    h.sw.setCoreFlowPriority(h.controller.getCoreFlowPriority());
-                    for (OFPortStatus ps: h.pendingPortStatusMsg)
-                        handlePortStatusMessage(h, ps, false);
-                    h.pendingPortStatusMsg.clear();
-                    h.readPropertyFromStorage();
-                    log.info("Switch {} bound to class {}, writeThrottle={}," +
-                            " description {}",
-                             new Object[] { h.sw, h.sw.getClass(),
-                                            h.sw.isWriteThrottleEnabled(),
-                                            description });
+                } catch (CounterException e) {
+                    h.counters.switchCounterRegistrationFailed
+                            .updateCounterNoFlush();
+                    log.warn("Could not register counters for switch {} ",
+                              h.getSwitchInfoString(), e);
                 }
-                catch (Exception ex) {
-                    String msg = getSwitchStateMessage(h, m,
-                            "Exception while reading description during handshake");
-                    throw new SwitchStateException(msg, ex);
-                }
-                // We need to set the new state /before/ we call addSwitchChannel
+                h.sw.setAccessFlowPriority(h.controller.getAccessFlowPriority());
+                h.sw.setCoreFlowPriority(h.controller.getCoreFlowPriority());
+                for (OFPortStatus ps: h.pendingPortStatusMsg)
+                    handlePortStatusMessage(h, ps, false);
+                h.pendingPortStatusMsg.clear();
+                h.readPropertyFromStorage();
+                log.info("Switch {} bound to class {}, writeThrottle={}," +
+                        " description {}",
+                         new Object[] { h.sw, h.sw.getClass(),
+                                        h.sw.isWriteThrottleEnabled(),
+                                    description });
+            // We need to set the new state /before/ we call addSwitchChannel
                 // because addSwitchChannel will eventually call setRole
                 // which can in turn decide that the switch doesn't support
                 // roles and transition the state straight to MASTER.
@@ -1384,11 +1385,19 @@ class OFChannelHandler
         } else if (e.getCause() instanceof IOException) {
             log.error("Disconnecting switch {} due to IO Error: {}",
                       getSwitchInfoString(), e.getCause().getMessage());
+            if (log.isDebugEnabled()) {
+                // still print stack trace if debug is enabled
+                log.debug("StackTrace for previous Exception: ", e.getCause());
+            }
             counters.switchDisconnectIOError.updateCounterWithFlush();
             ctx.getChannel().close();
         } else if (e.getCause() instanceof SwitchStateException) {
             log.error("Disconnecting switch {} due to switch state error: {}",
                       getSwitchInfoString(), e.getCause().getMessage());
+            if (log.isDebugEnabled()) {
+                // still print stack trace if debug is enabled
+                log.debug("StackTrace for previous Exception: ", e.getCause());
+            }
             counters.switchDisconnectSwitchStateException.updateCounterWithFlush();
             ctx.getChannel().close();
         } else if (e.getCause() instanceof MessageParseException) {
@@ -1407,7 +1416,8 @@ class OFChannelHandler
             counters.rejectedExecutionException.updateCounterWithFlush();
         } else {
             log.error("Error while processing message from switch "
-                                 + getSwitchInfoString(), e.getCause());
+                                 + getSwitchInfoString()
+                                 + "state " + this.state, e.getCause());
             counters.switchDisconnectOtherException.updateCounterWithFlush();
             ctx.getChannel().close();
         }

src/main/java/net/floodlightcontroller/core/internal/SwitchStateException.java

 /**
-*    Copyright 2011, Big Switch Networks, Inc. 
+*    Copyright 2011, Big Switch Networks, Inc.
 *    Originally created by David Erickson, Stanford University
-* 
+*
 *    Licensed under the Apache License, Version 2.0 (the "License"); you may
 *    not use this file except in compliance with the License. You may obtain
 *    a copy of the License at
@@ -18,7 +18,10 @@
 package net.floodlightcontroller.core.internal;
 
 /**
- * 
+ * We don't allow wrapping other exception in a switch state exception. We
+ * only log the SwitchStateExceptions message so the casuing exceptions
+ * stack trace is generally not available.
+ *
  */
 public class SwitchStateException extends IllegalArgumentException {
 
@@ -28,10 +31,6 @@ public class SwitchStateException extends IllegalArgumentException {
         super();
     }
 
-    public SwitchStateException(String arg0, Throwable arg1) {
-        super(arg0, arg1);
-    }
-
     public SwitchStateException(String arg0) {
         super(arg0);
     }