Propagate only negotiated afi/safi routes to adj-rib-out

When BGP session comes, empty entries for negotiated afi/safi are
created in peer's adj-rib-out. If routes belonging to afi/safi
that were not negotiated for the peer are present in loc-rib,
those entries are being propagated to the peer, resulting in
ModifiedNodeDoesNotExistException. Patch prevents this by allowing
routes for only the negotiated afi/safi to be propagated from
loc-rib to peer's adj-rib-out.

If routes are written to non-existent afi/safi table in adj-rib-out,
it results in transaction chain failure which leads to a scenario
where multiple threads block to get a lock on BGPPeer but the
thread handling transaction failure is waiting for the submitted
futures to exit. Patch fixes this scenario by preventing wait on
the submtited futures when handling transaction failures.

JIRA: BGPCEP-906
Change-Id: I836d1828c3d552e4d62be0688040490ec3f36912
Signed-off-by: Vikram Singh Kalakoti <vikramskalakoti@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 de24180685b3280734454ae51f4296b63b74e2b0..e4939f5722e04cea6a0ca6a9b9ed9d49f1dcd01f 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
@@ -484,12 +484,14 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
         tx.delete(LogicalDatastoreType.OPERATIONAL, ribOutTarget);
     }
 
-    final synchronized void releaseBindingChain() {
-        if (this.submitted != null) {
-            try {
-                this.submitted.get();
-            } catch (final InterruptedException | ExecutionException throwable) {
-                LOG.error("Write routes failed", throwable);
+    final synchronized void releaseBindingChain(final boolean isWaitForSubmitted) {
+        if (isWaitForSubmitted) {
+            if (this.submitted != null) {
+                try {
+                    this.submitted.get();
+                } catch (final InterruptedException | ExecutionException throwable) {
+                    LOG.error("Write routes failed", throwable);
+                }
             }
         }
         closeBindingChain();

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 344805f3843dca3dfeb76291ced4788474aa3247..62763fa44bd8510f15987e7375fe1b30ccb136f4 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
@@ -35,6 +35,7 @@ import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.checkerframework.checker.lock.qual.Holding;
+import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.mdsal.binding.api.RpcProviderService;
 import org.opendaylight.mdsal.binding.api.Transaction;
 import org.opendaylight.mdsal.binding.api.TransactionChain;
@@ -242,7 +243,7 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
 
     @Override
     public synchronized FluentFuture<? extends CommitInfo> close() {
-        final FluentFuture<? extends CommitInfo> future = releaseConnection();
+        final FluentFuture<? extends CommitInfo> future = releaseConnection(true);
         closeDomChain();
         setActive(false);
         return future;
@@ -522,6 +523,16 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
 
     @Override
     public synchronized FluentFuture<? extends CommitInfo> releaseConnection() {
+        return releaseConnection(true);
+    }
+
+    /**
+     * On transaction chain failure, we don't want to wait for future.
+     *
+     * @param isWaitForSubmitted if true, wait for submitted future before closing binding chain. if false, don't wait.
+     */
+    @Holding("this")
+    private @NonNull FluentFuture<? extends CommitInfo> releaseConnection(final boolean isWaitForSubmitted) {
         LOG.info("Closing session with peer");
         this.sessionUp = false;
         this.adjRibOutListenerSet.values().forEach(AdjRibOutListener::close);
@@ -538,7 +549,7 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
                 handleRestartTimer();
             }
         }
-        releaseBindingChain();
+        releaseBindingChain(isWaitForSubmitted);
 
         closeSession();
         return future;
@@ -623,11 +634,12 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
             TimeUnit.NANOSECONDS);
     }
 
+    @Holding("this")
     private void releaseConnectionGracefully() {
         if (getPeerRestartTime() > 0) {
             setRestartingState();
         }
-        releaseConnection();
+        releaseConnection(true);
     }
 
     @SuppressFBWarnings("IS2_INCONSISTENT_SYNC")
@@ -638,7 +650,7 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
 
     @Override
     public boolean supportsTable(final TablesKey tableKey) {
-        return this.sessionUp && getAfiSafisAdvertized().contains(tableKey);
+        return this.sessionUp && getAfiSafisAdvertized().contains(tableKey) && this.tables.contains(tableKey);
     }
 
     @Override
@@ -650,14 +662,14 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
     public synchronized void onTransactionChainFailed(final DOMTransactionChain chain,
             final DOMDataTreeTransaction transaction, final Throwable cause) {
         LOG.error("Transaction domChain failed.", cause);
-        releaseConnection();
+        releaseConnection(true);
     }
 
     @Override
     public synchronized void onTransactionChainFailed(final TransactionChain chain, final Transaction transaction,
             final Throwable cause) {
         LOG.error("Transaction domChain failed.", cause);
-        releaseConnection();
+        releaseConnection(false);
     }
 
     @Override
@@ -705,7 +717,7 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
         setGracefulPreferences(true, tablesToPreserve);
         this.currentSelectionDeferralTimerSeconds = selectionDeferralTimerSeconds;
         setLocalRestartingState(true);
-        return releaseConnection();
+        return releaseConnection(true);
     }
 
     @Override

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java
index acea1a7f152ce50de11e31472e0368c6d82af790..0eae50954775e643dacd05241b24e089529b0015 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java
@@ -341,8 +341,9 @@ final class LocRibWriter<C extends Routes & DataObject & ChoiceIn<Tables>, S ext
             newRoutes.addAll(entry.newBestPaths(this.ribSupport, e.getKey().getRouteId()));
         }
         updateLocRib(newRoutes, staleRoutes, tx);
-        this.peerTracker.getNonInternalPeers().parallelStream().forEach(
-            toPeer -> toPeer.refreshRibOut(this.entryDep, staleRoutes, newRoutes));
+        this.peerTracker.getNonInternalPeers().parallelStream()
+                .filter(toPeer -> toPeer.supportsTable(this.entryDep.getLocalTablesKey()))
+                .forEach(toPeer -> toPeer.refreshRibOut(this.entryDep, staleRoutes, newRoutes));
     }
 
     private void updateLocRib(final List<AdvertizedRoute<C, S, R, I>> newRoutes,

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java
index ec29fd0b05e8505e7dd603cc0530f5d97c23d49d..63535a1c6c54d7f6970c3e922782f5c65bdeeb36 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/state/BGPPeerStateImpl.java
@@ -16,6 +16,7 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.LongAdder;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.eclipse.jdt.annotation.NonNull;
@@ -58,8 +59,7 @@ public abstract class BGPPeerStateImpl extends DefaultRibReference implements BG
     private final LongAdder notificationReceivedCounter = new LongAdder();
     private final LongAdder erroneousUpdate = new LongAdder();
     private final String groupId;
-    @GuardedBy("this")
-    private boolean active;
+    private AtomicBoolean active = new AtomicBoolean(false);
 
     @GuardedBy("this")
     private final Map<TablesKey, PrefixesSentCounters> prefixesSent = new HashMap<>();
@@ -275,12 +275,12 @@ public abstract class BGPPeerStateImpl extends DefaultRibReference implements BG
     }
 
     @Override
-    public final synchronized boolean isActive() {
-        return this.active;
+    public final boolean isActive() {
+        return this.active.get();
     }
 
-    protected final synchronized void setActive(final boolean active) {
-        this.active = active;
+    protected final void setActive(final boolean active) {
+        this.active.set(active);
     }
 
     @Override