From 6f6f7b0ee4638689b9871215fc7f7a25f196b793 Mon Sep 17 00:00:00 2001 From: Kency Kurian Date: Thu, 23 Feb 2017 20:15:57 +0530 Subject: [PATCH] BGP Manager changes to support ECMP. This review ensures that when 2 DC-GW advertises routes for the same destination prefix, the vrfEntry DS are populated correctly with both the route paths. The stale fib entry flat map now includes nextHop also in its key, to align with the new vrfEntry model. Change-Id: I26526561a74228925030f0086109dbe182b876c9 Signed-off-by: Kency Kurian --- .../bgpmanager/BgpConfigurationManager.java | 60 +++++++++++-------- .../netvirt/bgpmanager/BgpUtil.java | 2 + .../netvirt/bgpmanager/FibDSWriter.java | 46 +++++++++++++- .../thrift/server/BgpThriftService.java | 6 +- 4 files changed, 86 insertions(+), 28 deletions(-) diff --git a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpConfigurationManager.java b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpConfigurationManager.java index 986e410cf9..0b5dd520ad 100755 --- a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpConfigurationManager.java +++ b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpConfigurationManager.java @@ -198,8 +198,8 @@ public class BgpConfigurationManager { private static String cPortStartup; private static CountDownLatch initer = new CountDownLatch(1); //static IITMProvider itmProvider; - //map> - private static Map> staledFibEntriesMap = new ConcurrentHashMap<>(); + //map> + private static Map> staledFibEntriesMap = new ConcurrentHashMap<>(); static final String BGP_ENTITY_TYPE_FOR_OWNERSHIP = "bgp"; static final String BGP_ENTITY_NAME = "bgp"; @@ -1419,7 +1419,7 @@ public class BgpConfigurationManager { Iterator updates = routes.getUpdatesIterator(); while (updates.hasNext()) { Update update = updates.next(); - Map> staleFibRdMap = BgpConfigurationManager.getStaledFibEntriesMap(); + Map> staleFibRdMap = BgpConfigurationManager.getStaledFibEntriesMap(); String rd = update.getRd(); String nexthop = update.getNexthop(); @@ -1475,7 +1475,6 @@ public class BgpConfigurationManager { int label, String routermac) throws InterruptedException, ExecutionException, TimeoutException { - Map> staleFibRdMap = BgpConfigurationManager.getStaledFibEntriesMap(); boolean addroute = false; long l3vni = 0L; VrfEntry.EncapType encapType = VrfEntry.EncapType.Mplsgre; @@ -1491,17 +1490,18 @@ public class BgpConfigurationManager { } if (!staledFibEntriesMap.isEmpty()) { // restart Scenario, as MAP is not empty. - Map map = staledFibEntriesMap.get(rd); + Map map = staledFibEntriesMap.get(rd); if (map != null) { - String nexthoplabel = map.get(prefix + "/" + plen); - if (null == nexthoplabel) { + String prefixNextHop = appendNextHopToPrefix(prefix + "/" + plen, nextHop); + Long labelInStaleMap = map.get(prefixNextHop); + if (null == labelInStaleMap) { // New Entry, which happened to be added during restart. addroute = true; } else { - map.remove(prefix + "/" + plen); - if (isRouteModified(nextHop, label, nexthoplabel)) { + map.remove(prefixNextHop); + if (isRouteModified(label, labelInStaleMap)) { LOG.debug("Route add ** {} ** {}/{} ** {} ** {} ", rd, prefix, plen, nextHop, label); - // Existing entry, where in Nexthop/Label got modified during restart + // Existing entry, where in Label got modified during restart addroute = true; } } @@ -1519,8 +1519,8 @@ public class BgpConfigurationManager { } } - private static boolean isRouteModified(String nexthop, int label, String nexthoplabel) { - return !nexthoplabel.isEmpty() && !nexthoplabel.equals(nexthop + "/" + label); + private static boolean isRouteModified(int label, Long labelInStaleMap) { + return labelInStaleMap != null && !labelInStaleMap.equals(label); } private static void replayNbrConfig(List neighbors, BgpRouter br) { @@ -1972,15 +1972,18 @@ public class BgpConfigurationManager { if (Thread.interrupted()) { return 0; } - Map map = staledFibEntriesMap.get(rd); + Map map = staledFibEntriesMap.get(rd); if (map != null) { - for (String prefix : map.keySet()) { + for (String key : map.keySet()) { if (Thread.interrupted()) { return 0; } + String prefix = extractPrefix(key); + String nextHop = extractNextHop(key); totalCleared++; - LOG.debug("BGP: RouteCleanup deletePrefix called for : rd:{}, prefix{}", rd, prefix); - fibDSWriter.removeFibEntryFromDS(rd, prefix); + LOG.debug("BGP: RouteCleanup deletePrefix called for : rd:{}, prefix{}, nextHop:{}", + rd, prefix, nextHop); + fibDSWriter.removeOrUpdateFibEntryFromDS(rd, prefix, nextHop); } } } @@ -2025,7 +2028,7 @@ public class BgpConfigurationManager { if (fibEntries.isPresent()) { List staleVrfTables = fibEntries.get().getVrfTables(); for (VrfTables vrfTable : staleVrfTables) { - Map staleFibEntMap = new HashMap<>(); + Map staleFibEntMap = new HashMap<>(); for (VrfEntry vrfEntry : vrfTable.getVrfEntry()) { if (RouteOrigin.value(vrfEntry.getOrigin()) != RouteOrigin.BGP) { //Stale marking and cleanup is only meant for the routes learned through BGP. @@ -2039,9 +2042,8 @@ public class BgpConfigurationManager { vrfEntry.getRoutePaths() .forEach( routePath -> staleFibEntMap.put( - vrfEntry.getDestPrefix(), - routePath.getNexthopAddress() + "/" - + routePath.getLabel())); + appendNextHopToPrefix(vrfEntry.getDestPrefix(), + routePath.getNexthopAddress()), routePath.getLabel())); } staledFibEntriesMap.put(vrfTable.getRouteDistinguisher(), staleFibEntMap); } @@ -2054,15 +2056,15 @@ public class BgpConfigurationManager { LOG.error("created {} staled entries ", totalStaledCount); } - //map> - public static Map> getStaledFibEntriesMap() { + //map> + public static Map> getStaledFibEntriesMap() { return staledFibEntriesMap; } //TODO: below function is for testing purpose with cli public static void onUpdateWithdrawRoute(String rd, String prefix, int plen, String nexthop) { LOG.debug("Route del ** {} ** {}/{} ", rd, prefix, plen); - fibDSWriter.removeFibEntryFromDS(rd, prefix + "/" + plen); + fibDSWriter.removeOrUpdateFibEntryFromDS(rd, prefix + "/" + plen, nexthop); } public boolean isBgpConnected() { @@ -2151,5 +2153,15 @@ public class BgpConfigurationManager { return bgpAlarms; } -} + private static String appendNextHopToPrefix(String prefix, String nextHop) { + return prefix + ":" + nextHop; + } + + private static String extractPrefix(String prefixNextHop) { + return prefixNextHop.split(":")[0]; + } + private static String extractNextHop(String prefixNextHop) { + return prefixNextHop.split(":")[1]; + } +} diff --git a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpUtil.java b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpUtil.java index b4da09f9f4..bfb762007a 100755 --- a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpUtil.java +++ b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/BgpUtil.java @@ -9,6 +9,7 @@ package org.opendaylight.netvirt.bgpmanager; import com.google.common.base.Optional; import com.google.common.util.concurrent.ThreadFactoryBuilder; + import java.util.concurrent.BlockingQueue; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -17,6 +18,7 @@ import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; + import org.opendaylight.controller.md.sal.binding.api.BindingTransactionChain; import org.opendaylight.controller.md.sal.binding.api.DataBroker; import org.opendaylight.controller.md.sal.common.api.data.AsyncTransaction; diff --git a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/FibDSWriter.java b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/FibDSWriter.java index 378de64b75..1d7405b1c6 100644 --- a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/FibDSWriter.java +++ b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/FibDSWriter.java @@ -7,13 +7,17 @@ */ package org.opendaylight.netvirt.bgpmanager; +import com.google.common.base.Optional; import com.google.common.base.Preconditions; +import java.util.Collections; import java.util.List; import java.util.stream.Collectors; import org.opendaylight.controller.md.sal.binding.api.DataBroker; import org.opendaylight.controller.md.sal.common.api.data.LogicalDatastoreType; +import org.opendaylight.controller.md.sal.common.api.data.ReadFailedException; +import org.opendaylight.genius.datastoreutils.SingleTransactionDataBroker; import org.opendaylight.netvirt.fibmanager.api.FibHelper; import org.opendaylight.netvirt.fibmanager.api.RouteOrigin; import org.opendaylight.yang.gen.v1.urn.opendaylight.netvirt.fibmanager.rev150330.FibEntries; @@ -31,9 +35,11 @@ import org.slf4j.LoggerFactory; public class FibDSWriter { private static final Logger LOG = LoggerFactory.getLogger(FibDSWriter.class); private final DataBroker dataBroker; + private final SingleTransactionDataBroker singleTxDB; public FibDSWriter(final DataBroker dataBroker) { this.dataBroker = dataBroker; + this.singleTxDB = new SingleTransactionDataBroker(dataBroker); } public synchronized void addFibEntryToDS(String rd, String macAddress, String prefix, List nextHopList, @@ -51,7 +57,6 @@ public class FibDSWriter { return; } LOG.debug("Created vrfEntry for {} nexthop {} label {}", prefix, nextHop, label); - } // Looking for existing prefix in MDSAL database @@ -99,6 +104,45 @@ public class FibDSWriter { } + public synchronized void removeOrUpdateFibEntryFromDS(String rd, String prefix, String nextHop) { + + if (rd == null || rd.isEmpty()) { + LOG.error("Prefix {} not associated with vpn", prefix); + return; + } + LOG.debug("Removing fib entry with destination prefix {} from vrf table for rd {} and nextHop {}", + prefix, rd, nextHop); + try { + InstanceIdentifier vrfEntryId = + InstanceIdentifier.builder(FibEntries.class) + .child(VrfTables.class, new VrfTablesKey(rd)) + .child(VrfEntry.class, new VrfEntryKey(prefix)).build(); + Optional existingVrfEntry = + singleTxDB.syncReadOptional(LogicalDatastoreType.CONFIGURATION, vrfEntryId); + List routePaths = + existingVrfEntry.transform(VrfEntry::getRoutePaths).or(Collections.EMPTY_LIST); + if (routePaths.size() == 1) { + if (routePaths.get(0).getNexthopAddress().equals(nextHop)) { + BgpUtil.delete(dataBroker, LogicalDatastoreType.CONFIGURATION, vrfEntryId); + } + } else { + routePaths.stream() + .map(routePath -> routePath.getNexthopAddress()) + .filter(nextHopAddress -> nextHopAddress.equals(nextHop)) + .findFirst() + .ifPresent(nh -> { + InstanceIdentifier routePathId = + FibHelper.buildRoutePathId(rd, prefix, nextHop); + BgpUtil.delete(dataBroker, LogicalDatastoreType.CONFIGURATION, + routePathId); + }); + } + } catch (ReadFailedException e) { + LOG.error("Error while reading vrfEntry for rd {}, prefix {}", rd, prefix); + return; + } + } + public synchronized void removeVrfFromDS(String rd) { LOG.debug("Removing vrf table for rd {}", rd); diff --git a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/thrift/server/BgpThriftService.java b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/thrift/server/BgpThriftService.java index 84dd9d27de..59c4909e5a 100644 --- a/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/thrift/server/BgpThriftService.java +++ b/vpnservice/bgpmanager/bgpmanager-impl/src/main/java/org/opendaylight/netvirt/bgpmanager/thrift/server/BgpThriftService.java @@ -165,9 +165,9 @@ public class BgpThriftService { int l2label, int l3label) { LOGGER.debug("Route del ** {} ** {}/{} ", rd, prefix, plen); - LOGGER.info("REMOVE: Removing Fib entry rd {} prefix {}", rd, prefix); - fibDSWriter.removeFibEntryFromDS(rd, prefix + "/" + plen); - LOGGER.info("REMOVE: Removed Fib entry rd {} prefix {}", rd, prefix); + LOGGER.info("REMOVE: Removing Fib entry rd {} prefix {} nexthop {}", rd, prefix, nexthop); + fibDSWriter.removeOrUpdateFibEntryFromDS(rd, prefix + "/" + plen, nexthop); + LOGGER.info("REMOVE: Removed Fib entry rd {} prefix {} nexthop {}", rd, prefix, nexthop); } public void onStartConfigResyncNotification() { -- 2.36.6