From ccb7e5bda0598185f98d52ddd16e49ae4d48e5aa Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sun, 17 May 2015 00:34:50 +0200 Subject: [PATCH] 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 --- .../DeviceRequestFailedException.java | 35 ++++++++++++++++ .../api/connection/OutboundQueue.java | 3 ++ .../connection/ConnectionAdapterImpl.java | 10 ++--- .../core/connection/OutboundQueueImpl.java | 42 ++++++++++++++----- 4 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 openflow-protocol-api/src/main/java/org/opendaylight/openflowjava/protocol/api/connection/DeviceRequestFailedException.java 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 index 00000000..a353886d --- /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 1291c22c..06b66c5e 100644 --- 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 6d9013ec..91f4ceb1 100644 --- 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 f133082f..e8e2307c 100644 --- 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; } -- 2.36.6