BUG-7976: Race between peer removal and routes update 10/53810/7
authorClaudio D. Gasparini <claudio.gasparini@pantheon.tech>
Fri, 24 Mar 2017 19:26:08 +0000 (20:26 +0100)
committerRobert Varga <nite@hq.sk>
Tue, 28 Mar 2017 18:37:36 +0000 (18:37 +0000)
There is a race condition when we are advertizing routes,
and a session peer goes down which will end in
removing the peer from DS.
Since advertizement and removal are done by 2 different tx,
we need to do it blocking mode, and not route should be
advertized until peer is removed and vice versa, peer
should not be removed until routes are advertized.
Otherwise toure update will try to update a peer/path which
is not longer present.
Fix by rework PeerExportGroup & ExportPolicyPeerTracker,
now instead of generate PeerExportGroup each time a peer
is reg/unreg,  we make PeerExportGroup work as a
blocking registry to solve previous issue.

Change-Id: I46a27871bfa2aa2a632e3bfb76061105c62bb6d2
Signed-off-by: Claudio D. Gasparini <claudio.gasparini@pantheon.tech>
bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/AddPathAbstractRouteEntry.java
bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/base/BaseAbstractRouteEntry.java
bgp/path-selection-mode/src/test/java/org/opendaylight/protocol/bgp/mode/impl/AbstractRouteEntryTest.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ExportPolicyPeerTrackerImpl.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/PeerExportGroupImpl.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/PeerExportGroupRegistry.java [new file with mode: 0644]
bgp/rib-spi/src/main/java/org/opendaylight/protocol/bgp/rib/spi/PeerExportGroup.java

index 012d196d40695e7e3829ecbadaadedd5ff208d6a..6fad0735593706cc5a4fe41a9f49fec0523d9cb8 100644 (file)
@@ -195,19 +195,18 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry {
             final PeerExportGroup peerGroup = peerPT.getPeerGroup(role);
             if (peerGroup != null) {
                 final ContainerNode effectiveAttributes = peerGroup.effectiveAttributes(getRoutePeerIdRole(peerPT, routePeerId), attributes);
-                for (final Map.Entry<PeerId, PeerExporTuple> pid : peerGroup.getPeers()) {
-                    final PeerId destPeer = pid.getKey();
+                peerGroup.forEach((destPeer, rootPath) -> {
                     final boolean destPeerSupAddPath = peerPT.isAddPathSupportedByPeer(destPeer);
                     if (filterRoutes(routePeerId, destPeer, peerPT, localTK, role) &&
                         peersSupportsAddPathOrIsFirstBestPath(destPeerSupAddPath, isFirstBestPath)) {
                         if (destPeerSupAddPath) {
-                            update(destPeer, getAdjRibOutYII(ribSup, pid.getValue().getYii(), routeIdAddPath, localTK), effectiveAttributes,
+                            update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeIdAddPath, localTK), effectiveAttributes,
                                 addPathValue, ribSup, tx);
                         } else if(!this.oldNonAddPathBestPathTheSame){
-                            update(destPeer, getAdjRibOutYII(ribSup, pid.getValue().getYii(), routeId, localTK), effectiveAttributes, value, ribSup, tx);
+                            update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeId, localTK), effectiveAttributes, value, ribSup, tx);
                         }
                     }
-                }
+                });
             }
         }
     }
