From d5e8364cf33421d7103d5146bfcc4d36c308fc75 Mon Sep 17 00:00:00 2001 From: Rob Adams <rob.adams@bigswitch.com> Date: Wed, 19 Sep 2012 18:06:22 -0700 Subject: [PATCH] Add LogMessageDoc annotation and some initial work at documenting the log messages in floodlight --- .../core/annotations/LogMessageCategory.java | 34 ++++ .../core/annotations/LogMessageDoc.java | 63 +++++++ .../core/annotations/LogMessageDocs.java | 36 ++++ .../core/internal/Controller.java | 159 +++++++++++++++++- .../core/internal/OFSwitchImpl.java | 6 +- .../core/internal/RoleChanger.java | 22 ++- .../TerminationStorageExceptionHandler.java | 40 ----- .../internal/DeviceManagerImpl.java | 5 +- 8 files changed, 314 insertions(+), 51 deletions(-) create mode 100644 src/main/java/net/floodlightcontroller/core/annotations/LogMessageCategory.java create mode 100644 src/main/java/net/floodlightcontroller/core/annotations/LogMessageDoc.java create mode 100644 src/main/java/net/floodlightcontroller/core/annotations/LogMessageDocs.java delete mode 100644 src/main/java/net/floodlightcontroller/core/internal/TerminationStorageExceptionHandler.java diff --git a/src/main/java/net/floodlightcontroller/core/annotations/LogMessageCategory.java b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageCategory.java new file mode 100644 index 000000000..e9abf02a2 --- /dev/null +++ b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageCategory.java @@ -0,0 +1,34 @@ +/** +* Copyright 2012, 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 +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +* License for the specific language governing permissions and limitations +* under the License. +**/ + +package net.floodlightcontroller.core.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** + * Annotation used to set the category for log messages for a class + * @author readams + */ +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface LogMessageCategory { + /** + * The category for the log messages for this class + * @return + */ + String value() default "Core"; +} diff --git a/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDoc.java b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDoc.java new file mode 100644 index 000000000..e26218688 --- /dev/null +++ b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDoc.java @@ -0,0 +1,63 @@ +/** +* Copyright 2012, 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 +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +* License for the specific language governing permissions and limitations +* under the License. +**/ + +package net.floodlightcontroller.core.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** + * Annotation used to document log messages. This can be used to generate + * documentation on syslog output. + * @author readams + */ +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface LogMessageDoc { + public static final String NO_ACTION = "No action is required."; + public static final String UNKNOWN_ERROR = "An unknown error occured"; + public static final String GENERIC_ACTION = + "Examine the returned error or exception and take " + + "appropriate action."; + public static final String CHECK_SWITCH = + "Check the health of the indicated switch. " + + "Test and troubleshoot IP connectivity."; + public static final String CHECK_CONTROLLER = + "Verify controller system health, CPU usage, and memory. " + + "Rebooting the controller node may help if the controller " + + "node is in a distressed state."; + + /** + * The log level for the log message + * @return the log level as a tring + */ + String level() default "INFO"; + /** + * The message that will be printed + * @return the message + */ + String message() default UNKNOWN_ERROR; + /** + * An explanation of the meaning of the log message + * @return the explanation + */ + String explanation() default UNKNOWN_ERROR; + /** + * The recommendated action associated with the log message + * @return the recommendation + */ + String recommendation() default NO_ACTION; +} diff --git a/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDocs.java b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDocs.java new file mode 100644 index 000000000..2e9b7c6f4 --- /dev/null +++ b/src/main/java/net/floodlightcontroller/core/annotations/LogMessageDocs.java @@ -0,0 +1,36 @@ +/** +* Copyright 2012, 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 +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +* License for the specific language governing permissions and limitations +* under the License. +**/ + +package net.floodlightcontroller.core.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Target; + +/** + * Annotation used to document log messages. This can be used to generate + * documentation on syslog output. This version allows multiple log messages + * to be documentated on an interface. + * @author readams + */ +@Target(ElementType.METHOD) +public @interface LogMessageDocs { + /** + * A list of {@link LogMessageDoc} elements + * @return the list of log message doc + */ + LogMessageDoc[] value(); +} diff --git a/src/main/java/net/floodlightcontroller/core/internal/Controller.java b/src/main/java/net/floodlightcontroller/core/internal/Controller.java index a29f16a4a..58d3f965f 100644 --- a/src/main/java/net/floodlightcontroller/core/internal/Controller.java +++ b/src/main/java/net/floodlightcontroller/core/internal/Controller.java @@ -57,6 +57,8 @@ import net.floodlightcontroller.core.IListener.Command; import net.floodlightcontroller.core.IOFSwitch; import net.floodlightcontroller.core.IOFSwitchFilter; import net.floodlightcontroller.core.IOFSwitchListener; +import net.floodlightcontroller.core.annotations.LogMessageDoc; +import net.floodlightcontroller.core.annotations.LogMessageDocs; import net.floodlightcontroller.core.internal.OFChannelState.HandshakeState; import net.floodlightcontroller.core.util.ListenerDispatcher; import net.floodlightcontroller.core.web.CoreWebRoutable; @@ -135,6 +137,9 @@ public class Controller implements IFloodlightProviderService, IStorageSourceListener { protected static Logger log = LoggerFactory.getLogger(Controller.class); + + private static final String ERROR_DATABASE = + "The controller could not communicate with the system database."; protected BasicFactory factory; protected ConcurrentMap<OFType, @@ -424,6 +429,9 @@ public class Controller implements IFloodlightProviderService, } @Override + @LogMessageDoc(message="New switch connection from {ip address}", + explanation="A new switch has connected from the " + + "specified IP address") public void channelConnected(ChannelHandlerContext ctx, ChannelStateEvent e) throws Exception { log.info("New switch connection from {}", @@ -441,6 +449,8 @@ public class Controller implements IFloodlightProviderService, } @Override + @LogMessageDoc(message="Disconnected switch {switch information}", + explanation="The specified switch has disconnected.") public void channelDisconnected(ChannelHandlerContext ctx, ChannelStateEvent e) throws Exception { if (sw != null && state.hsState == HandshakeState.READY) { @@ -459,6 +469,47 @@ public class Controller implements IFloodlightProviderService, } @Override + @LogMessageDocs({ + @LogMessageDoc(level="ERROR", + message="Disconnecting switch {switch} due to read timeout", + explanation="The connected switch has failed to send any " + + "messages or respond to echo requests", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="ERROR", + message="Disconnecting switch {switch}: failed to " + + "complete handshake", + explanation="The switch did not respond correctly " + + "to handshake messages", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="ERROR", + message="Disconnecting switch {switch} due to IO Error: {}", + explanation="There was an error communicating with the switch", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="ERROR", + message="Disconnecting switch {switch} due to switch " + + "state error: {error}", + explanation="The switch sent an unexpected message", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="ERROR", + message="Disconnecting switch {switch} due to " + + "message parse failure", + explanation="Could not parse a message from the switch", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="ERROR", + message="Terminating controller due to storage exception", + explanation=ERROR_DATABASE, + recommendation=LogMessageDoc.CHECK_CONTROLLER), + @LogMessageDoc(level="ERROR", + message="Could not process message: queue full", + explanation="OpenFlow messages are arriving faster than " + + " the controller can process them.", + recommendation=LogMessageDoc.CHECK_CONTROLLER), + @LogMessageDoc(level="ERROR", + message="Error while processing message " + + "from switch {switch} {cause}", + explanation="An error occurred processing the switch message", + recommendation=LogMessageDoc.GENERIC_ACTION), + }) public void exceptionCaught(ChannelHandlerContext ctx, ExceptionEvent e) throws Exception { if (e.getCause() instanceof ReadTimeoutException) { @@ -532,6 +583,11 @@ public class Controller implements IFloodlightProviderService, /** * Process the request for the switch description */ + @LogMessageDoc(level="ERROR", + message="Exception in reading description " + + " during handshake {exception}", + explanation="Could not process the switch description string", + recommendation=LogMessageDoc.CHECK_SWITCH) void processSwitchDescReply() { try { // Read description, if it has been updated @@ -600,7 +656,7 @@ public class Controller implements IFloodlightProviderService, // Ignore } catch (Exception ex) { log.error("Exception in reading description " + - " during handshake - {}", ex); + " during handshake", ex); } } @@ -698,6 +754,11 @@ public class Controller implements IFloodlightProviderService, * removedSwitch notification):1 * */ + @LogMessageDoc(level="ERROR", + message="Invalid role value in role reply message", + explanation="Was unable to set the HA role (master or slave) " + + "for the controller.", + recommendation=LogMessageDoc.CHECK_CONTROLLER) protected void handleRoleReplyMessage(OFVendor vendorMessage, OFRoleReplyVendorData roleReplyVendorData) { // Map from the role code in the message to our role enum @@ -818,6 +879,23 @@ public class Controller implements IFloodlightProviderService, * @throws IOException * @throws SwitchStateException */ + @LogMessageDocs({ + @LogMessageDoc(level="WARN", + message="Config Reply from {switch} has " + + "miss length set to {length}", + explanation="The controller requires that the switch " + + "use a miss length of 0xffff for correct " + + "function", + recommendation="Use a different switch to ensure " + + "correct function"), + @LogMessageDoc(level="WARN", + message="Received ERROR from sw {switch} that " + +"indicates roles are not supported " + +"but we have received a valid " + +"role reply earlier", + explanation="The switch sent a confusing message to the" + + "controller"), + }) protected void processOFMessage(OFMessage m) throws IOException, SwitchStateException { boolean shouldHandleMessage = false; @@ -874,7 +952,7 @@ public class Controller implements IFloodlightProviderService, } else { log.warn("Config Reply from {} has " + "miss length set to {}", - sw, cr.getMissSendLength()); + sw, cr.getMissSendLength() & 0xffff); } state.hasGetConfigReply = true; checkSwitchReady(); @@ -1105,6 +1183,18 @@ public class Controller implements IFloodlightProviderService, * be allocated in this function * @throws IOException */ + @LogMessageDocs({ + @LogMessageDoc(level="ERROR", + message="Ignoring PacketIn (Xid = {xid}) because the data" + + " field is empty.", + explanation="The switch sent an improperly-formatted PacketIn" + + " message", + recommendation=LogMessageDoc.CHECK_SWITCH), + @LogMessageDoc(level="WARN", + message="Unhandled OF Message: {} from {}", + explanation="The switch sent a message not handled by " + + "the controller"), + }) protected void handleMessage(IOFSwitch sw, OFMessage m, FloodlightContext bContext) throws IOException { @@ -1175,7 +1265,7 @@ public class Controller implements IFloodlightProviderService, } pktinProcTime.recordEndTimePktIn(sw, m, bc); } else { - log.error("Unhandled OF Message: {} from {}", m, sw); + log.warn("Unhandled OF Message: {} from {}", m, sw); } if ((bContext == null) && (bc != null)) flcontext_free(bc); @@ -1187,6 +1277,14 @@ public class Controller implements IFloodlightProviderService, * @param sw The switch that sent the error * @param error The error message */ + @LogMessageDoc(level="ERROR", + message="Error {error type} {error code} from {switch}", + explanation="The switch responded with an unexpected error" + + "to an OpenFlow message from the controller", + recommendation="This could indicate improper network operation. " + + "If the problem persists restarting the switch and " + + "controller may help." + ) protected void logError(IOFSwitch sw, OFError error) { int etint = 0xffff & error.getErrorType(); if (etint < 0 || etint >= OFErrorType.values().length) { @@ -1238,6 +1336,19 @@ public class Controller implements IFloodlightProviderService, */ // TODO: need to rethink locking and the synchronous switch update. // We can / should also handle duplicate DPIDs in connectedSwitches + @LogMessageDoc(level="ERROR", + message="New switch added {switch} for already-added switch {switch}", + explanation="A switch with the same DPID as another switch " + + "connected to the controller. This can be caused by " + + "multiple switches configured with the same DPID, or " + + "by a switch reconnected very quickly after " + + "disconnecting.", + recommendation="If this happens repeatedly, it is likely there " + + "are switches with duplicate DPIDs on the network. " + + "Reconfigure the appropriate switches. If it happens " + + "very rarely, then it is likely this is a transient " + + "network problem that can be ignored." + ) protected void addSwitch(IOFSwitch sw) { // TODO: is it safe to modify the HashMap without holding // the old switch's lock? @@ -1412,6 +1523,17 @@ public class Controller implements IFloodlightProviderService, } @Override + @LogMessageDocs({ + @LogMessageDoc(message="Failed to inject OFMessage {message} onto " + + "a null switch", + explanation="Failed to process a message because the switch " + + " is no longer connected."), + @LogMessageDoc(level="ERROR", + message="Error reinjecting OFMessage on switch {switch}", + explanation="An I/O error occured while attempting to " + + "process an OpenFlow message", + recommendation=LogMessageDoc.CHECK_SWITCH) + }) public boolean injectOfMessage(IOFSwitch sw, OFMessage msg, FloodlightContext bc) { if (sw == null) { @@ -1442,6 +1564,8 @@ public class Controller implements IFloodlightProviderService, } @Override + @LogMessageDoc(message="Calling System.exit", + explanation="The controller is terminating") public synchronized void terminate() { log.info("Calling System.exit"); System.exit(1); @@ -1721,6 +1845,15 @@ public class Controller implements IFloodlightProviderService, * @return A valid role if role information is specified in the * config params, otherwise null */ + @LogMessageDocs({ + @LogMessageDoc(message="Controller role set to {role}", + explanation="Setting the initial HA role to "), + @LogMessageDoc(level="ERROR", + message="Invalid current role value: {role}", + explanation="An invalid HA role value was read from the " + + "properties file", + recommendation=LogMessageDoc.CHECK_CONTROLLER) + }) protected Role getInitialRole(Map<String, String> configParams) { Role role = null; String roleString = configParams.get("role"); @@ -1753,7 +1886,7 @@ public class Controller implements IFloodlightProviderService, } } - log.info("Controller roles set to {}", role); + log.info("Controller role set to {}", role); return role; } @@ -1762,6 +1895,19 @@ public class Controller implements IFloodlightProviderService, * Tell controller that we're ready to accept switches loop * @throws IOException */ + @LogMessageDocs({ + @LogMessageDoc(message="Listening for switch connections on {address}", + explanation="The controller is ready and listening for new" + + " switch connections"), + @LogMessageDoc(message="Storage exception in controller " + + "updates loop; terminating process", + explanation=ERROR_DATABASE, + recommendation=LogMessageDoc.CHECK_CONTROLLER), + @LogMessageDoc(level="ERROR", + message="Exception in controller updates loop", + explanation="Failed to dispatch controller event", + recommendation=LogMessageDoc.GENERIC_ACTION) + }) public void run() { if (log.isDebugEnabled()) { logListeners(); @@ -1882,6 +2028,11 @@ public class Controller implements IFloodlightProviderService, /** * Startup all of the controller's components */ + @LogMessageDoc(message="Waiting for storage source", + explanation="The system database is not yet ready", + recommendation="If this message persists, this indicates " + + "that the system database has failed to start. " + + LogMessageDoc.CHECK_CONTROLLER) public void startupComponents() { // Create the table names we use storageSource.createTable(CONTROLLER_TABLE_NAME, null); diff --git a/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java index 58395cf1f..3f42059d9 100644 --- a/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java +++ b/src/main/java/net/floodlightcontroller/core/internal/OFSwitchImpl.java @@ -193,9 +193,9 @@ public class OFSwitchImpl implements IOFSwitch { // Warn if programming a flow matching broadcast destination if ((match.getWildcards() & OFMatch.OFPFW_DL_DST) == 0 && Arrays.equals(match.getDataLayerDestination(), bcast)) { - log.warn("Programming flow with -1 destination addr"); - log.warn("swId {}, stack trace {}", - stringId, new Exception().getStackTrace()); + log.warn("Programming flow with -1 destination addr: " + + "swId {}, stack trace {}", + stringId, new Exception().getStackTrace()); } } Map<OFSwitchImpl,List<OFMessage>> msg_buffer_map = local_msg_buffer.get(); diff --git a/src/main/java/net/floodlightcontroller/core/internal/RoleChanger.java b/src/main/java/net/floodlightcontroller/core/internal/RoleChanger.java index 673790748..fbbd80ae7 100644 --- a/src/main/java/net/floodlightcontroller/core/internal/RoleChanger.java +++ b/src/main/java/net/floodlightcontroller/core/internal/RoleChanger.java @@ -13,6 +13,7 @@ import org.slf4j.LoggerFactory; import net.floodlightcontroller.core.IFloodlightProviderService.Role; import net.floodlightcontroller.core.IOFSwitch; +import net.floodlightcontroller.core.annotations.LogMessageDoc; /** * This class handles sending of RoleRequest messages to all connected switches. @@ -155,6 +156,11 @@ public class RoleChanger { } } + @LogMessageDoc(level="ERROR", + message="RoleRequestWorker task had an uncaught exception.", + explanation="An unknown occured while processing an HA " + + "role change event.", + recommendation=LogMessageDoc.GENERIC_ACTION) protected class RoleRequestWorker extends Thread { @Override public void run() { @@ -184,8 +190,8 @@ public class RoleChanger { } catch (Exception e) { // Should never get here - log.error("RoleRequestWorker task had an uncaught exception. " - + "Task is terminating. {}", e); + log.error("RoleRequestWorker task had an uncaught exception. ", + e); } finally { // Be nice in case we earlier caught InterruptedExecution @@ -225,6 +231,12 @@ public class RoleChanger { * @param switches the collection of switches to send the request too * @param role the role to request */ + @LogMessageDoc(level="WARN", + message="Failed to send role request message " + + "to switch {switch}: {message}. Disconnecting", + explanation="An I/O error occurred while attempting to change " + + "the switch HA role.", + recommendation=LogMessageDoc.CHECK_SWITCH) protected void sendRoleRequest(Collection<OFSwitchImpl> switches, Role role, long cookie) { // There are three cases to consider: @@ -290,6 +302,12 @@ public class RoleChanger { * @param switches the collection of switches to send the request too * @param cookie the cookie of the request */ + @LogMessageDoc(level="WARN", + message="Timeout while waiting for role reply from switch {switch}." + + " Disconnecting", + explanation="Timed out waiting for the switch to respond to " + + "a request to change the HA role.", + recommendation=LogMessageDoc.CHECK_SWITCH) protected void verifyRoleReplyReceived(Collection<OFSwitchImpl> switches, long cookie) { for (OFSwitchImpl sw: switches) { diff --git a/src/main/java/net/floodlightcontroller/core/internal/TerminationStorageExceptionHandler.java b/src/main/java/net/floodlightcontroller/core/internal/TerminationStorageExceptionHandler.java deleted file mode 100644 index 9b27ae1db..000000000 --- a/src/main/java/net/floodlightcontroller/core/internal/TerminationStorageExceptionHandler.java +++ /dev/null @@ -1,40 +0,0 @@ -/** -* 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 -* -* http://www.apache.org/licenses/LICENSE-2.0 -* -* Unless required by applicable law or agreed to in writing, software -* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -* License for the specific language governing permissions and limitations -* under the License. -**/ - -package net.floodlightcontroller.core.internal; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -import net.floodlightcontroller.core.IFloodlightProviderService; -import net.floodlightcontroller.storage.IStorageExceptionHandler; - -public class TerminationStorageExceptionHandler implements IStorageExceptionHandler { - protected static Logger log = LoggerFactory.getLogger(TerminationStorageExceptionHandler.class); - - private IFloodlightProviderService floodlightProvider; - - TerminationStorageExceptionHandler(IFloodlightProviderService floodlightProvider) { - this.floodlightProvider = floodlightProvider; - } - - @Override - public void handleException(Exception exc) { - log.error("Storage exception while asynchronous storage operation; terminating process", exc); - floodlightProvider.terminate(); - } -} diff --git a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java index c6f32a47d..c1c16e3dc 100755 --- a/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java +++ b/src/main/java/net/floodlightcontroller/devicemanager/internal/DeviceManagerImpl.java @@ -1518,8 +1518,9 @@ IFlowReconcileListener, IInfoProvider, IHAListener { device.getDeviceKey(), emptyToKeep); } if (!deviceMap.remove(device.getDeviceKey(), device)) { - logger.info("device map does not have this device -" + - device.toString()); + if (logger.isDebugEnabled()) + logger.debug("device map does not have this device -" + + device.toString()); } } -- GitLab