BUG-8021: Race between Peer structure creation and route init.

There is a possibility of race between Peer structure
creation and route initialization/advertizement.

Fix by introducing a flag, which indicates whether
peer's structure has been created and is able to be updated.

Change-Id: Ia9515d2017cb5213ce286e463712092c87764e10
Signed-off-by: Claudio D. Gasparini <claudio.gasparini@pantheon.tech>

diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractRouteEntry.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractRouteEntry.java
index 41533b217b5e8f1f44a1d9bbe0c3c237ce2e0767..48485bd2fdae28462fc9f2b473073955f14a2e43 100644 (file)
--- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractRouteEntry.java
+++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/spi/AbstractRouteEntry.java
@@ -68,11 +68,11 @@ public abstract class AbstractRouteEntry implements RouteEntry {
     }
 
     protected final boolean filterRoutes(final PeerId rootPeer, final PeerId destPeer, final ExportPolicyPeerTracker peerPT,        final TablesKey localTK, final PeerRole destPeerRole) {
-        return !rootPeer.equals(destPeer) && isTableSupported(destPeer, peerPT, localTK) && !PeerRole.Internal.equals(destPeerRole);
+        return !rootPeer.equals(destPeer) && isTableSupportedAndReady(destPeer, peerPT, localTK) && !PeerRole.Internal.equals(destPeerRole);
     }
 