index 395e51cb4854781044c90d951e8338eb59893577..3f87a4d6afc0a9ecec6b6a180fa13e030a2db1af 100644 (file)
@@ -163,8 +163,8 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry {
 
     @VisibleForTesting
     private void fillAdjRibsOut(final ContainerNode attributes, final NormalizedNode<?, ?> value,
-        final PathArgument routeId, final PeerId routePeerId, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final RIBSupport ribSup,
-        final DOMDataWriteTransaction tx) {
+        final PathArgument routeId, final PeerId routePeerId, final ExportPolicyPeerTracker peerPT,
+        final TablesKey localTK, final RIBSupport ribSup, final DOMDataWriteTransaction tx) {
         /*
          * We need to keep track of routers and populate adj-ribs-out, too. If we do not, we need to
          * expose from which client a particular route was learned from in the local RIB, and have
@@ -177,10 +177,14 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry {
         for (final PeerRole role : PeerRole.values()) {
             final PeerExportGroup peerGroup = peerPT.getPeerGroup(role);
             if (peerGroup != null) {
-                final ContainerNode effAttrib = peerGroup.effectiveAttributes(getRoutePeerIdRole(peerPT, routePeerId), attributes);
-                peerGroup.getPeers().stream()
-                    .filter(pid -> filterRoutes(routePeerId, pid.getKey(), peerPT, localTK, getRoutePeerIdRole(peerPT, pid.getKey())))
-                    .forEach(pid -> update(pid.getKey(), getAdjRibOutYII(ribSup, pid.getValue().getYii(), routeId, localTK), effAttrib, value, ribSup, tx));
+                final ContainerNode effAttrib = peerGroup.effectiveAttributes(getRoutePeerIdRole(peerPT, routePeerId),
+                    attributes);
+                peerGroup.forEach((destPeer, rootPath) -> {
+                    if (!filterRoutes(routePeerId, destPeer, peerPT, localTK, getRoutePeerIdRole(peerPT, destPeer))) {
+                        return;
+                    }
+                    update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeId, localTK), effAttrib, value, ribSup, tx);
+                });
             }
         }
     }
index ae78af67554472c573ebc22b0f27a4b231a81a63..a6f5f91155cd4009376fd496930eb1899be206f0 100644 (file)
@@ -8,23 +8,28 @@
 
 package org.opendaylight.protocol.bgp.mode.impl;
 
+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;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.primitives.UnsignedInteger;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.function.BiConsumer;
 import org.mockito.Mock;
-import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
 import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType;
 import org.opendaylight.controller.md.sal.dom.api.DOMDataWriteTransaction;
 import org.opendaylight.protocol.bgp.rib.spi.ExportPolicyPeerTracker;
 import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup;
+import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup.PeerExporTuple;
 import org.opendaylight.protocol.bgp.rib.spi.RIBSupport;
 import org.opendaylight.protocol.bgp.rib.spi.RibSupportUtils;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.inet.rev150305.ipv4.routes.Ipv4Routes;
@@ -54,7 +59,7 @@ import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
 import org.opendaylight.yangtools.yang.data.impl.schema.Builders;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.impl.ImmutableContainerNodeBuilder;
 
-public class AbstractRouteEntryTest {
+public abstract class AbstractRouteEntryTest {
     protected static final long REMOTE_PATH_ID = 1;
     protected static final PeerId PEER_ID = new PeerId("bgp://42.42.42.42");
     protected static final YangInstanceIdentifier PEER_YII2 = YangInstanceIdentifier.of(QName.create("urn:opendaylight:params:xml:ns:yang:bgp-inet:test", "2015-03-05", "peer2"));
@@ -65,7 +70,6 @@ public class AbstractRouteEntryTest {
         .node(LocRib.QNAME).node(Tables.QNAME).node(RibSupportUtils.toYangTablesKey(TABLES_KEY)).getPathArguments());
     private static final long PATH_ID = 1;
     private static final PeerId PEER_ID2 = new PeerId("bgp://43.43.43.43");
-    private static final PeerId PEER_DISCONNECTED = new PeerId("bgp://44.44.44.44");
     private static final String PREFIX = "1.2.3.4/32";
     private static final String PREFIX2 = "2.2.2.2/32";
     private static final YangInstanceIdentifier PEER_YII = YangInstanceIdentifier.of(QName.create("urn:opendaylight:params:xml:ns:yang:bgp-inet:test", "2015-03-05", "peer1"));
@@ -108,7 +112,7 @@ public class AbstractRouteEntryTest {
     protected void setUp() {
         MockitoAnnotations.initMocks(this);
         this.yIIChanges = new ArrayList<>();
-        this.peerPT = Mockito.mock(ExportPolicyPeerTracker.class);
+        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);
@@ -129,89 +133,100 @@ public class AbstractRouteEntryTest {
     }
 
     private void mockTransactionChain() {
-        Mockito.doAnswer(invocation -> {
+        doAnswer(invocation -> {
             final Object[] args = invocation.getArguments();
-            yIIChanges.add((YangInstanceIdentifier) args[1]);
+            this.yIIChanges.add((YangInstanceIdentifier) args[1]);
             return args[1];
-        }).when(this.tx).put(Mockito.any(LogicalDatastoreType.class), Mockito.any(YangInstanceIdentifier.class), Mockito.any(NormalizedNode.class));
+        }).when(this.tx).put(any(LogicalDatastoreType.class), any(YangInstanceIdentifier.class), any(NormalizedNode.class));
 
-        Mockito.doAnswer(invocation -> {
+        doAnswer(invocation -> {
             final Object[] args = invocation.getArguments();
-            if (routePaYii.equals(args[1])) {
-                yIIChanges.remove(routePaYii);
-            } else if (routePaAddPathYii.equals(args[1])) {
-                yIIChanges.remove(routePaAddPathYii);
-            } else if (routeRiboutYii.equals(args[1])) {
-                yIIChanges.remove(routeRiboutYii);
-                yIIChanges.remove(routeAddRiboutAttYii);
-            } else if (routeAddRiboutYii.equals(args[1])) {
-                yIIChanges.remove(routeAddRiboutYii);
-                yIIChanges.remove(routeAddRiboutAttYii);
+            if (this.routePaYii.equals(args[1])) {
+                this.yIIChanges.remove(this.routePaYii);
+            } else if (this.routePaAddPathYii.equals(args[1])) {
+                this.yIIChanges.remove(this.routePaAddPathYii);
+            } else if (this.routeRiboutYii.equals(args[1])) {
+                this.yIIChanges.remove(this.routeRiboutYii);
+                this.yIIChanges.remove(this.routeAddRiboutAttYii);
+            } else if (this.routeAddRiboutYii.equals(args[1])) {
+                this.yIIChanges.remove(this.routeAddRiboutYii);
+                this.yIIChanges.remove(this.routeAddRiboutAttYii);
             }
             return args[1];
-        }).when(this.tx).delete(Mockito.any(LogicalDatastoreType.class), Mockito.any(YangInstanceIdentifier.class));
+        }).when(this.tx).delete(any(LogicalDatastoreType.class), any(YangInstanceIdentifier.class));
     }
 
     private void mockExportGroup() {
-        Mockito.doReturn(this.attributes).when(this.peg).effectiveAttributes(Mockito.any(PeerRole.class), Mockito.any(ContainerNode.class));
-        Mockito.doReturn(null).when(this.pegNot).effectiveAttributes(Mockito.any(PeerRole.class), Mockito.any(ContainerNode.class));
-
-        Map<PeerId, PeerExportGroup.PeerExporTuple> peers = new HashMap<>();
-        Mockito.doReturn(ImmutableList.copyOf(peers.entrySet())).when(this.pegNot).getPeers();
-        Mockito.doReturn(true).when(this.pegNot).containsPeer(Mockito.any(PeerId.class));
+        doReturn(this.attributes).when(this.peg).effectiveAttributes(any(PeerRole.class), any(ContainerNode.class));
+        doReturn(null).when(this.pegNot).effectiveAttributes(any(PeerRole.class), any(ContainerNode.class));
+
+        final Map<PeerId, PeerExportGroup.PeerExporTuple> peers = new HashMap<>();
+        doAnswer(invocation -> {
+            final BiConsumer action = (BiConsumer) invocation.getArguments()[0];
+            for (final Map.Entry<PeerId, PeerExporTuple> pid : peers.entrySet()) {
+                action.accept(pid.getKey(), pid.getValue().getYii());
+            }
+            return null;
+        }).when(this.pegNot).forEach(any());
+        doReturn(true).when(this.pegNot).containsPeer(any(PeerId.class));
 
         peers.put(PEER_ID, new PeerExportGroup.PeerExporTuple(PEER_YII, PeerRole.Ibgp));
         peers.put(PEER_ID2, new PeerExportGroup.PeerExporTuple(PEER_YII2, PeerRole.Ibgp));
-
-        Mockito.doReturn(ImmutableList.copyOf(peers.entrySet())).when(this.peg).getPeers();
+        doAnswer(invocation -> {
+            final BiConsumer action = (BiConsumer) invocation.getArguments()[0];
+            for (final Map.Entry<PeerId, PeerExporTuple> pid : peers.entrySet()) {
+                action.accept(pid.getKey(), pid.getValue().getYii());
+            }
+            return null;
+        }).when(this.peg).forEach(any());
     }
 
     private void mockExportPolicies() {
-        Mockito.doReturn(true).when(this.peerPT).isTableSupported(PEER_ID);
-        Mockito.doReturn(false).when(this.peerPT).isTableSupported(PEER_ID2);
-        Mockito.doAnswer(invocation -> {
+        doReturn(true).when(this.peerPT).isTableSupported(PEER_ID);
+        doReturn(false).when(this.peerPT).isTableSupported(PEER_ID2);
+        doAnswer(invocation -> {
             final Object[] args = invocation.getArguments();
             if (PeerRole.Ibgp.equals(args[0])) {
-                return peg;
+                return this.peg;
             } else if (PeerRole.Ebgp.equals(args[0])) {
-                return pegNot;
+                return this.pegNot;
             } else {
                 return null;
             }
-        }).when(this.peerPT).getPeerGroup(Mockito.any(PeerRole.class));
+        }).when(this.peerPT).getPeerGroup(any(PeerRole.class));
 
-        Mockito.doReturn(true).when(this.peerPT).isAddPathSupportedByPeer(PEER_ID);
-        Mockito.doReturn(false).when(this.peerPT).isAddPathSupportedByPeer(PEER_ID2);
+        doReturn(true).when(this.peerPT).isAddPathSupportedByPeer(PEER_ID);
+        doReturn(false).when(this.peerPT).isAddPathSupportedByPeer(PEER_ID2);
     }
 
     private void mockRibSupport() {
-        Mockito.doReturn(ROUTE_ATTRIBUTES_IDENTIFIER).when(this.ribSupport).routeAttributesIdentifier();
-        Mockito.doReturn(ROUTE_ID_PA_ADD_PATH).when(this.ribSupport).getRouteIdAddPath(Mockito.any(Long.class), Mockito.eq(ROUTE_ID_PA_ADD_PATH));
-        Mockito.doReturn(null).when(this.ribSupport).getRouteIdAddPath(Mockito.any(Long.class), Mockito.eq(ROUTE_ID_PA));
-        Mockito.doAnswer(invocation -> {
+        doReturn(ROUTE_ATTRIBUTES_IDENTIFIER).when(this.ribSupport).routeAttributesIdentifier();
+        doReturn(ROUTE_ID_PA_ADD_PATH).when(this.ribSupport).getRouteIdAddPath(any(Long.class), eq(ROUTE_ID_PA_ADD_PATH));
+        doReturn(null).when(this.ribSupport).getRouteIdAddPath(any(Long.class), eq(ROUTE_ID_PA));
+        doAnswer(invocation -> {
             final Object[] args = invocation.getArguments();
             final YangInstanceIdentifier yii = (YangInstanceIdentifier) args[0];
             final PathArgument paa = (PathArgument) args[1];
 
             if (ROUTE_ID_PA.equals(paa)) {
-                if (yii.equals(locRibTargetYii)) {
-                    return routePaYii;
-                } else if (yii.equals(locRibOutTargetYii)) {
-                    return routeRiboutYii;
-                } else if (yii.equals(locRibOutTargetYiiPeer2)) {
-                    return routeRiboutYiiPeer2;
+                if (yii.equals(this.locRibTargetYii)) {
+                    return this.routePaYii;
+                } else if (yii.equals(this.locRibOutTargetYii)) {
+                    return this.routeRiboutYii;
+                } else if (yii.equals(this.locRibOutTargetYiiPeer2)) {
+                    return this.routeRiboutYiiPeer2;
                 }
             } else if (ROUTE_ID_PA_ADD_PATH.equals(paa)) {
-                if (yii.equals(locRibTargetYii)) {
-                    return routePaAddPathYii;
-                } else if (yii.equals(locRibOutTargetYii)) {
-                    return routeAddRiboutYii;
-                } else if (yii.equals(locRibOutTargetYiiPeer2)) {
-                    return routeAddRiboutYiiPeer2;
+                if (yii.equals(this.locRibTargetYii)) {
+                    return this.routePaAddPathYii;
+                } else if (yii.equals(this.locRibOutTargetYii)) {
+                    return this.routeAddRiboutYii;
+                } else if (yii.equals(this.locRibOutTargetYiiPeer2)) {
+                    return this.routeAddRiboutYiiPeer2;
                 }
             }
             return null;
-        }).when(this.ribSupport).routePath(Mockito.any(YangInstanceIdentifier.class), Mockito.any(PathArgument.class));
+        }).when(this.ribSupport).routePath(any(YangInstanceIdentifier.class), any(PathArgument.class));
     }
 
     private NormalizedNode<?, ?> createAttr() {
index 1807dfe11eebfe0f092e5b4572b9c777c9962baa..be0f6a4c32134e6b861b8aa154b18ca491a9cef3 100644 (file)
@@ -7,23 +7,17 @@
  */
 package org.opendaylight.protocol.bgp.rib.impl;
 
-import static java.util.function.Function.identity;
-import static java.util.stream.Collectors.toMap;
-
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableMap;
-import java.util.Collections;
 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 java.util.stream.Collectors;
 import javax.annotation.concurrent.GuardedBy;
 import javax.annotation.concurrent.ThreadSafe;
+import org.opendaylight.protocol.bgp.rib.impl.spi.PeerExportGroupRegistry;
 import org.opendaylight.protocol.bgp.rib.spi.ExportPolicyPeerTracker;
-import org.opendaylight.protocol.bgp.rib.spi.IdentifierUtils;
 import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup;
 import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup.PeerExporTuple;
 import org.opendaylight.protocol.concepts.AbstractRegistration;
@@ -47,28 +41,36 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
     private final Set<PeerId> peerTables = new HashSet<>();
     private final PolicyDatabase policyDatabase;
     private final TablesKey localTableKey;
-    private volatile Map<PeerRole, PeerExportGroup> groups = Collections.emptyMap();
+    @GuardedBy("this")
+    private final Map<PeerRole, PeerExportGroupRegistry> groups = new EnumMap<>(PeerRole.class);
 
     ExportPolicyPeerTrackerImpl(final PolicyDatabase policyDatabase, final TablesKey localTablesKey) {
         this.policyDatabase = Preconditions.checkNotNull(policyDatabase);
         this.localTableKey = localTablesKey;
     }
 
-    private synchronized void createGroups(final Map<YangInstanceIdentifier, PeerRole> peerPathRoles) {
-        if (!peerPathRoles.isEmpty()) {
-            final Map<PeerRole, Map<PeerId, PeerExporTuple>> immutablePeers = peerPathRoles.entrySet().stream()
-                .collect(Collectors.groupingBy(Map.Entry::getValue, toMap(peer -> IdentifierUtils.peerKeyToPeerId(peer
-                    .getKey()), peer -> new PeerExporTuple(peer.getKey(), peer.getValue()))));
+    private synchronized AbstractRegistration addToExportGroups(final PeerId peerId,
+        final YangInstanceIdentifier peerPath, final PeerRole peerRole) {
+        final PeerExportGroupRegistry peerExp = this.groups.computeIfAbsent(peerRole,
+            k -> new PeerExportGroupImpl(this.policyDatabase.exportPolicyForRole(peerRole)));
 
-            this.groups = peerPathRoles.values().stream().collect(Collectors.toSet()).stream()
-                .collect(toMap(identity(), role -> new PeerExportGroupImpl(ImmutableMap.copyOf(immutablePeers.get(role)),
-                    this.policyDatabase.exportPolicyForRole(role)), (oldKey, newKey) -> oldKey, () -> new EnumMap<>(PeerRole.class)));
-        }
+        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()) {
+                    ExportPolicyPeerTrackerImpl.this.groups.remove(peerRole);
+                }
+            }
+        };
     }
 
     @Override
