BUG-2109 : cleaned BGP sessions on session closed.

Added proper close from BGP peer. Negotiation failed method now called from both sub and superclass.

Change-Id: I83e922df826c7fbb9130ca637d2dfe99d3e93a27
Signed-off-by: Dana Kutenicsova <dkutenic@cisco.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 0192bb9daae7d3fb9800779df2d5b47160a31f4a..e865690bbec291ca4da963b577d12fa9492f6b79 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
@@ -10,14 +10,10 @@ package org.opendaylight.protocol.bgp.rib.impl;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-
 import io.netty.channel.Channel;
 import io.netty.util.concurrent.Promise;
-
 import java.util.concurrent.TimeUnit;
-
 import javax.annotation.concurrent.GuardedBy;
-
 import org.opendaylight.protocol.bgp.parser.BGPDocumentedException;
 import org.opendaylight.protocol.bgp.parser.BGPError;
 import org.opendaylight.protocol.bgp.rib.impl.spi.BGPPeerRegistry;
@@ -94,11 +90,11 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
         Preconditions.checkState(this.state == State.Idle);
 
         // Check if peer is configured in registry before retrieving preferences
-        if (!registry.isPeerConfigured(getRemoteIp())) {
+        if (!this.registry.isPeerConfigured(getRemoteIp())) {
             final BGPDocumentedException cause = new BGPDocumentedException(
                     "BGP peer with ip: " + getRemoteIp()
                     + " not configured, check configured peers in : "
-                    + registry, BGPError.CEASE);
+                    + this.registry, BGPError.CEASE);
             negotiationFailed(cause);
             return;
         }
@@ -112,26 +108,28 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
         }
         this.sendMessage(new OpenBuilder().setMyAsNumber(as).setHoldTimer(preferences.getHoldTime()).setBgpIdentifier(
                 preferences.getBgpId()).setBgpParameters(preferences.getParams()).build());
-        this.state = State.OpenSent;
-
-        this.channel.eventLoop().schedule(new Runnable() {
-            @Override
-            public void run() {
-                if (AbstractBGPSessionNegotiator.this.state != State.Finished) {
-                    AbstractBGPSessionNegotiator.this.sendMessage(buildErrorNotify(BGPError.HOLD_TIMER_EXPIRED));
-                    negotiationFailed(new BGPDocumentedException("HoldTimer expired", BGPError.FSM_ERROR));
-                    AbstractBGPSessionNegotiator.this.state = State.Finished;
+        if (this.state != State.Finished) {
+            this.state = State.OpenSent;
+
+            this.channel.eventLoop().schedule(new Runnable() {
+                @Override
+                public void run() {
+                    if (AbstractBGPSessionNegotiator.this.state != State.Finished) {
+                        AbstractBGPSessionNegotiator.this.sendMessage(buildErrorNotify(BGPError.HOLD_TIMER_EXPIRED));
+                        negotiationFailed(new BGPDocumentedException("HoldTimer expired", BGPError.FSM_ERROR));
+                        AbstractBGPSessionNegotiator.this.state = State.Finished;
+                    }
                 }
-            }
-        }, INITIAL_HOLDTIMER, TimeUnit.MINUTES);
+            }, INITIAL_HOLDTIMER, TimeUnit.MINUTES);
+        }
     }
 
     private BGPSessionPreferences getPreferences() {
-        return registry.getPeerPreferences(getRemoteIp());
+        return this.registry.getPeerPreferences(getRemoteIp());
     }
 
     private IpAddress getRemoteIp() {
-        return StrictBGPPeerRegistry.getIpAddress(channel.remoteAddress());
+        return StrictBGPPeerRegistry.getIpAddress(this.channel.remoteAddress());
     }
 
     @Override
@@ -175,14 +173,14 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
 
     private void handleOpen(final Open openObj) {
         try {
-            sessionValidator.validate(openObj, getPreferences());
+            this.sessionValidator.validate(openObj, getPreferences());
         } catch (final BGPDocumentedException e) {
             negotiationFailed(e);
             return;
         }
 
         try {
-            final BGPSessionListener peer = registry.getPeer(getRemoteIp(), getSourceId(openObj, getPreferences()), getDestinationId(openObj, getPreferences()));
+            final BGPSessionListener peer = this.registry.getPeer(getRemoteIp(), getSourceId(openObj, getPreferences()), getDestinationId(openObj, getPreferences()));
             this.sendMessage(new KeepaliveBuilder().build());
             this.session = new BGPSessionImpl(peer, this.channel, openObj, getPreferences().getHoldTime());
             this.state = State.OpenConfirm;
@@ -193,9 +191,14 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti
         }
     }
 
-    private void negotiationFailed(final BGPDocumentedException e) {
+    @Override
+    protected void negotiationFailed(final Throwable e) {
         LOG.warn("Channel {} negotiation failed: {}", this.channel, e.getMessage());
-        this.sendMessage(buildErrorNotify(e.getError()));
+        if (e instanceof BGPDocumentedException) {
+            // although sendMessage() can also result in calling this method, it won't create a cycle. In case sendMessage() fails to
+            // deliver the message, this method gets called with different exception (definitely not with BGPDocumentedException).
+            this.sendMessage(buildErrorNotify(((BGPDocumentedException)e).getError()));
+        }
         super.negotiationFailed(e);
         this.state = State.Finished;
     }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
index a3949d4d872d64a824f4c534b23ea4f352b6bd86..266a32d2fff2284c0c5665aa5319704dd37c3def 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
@@ -12,12 +12,9 @@ import com.google.common.base.Objects;
 import com.google.common.base.Objects.ToStringHelper;
 import com.google.common.base.Preconditions;
 import com.google.common.net.InetAddresses;
-
 import java.util.HashSet;
 import java.util.Set;
-
 import javax.annotation.concurrent.GuardedBy;
-
 import org.opendaylight.protocol.bgp.rib.impl.spi.AdjRIBsOutRegistration;
 import org.opendaylight.protocol.bgp.rib.impl.spi.RIB;
 import org.opendaylight.protocol.bgp.rib.impl.spi.ReusableBGPPeer;
@@ -97,25 +94,24 @@ public class BGPPeer implements ReusableBGPPeer, Peer, AutoCloseable {
             this.rib.clearTable(this, key);
         }
 
-        if (reg != null) {
-            reg.close();
-            reg = null;
+        if (this.reg != null) {
+            this.reg.close();
+            this.reg = null;
         }
 
         this.tables.clear();
-        this.session = null;
     }
 
     @Override
     public void onSessionDown(final BGPSession session, final Exception e) {
         LOG.info("Session with peer {} went down", this.name, e);
-        cleanup();
+        releaseConnection();
     }
 
     @Override
     public void onSessionTerminated(final BGPSession session, final BGPTerminationReason cause) {
         LOG.info("Session with peer {} terminated: {}", this.name, cause);
-        cleanup();
+        releaseConnection();
     }
 
     @Override