From 94f7f6bee9c8467cdd240aac4702c3e49f5306e1 Mon Sep 17 00:00:00 2001 From: "Claudio D. Gasparini" Date: Mon, 8 Aug 2016 08:17:52 +0200 Subject: [PATCH] BUG-6317 Do not withdraw route when best path has changed Route should be withdrawn only if there is not other best path which can be announced intead. Change-Id: Ibc2ae0bf2d05a90e06eae34a12f8e6b98f64fb7c Signed-off-by: Claudio D. Gasparini --- .../impl/add/AddPathAbstractRouteEntry.java | 37 ++++++++++++++++--- .../all/paths/AbstractAllPathsRouteEntry.java | 14 +------ .../add/n/paths/AbstractNPathsRouteEntry.java | 14 +------ .../impl/base/BaseAbstractRouteEntry.java | 4 +- .../yang/bgp/rib/impl/BGPPSMImplModule.java | 4 +- 5 files changed, 39 insertions(+), 34 deletions(-) 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 912349838b..7725180ceb 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,8 +7,11 @@ */ package org.opendaylight.protocol.bgp.mode.impl.add; +import com.google.common.collect.Lists; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Optional; import javax.annotation.concurrent.NotThreadSafe; import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType; import org.opendaylight.controller.md.sal.dom.api.DOMDataWriteTransaction; @@ -43,8 +46,8 @@ import org.slf4j.LoggerFactory; public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { private static final Logger LOG = LoggerFactory.getLogger(AddPathAbstractRouteEntry.class); - protected List bestPath; - protected List bestPathRemoved; + private List bestPath; + private List bestPathRemoved; protected OffsetMap offsets = OffsetMap.EMPTY; protected ContainerNode[] values = new ContainerNode[0]; protected Long[] pathsId = new Long[0]; @@ -199,7 +202,7 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { return this.offsets.isEmpty(); } - protected void selectBest(final RouteKey key, final AddPathSelector selector) { + private void selectBest(final RouteKey key, final AddPathSelector selector) { final int offset = this.offsets.offsetOf(key); final ContainerNode attributes = this.offsets.getValue(this.values, offset); final Long pathId = this.offsets.getValue(this.pathsId, offset); @@ -219,16 +222,40 @@ public abstract class AddPathAbstractRouteEntry extends AbstractRouteEntry { * FIXME: optimize flaps by making sure we consider stability of currently-selected route. */ final AddPathSelector selector = new AddPathSelector(localAs); - keyList.forEach(key -> selectBest(key, selector)); + Lists.reverse(keyList).forEach(key -> selectBest(key, selector)); LOG.trace("Best path selected {}", this.bestPath); return selector.result(); } - protected boolean isFirstBestPath(final int bestPathPosition) { + private boolean isFirstBestPath(final int bestPathPosition) { return bestPathPosition == 0; } private boolean peersSupportsAddPathOrIsFirstBestPath(final boolean peerSupportsAddPath, final boolean isFirstBestPath) { return !(!peerSupportsAddPath && !isFirstBestPath); } + + protected boolean isBestPathNew(final List newBestPathList) { + filterRemovedPaths(newBestPathList); + if (this.bestPathRemoved != null && !this.bestPathRemoved.isEmpty() || newBestPathList != null && !newBestPathList.equals(this.bestPath)) { + this.bestPath = newBestPathList; + LOG.trace("Actual Best {}, removed best {}", this.bestPath, this.bestPathRemoved); + return true; + } + return false; + } + + private void filterRemovedPaths(final List newBestPathList) { + if(this.bestPath == null) { + return; + } + this.bestPathRemoved = new ArrayList<>(this.bestPath); + this.bestPath.forEach(oldBest -> { + final Optional present = newBestPathList.stream() + .filter(newBest -> newBest.getPathId() == oldBest.getPathId() && newBest.getRouteKey() == oldBest.getRouteKey()).findAny(); + if(present.isPresent()) { + this.bestPathRemoved.remove(oldBest); + } + }); + } } diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/all/paths/AbstractAllPathsRouteEntry.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/all/paths/AbstractAllPathsRouteEntry.java index cc815b1649..c9916f33f0 100644 --- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/all/paths/AbstractAllPathsRouteEntry.java +++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/all/paths/AbstractAllPathsRouteEntry.java @@ -21,8 +21,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; abstract class AbstractAllPathsRouteEntry extends AddPathAbstractRouteEntry { - private static final Logger LOG = LoggerFactory.getLogger(AbstractAllPathsRouteEntry.class); - @Override public final boolean selectBest(final long localAs) { final List newBestPathList = new ArrayList<>(); @@ -45,16 +43,6 @@ abstract class AbstractAllPathsRouteEntry extends AddPathAbstractRouteEntry { } } } - - if(this.bestPath != null) { - this.bestPathRemoved = new ArrayList<>(this.bestPath); - } - - if (!newBestPathList.equals(this.bestPath) || this.bestPathRemoved != null && this.bestPathRemoved.removeAll(newBestPathList)) { - this.bestPath = newBestPathList; - LOG.trace("Actual Best {}, removed best {}", this.bestPath, this.bestPathRemoved); - return true; - } - return false; + return isBestPathNew(newBestPathList); } } diff --git a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/n/paths/AbstractNPathsRouteEntry.java b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/n/paths/AbstractNPathsRouteEntry.java index f95abcd586..f1faf1634a 100644 --- a/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/n/paths/AbstractNPathsRouteEntry.java +++ b/bgp/path-selection-mode/src/main/java/org/opendaylight/protocol/bgp/mode/impl/add/n/paths/AbstractNPathsRouteEntry.java @@ -17,8 +17,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; abstract class AbstractNPathsRouteEntry extends AddPathAbstractRouteEntry { - private static final Logger LOG = LoggerFactory.getLogger(AbstractNPathsRouteEntry.class); - private final long nBestPaths; AbstractNPathsRouteEntry(final Long nBestPaths) { @@ -35,16 +33,6 @@ abstract class AbstractNPathsRouteEntry extends AddPathAbstractRouteEntry { newBestPathList.add(newBest); keyList.remove(newBest.getRouteKey()); } - - if(this.bestPath != null) { - this.bestPathRemoved = new ArrayList<>(this.bestPath); - } - if (!newBestPathList.equals(this.bestPath) || - this.bestPathRemoved != null && this.bestPathRemoved.removeAll(newBestPathList) && !this.bestPathRemoved.isEmpty()) { - this.bestPath = newBestPathList; - LOG.trace("Actual Best {}, removed best {}", this.bestPath, this.bestPathRemoved); - return true; - } - return false; + return isBestPathNew(newBestPathList); } } 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 9d984ee7e0..fe1cf37f0e 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 @@ -87,7 +87,9 @@ abstract class BaseAbstractRouteEntry extends AbstractRouteEntry { final BaseBestPath newBestPath = selector.result(); final boolean modified = newBestPath == null || !newBestPath.equals(this.bestPath); if (modified) { - this.removedBestPath = this.bestPath; + if(this.offsets.isEmpty()) { + this.removedBestPath = this.bestPath; + } LOG.trace("Previous best {}, current best {}", this.bestPath, newBestPath); this.bestPath = newBestPath; } diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/controller/config/yang/bgp/rib/impl/BGPPSMImplModule.java b/bgp/rib-impl/src/main/java/org/opendaylight/controller/config/yang/bgp/rib/impl/BGPPSMImplModule.java index 32d5860505..6347379aae 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/controller/config/yang/bgp/rib/impl/BGPPSMImplModule.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/controller/config/yang/bgp/rib/impl/BGPPSMImplModule.java @@ -31,14 +31,14 @@ public final class BGPPSMImplModule extends AbstractBGPPSMImplModule { private final BgpTableType pathFamilyDependency; private final PathSelectionMode strategyFactory; - public AutoCloseableBestPathSelectionStrategy(final BgpTableType pathFamilyDependency, final PathSelectionMode strategyFactory) { + AutoCloseableBestPathSelectionStrategy(final BgpTableType pathFamilyDependency, final PathSelectionMode strategyFactory) { this.pathFamilyDependency = Preconditions.checkNotNull(pathFamilyDependency); this.strategyFactory = Preconditions.checkNotNull(strategyFactory); } @Override public void close() throws Exception { - //no op + this.strategyFactory.close(); } @Override -- 2.36.6