Non ipv4 advertising peer causes BGP session flaps

During the session establishment phase, if a peer
doesn't advertise ipv4-unicast, controller
automatically adds ipv4-unicast as supported
family for the session to support classic BGP
sessions.

Currently, we are adding Ipv4 family in the
BGPPeer but not updating AdjRibInWriter which
causes the BGP session to not recover from
session flap. Patch fixes this issue by adding
ipv4 support before updating AdjRibInWriter.

JIRA: BGPCEP-910
Change-Id: I669104a17d603e0494f9ac7090a8fd671f67a3a5
Signed-off-by: Vikram Singh Kalakoti <vikramskalakoti@gmail.com>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit f54f4bd31f5ceb0a15edacb9b2884a63a21cb52d)

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 937382a0f9a71c5ae1eede7e28d7eff88dd949fd..fd32a434609f8bcbe7128057a4e0d921fb0923a5 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
@@ -378,7 +378,9 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
 
         this.addPathTableMaps = mapTableTypesFamilies(addPathTablesType);
         final boolean restartingLocally = isLocalRestarting();
-
+        if (!restartingLocally) {
+            addBgp4Support();
+        }
         if (!isRestartingGracefully()) {
             this.rawIdentifier = InetAddresses.forString(session.getBgpId().getValue()).getAddress();
             this.peerId = RouterIds.createPeerId(session.getBgpId());
@@ -450,7 +452,9 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
         }
 
         if (!restartingLocally) {
-            addBgp4Support();
+            if (!setTables.contains(IPV4_UCAST_TABLE_KEY)) {
+                createAdjRibOutListener(IPV4_UCAST_TABLE_KEY, false);
+            }
             for (final TablesKey key : getAfiSafisAdvertized()) {
                 createAdjRibOutListener(key, true);
             }
@@ -482,7 +486,6 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
             final HashSet<TablesKey> newSet = new HashSet<>(this.tables);
             newSet.add(IPV4_UCAST_TABLE_KEY);
             this.tables = ImmutableSet.copyOf(newSet);
-            createAdjRibOutListener(IPV4_UCAST_TABLE_KEY, false);
         }
     }
 

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/GracefulRestartTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/GracefulRestartTest.java
index ea0f8d14dde3366a8eacece9040bbdfd76063ef6..98c04ece84f98f718d99ce481ccc9d155edb4c75 100644 (file)
--- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/GracefulRestartTest.java
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/GracefulRestartTest.java
@@ -9,6 +9,7 @@ package org.opendaylight.protocol.bgp.rib.impl;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.opendaylight.protocol.bgp.rib.impl.CheckUtil.checkIdleState;
 import static org.opendaylight.protocol.bgp.rib.impl.CheckUtil.checkStateIsNotRestarting;
@@ -74,7 +75,9 @@ import org.opendaylight.yangtools.yang.common.Uint8;
 public class GracefulRestartTest extends AbstractAddPathTest {
 
     private BGPSessionImpl session;
+    private BGPSessionImpl sessionv6;
     private BGPPeer peer;
+    private BGPPeer nonIpv4;
     private final Set<TablesKey> afiSafiAdvertised = new HashSet<>();
     private final Set<TablesKey> gracefulAfiSafiAdvertised = new HashSet<>();
     private RIBImpl ribImpl;
@@ -140,6 +143,28 @@ public class GracefulRestartTest extends AbstractAddPathTest {
         super.tearDown();
     }
 
+    /**
+     * Test graceful restart of a non-IPv4 peer.
+     * {@link BGPPeer#releaseConnection(boolean)} should not throw NPE and binding chain should be closed
+     * successfully.
+     *
+     * @throws InterruptedException on create peer session.
+     */
+    @Test
+    public void nonIpv4PeerGracefulRestart() throws InterruptedException {
+        BgpParameters parametersv6 = PeerUtil.createBgpParameters(Collections.singletonList(IPV6_TABLES_KEY),
+                Collections.emptyList(), Collections.singletonMap(IPV6_TABLES_KEY, true),
+                GRACEFUL_RESTART_TIME);
+        gracefulAfiSafiAdvertised.add(IPV6_TABLES_KEY);
+        this.nonIpv4 = configurePeer(this.tableRegistry, PEER2, this.ribImpl, parametersv6, PeerRole.Ibgp,
+                this.serverRegistry, afiSafiAdvertised, gracefulAfiSafiAdvertised);
+        this.sessionv6 = createPeerSession(PEER2, parametersv6, this.listener);
+        final Open open = createClassicOpen(true);
+        this.sessionv6.writeAndFlush(open);
+        checkIdleState(this.nonIpv4);
+        assertNull(this.nonIpv4.bindingChain);
+    }
+
     /**
      * Test correct behavior when connection restart is unnoticed.
      * "Correct" means that the previous TCP session MUST be closed, and the new one retained.