From: Claudio D. Gasparini Date: Sat, 20 Jan 2018 12:08:18 +0000 (+0100) Subject: BGPCEP-672: Fix key storage un adj-rib-out X-Git-Tag: release/oxygen~20 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=c9364e0e388f04105ca2f2cb5718dd6f15b781c2;p=bgpcep.git BGPCEP-672: Fix key storage un adj-rib-out list key must not be null, therefore path-id should always be included. Reserving PATH-ID 0 for non-supporting add-path. Change-Id: I9b0af92e91e7c0b1c7281230a804c7adeff2dbca Signed-off-by: Claudio D. Gasparini --- diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/AddPathAbstractRouteEntry.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/AddPathAbstractRouteEntry.java index ef91b92b0a..d6854b0b20 100644 --- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/AddPathAbstractRouteEntry.java +++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/AddPathAbstractRouteEntry.java @@ -7,6 +7,8 @@ */ package org.opendaylight.protocol.bgp.mode.impl.add; +import static org.opendaylight.protocol.bgp.parser.spi.PathIdUtil.NON_PATH_ID; + import com.google.common.collect.Lists; import com.google.common.primitives.UnsignedInteger; import java.util.ArrayList; @@ -132,7 +134,9 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { if (this.removedPaths != null) { this.removedPaths.forEach(removedPath -> { final PathArgument routeIdAddPath = ribSupport.getRouteIdAddPath(removedPath.getPathId(), routeIdPA); - fillAdjRibsOut(true, null, null, null, routeIdPA, routeIdAddPath, + final PathArgument routeIdAddPathDefault = ribSupport.getRouteIdAddPath(NON_PATH_ID, routeIdPA); + fillAdjRibsOut(true, null, null, null, + routeIdAddPathDefault, routeIdAddPath, RouterIds.createPeerId(removedPath.getRouteId()), peerPT, localTK, ribSupport, tx); }); this.removedPaths = null; @@ -164,35 +168,36 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { final PeerExportGroup peerGroup, final boolean destPeerSupAddPath, final BestPath path, final YangInstanceIdentifier rootPath, final TablesKey localTK, final RIBSupport ribSup, final DOMDataWriteTransaction tx) { - final PathArgument routeIdAddPath = ribSup.getRouteIdAddPath(path.getPathId(), routeId); final ContainerNode effectiveAttributes = peerGroup .effectiveAttributes(getRoutePeerIdRole(peerPT,path.getPeerId()), path.getAttributes()); + PathArgument routeIdAddPath; if (destPeerSupAddPath) { - writeRoute(destPeer, getAdjRibOutYII(ribSup, rootPath, routeIdAddPath, localTK), effectiveAttributes, - createValue(routeIdAddPath, path), ribSup, tx); + routeIdAddPath = ribSup.getRouteIdAddPath(path.getPathId(), routeId); } else { - writeRoute(destPeer, getAdjRibOutYII(ribSup, rootPath, routeId, localTK), effectiveAttributes, - createValue(routeId, path), ribSup, tx); + routeIdAddPath = ribSup.getRouteIdAddPath(NON_PATH_ID, routeId); } + writeRoute(destPeer, getAdjRibOutYII(ribSup, rootPath, routeIdAddPath, localTK), effectiveAttributes, + createValue(routeIdAddPath, path), ribSup, tx); } private void addPathToDataStore(final BestPath path, final boolean isFirstBestPath, final PathArgument routeIdPA, final YangInstanceIdentifier locRibTarget, final RIBSupport ribSup, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final DOMDataWriteTransaction tx) { final PathArgument routeIdAddPath = ribSup.getRouteIdAddPath(path.getPathId(), routeIdPA); + final PathArgument routeIdAddPathDefault = ribSup.getRouteIdAddPath(NON_PATH_ID, routeIdPA); final YangInstanceIdentifier pathAddPathTarget = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdAddPath); final MapEntryNode addPathValue = createValue(routeIdAddPath, path); - final MapEntryNode value = createValue(routeIdPA, path); + final MapEntryNode defaultValue = createValue(routeIdAddPathDefault, path); LOG.trace("Selected best value {}", addPathValue); fillLocRib(pathAddPathTarget, addPathValue, tx); - fillAdjRibsOut(isFirstBestPath, path.getAttributes(), value, addPathValue, routeIdPA, routeIdAddPath, - path.getPeerId(), peerPT, localTK, - ribSup, tx); + fillAdjRibsOut(isFirstBestPath, path.getAttributes(), defaultValue, addPathValue, routeIdAddPathDefault, + routeIdAddPath, path.getPeerId(), peerPT, localTK, ribSup, tx); } private void fillAdjRibsOut(final boolean isFirstBestPath, final ContainerNode attributes, - final NormalizedNode value, final MapEntryNode addPathValue, final PathArgument routeId, + final MapEntryNode defaultValue, final MapEntryNode addPathValue, + final PathArgument routeIdAddPathDefault, final PathArgument routeIdAddPath, final PeerId routePeerId, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final RIBSupport ribSup, final DOMDataWriteTransaction tx) { /* @@ -217,8 +222,8 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeIdAddPath, localTK), effectiveAttributes, addPathValue, ribSup, tx); } else if (!this.oldNonAddPathBestPathTheSame) { - update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeId, localTK), - effectiveAttributes, value, ribSup, tx); + update(destPeer, getAdjRibOutYII(ribSup, rootPath, routeIdAddPathDefault, localTK), + effectiveAttributes, defaultValue, ribSup, tx); } } }); diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/base/BaseAbstractRouteEntry.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/base/BaseAbstractRouteEntry.java index e3381a83c9..4cae936af5 100644 --- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/base/BaseAbstractRouteEntry.java +++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/base/BaseAbstractRouteEntry.java @@ -23,6 +23,7 @@ import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode; +import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNodes; import org.slf4j.Logger; @@ -119,10 +120,14 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry { final BaseBestPath path = this.bestPath; final PeerRole destPeerRole = getRoutePeerIdRole(peerPT, destPeer); if (filterRoutes(path.getPeerId(), destPeer, peerPT, localTK, destPeerRole)) { + PathArgument routeIdDest = ribSupport.getRouteIdAddPath(path.getPathId(), routeId); + if (routeIdDest == null) { + routeIdDest = routeId; + } final ContainerNode effAttrib = peerGroup.effectiveAttributes( getRoutePeerIdRole(peerPT, path.getPeerId()), path.getAttributes()); - writeRoute(destPeer, getAdjRibOutYII(ribSupport, rootPath, routeId, localTK), effAttrib, - createValue(routeId, path), ribSupport, tx); + writeRoute(destPeer, getAdjRibOutYII(ribSupport, rootPath, routeIdDest, localTK), effAttrib, + createValue(routeIdDest, path), ribSupport, tx); } } } @@ -131,34 +136,29 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry { final YangInstanceIdentifier locRibTarget, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final RIBSupport ribSup, final DOMDataWriteTransaction tx) { LOG.trace("Best Path removed {}", path); - final PathArgument routeIdAddPath = ribSup.getRouteIdAddPath(path.getPathId(), routeIdPA); - final YangInstanceIdentifier pathTarget = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdPA); - YangInstanceIdentifier pathAddPathTarget = null; - if (routeIdAddPath != null) { - pathAddPathTarget = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdAddPath); + PathArgument routeIdTarget = ribSup.getRouteIdAddPath(path.getPathId(), routeIdPA); + if (routeIdTarget == null) { + routeIdTarget = routeIdPA; } - fillLocRib(pathAddPathTarget == null ? pathTarget : pathAddPathTarget, null, tx); - fillAdjRibsOut(null, null, routeIdPA, path.getPeerId(), peerPT, localTK, ribSup, tx); + fillLocRib(ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdTarget), null, tx); + fillAdjRibsOut(null, null, routeIdTarget, path.getPeerId(), peerPT, localTK, ribSup, tx); } private void addPathToDataStore(final BestPath path, final PathArgument routeIdPA, final YangInstanceIdentifier locRibTarget, final RIBSupport ribSup, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final DOMDataWriteTransaction tx) { - final PathArgument routeIdAddPath = ribSup.getRouteIdAddPath(path.getPathId(), routeIdPA); - final YangInstanceIdentifier pathTarget = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdPA); - final NormalizedNode value = createValue(routeIdPA, path); - NormalizedNode addPathValue = null; - YangInstanceIdentifier pathAddPathTarget = null; - if (routeIdAddPath == null) { - LOG.trace("Selected best value {}", value); - } else { - pathAddPathTarget = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdAddPath); - addPathValue = createValue(routeIdAddPath, path); - LOG.trace("Selected best value {}", addPathValue); + PathArgument routeIdDest = ribSup.getRouteIdAddPath(path.getPathId(), routeIdPA); + if (routeIdDest == null) { + routeIdDest = routeIdPA; } - fillLocRib(pathAddPathTarget == null ? pathTarget : pathAddPathTarget, - addPathValue == null ? value : addPathValue, tx); - fillAdjRibsOut(path.getAttributes(), value, routeIdPA, path.getPeerId(), peerPT, localTK, ribSup, tx); + + final MapEntryNode value = createValue(routeIdDest, path); + LOG.trace("Selected best value {}", value); + + final YangInstanceIdentifier pathAddPathTarget + = ribSup.routePath(locRibTarget.node(ROUTES_IDENTIFIER), routeIdDest); + fillLocRib(pathAddPathTarget, value, tx); + fillAdjRibsOut(path.getAttributes(), value, routeIdDest, path.getPeerId(), peerPT, localTK, ribSup, tx); } final OffsetMap getOffsets() { @@ -166,7 +166,7 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry { } @VisibleForTesting - private void fillAdjRibsOut(final ContainerNode attributes, final NormalizedNode value, + private void fillAdjRibsOut(final ContainerNode attributes, final MapEntryNode value, final PathArgument routeId, final PeerId routePeerId, final ExportPolicyPeerTracker peerPT, final TablesKey localTK, final RIBSupport ribSup, final DOMDataWriteTransaction tx) { /* 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 ec58b69229..ec2a4a0256 100644 --- 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 @@ -91,7 +91,7 @@ public abstract class AbstractRouteEntry implements RouteEntry { .node(RibSupportUtils.toYangTablesKey(localTK)).node(ROUTES_IDENTIFIER), routeId); } - protected PeerRole getRoutePeerIdRole(final ExportPolicyPeerTracker peerPT, final PeerId routePeerId) { + protected static PeerRole getRoutePeerIdRole(final ExportPolicyPeerTracker peerPT, final PeerId routePeerId) { for (final PeerRole role : PeerRole.values()) { final PeerExportGroup peerGroup = peerPT.getPeerGroup(role); if (peerGroup != null && peerGroup.containsPeer(routePeerId)) { 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 bdd4fb8000..4c68e9a8ec 100644 --- 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 @@ -204,6 +204,9 @@ final class LocRibWriter implements AutoCloseable, TotalPrefixesCounter, TotalPa if (!table.getDataBefore().isPresent() && this.exportPolicyPeerTracker.isTableSupported(peerIdOfNewPeer)) { this.exportPolicyPeerTracker.registerPeerAsInitialized(peerIdOfNewPeer); LOG.debug("Peer {} table has been created, inserting existent routes", peerIdOfNewPeer); + if (this.routeEntries.isEmpty()) { + return; + } final PeerRole newPeerRole = this.exportPolicyPeerTracker.getRole(IdentifierUtils.peerPath(rootPath)); final PeerExportGroup peerGroup = this.exportPolicyPeerTracker.getPeerGroup(newPeerRole); this.routeEntries.forEach((key, value) -> value.writeRoute(peerIdOfNewPeer, key, diff --git a/config-loader/config-loader-impl/src/main/java/org/opendaylight/bgpcep/config/loader/impl/ConfigLoaderImpl.java b/config-loader/config-loader-impl/src/main/java/org/opendaylight/bgpcep/config/loader/impl/ConfigLoaderImpl.java index 081fc8719d..9199d78b95 100644 --- a/config-loader/config-loader-impl/src/main/java/org/opendaylight/bgpcep/config/loader/impl/ConfigLoaderImpl.java +++ b/config-loader/config-loader-impl/src/main/java/org/opendaylight/bgpcep/config/loader/impl/ConfigLoaderImpl.java @@ -147,6 +147,7 @@ public final class ConfigLoaderImpl implements ConfigLoader, AutoCloseable { } private class ConfigLoaderImplRunnable implements Runnable { + @GuardedBy("this") private final WatchService watchService; ConfigLoaderImplRunnable(final WatchService watchService) { @@ -156,14 +157,14 @@ public final class ConfigLoaderImpl implements ConfigLoader, AutoCloseable { @Override public void run() { while (!Thread.currentThread().isInterrupted()) { - handleChanges(this.watchService); + handleChanges(); } } - private synchronized void handleChanges(final WatchService watch) { + private synchronized void handleChanges() { final WatchKey key; try { - key = watch.take(); + key = this.watchService.take(); } catch (final InterruptedException | ClosedWatchServiceException e) { if (!ConfigLoaderImpl.this.closed) { LOG.warn(INTERRUPTED, e);