-    public synchronized AbstractRegistration registerPeer(final PeerId peerId, final SendReceive sendReceive, final YangInstanceIdentifier peerPath,
-        final PeerRole peerRole, final Optional<SimpleRoutingPolicy> optSimpleRoutingPolicy) {
+    public synchronized AbstractRegistration registerPeer(final PeerId peerId, final SendReceive sendReceive,
+        final YangInstanceIdentifier peerPath, final PeerRole peerRole,
+        final Optional<SimpleRoutingPolicy> optSimpleRoutingPolicy) {
         if (sendReceive != null) {
             this.peerAddPathTables.put(peerId, sendReceive);
             LOG.debug("Supported Add BestPath table {} added to peer {}", sendReceive, peerId);
@@ -79,7 +81,7 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
         }
         this.peerRoles.put(peerPath, peerRole);
         LOG.debug("Supported table {} added to peer {} role {}", this.localTableKey, peerId, peerRole);
-        createGroups(this.peerRoles);
+        final AbstractRegistration registration = addToExportGroups(peerId, peerPath, peerRole);
 
         final Object lock = this;
         return new AbstractRegistration() {
@@ -93,7 +95,7 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
                     ExportPolicyPeerTrackerImpl.this.peerTables.remove(peerId);
                     LOG.debug("Removed peer {} from supported table {}", peerId, ExportPolicyPeerTrackerImpl.this.localTableKey);
                     ExportPolicyPeerTrackerImpl.this.peerRoles.remove(peerPath);
-                    createGroups(ExportPolicyPeerTrackerImpl.this.peerRoles);
+                    registration.close();
                 }
             }
         };
