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 <cgaspari@cisco.com>

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 912349838b6308a1437dcdd7d3eedf1350d37b0d..7725180ceb3612caa039e9b9400e47c686181916 100644 (file)
--- 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<AddPathBestPath> bestPath;
-    protected List<AddPathBestPath> bestPathRemoved;
+    private List<AddPathBestPath> bestPath;
+    private List<AddPathBestPath> 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<AddPathBestPath> 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<AddPathBestPath> newBestPathList) {
+        if(this.bestPath == null) {
+            return;
+        }
+        this.bestPathRemoved = new ArrayList<>(this.bestPath);
+        this.bestPath.forEach(oldBest -> {
+            final Optional<AddPathBestPath> 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 cc815b1649c8920f1342ec3342a364826896f0f0..c9916f33f0126e746ce8403b5ca991dff55fc98b 100644 (file)
--- 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<AddPathBestPath> 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 f95abcd586b1f09bb4bff10f6ef7d3f51c640222..f1faf1634ae02bc21bc016353d19bc2a7314e20c 100644 (file)
--- 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 9d984ee7e0edca0917ec94a4002a61d91818d22c..fe1cf37f0e325fdfe97dabde1bf15439c13718d1 100644 (file)
--- 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 32d58605050ccb6fd145227c86a6e0bfc44626f9..6347379aaee4c229fb6a13477bf8e95618912392 100644 (file)
--- 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