BUG-4835: Routes no withdrawn when the same prefixes annonced. 08/33208/3
authorClaudio D. Gasparini <cgaspari@cisco.com>
Tue, 19 Jan 2016 12:58:57 +0000 (13:58 +0100)
committerGerrit Code Review <gerrit@opendaylight.org>
Thu, 21 Jan 2016 18:07:48 +0000 (18:07 +0000)
Routes not withdrawn when eBGP peers are disconnected and
the same prefixes annonced
Fix by shrink OFFSET

Change-Id: I7756c677b34372f5e0c19aefdd14bca74a226d78
Signed-off-by: Claudio D. Gasparini <cgaspari@cisco.com>
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractRouteEntry.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ComplexRouteEntry.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/LocRibWriter.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/OffsetMap.java
bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/SimpleRouteEntry.java
bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/OffsetMapTest.java

index c38c4c64d91e729b61b10a1aac9abdbcb61003f5..a8dd654ace8be376cac96c67def1427c0b7bac29 100644 (file)
@@ -58,11 +58,16 @@ abstract class AbstractRouteEntry {
         return addRoute(routerId, advertisedAttrs);
     }
 
-    // Indicates whether this was the last route
-    protected final boolean removeRoute(final int offset) {
+    /**
+     * Remove route
+     * @param routerId
+     * @param offset
+     * @return true if it was the last route
+     */
+    protected final boolean removeRoute(final UnsignedInteger routerId, final int offset) {
         if (this.offsets.size() != 1) {
-            // FIXME: actually shrink the array
-            this.offsets.setValue(this.values, offset, null);
+            this.values = this.offsets.removeValue(this.values, offset);
+            this.offsets = this.offsets.without(routerId);
             return false;
         } else {
             return true;
index 071c64ed0376cde4876540f05a876986ac18aeff..eb67f79fc61519edf6b766e728be3376d550eedb 100644 (file)
@@ -42,10 +42,8 @@ final class ComplexRouteEntry extends AbstractRouteEntry {
     boolean removeRoute(final UnsignedInteger routerId) {
         final OffsetMap map = getOffsets();
         final int offset = map.offsetOf(routerId);
-
-        final boolean ret = removeRoute(offset);
-        // FIXME: actually shrink the array
-        map.setValue(this.values, offset, null);
+        final boolean ret = removeRoute(routerId, offset);
+        this.values = map.removeValue(this.values, offset);
         return ret;
     }
 }
index c517212c754f55df30d6fb554d8fcfa6910ee193..1f853a5adc143ae0deffb34f7bccb615d7f519aa 100644 (file)
@@ -128,10 +128,12 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
              * We use two-stage processing here in hopes that we avoid duplicate
              * calculations when multiple peers have changed a particular entry.
              */
-            final Map<RouteUpdateKey, AbstractRouteEntry> toUpdate = update(tx, changes);
+            final Map<YangInstanceIdentifier, PeerId> deletedPeers = new HashMap<>();
+            final Map<RouteUpdateKey, AbstractRouteEntry> toUpdate = update(tx, changes, deletedPeers);
 
             // Now walk all updated entries
-            walkThrough(tx, toUpdate);
+            walkThrough(tx, toUpdate, deletedPeers);
+            removeDeletedPeersFromExportPolicyTracker(deletedPeers);
         } catch (final Exception e) {
             LOG.error("Failed to completely propagate updates {}, state is undefined", changes, e);
         } finally {
@@ -139,31 +141,30 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
         }
     }
 
+    private void removeDeletedPeersFromExportPolicyTracker(final Map<YangInstanceIdentifier, PeerId> deletedPeers) {
+        for (final Entry<YangInstanceIdentifier, PeerId> peerPath : deletedPeers.entrySet()) {
+            this.peerPolicyTracker.peerRoleChanged(peerPath.getKey(), null);
+        }
+    }
+
     private Map<RouteUpdateKey, AbstractRouteEntry> update(final DOMDataWriteTransaction tx,
-        final Collection<DataTreeCandidate> changes) {
+        final Collection<DataTreeCandidate> changes, final Map<YangInstanceIdentifier, PeerId> deletedPeers) {
         final Map<RouteUpdateKey, AbstractRouteEntry> ret = new HashMap<>();
 
         for (final DataTreeCandidate tc : changes) {
             final YangInstanceIdentifier rootPath = tc.getRootPath();
             final DataTreeCandidateNode rootNode = tc.getRootNode();
-            //Perform first PeerRoleChange, in case where peer is not deleted, since a new
-            //peer added needs to be add to peerPolicyTracker before process tables
-            if (!rootNode.getModificationType().equals(ModificationType.DELETE)) {
-                filterOutPeerRole(rootNode, rootPath);
-            }
-            filterOutChangesToSupportedTables(rootNode, rootPath, tx);
-            filterOutAnyChangeOutsideEffRibsIn(rootNode, rootPath, ret, tx);
-            //If Peer is removed we need to filterOutPeerRole after all routes are removed, then peer can
-            // be removed from peerPolicyTracker
-            if (rootNode.getModificationType().equals(ModificationType.DELETE)) {
-                filterOutPeerRole(rootNode, rootPath);
-            }
+            final NodeIdentifierWithPredicates peerKey = IdentifierUtils.peerKey(rootPath);
+            final PeerId peerId = IdentifierUtils.peerId(peerKey);
+            filterOutPeerRole(peerId, rootNode, rootPath, deletedPeers);
+            filterOutChangesToSupportedTables(peerId, rootNode, rootPath, tx);
+            filterOutAnyChangeOutsideEffRibsIn(peerId, rootNode, ret, tx);
         }
 
         return ret;
     }
 
-    private void filterOutAnyChangeOutsideEffRibsIn(final DataTreeCandidateNode rootNode, final YangInstanceIdentifier rootPath,
+    private void filterOutAnyChangeOutsideEffRibsIn(final PeerId peerId, final DataTreeCandidateNode rootNode,
         final Map<RouteUpdateKey, AbstractRouteEntry> ret, final DOMDataWriteTransaction tx) {
         final DataTreeCandidateNode ribIn = rootNode.getModifiedChild(EFFRIBIN_NID);
         if (ribIn == null) {
@@ -175,16 +176,13 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
             LOG.debug("Skipping change {}", rootNode.getIdentifier());
             return;
         }
-        final NodeIdentifierWithPredicates peerKey = IdentifierUtils.peerKey(rootPath);
-        final PeerId peerId = IdentifierUtils.peerId(peerKey);
         updateNodes(table, peerId, tx, ret);
     }
 
-    private void filterOutChangesToSupportedTables(final DataTreeCandidateNode rootNode, final YangInstanceIdentifier rootPath, final DOMDataWriteTransaction tx) {
+    private void filterOutChangesToSupportedTables(final PeerId peerIdOfNewPeer, final DataTreeCandidateNode rootNode, final YangInstanceIdentifier rootPath, final DOMDataWriteTransaction tx) {
         final DataTreeCandidateNode tablesChange = rootNode.getModifiedChild(AbstractPeerRoleTracker.PEER_TABLES);
 
         if (tablesChange != null) {
-            final PeerId peerIdOfNewPeer = IdentifierUtils.peerId((NodeIdentifierWithPredicates) IdentifierUtils.peerPath(rootPath).getLastPathArgument());
             final PeerRole newPeerRole = this.peerPolicyTracker.getRole(IdentifierUtils.peerPath(rootPath));
             final PeerExportGroup peerGroup = this.peerPolicyTracker.getPeerGroup(newPeerRole);
 
@@ -194,13 +192,11 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
                     for (Map.Entry<PathArgument, AbstractRouteEntry> entry : this.routeEntries.entrySet()) {
                         if(isTableSupported(peerIdOfNewPeer)) {
                             final AbstractRouteEntry routeEntry = entry.getValue();
-                            final ContainerNode attributes = routeEntry.attributes();
                             final PathArgument routeId = entry.getKey();
                             final YangInstanceIdentifier routeTarget = getRouteTarget(rootPath, routeId);
                             final NormalizedNode<?, ?> value = routeEntry.createValue(routeId);
                             final PeerId routePeerId = RouterIds.createPeerId(routeEntry.getBestRouterId());
-                            final ContainerNode effectiveAttributes = peerGroup.effectiveAttributes(routePeerId, attributes);
-
+                            final ContainerNode effectiveAttributes = peerGroup.effectiveAttributes(routePeerId, routeEntry.attributes());
                             if (effectiveAttributes != null && value != null) {
                                 LOG.debug("Write route {} to peer AdjRibsOut {}", value, peerIdOfNewPeer);
                                 tx.put(LogicalDatastoreType.OPERATIONAL, routeTarget, value);
@@ -217,14 +213,19 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
         return this.ribSupport.routePath(rootPath.node(AdjRibOut.QNAME).node(Tables.QNAME).node(this.tableKey).node(ROUTES_IDENTIFIER), routeId);
     }
 
-    private void filterOutPeerRole(final DataTreeCandidateNode rootNode, final YangInstanceIdentifier rootPath) {
+    private void filterOutPeerRole(final PeerId peerId, final DataTreeCandidateNode rootNode, final YangInstanceIdentifier rootPath, final Map<YangInstanceIdentifier, PeerId> deletedPeers) {
         final DataTreeCandidateNode roleChange = rootNode.getModifiedChild(AbstractPeerRoleTracker.PEER_ROLE_NID);
         if (roleChange != null) {
-            this.peerPolicyTracker.onDataTreeChanged(roleChange, IdentifierUtils.peerPath(rootPath));
+            if (!rootNode.getModificationType().equals(ModificationType.DELETE)) {
+                this.peerPolicyTracker.onDataTreeChanged(roleChange, IdentifierUtils.peerPath(rootPath));
+            } else {
+                deletedPeers.put(IdentifierUtils.peerPath(rootPath), peerId);
+            }
         }
     }
 
-    private void updateNodes(final DataTreeCandidateNode table, final PeerId peerId, final DOMDataWriteTransaction tx, final Map<RouteUpdateKey, AbstractRouteEntry> routes) {
+    private void updateNodes(final DataTreeCandidateNode table, final PeerId peerId, final DOMDataWriteTransaction tx,
+        final Map<RouteUpdateKey, AbstractRouteEntry> routes) {
         for (final DataTreeCandidateNode child : table.getChildNodes()) {
             LOG.debug("Modification type {}", child.getModificationType());
             if ((Attributes.QNAME).equals(child.getIdentifier().getNodeType())) {
@@ -241,59 +242,72 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
 
     private void updateRoutesEntries(final DataTreeCandidateNode child, final PeerId peerId, final Map<RouteUpdateKey, AbstractRouteEntry> routes) {
         final UnsignedInteger routerId = RouterIds.routerIdForPeerId(peerId);
-        for (final DataTreeCandidateNode route : this.ribSupport.changedRoutes(child)) {
+        final Collection<DataTreeCandidateNode> modifiedRoutes = this.ribSupport.changedRoutes(child);
+        for (final DataTreeCandidateNode route : modifiedRoutes) {
             final PathArgument routeId = route.getIdentifier();
             AbstractRouteEntry entry = this.routeEntries.get(routeId);
 
             final Optional<NormalizedNode<?, ?>> maybeData = route.getDataAfter();
+            final RouteUpdateKey routeUpdateKey = new RouteUpdateKey(peerId, routeId);
             if (maybeData.isPresent()) {
                 if (entry == null) {
                     entry = createEntry(routeId);
                 }
                 entry.addRoute(routerId, this.attributesIdentifier, maybeData.get());
-            } else if (entry != null && entry.removeRoute(routerId)) {
-                this.routeEntries.remove(routeId);
-                entry = null;
-                LOG.trace("Removed route from {}", routerId);
+            } else if (entry != null) {
+                if(entry.removeRoute(routerId)) {
+                    this.routeEntries.remove(routeId);
+                    LOG.trace("Removed route from {}", routerId);
+                    entry = null;
+                }else {
+                    routes.put(routeUpdateKey, null);
+                }
             }
             LOG.debug("Updated route {} entry {}", routeId, entry);
-            routes.put(new RouteUpdateKey(peerId, routeId), entry);
+            routes.put(routeUpdateKey, entry);
         }
     }
 
-    private void walkThrough(final DOMDataWriteTransaction tx, final Map<RouteUpdateKey, AbstractRouteEntry> toUpdate) {
+    private void walkThrough(final DOMDataWriteTransaction tx, final Map<RouteUpdateKey, AbstractRouteEntry> toUpdate,
+        final Map<YangInstanceIdentifier, PeerId> deletedPeers) {
         for (final Entry<RouteUpdateKey, AbstractRouteEntry> e : toUpdate.entrySet()) {
             LOG.trace("Walking through {}", e);
             final AbstractRouteEntry entry = e.getValue();
             final RouteUpdateKey key = e.getKey();
             final NormalizedNode<?, ?> value;
-
+            final PathArgument routeId = key.getRouteId();
+            PeerId routePeerId = key.getPeerId();
             if (entry != null) {
                 if (!entry.selectBest(this.ourAs)) {
                     // Best path has not changed, no need to do anything else. Proceed to next route.
                     LOG.trace("Continuing");
                     continue;
                 }
-                value = entry.createValue(key.getRouteId());
+                routePeerId = RouterIds.createPeerId(entry.getBestRouterId());
+                value = entry.createValue(routeId);
                 LOG.trace("Selected best value {}", value);
             } else {
                 value = null;
             }
+            fillLocRib(tx, entry, value, routeId);
+            fillAdjRibsOut(tx, entry, value, routeId, routePeerId, deletedPeers);
+        }
+    }
 
-            final YangInstanceIdentifier writePath = this.ribSupport.routePath(this.locRibTarget.node(ROUTES_IDENTIFIER), key.getRouteId());
-            if (value != null) {
-                LOG.debug("Write route to LocRib {}", value);
-                tx.put(LogicalDatastoreType.OPERATIONAL, writePath, value);
-            } else {
-                LOG.debug("Delete route from LocRib {}", entry);
-                tx.delete(LogicalDatastoreType.OPERATIONAL, writePath);
-            }
-            fillAdjRibsOut(tx, entry, value, key);
+    private void fillLocRib(final DOMDataWriteTransaction tx, final AbstractRouteEntry entry, final NormalizedNode<?, ?> value, final PathArgument routeId) {
+        final YangInstanceIdentifier writePath = this.ribSupport.routePath(this.locRibTarget.node(ROUTES_IDENTIFIER), routeId);
+        if (value != null) {
+            LOG.debug("Write route to LocRib {}", value);
+            tx.put(LogicalDatastoreType.OPERATIONAL, writePath, value);
+        } else {
+            LOG.debug("Delete route from LocRib {}", entry);
+            tx.delete(LogicalDatastoreType.OPERATIONAL, writePath);
         }
     }
 
     @VisibleForTesting
-    private void fillAdjRibsOut(final DOMDataWriteTransaction tx, final AbstractRouteEntry entry, final NormalizedNode<?, ?> value, final RouteUpdateKey key) {
+    private void fillAdjRibsOut(final DOMDataWriteTransaction tx, final AbstractRouteEntry entry, final NormalizedNode<?, ?> value,
+        final PathArgument routeId, final PeerId routePeerId, final Map<YangInstanceIdentifier, PeerId> deletedPeers) {
         /*
          * 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
@@ -307,20 +321,18 @@ final class LocRibWriter implements AutoCloseable, DOMDataTreeChangeListener {
         for (final PeerRole role : PeerRole.values()) {
             final PeerExportGroup peerGroup = this.peerPolicyTracker.getPeerGroup(role);
             if (peerGroup != null) {
-                final PeerId peerId = key.getPeerId();
-                final ContainerNode effectiveAttributes = peerGroup.effectiveAttributes(peerId, attributes);
-                if (effectiveAttributes != null) {
-                    for (final Entry<PeerId, YangInstanceIdentifier> pid : peerGroup.getPeers()) {
-                        if (!peerId.equals(pid.getKey()) && isTableSupported(pid.getKey())) {
-                            final YangInstanceIdentifier routeTarget = getRouteTarget(pid.getValue(), key.getRouteId());
-                            if (value != null) {
-                                LOG.debug("Write route {} to peers AdjRibsOut {}", value, pid.getKey());
-                                tx.put(LogicalDatastoreType.OPERATIONAL, routeTarget, value);
-                                tx.put(LogicalDatastoreType.OPERATIONAL, routeTarget.node(this.attributesIdentifier), effectiveAttributes);
-                            } else {
-                                LOG.trace("Removing {} from transaction for peer {}", routeTarget, pid.getKey());
-                                tx.delete(LogicalDatastoreType.OPERATIONAL, routeTarget);
-                            }
+                final ContainerNode effectiveAttributes = peerGroup.effectiveAttributes(routePeerId, attributes);
+                for (final Entry<PeerId, YangInstanceIdentifier> pid : peerGroup.getPeers()) {
+                    final PeerId peerDestiny = pid.getKey();
+                    if (!routePeerId.equals(peerDestiny) && isTableSupported(peerDestiny) && !deletedPeers.containsValue(peerDestiny)) {
+                        final YangInstanceIdentifier routeTarget = getRouteTarget(pid.getValue(), routeId);
+                        if (value != null && effectiveAttributes != null) {
+                            LOG.debug("Write route {} to peers AdjRibsOut {}", value, peerDestiny);
+                            tx.put(LogicalDatastoreType.OPERATIONAL, routeTarget, value);
+                            tx.put(LogicalDatastoreType.OPERATIONAL, routeTarget.node(this.attributesIdentifier), effectiveAttributes);
+                        } else {
+                            LOG.trace("Removing {} from transaction for peer {}", routeTarget, peerDestiny);
+                            tx.delete(LogicalDatastoreType.OPERATIONAL, routeTarget);
                         }
                     }
                 }
index 9f7710d2e76379a0e81d63f4eef12d1d9d504ddf..6819ba42f33271f8b1dc364683e0ca20e63e6f04 100644 (file)
@@ -15,6 +15,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.primitives.UnsignedInteger;
 import java.lang.reflect.Array;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
@@ -24,7 +25,7 @@ import java.util.Set;
  * A map of Router identifier to an offset. Used to maintain a simple
  * offset-based lookup across multiple {@link AbstractRouteEntry} objects,
  * which share either contributors or consumers.
- *
+ * <p/>
  * We also provide utility reformat methods, which provide access to
  * array members and array management features.
  */
@@ -75,6 +76,15 @@ final class OffsetMap {
         return OFFSETMAPS.getUnchecked(b.build());
     }
 
+    OffsetMap without(final UnsignedInteger routerId) {
+        final Builder<UnsignedInteger> b = ImmutableSet.builder();
+        final ArrayList<UnsignedInteger> newArray = new ArrayList(Arrays.asList(this.routerIds));
+        newArray.remove(routerId);
+        final UnsignedInteger[] ret = new UnsignedInteger[newArray.size()];
+        b.add(newArray.toArray(ret));
+        return OFFSETMAPS.getUnchecked(b.build());
+    }
+
     <T> T getValue(final T[] array, final int offset) {
         Preconditions.checkArgument(offset >= 0, "Invalid negative offset %s", offset);
         Preconditions.checkArgument(offset < routerIds.length, "Invalid offset %s for %s router IDs", offset, routerIds.length);
@@ -97,4 +107,13 @@ final class OffsetMap {
 
         return ret;
     }
+
+    <T> T[] removeValue(final T[] array, final int offset) {
+        Preconditions.checkArgument(offset >= 0, "Invalid negative offset %s", offset);
+        Preconditions.checkArgument(offset < routerIds.length, "Invalid offset %s for %s router IDs", offset, routerIds.length);
+        final ArrayList<T> newArray = new ArrayList(Arrays.asList(array));
+        newArray.remove(offset);
+        final T[] ret = (T[]) Array.newInstance(array.getClass().getComponentType(), newArray.size());
+        return newArray.toArray(ret);
+    }
 }
index 13923b7d8252ffe9424138f569b4e46642a8e310..1dd90c808202cb78bd1db5c357ed636487e6d3a0 100644 (file)
@@ -17,7 +17,7 @@ import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.DataContaine
 final class SimpleRouteEntry extends AbstractRouteEntry {
     @Override
     boolean removeRoute(UnsignedInteger routerId) {
-        return removeRoute(getOffsets().offsetOf(routerId));
+        return removeRoute(routerId, getOffsets().offsetOf(routerId));
     }
 
     @Override
index 6136eecb641051d27ca1de0144c5c450391d489b..0421e61e9efaef3b5013963cc3769d57711879e1 100644 (file)
@@ -26,8 +26,10 @@ public class OffsetMapTest {
         assertEquals(EXPECTED_ROUTER_OFFSET, offsetMap.offsetOf(RouterIds.routerIdForAddress(LOCAL_ADDRESS)));
         assertEquals(LOCAL_ADDRESS_DECIMAL, offsetMap.getRouterId(EXPECTED_ROUTER_OFFSET).intValue());
 
-        assertEquals(EXPECTED_VALUE, (int)offsetMap.getValue(TESTED_VALUES, EXPECTED_ROUTER_OFFSET));
+        assertEquals(EXPECTED_VALUE, (int) offsetMap.getValue(TESTED_VALUES, EXPECTED_ROUTER_OFFSET));
         offsetMap.setValue(TESTED_VALUES, EXPECTED_ROUTER_OFFSET, CHANGED_VALUE);
-        assertEquals(CHANGED_VALUE, (int)offsetMap.getValue(TESTED_VALUES, EXPECTED_ROUTER_OFFSET));
+        assertEquals(CHANGED_VALUE, (int) offsetMap.getValue(TESTED_VALUES, EXPECTED_ROUTER_OFFSET));
+        assertEquals(TESTED_VALUES.length -1,offsetMap.removeValue(TESTED_VALUES, 0).length);
+        assertEquals(0, offsetMap.without(RouterIds.routerIdForAddress(LOCAL_ADDRESS)).size());
     }
 }