@@ -119,5 +121,4 @@ final class ExportPolicyPeerTrackerImpl implements ExportPolicyPeerTracker {
         final SendReceive sendReceive = this.peerAddPathTables.get(peerId);
         return sendReceive != null && (sendReceive.equals(SendReceive.Both) || sendReceive.equals(SendReceive.Receive));
     }
-
 }
\ No newline at end of file
index 7a5b42f156f7389d77323054464ae4d878b6f978..dd6dbc66d21c390562e65a9bc7382adb969ba977 100644 (file)
@@ -9,18 +9,23 @@ package org.opendaylight.protocol.bgp.rib.impl;
 
 import com.google.common.base.Preconditions;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.Map;
-import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup;
+import java.util.function.BiConsumer;
+import javax.annotation.concurrent.GuardedBy;
+import org.opendaylight.protocol.bgp.rib.impl.spi.PeerExportGroupRegistry;
+import org.opendaylight.protocol.concepts.AbstractRegistration;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerId;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerRole;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode;
 
-final class PeerExportGroupImpl implements PeerExportGroup {
-    private final Map<PeerId, PeerExporTuple> peers;
+final class PeerExportGroupImpl implements PeerExportGroupRegistry {
+    @GuardedBy("this")
+    private final Map<PeerId, PeerExporTuple> peers = new HashMap<>();
     private final AbstractExportPolicy policy;
 
-    public PeerExportGroupImpl(final Map<PeerId, PeerExporTuple> peers, final AbstractExportPolicy policy) {
-        this.peers = Preconditions.checkNotNull(peers);
+    public PeerExportGroupImpl(final AbstractExportPolicy policy) {
         this.policy = Preconditions.checkNotNull(policy);
     }
 
@@ -31,11 +36,47 @@ final class PeerExportGroupImpl implements PeerExportGroup {
 
     @Override
     public Collection<Map.Entry<PeerId, PeerExporTuple>> getPeers() {
-        return this.peers.entrySet();
+        synchronized (this.peers) {
+            return this.peers.entrySet();
+        }
     }
 
     @Override
     public boolean containsPeer(final PeerId routePeerId) {
-        return this.peers.containsKey(routePeerId);
+        synchronized (this.peers) {
+            return this.peers.containsKey(routePeerId);
+        }
+    }
+
+    @Override
+    public void forEach(final BiConsumer<PeerId, YangInstanceIdentifier> action) {
+        synchronized (this.peers) {
+            for (final Map.Entry<PeerId, PeerExporTuple> pid : this.peers.entrySet()) {
+                action.accept(pid.getKey(), pid.getValue().getYii());
+            }
+        }
+    }
+
+    @Override
+    public AbstractRegistration registerPeer(final PeerId peerId, final PeerExporTuple peerExporTuple) {
+        synchronized (this.peers) {
+            this.peers.put(peerId, peerExporTuple);
+        }
+
+        return new AbstractRegistration() {
+            @Override
+            protected void removeRegistration() {
+                synchronized (PeerExportGroupImpl.this.peers) {
+                    PeerExportGroupImpl.this.peers.remove(peerId);
+                }
+            }
+        };
+    }
+
+    @Override
+    public boolean isEmpty() {
+        synchronized (this.peers) {
+            return this.peers.isEmpty();
+        }
     }
 }
\ No newline at end of file
diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/PeerExportGroupRegistry.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/spi/PeerExportGroupRegistry.java
new file mode 100644 (file)
index 0000000..844ec94
--- /dev/null
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2017 Pantheon Technologies s.r.o. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+package org.opendaylight.protocol.bgp.rib.impl.spi;
+
+import com.google.common.annotations.Beta;
+import org.opendaylight.protocol.bgp.rib.spi.PeerExportGroup;
+import org.opendaylight.protocol.concepts.AbstractRegistration;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerId;
+
+/**
+ * PeerExportGroup Registry. Register should be maintained in blocking mode to avoid race between
+ * updating the routes and notify to a peer which doesn't longer exist. BUG-7676
+ */
+@Beta
+public interface PeerExportGroupRegistry extends PeerExportGroup {
+    /**
+     * Register peer to this exportGroup
+     *
+     * @param peerId         Peer Id
+     * @param peerExporTuple Peer Export Tuple
+     * @return registration ticket which take allows to unregister peer
+     */
+    AbstractRegistration registerPeer(PeerId peerId, PeerExporTuple peerExporTuple);
+
+    /**
+     * @return true if none peer is registered.
+     */
+    boolean isEmpty();
+}
index ff7e91798d2e7179764310e696b2270c1b4a8472..31bb7ed57cd1283f3495bfaea44c9d1f569b3732 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.protocol.bgp.rib.spi;
 
 import java.util.Collection;
 import java.util.Map;
+import java.util.function.BiConsumer;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerId;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.rib.rev130925.PeerRole;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
@@ -48,8 +49,22 @@ public interface PeerExportGroup {
 
     /**
      * @return map of peer
+     * @deprecated Use {@link #forEach}
      */
+    @Deprecated
     Collection<Map.Entry<PeerId, PeerExporTuple>> getPeers();
 
+    /**
+     *
+     * @param routePeerId PeerId
+     * @return true if peer is present on this export group
+     */
     boolean containsPeer(PeerId routePeerId);
+
+    /**
+     * Applies the given action for each entry in this PeerExportGroup on synchronized mode
+     *
+     * @param action action to be applied
+     */
+    void forEach(BiConsumer<PeerId, YangInstanceIdentifier> action);
 }