BUG-3219: Fix OutboundQueue request error reporting

Original patch failed to describe how errors are reported when they
arrive in an ErrorMessage or are generated by the local system.

The only error path interface is the onFailure() method in the callback
the user specifies. This fits naturally, as we now define
DeviceRequestFailedException to carry the encapsulated message and pass
it to the callback.

Change-Id: Ie8eb62558991ceb7f6d5eb8d7cd547aecaf63f19
Signed-off-by: Robert Varga <rovarga@cisco.com>

diff --git a/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/DeviceRequestFailedException.java b/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/DeviceRequestFailedException.java
new file mode 100644 (file)
index 0000000..a353886
--- /dev/null
+++ b/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/DeviceRequestFailedException.java
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2015 Pantheon Technologies s.r.o. and others. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.openflowjava.protocol.api.connection;
+
+import com.google.common.base.Preconditions;
+import javax.annotation.Nonnull;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.Error;
+
+/**
+ * Exception which is used to report that a particular request failed on the
+ * remote device (switch).
+ */
+public class DeviceRequestFailedException extends Exception {
+    private static final long serialVersionUID = 1L;
+    private final Error error;
+
+    public DeviceRequestFailedException(final String message, @Nonnull final Error error) {
+        super(message);
+        this.error = Preconditions.checkNotNull(error);
+    }
+
+    public DeviceRequestFailedException(final String message, @Nonnull final Error error, final Throwable cause) {
+        super(message, cause);
+        this.error = Preconditions.checkNotNull(error);
+    }
+
+    @Nonnull public Error getError() {
+        return error;
+    }
+}