-    private boolean isTableSupported(final PeerId destPeer, final ExportPolicyPeerTracker peerPT, final TablesKey localTK) {
-        if (!peerPT.isTableSupported(destPeer)) {
+    private boolean isTableSupportedAndReady(final PeerId destPeer, final ExportPolicyPeerTracker peerPT, final TablesKey localTK) {
+        if (!peerPT.isTableSupported(destPeer) || !peerPT.isTableStructureInitialized(destPeer)) {
             LOG.trace("Route rejected, peer {} does not support this table type {}", destPeer, localTK);
             return false;
         }

diff --git a/bgp/path-selection-mode/src/test/java/org/opendaylight/protocol/bgp/mode/impl/AbstractRouteEntryTest.java b/bgp/path-selection-mode/src/test/java/org/opendaylight/protocol/bgp/mode/impl/AbstractRouteEntryTest.java
index a6f5f91155cd4009376fd496930eb1899be206f0..3960f61a74450cf4ae9ee6aff916b27c0da159f5 100644 (file)
--- a/bgp/path-selection-mode/src/test/java/org/opendaylight/protocol/bgp/mode/impl/AbstractRouteEntryTest.java
+++ b/bgp/path-selection-mode/src/test/java/org/opendaylight/protocol/bgp/mode/impl/AbstractRouteEntryTest.java
@@ -12,7 +12,6 @@ import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
 import static org.opendaylight.protocol.bgp.mode.impl.base.BasePathSelectorTest.ATTRS_EXTENSION_Q;
 import static org.opendaylight.protocol.bgp.mode.impl.base.BasePathSelectorTest.SEGMENTS_NID;
 
@@ -112,7 +111,6 @@ public abstract class AbstractRouteEntryTest {
     protected void setUp() {
         MockitoAnnotations.initMocks(this);
         this.yIIChanges = new ArrayList<>();
-        this.peerPT = mock(ExportPolicyPeerTracker.class);
         this.attributes = createAttr();
         this.locRibTargetYii = LOC_RIB_TARGET.node(ROUTES_IDENTIFIER);
         this.locRibOutTargetYii = PEER_YII.node(AdjRibOut.QNAME).node(Tables.QNAME).node(RibSupportUtils.toYangTablesKey(TABLES_KEY)).node(ROUTES_IDENTIFIER);
@@ -182,6 +180,7 @@ public abstract class AbstractRouteEntryTest {
     }
 
     private void mockExportPolicies() {
+        doReturn(true).when(this.peerPT).isTableStructureInitialized(any(PeerId.class));
         doReturn(true).when(this.peerPT).isTableSupported(PEER_ID);
         doReturn(false).when(this.peerPT).isTableSupported(PEER_ID2);
         doAnswer(invocation -> {

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTrackerImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTrackerImpl.java
index be0f6a4c32134e6b861b8aa154b18ca491a9cef3..0f567dd02d6b4b651f4860f1c6e96b53888e584f 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTrackerImpl.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTrackerImpl.java
@@ -10,10 +10,8 @@ package org.opendaylight.protocol.bgp.rib.impl;
 import com.google.common.base.Preconditions;
 import java.util.EnumMap;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Set;
 import javax.annotation.concurrent.GuardedBy;
 import javax.annotation.concurrent.ThreadSafe;
 import org.opendaylight.protocol.bgp.rib.impl.spi.PeerExportGroupRegistry;
@@ -30,6 +28,15 @@ import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+/**
+ * There is one ExportPolicyPeerTracker per table
+ *  - peerTables: keep track of registered peers, the ones which support this table.
+ *  - peerTables: flag indicates whether the structure of the peer has been created, and therefore it can start
+ *  to be updated.
+ *  - peerAddPathTables: keeps track of peer which supports Additional Path for this table and which Add Path
+ *  configuration they are using.
+ *  - groups: Contains peers grouped by peerRole and therefore sharing the same export policy.
+ */
 @ThreadSafe
 final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
     private static final Logger LOG = LoggerFactory.getLogger(ExportPolicyPeerTrackerImpl.class);
@@ -38,7 +45,7 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
     @GuardedBy("this")
     private final Map<PeerId, SendReceive> peerAddPathTables = new HashMap<>();
     @GuardedBy("this")
-    private final Set<PeerId> peerTables = new HashSet<>();
+    private final Map<PeerId, Boolean> peerTables = new HashMap<>();
     private final PolicyDatabase policyDatabase;
     private final TablesKey localTableKey;
     @GuardedBy("this")
@@ -54,13 +61,13 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
         final PeerExportGroupRegistry peerExp = this.groups.computeIfAbsent(peerRole,
             k -> new PeerExportGroupImpl(this.policyDatabase.exportPolicyForRole(peerRole)));
 
-        final AbstractRegistration registration =  peerExp.registerPeer(peerId, new PeerExporTuple(peerPath, peerRole));
+        final AbstractRegistration registration = peerExp.registerPeer(peerId, new PeerExporTuple(peerPath, peerRole));
 
         return new AbstractRegistration() {
             @Override
             protected void removeRegistration() {
                 registration.close();
-                if(ExportPolicyPeerTrackerImpl.this.groups.get(peerRole).isEmpty()) {
+                if (ExportPolicyPeerTrackerImpl.this.groups.get(peerRole).isEmpty()) {
                     ExportPolicyPeerTrackerImpl.this.groups.remove(peerRole);
                 }
             }
@@ -77,7 +84,7 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
         }
         final SimpleRoutingPolicy simpleRoutingPolicy = optSimpleRoutingPolicy.orElse(null);
         if (SimpleRoutingPolicy.AnnounceNone != simpleRoutingPolicy) {
-            this.peerTables.add(peerId);
+            this.peerTables.put(peerId, false);
         }
         this.peerRoles.put(peerPath, peerRole);
         LOG.debug("Supported table {} added to peer {} role {}", this.localTableKey, peerId, peerRole);
@@ -108,7 +115,7 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
 
     @Override
     public synchronized boolean isTableSupported(final PeerId peerId) {
-        return this.peerTables.contains(peerId);
+        return this.peerTables.containsKey(peerId);
     }
 
     @Override
@@ -121,4 +128,14 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
         final SendReceive sendReceive = this.peerAddPathTables.get(peerId);
         return sendReceive != null && (sendReceive.equals(SendReceive.Both) || sendReceive.equals(SendReceive.Receive));
     }
+
+    @Override
+    public synchronized void registerPeerAsInitialized(final PeerId peerId) {
+        this.peerTables.computeIfPresent(peerId, (k, v) -> true);
+    }
+
+    @Override
+    public synchronized boolean isTableStructureInitialized(final PeerId peerId) {
+        return this.peerTables.get(peerId);
+    }
 }
\ No newline at end of file

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 6e280fefab30c9b80584ca24a42e15c3f443146e..3fcd3ab39a60a60c327c290ee10d4156d534a610 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
@@ -190,11 +190,13 @@ final class LocRibWriter implements AutoCloseable, TotalPrefixesCounter, TotalPa
     private void initializeTableWithExistentRoutes(final DataTreeCandidateNode table, final PeerId peerIdOfNewPeer, final YangInstanceIdentifier rootPath,
         final DOMDataWriteTransaction tx) {
         if (!table.getDataBefore().isPresent() && this.exportPolicyPeerTracker.isTableSupported(peerIdOfNewPeer)) {
+            this.exportPolicyPeerTracker.registerPeerAsInitialized(peerIdOfNewPeer);
             LOG.debug("Peer {} table has been created, inserting existent routes", peerIdOfNewPeer);
             final PeerRole newPeerRole = this.exportPolicyPeerTracker.getRole(IdentifierUtils.peerPath(rootPath));
             final PeerExportGroup peerGroup = this.exportPolicyPeerTracker.getPeerGroup(newPeerRole);
             this.routeEntries.entrySet().forEach(entry -> entry.getValue().writeRoute(peerIdOfNewPeer, entry.getKey(),
-                rootPath.getParent().getParent().getParent(), peerGroup, this.localTablesKey, this.exportPolicyPeerTracker, this.ribSupport, tx));
+                rootPath.getParent().getParent().getParent(), peerGroup, this.localTablesKey,
+                this.exportPolicyPeerTracker, this.ribSupport, tx));
         }
     }
 

diff --git a/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java b/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java
index 182353573c4641f151d68ef53987ca25c7765e6a..bbbbbdd8a85c821eb297e80787093b9f9bf0db54 100644 (file)
--- a/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java
+++ b/bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/ExportPolicyPeerTracker.java
@@ -64,7 +64,7 @@ public interface ExportPolicyPeerTracker {
 
     /**
      * Check whether Peer supports Add Path
-     * @param peerId
+     * @param peerId of peer
      * @return true if add-path is supported
      */
     boolean isAddPathSupportedByPeer(@Nonnull PeerId peerId);
@@ -78,4 +78,19 @@ public interface ExportPolicyPeerTracker {
     @Deprecated
     default void peerRoleChanged(@Nonnull YangInstanceIdentifier peerPath,  @Nullable PeerRole role) {
     }
+
+    /**
+     * Flags peers once empty structure has been created, then changes under it can
+     * be applied
+     *
+     * @param peerId of peer
+     */
+    void registerPeerAsInitialized(PeerId peerId);
+
+    /**
+     * check whether the peer supports the table
+     * @param peerId of peer
+     * @return true if peer supports table
+     */
+    boolean isTableStructureInitialized(@Nonnull PeerId peerId);
 }