Bug-6662: On connection reset by peer, sometimes re-connection attempt stops after...

This issue happens when the neighbor config is removed from peer router while retaining the BGP router config.
Under this scenario, since BGP router config is still present, router is listening on BGP port. So when ODL
tries to connect, the connection succeeds. However the BGP negotiation fails as neighbor config has been removed
from router.

BGPReconnectPromise.ClosedChannelHandler#channelInactive triggers the reconnect, but the actual connection
retries as per the connection stragegy are done from BGPProtocolSessionPromise.BootstrapConnectListener#operationComplete

In failure scenario, since connection succeeds but negotiation fails, failure is not detected in BGPProtocolSessionPromise.BootstrapConnectListener#operationComplete
and so no reconnection attempts are made.

When negotiation fails, AbstractBGPSessionNegotiator#negotiationFailedCloseChannel is called, which causes BGPReconnectPromise.ClosedChannelHandler#channelInactive
to get called. Fix involves detecting negotiation failure here, and calling BGPProtocolSessionPromise#reconnect.

The scheduled job in AbstractBGPSessionNegotiator to detect hold-timer expiry during negotiation is also being cancelled
from AbstractBGPSessionNegotiator#negotiationFailedCloseChannel as the job is not needed when channel is closed.

Change-Id: Ie4aecd1f99312476abd75f008c0a3ee4ce1d44a6
Signed-off-by: Ajay <ajayl.bro@gmail.com>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
index 9c723e237a56fb7790ab936a10f8b5a5378f81af..420e27e816e2d72bca18942107041fe4fc6d2b6b 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java
@@ -16,6 +16,7 @@ import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.util.concurrent.Promise;
+import io.netty.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.concurrent.GuardedBy;
 import org.opendaylight.protocol.bgp.parser.BGPDocumentedException;
@@ -80,6 +81,8 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
     private State state = State.IDLE;
     @GuardedBy("this")
     private BGPSessionImpl session;