diff --git a/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/OutboundQueue.java b/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/OutboundQueue.java
index 1291c22c706c11a39f516b51e1ca639dc4f6c09d..06b66c5e046f320537f8dcddefb42db981a10d86 100644 (file)
--- a/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/OutboundQueue.java
+++ b/openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/OutboundQueue.java
@@ -30,6 +30,9 @@ public interface OutboundQueue {
      * with a response, the object reported will be non-null. If the request's completion
      * is implied by a barrier, the object reported will be null.
      *
+     * If this request fails on the remote device, {@link FutureCallback#onFailure(Throwable)
+     * will be called with an instance of {@link DeviceRequestFailedException}.
+     *
      * @param xid Previously-reserved XID
      * @param message Message which should be sent out, or null if the reservation
      *                should be cancelled.

diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/ConnectionAdapterImpl.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/ConnectionAdapterImpl.java
index 6d9013ec893e20b5a5a49306073d7ff10da44e16..91f4ceb1f9dd4854c95b4f2b34c359f16c019731 100644 (file)
--- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/ConnectionAdapterImpl.java
+++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/ConnectionAdapterImpl.java
@@ -283,16 +283,16 @@ public class ConnectionAdapterImpl implements ConnectionFacade {
                 messageListener.onEchoRequestMessage((EchoRequestMessage) message);
                 statisticsCounters.incrementCounter(CounterEventTypes.US_MESSAGE_PASS);
             } else if (message instanceof ErrorMessage) {
-                messageListener.onErrorMessage((ErrorMessage) message);
-                if (outputManager != null) {
-                    outputManager.onMessage((OfHeader) message);
+                // Send only unmatched errors
+                if (outputManager == null || !outputManager.onMessage((OfHeader) message)) {
+                    messageListener.onErrorMessage((ErrorMessage) message);
                 }
                 statisticsCounters.incrementCounter(CounterEventTypes.US_MESSAGE_PASS);
             } else if (message instanceof ExperimenterMessage) {
-                messageListener.onExperimenterMessage((ExperimenterMessage) message);
                 if (outputManager != null) {
                     outputManager.onMessage((OfHeader) message);
                 }
+                messageListener.onExperimenterMessage((ExperimenterMessage) message);
                 statisticsCounters.incrementCounter(CounterEventTypes.US_MESSAGE_PASS);
             } else if (message instanceof FlowRemovedMessage) {
                 messageListener.onFlowRemovedMessage((FlowRemovedMessage) message);
@@ -302,10 +302,10 @@ public class ConnectionAdapterImpl implements ConnectionFacade {
                 messageListener.onHelloMessage((HelloMessage) message);
                 statisticsCounters.incrementCounter(CounterEventTypes.US_MESSAGE_PASS);
             } else if (message instanceof MultipartReplyMessage) {
-                messageListener.onMultipartReplyMessage((MultipartReplyMessage) message);
                 if (outputManager != null) {
                     outputManager.onMessage((OfHeader) message);
                 }
+                messageListener.onMultipartReplyMessage((MultipartReplyMessage) message);
                 statisticsCounters.incrementCounter(CounterEventTypes.US_MESSAGE_PASS);
             } else if (message instanceof PacketInMessage) {
                 messageListener.onPacketInMessage((PacketInMessage) message);

diff --git a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/OutboundQueueImpl.java b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/OutboundQueueImpl.java
index f133082ff76e41ff8455dcc5a607ef4b7bf7c15b..e8e2307ca2be3108053d94cba24013377b7e1989 100644 (file)
--- a/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/OutboundQueueImpl.java
+++ b/openflow-protocol-impl/src/main/java/org/opendaylight/openflowjava/protocol/impl/core/connection/OutboundQueueImpl.java
@@ -8,10 +8,13 @@
 package org.opendaylight.openflowjava.protocol.impl.core.connection;
 
 import com.google.common.base.Preconditions;
+import com.google.common.base.Verify;
 import com.google.common.util.concurrent.FutureCallback;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
 import javax.annotation.Nonnull;
 import org.opendaylight.openflowjava.protocol.api.connection.OutboundQueue;
+import org.opendaylight.openflowjava.protocol.api.connection.DeviceRequestFailedException;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.Error;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.openflow.protocol.rev130731.OfHeader;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -199,10 +202,22 @@ final class OutboundQueueImpl implements OutboundQueue {
         }
     }
 
-    private boolean xidInRance(final long xid) {
+    // Argument is 'long' to explicitly convert before performing operations
+    private boolean xidInRange(final long xid) {
         return xid < endXid && (xid >= baseXid || baseXid > endXid);
     }
 
+    private static boolean completeEntry(final OutboundQueueEntry entry, final OfHeader response) {
+        if (response instanceof Error) {
+            final Error err = (Error)response;
+            LOG.debug("Device-reported request XID {} failed {}:{}", response.getXid(), err.getTypeString(), err.getCodeString());
+            entry.fail(new DeviceRequestFailedException("Device-side failure", err));
+            return true;
+        } else {
+            return entry.complete(response);
+        }
+    }
+
     /**
      * Return the request entry corresponding to a response. Returns null
      * if there is no request matching the response.
@@ -212,7 +227,7 @@ final class OutboundQueueImpl implements OutboundQueue {
      */
     OutboundQueueEntry pairRequest(@Nonnull final OfHeader response) {
         final Long xid = response.getXid();
-        if (!xidInRance(xid)) {
+        if (!xidInRange(xid)) {
             LOG.debug("Queue {} {}/{} ignoring XID {}", this, baseXid, queue.length, xid);
             return null;
         }
@@ -224,16 +239,23 @@ final class OutboundQueueImpl implements OutboundQueue {
             return null;
         }
 
-        if (entry.complete(response)) {
+        if (entry.isBarrier()) {
+            // This has been a barrier -- make sure we complete all preceding requests.
+            // XXX: Barriers are expected to complete in one message.
+            //      If this assumption is changed, this logic will need to be expanded
+            //      to ensure that the requests implied by the barrier are reported as
+            //      completed *after* the barrier.
+            LOG.trace("Barrier XID {} completed, cascading completion to XIDs {} to {}", xid, baseXid + lastBarrierOffset + 1, xid - 1);
+            completeRequests(offset);
+            lastBarrierOffset = offset;
+
+            final boolean success = completeEntry(entry, response);
+            Verify.verify(success, "Barrier request failed to complete");
+            completeCount++;
+        } else if (completeEntry(entry, response)) {
             completeCount++;
-
-            // This has been a barrier -- make sure we complete all preceding requests
-            if (entry.isBarrier()) {
-                LOG.debug("Barrier XID {} completed, cascading completion to XIDs {} to {}", xid, baseXid + lastBarrierOffset + 1, xid - 1);
-                completeRequests(offset);
-                lastBarrierOffset = offset;
-            }
         }
+
         return entry;
     }