Fix NPE while accessing DomTxChain when bgp/app peer singleton service is restarted

Due to cluster partition/heal, singleton service associated with bgp/app
peer instance can get restarted. Create/close of DomTxChain instance used
by the service was not being handled properly resulting in NPE and failure
to register the peer which caused the BGP connection to not get reestablished.

JIRA: BGPCEP-878
Change-Id: I436ad0877db19c65463bf7b7e09faa3b2b42e5a0
Signed-off-by: Ajay Lele <ajayslele@gmail.com>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
index 561490ae54d4739e1ec29477dbffa1eb42381e50..b85ec93456d58b812dc617667f178da3c4f29d37 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
@@ -105,7 +105,7 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
         this.clusterId = clusterId;
         this.localAs = localAs;
         this.rib = rib;
-        this.domChain = this.rib.createPeerDOMChain(this);
+        createDomChain();
     }
 
     AbstractPeer(
@@ -503,6 +503,13 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
         }
     }
 
+    final synchronized void createDomChain() {
+        if (this.domChain == null) {
+            LOG.info("Creating DOM peer chain {}", getPeerId());
+            this.domChain = this.rib.createPeerDOMChain(this);
+        }
+    }
+
     final synchronized void closeDomChain() {
         if (this.domChain != null) {
             LOG.info("Closing DOM peer chain {}", getPeerId());

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
index a07686dde88009e19ea8352a9f79ce3b7d56449a..b3ee1750b69269cc9d76c47f5f8976f97f33e12f 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
@@ -149,6 +149,7 @@ public class ApplicationPeer extends AbstractPeer implements ClusteredDOMDataTre
         localTables.forEach(tablesKey -> this.supportedTables.add(RibSupportUtils.toYangTablesKey(tablesKey)));
         setAdvertizedGracefulRestartTableTypes(Collections.emptyList());
 
+        createDomChain();
         this.adjRibInWriter = AdjRibInWriter.create(this.rib.getYangRibId(), PeerRole.Internal, this);
         final RIBSupportContextRegistry context = this.rib.getRibSupportContext();
         final RegisterAppPeerListener registerAppPeerListener = () -> {

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 146d69be54bf986891bf60fa73bd72e224afa29b..a6f2142f85aa62ab04f800f18a2c6af7eb719ba4 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
@@ -235,6 +235,7 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
     }
 
     public synchronized void instantiateServiceInstance() {
+        createDomChain();
         this.ribWriter = AdjRibInWriter.create(this.rib.getYangRibId(), this.peerRole, this);
         setActive(true);
     }

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
index 4dd599dfad4d2b2a78c0db5ee6acbbb8d45286c1..425023b96ccb4bf5efcc9917547f1c2cd2a3467f 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeer.java
@@ -154,6 +154,9 @@ public class BgpPeer implements PeerBean, BGPPeerStateConsumer {
 
     @Override
     public void close() {
+        if (this.bgpPeerSingletonService != null) {
+            this.bgpPeerSingletonService = null;
+        }
         if (this.serviceRegistration != null) {
             this.serviceRegistration.unregister();
             this.serviceRegistration = null;
@@ -170,9 +173,7 @@ public class BgpPeer implements PeerBean, BGPPeerStateConsumer {
     @Override
     public synchronized FluentFuture<? extends CommitInfo> closeServiceInstance() {
         if (this.bgpPeerSingletonService != null) {
-            final FluentFuture<? extends CommitInfo> fut = this.bgpPeerSingletonService.closeServiceInstance();
-            this.bgpPeerSingletonService = null;
-            return fut;
+            return this.bgpPeerSingletonService.closeServiceInstance();
         }
         return CommitInfo.emptyFluentFuture();
     }

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeerTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeerTest.java
index d4a0b365b5f62ae11b263caebf0d9208f953e6f7..4999c1e3a1a9be80fbb5641575ba65ab9c7a7b1c 100644 (file)
--- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeerTest.java
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/AppPeerTest.java
@@ -10,11 +10,13 @@ package org.opendaylight.protocol.bgp.rib.impl.config;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.internal.verification.VerificationModeFactory.times;
 
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mockito;
+import org.opendaylight.mdsal.dom.api.DOMTransactionChainListener;
 import org.opendaylight.yang.gen.v1.http.openconfig.net.yang.bgp.rev151009.bgp.neighbor.group.ConfigBuilder;
 import org.opendaylight.yang.gen.v1.http.openconfig.net.yang.bgp.rev151009.bgp.neighbors.Neighbor;
 import org.opendaylight.yang.gen.v1.http.openconfig.net.yang.bgp.rev151009.bgp.neighbors.NeighborBuilder;
@@ -42,27 +44,37 @@ public class AppPeerTest extends AbstractConfig {
         APP_PEER.start(this.rib, this.neighbor, null, this.peerGroupLoader, this.tableTypeRegistry);
         Mockito.verify(this.rib).getYangRibId();
         Mockito.verify(this.rib).getService();
+        Mockito.verify(this.rib).createPeerDOMChain(any(DOMTransactionChainListener.class));
         Mockito.verify(this.rib, times(1)).getLocalTablesKeys();
 
         APP_PEER.instantiateServiceInstance();
         Mockito.verify(this.rib, times(3)).getYangRibId();
         Mockito.verify(this.rib, times(2)).getRibSupportContext();
         Mockito.verify(this.rib, times(2)).getLocalTablesKeys();
+        Mockito.verify(this.rib, times(2)).createPeerDOMChain(any(DOMTransactionChainListener.class));
         Mockito.verify(this.domTx).newWriteOnlyTransaction();
 
         APP_PEER.closeServiceInstance();
+        Mockito.verify(this.domTx, times(2)).close();
         APP_PEER.close();
 
         APP_PEER.restart(this.rib, null, this.peerGroupLoader, this.tableTypeRegistry);
         APP_PEER.instantiateServiceInstance();
         Mockito.verify(this.rib, times(6)).getYangRibId();
         Mockito.verify(this.rib, times(4)).getService();
+        Mockito.verify(this.rib, times(4)).createPeerDOMChain(any(DOMTransactionChainListener.class));
         Mockito.verify(this.listener, times(2)).close();
 
         assertTrue(APP_PEER.containsEqualConfiguration(this.neighbor));
         assertFalse(APP_PEER.containsEqualConfiguration(new NeighborBuilder()
                 .setNeighborAddress(new IpAddress(new Ipv4Address("127.0.0.2"))).build()));
         APP_PEER.closeServiceInstance();
+        Mockito.verify(this.domTx, times(4)).close();
+
+        APP_PEER.instantiateServiceInstance();
+        Mockito.verify(this.rib, times(6)).createPeerDOMChain(any(DOMTransactionChainListener.class));
+        APP_PEER.closeServiceInstance();
+        Mockito.verify(this.domTx, times(6)).close();
         APP_PEER.close();
     }
-}
\ No newline at end of file
+}

diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeerTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeerTest.java
index aa658e008ddee6e81f8c1230de1ad71854d88743..44cc622c96c04c3f5d53c56244674cd99f73e5d3 100644 (file)
--- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeerTest.java
+++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/config/BgpPeerTest.java
@@ -154,8 +154,10 @@ public class BgpPeerTest extends AbstractConfig {
         }
         this.bgpPeer.setServiceRegistration(this.serviceRegistration);
         this.bgpPeer.closeServiceInstance();
-        this.bgpPeer.close();
+        verify(this.bgpPeerRegistry).removePeer(any());
         verify(this.future).cancel(true);
+        this.bgpPeer.close();
+        verify(this.serviceRegistration).unregister();
 
         this.bgpPeer.restart(this.rib, null, this.peerGroupLoader, this.tableTypeRegistry);
         this.bgpPeer.instantiateServiceInstance();
@@ -170,9 +172,19 @@ public class BgpPeerTest extends AbstractConfig {
         verify(this.bgpPeerRegistry).removePeer(any(IpAddress.class));
 
         this.bgpPeer.closeServiceInstance();
+        verify(this.bgpPeerRegistry, times(2)).removePeer(any());
+        verify(this.future, times(2)).cancel(true);
+
+        this.bgpPeer.instantiateServiceInstance();
+        verify(this.bgpPeerRegistry, times(3)).addPeer(any(), any(), any());
+        verify(this.dispatcher, times(3)).createReconnectingClient(any(InetSocketAddress.class),
+                any(), anyInt(), any(KeyMapping.class));
+
+        this.bgpPeer.closeServiceInstance();
+        verify(this.bgpPeerRegistry, times(3)).removePeer(any());
+        verify(this.future, times(3)).cancel(true);
         this.bgpPeer.close();
         verify(this.serviceRegistration).unregister();
-        verify(this.future, times(2)).cancel(true);
 
         final Neighbor neighborDiffConfig = new NeighborBuilder().setNeighborAddress(NEIGHBOR_ADDRESS)
                 .setAfiSafis(createAfiSafi()).build();
@@ -180,4 +192,4 @@ public class BgpPeerTest extends AbstractConfig {
         assertTrue(this.bgpPeer.containsEqualConfiguration(neighborDiffConfig));
         this.bgpPeer.close();
     }
-}
\ No newline at end of file
+}