+    @GuardedBy("this")
+    private ScheduledFuture<?> pending;
 
     AbstractBGPSessionNegotiator(final Promise<BGPSessionImpl> promise, final Channel channel,
             final BGPPeerRegistry registry) {
@@ -114,10 +117,11 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
                 preferences.getBgpId()).setBgpParameters(preferences.getParams()).build());
             if (this.state != State.FINISHED) {
                 this.state = State.OPEN_SENT;
-                this.channel.eventLoop().schedule(new Runnable() {
+                this.pending = this.channel.eventLoop().schedule(new Runnable() {
                     @Override
                     public void run() {
                         synchronized (AbstractBGPSessionNegotiator.this) {
+                            AbstractBGPSessionNegotiator.this.pending = null;
                             if (AbstractBGPSessionNegotiator.this.state != State.FINISHED) {
                                 AbstractBGPSessionNegotiator.this
                                     .sendMessage(buildErrorNotify(BGPError.HOLD_TIMER_EXPIRED));
@@ -253,7 +257,12 @@ abstract class AbstractBGPSessionNegotiator extends ChannelInboundHandlerAdapter
     private void negotiationFailedCloseChannel(final Throwable cause) {
         LOG.debug("Negotiation on channel {} failed", this.channel, cause);
         this.channel.close();
-        this.promise.setFailure(cause);
+        synchronized (AbstractBGPSessionNegotiator.this) {
+            if (this.pending != null && this.pending.isCancellable()) {
+                this.pending.cancel(true);
+                this.pending = null;
+            }
+        }
     }
 
     private void sendMessage(final Notification msg) {

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java
old mode 100755 (executable)
new mode 100644 (file)
index ea21d13..64c943c
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPProtocolSessionPromise.java
@@ -32,7 +32,7 @@ public class BGPProtocolSessionPromise<S extends BGPSession> extends DefaultProm
     private final int retryTimer;
     private final Bootstrap bootstrap;
     @GuardedBy("this")
-    private Future<?> pending;
+    private ChannelFuture pending;
 
     public BGPProtocolSessionPromise(InetSocketAddress remoteAddress, int retryTimer, Bootstrap bootstrap) {
         super(GlobalEventExecutor.INSTANCE);
@@ -61,6 +61,27 @@ public class BGPProtocolSessionPromise<S extends BGPSession> extends DefaultProm
         }
     }
 
+    public synchronized void reconnect() {
+        if (this.retryTimer == 0) {
+            LOG.debug("Retry timer value is 0. Reconnection will not be attempted");
+            this.setFailure(this.pending.cause());
+            return;
+        }
+
+        final BGPProtocolSessionPromise lock = this;
+        final EventLoop loop = this.pending.channel().eventLoop();
+        loop.schedule(new Runnable() {
+            @Override
+            public void run() {
+                LOG.debug("Attempting to connect to {}", BGPProtocolSessionPromise.this.address);
+                final ChannelFuture reconnectFuture = BGPProtocolSessionPromise.this.bootstrap.connect();
+                reconnectFuture.addListener(new BootstrapConnectListener(lock));
+                BGPProtocolSessionPromise.this.pending = reconnectFuture;
+            }
+        }, this.retryTimer, TimeUnit.SECONDS);
+        LOG.debug("Next reconnection attempt in {}s", this.retryTimer);
+    }
+
     @Override
     public synchronized boolean cancel(final boolean mayInterruptIfRunning) {
         if (super.cancel(mayInterruptIfRunning)) {
@@ -98,24 +119,7 @@ public class BGPProtocolSessionPromise<S extends BGPSession> extends DefaultProm
                     BGPProtocolSessionPromise.LOG.debug("Promise {} connection successful", this.lock);
                 } else {
                     BGPProtocolSessionPromise.LOG.debug("Attempt to connect to {} failed", BGPProtocolSessionPromise.this.address, channelFuture.cause());
-
-                    if (BGPProtocolSessionPromise.this.retryTimer == 0) {
-                        BGPProtocolSessionPromise.LOG.debug("Retry timer value is 0. Reconnection will not be attempted");
-                        BGPProtocolSessionPromise.this.setFailure(channelFuture.cause());
-                        return;
-                    }
-
-                    final EventLoop loop = channelFuture.channel().eventLoop();
-                    loop.schedule(new Runnable() {
-                        @Override
-                        public void run() {
-                            BGPProtocolSessionPromise.LOG.debug("Attempting to connect to {}", BGPProtocolSessionPromise.this.address);
-                            final Future reconnectFuture = BGPProtocolSessionPromise.this.bootstrap.connect();
-                            reconnectFuture.addListener(BGPProtocolSessionPromise.BootstrapConnectListener.this);
-                            BGPProtocolSessionPromise.this.pending = reconnectFuture;
-                        }
-                    }, BGPProtocolSessionPromise.this.retryTimer, TimeUnit.SECONDS);
-                    BGPProtocolSessionPromise.LOG.debug("Next reconnection attempt in {}s", BGPProtocolSessionPromise.this.retryTimer);
+                    BGPProtocolSessionPromise.this.reconnect();
                 }
             }
         }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPReconnectPromise.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPReconnectPromise.java
old mode 100755 (executable)
new mode 100644 (file)
index 1de327f..4123940
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPReconnectPromise.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/protocol/BGPReconnectPromise.java
@@ -32,7 +32,7 @@ public class BGPReconnectPromise<S extends BGPSession> extends DefaultPromise<Vo
     private final int retryTimer;
     private final Bootstrap bootstrap;
     private final ChannelPipelineInitializer initializer;
-    private Future<S> pending;
+    private BGPProtocolSessionPromise<S> pending;
 
     public BGPReconnectPromise(final EventExecutor executor, final InetSocketAddress address,
                                final int retryTimer, final Bootstrap bootstrap,
@@ -68,7 +68,7 @@ public class BGPReconnectPromise<S extends BGPSession> extends DefaultPromise<Vo
         });
     }
 
-    public Future<S> connectSessionPromise(final InetSocketAddress address, final int retryTimer, final Bootstrap bootstrap,
+    public BGPProtocolSessionPromise<S> connectSessionPromise(final InetSocketAddress address, final int retryTimer, final Bootstrap bootstrap,
                                   final ChannelPipelineInitializer initializer) {
         final BGPProtocolSessionPromise sessionPromise = new BGPProtocolSessionPromise(address, retryTimer, bootstrap);
         final ChannelHandler chInit = new ChannelInitializer<SocketChannel>() {
@@ -121,7 +121,8 @@ public class BGPReconnectPromise<S extends BGPSession> extends DefaultPromise<Vo
             }
 
             if (!this.promise.isInitialConnectFinished()) {
-                LOG.debug("Connection to {} was dropped during negotiation", this.promise.address);
+                LOG.debug("Connection to {} was dropped during negotiation, reattempting", this.promise.address);
+                this.promise.pending.reconnect();
                 return;
             }