Change route table rib to be a radix tree instead of NodeMap

Summary:
The crux of the change here is replacing NodeMap rib
with a radix tree. This makes the lookups much faster than scanning
the node map for LPM. Clone and publish apis have been implemented
to publish and clone contained routes.

We still use NodeMap when computing RouteTable deltas so we
take the rib and convert it into a NodeMap during delta computation.
Yogi is looking at more optimal ways of doing radix tree diffs.
There is some janky ness here with ownership for these on the
fly NodeMaps being passed to NodeMapDelta, we could perhaps do
better with template type traits and just pass unique_ptrs in
this case. But we may revise this if we get to a better approach
of diffing radix trees anyways.

Another change is when deduping routes we can't rely on ribs
being NodeMaps. So I do the following
- For all routes that are in both oldRib and newRib
 a) If attributes are same, copy oldRoute into the newRib
 b) If attributes are different inherit generation number
If any route from oldRib does not exist in newRib, or if
the sizes are different we consider the ribs as not being
same.

This probably needs more polishing, but sending it out for
early feedback.

Reviewed By: yogeshwer

Differential Revision: D2436211

fb-gh-sync-id: b3bec59f8fd4e818b23e0567da72928ebb61fd67

fboss/agent/Main.cpp

@@ -53,6 +53,9 @@ DEFINE_int32(stat_publish_interval_ms, 1000,
              "How frequently to publish thread-local stats back to the "
              "global store.  This should generally be less than 1 second.");
 DEFINE_int32(thrift_idle_timeout, 60, "Thrift idle timeout in seconds.");
+// Programming 16K routes can take 20+ seconds
+DEFINE_int32(thrift_task_expire_timeout, 30,
+    "Thrift task expire timeout in seconds.");
 DEFINE_bool(tun_intf, true,
             "Create tun interfaces to allow other processes to "
             "send and receive traffic via the switch ports");
@@ -266,6 +269,8 @@ int fbossMain(int argc, char** argv, PlatformInitFn initPlatform) {
 
   // Start the thrift server
   ThriftServer server;
+  server.setTaskExpireTime(std::chrono::milliseconds(
+        thrift_task_expire_timeout * 1000));
   server.getEventBaseManager()->setEventBase(&eventBase, false);
   server.setInterface(handler);
   server.setDuplex(true);

fboss/agent/ThriftHandler.cpp

@@ -485,9 +485,9 @@ void ThriftHandler::getPortStatus(map<int32_t, PortStatus>& statusMap,
 void ThriftHandler::getRouteTable(std::vector<UnicastRoute>& route) {
   ensureConfigured();
   for (const auto& routeTable : (*sw_->getState()->getRouteTables())) {
-    for (const auto& ipv4Rib : routeTable->getRibV4()->getAllNodes()) {
+    for (const auto& ipv4Rib : routeTable->getRibV4()->routes()) {
       UnicastRoute tempRoute;
-      auto ipv4 = ipv4Rib.second.get();
+      auto ipv4 = ipv4Rib.value().get();
       auto fwdInfo = ipv4->getForwardInfo();
       tempRoute.dest.ip = toBinaryAddress(ipv4->prefix().network);
       tempRoute.dest.prefixLength = ipv4->prefix().mask;
@@ -497,9 +497,9 @@ void ThriftHandler::getRouteTable(std::vector<UnicastRoute>& route) {
       }
       route.push_back(tempRoute);
     }
-    for (const auto& ipv6Rib : routeTable->getRibV6()->getAllNodes()) {
+    for (const auto& ipv6Rib : routeTable->getRibV6()->routes()) {
       UnicastRoute tempRoute;
-      auto ipv6 = ipv6Rib.second.get();
+      auto ipv6 = ipv6Rib.value().get();
       auto fwdInfo = ipv6->getForwardInfo();
       tempRoute.dest.ip = toBinaryAddress(
                                                      ipv6->prefix().network);

fboss/agent/hw/bcm/BcmSwitch.cpp

@@ -43,6 +43,7 @@
 #include "fboss/agent/state/Interface.h"
 #include "fboss/agent/state/InterfaceMap.h"
 #include "fboss/agent/state/NodeMapDelta.h"
+#include "fboss/agent/state/NodeMapDelta-defs.h"
 #include "fboss/agent/state/Port.h"
 #include "fboss/agent/state/PortMap.h"
 #include "fboss/agent/state/StateDelta.h"

fboss/agent/state/NodeMapDelta-defs.h

@@ -13,15 +13,16 @@
 
 namespace facebook { namespace fboss {
 
-template<typename MAP, typename VALUE>
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
 std::shared_ptr<typename MAP::Node>
-  NodeMapDelta<MAP, VALUE>::Iterator::nullNode_;
+  NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator::nullNode_;
 
-template<typename MAP, typename VALUE>
-NodeMapDelta<MAP, VALUE>::Iterator::Iterator(const MapType* oldMap,
-                                             typename MapType::Iterator oldIt,
-                                             const MapType* newMap,
-                                             typename MapType::Iterator newIt)
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator::Iterator(
+    const MapType* oldMap,
+    typename MapType::Iterator oldIt,
+    const MapType* newMap,
+    typename MapType::Iterator newIt)
   : oldIt_(oldIt),
     newIt_(newIt),
     oldMap_(oldMap),
@@ -37,17 +38,17 @@ NodeMapDelta<MAP, VALUE>::Iterator::Iterator(const MapType* oldMap,
   updateValue();
 }
 
-template<typename MAP, typename VALUE>
-NodeMapDelta<MAP, VALUE>::Iterator::Iterator()
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator::Iterator()
   : oldIt_(),
     newIt_(),
     oldMap_(nullptr),
     newMap_(nullptr),
     value_(nullNode_, nullNode_) {
 }
 
-template<typename MAP, typename VALUE>
-void NodeMapDelta<MAP, VALUE>::Iterator::updateValue() {
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+void NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator::updateValue() {
   if (oldIt_ == oldMap_->end()) {
     if (newIt_ == newMap_->end()) {
       value_.reset(nullNode_, nullNode_);
@@ -71,8 +72,8 @@ void NodeMapDelta<MAP, VALUE>::Iterator::updateValue() {
   }
 }
 
-template<typename MAP, typename VALUE>
-void NodeMapDelta<MAP, VALUE>::Iterator::advance() {
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+void NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator::advance() {
   // If we have already hit the end of one side, advance the other.
   // We are immediately done after this.
   if (oldIt_ == oldMap_->end()) {

fboss/agent/state/NodeMapDelta.h

@@ -21,29 +21,60 @@ namespace facebook { namespace fboss {
 template<typename NODE>
 class DeltaValue;
 
+template<typename MAP>
+class MapPointerTraits {
+ public:
+   using RawConstPointerType = const MAP*;
+   // Embed const in MapPointerType for raw pointers.
+   // Since we want to use both raw and unique_ptr,
+   // we don't want to add const to the Map pointer type
+   // in function declarations, where we will want to move
+   // from unique_ptr (moving from const unique_ptr is not
+   // permitted since its a modifying operation).
+   using MapPointerType = const MAP*;
+   static RawConstPointerType getRawPointer(MapPointerType map) {
+      return map;
+   }
+};
+
+template <typename MAP>
+class MapUniquePointerTraits {
+ public:
+   using RawConstPointerType = const MAP*;
+   // Non const MapPointer type, const unique_ptr is
+   // severly restrictive when received as a function
+   // argument, since it can't be moved from.
+   using MapPointerType = std::unique_ptr<MAP>;
+   static RawConstPointerType getRawPointer(const MapPointerType& map) {
+      return map.get();
+   }
+};
 /*
  * NodeMapDelta contains code for examining the differences between two NodeMap
  * objects.
  *
  * The main function of this class is the Iterator that it provides.  This
  * allows caller to walk over the changed, added, and removed nodes.
  */
-template<typename MAP, typename VALUE = DeltaValue<typename MAP::Node>>
+template<typename MAP,
+  typename VALUE = DeltaValue<typename MAP::Node>,
+  typename MAPPOINTERTRAITS = MapPointerTraits<MAP>>
 class NodeMapDelta {
  public:
-  typedef MAP MapType;
-  typedef typename MAP::Node Node;
+  using MapPointerType = typename MAPPOINTERTRAITS::MapPointerType;
+  using RawConstPointerType = typename MAPPOINTERTRAITS::RawConstPointerType;
+  using Node = typename MAP::Node;
   class Iterator;
 
-  NodeMapDelta(const MapType* oldMap, const MapType* newMap)
-    : old_(oldMap),
-      new_(newMap) {}
+  NodeMapDelta(MapPointerType&& oldMap, MapPointerType&& newMap)
+    : old_(std::move(oldMap)),
+      new_(std::move(newMap)) {}
 
-  const MapType* getOld() const {
-    return old_;
+  RawConstPointerType getOld() const {
+    return MAPPOINTERTRAITS::getRawPointer(old_);
   }
-  const MapType* getNew() const {
-    return new_;
+  RawConstPointerType getNew() const {
+    return MAPPOINTERTRAITS::getRawPointer(new_);
   }
 
   /*
@@ -58,12 +89,17 @@ class NodeMapDelta {
 
  private:
   /*
-   * Note that we assume NodeMapDelta is always used by StateDelta.  StateDelta
-   * holds a shared_ptr to the old and new SwitchState objects, so that we can
-   * use raw pointers here, rather than shared_ptrs.
+   * NodeMapDelta is used by StateDelta.  StateDelta holds a shared_ptr to
+   * the old and new SwitchState objects, which in turn holds
+   * shared_ptrs to NodeMaps, hence in the common case use raw pointers here
+   * and don't bother with ownership. However there are cases where collections
+   * are not easily mapped to a NodeMap structure, but we still want to box
+   * them into a NodeMap while computing delta. In this case NodeMap is created
+   * on the fly and we pass ownership to NodeMapDelta object via a unique_ptr.
+   * See MapPointerTraits and MapUniquePointerTraits classes for details.
    */
-  const MapType* old_;
-  const MapType* new_;
+  MapPointerType old_;
+  MapPointerType new_;
 };
 
 template<typename NODE>
@@ -102,8 +138,8 @@ class DeltaValue {
  * An iterator for walking over the Nodes that changed between the two
  * NodeMaps.
  */
-template<typename MAP, typename VALUE>
-class NodeMapDelta<MAP, VALUE>::Iterator {
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+class NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator {
  public:
   typedef MAP MapType;
   typedef typename MAP::Node Node;
@@ -161,35 +197,35 @@ class NodeMapDelta<MAP, VALUE>::Iterator {
   static std::shared_ptr<Node> nullNode_;
 };
 
-template<typename MAP, typename VALUE>
-typename NodeMapDelta<MAP, VALUE>::Iterator
-NodeMapDelta<MAP, VALUE>::begin() const {
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+typename NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator
+NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::begin() const {
   if (old_ == new_) {
     return end();
   }
   // To support deltas where the old node is null (to represent newly created
   // nodes), point the old side of the iterator at the new node, but start it
   // at the end of the map.
   if (!old_) {
-    return Iterator(new_, new_->end(), new_, new_->begin());
+    return Iterator(getNew(), new_->end(), getNew(), new_->begin());
   }
   // And vice-versa for the new node being null (to represent removed nodes).
   if (!new_) {
-    return Iterator(old_, old_->begin(), old_, old_->end());
+    return Iterator(getOld(), old_->begin(), getOld(), old_->end());
   }
-  return Iterator(old_, old_->begin(), new_, new_->begin());
+  return Iterator(getOld(), old_->begin(), getNew(), new_->begin());
 }
 
-template<typename MAP, typename VALUE>
-typename NodeMapDelta<MAP, VALUE>::Iterator
-NodeMapDelta<MAP, VALUE>::end() const {
+template<typename MAP, typename VALUE, typename MAPPOINTERTRAITS>
+typename NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::Iterator
+NodeMapDelta<MAP, VALUE, MAPPOINTERTRAITS>::end() const {
   if (!old_) {
-    return Iterator(new_, new_->end(), new_, new_->end());
+    return Iterator(getNew(), new_->end(), getNew(), new_->end());
   }
   if (!new_) {
-    return Iterator(old_, old_->end(), old_, old_->end());
+    return Iterator(getOld(), old_->end(), getOld(), old_->end());
   }
-  return Iterator(old_, old_->end(), new_, new_->end());
+  return Iterator(getOld(), old_->end(), getNew(), new_->end());
 }
 
 }} // facebook::fboss

fboss/agent/state/RouteDelta.h

@@ -14,20 +14,45 @@
 #include "fboss/agent/state/RouteTable.h"
 #include "fboss/agent/state/RouteTableMap.h"
 
+
 namespace facebook { namespace fboss {
 
 class RouteTablesDelta : public DeltaValue<RouteTable> {
  public:
-  typedef NodeMapDelta<RouteTableRib<folly::IPAddressV4>> RoutesV4Delta;
-  typedef NodeMapDelta<RouteTableRib<folly::IPAddressV6>> RoutesV6Delta;
+  using NodeMapRibV4 = RouteTableRibNodeMap<folly::IPAddressV4>;
+  using NodeMapRibV6 = RouteTableRibNodeMap<folly::IPAddressV6>;
+  using RoutesV4Delta = NodeMapDelta<NodeMapRibV4,
+        DeltaValue<NodeMapRibV4::Node>,
+        MapUniquePointerTraits<NodeMapRibV4>>;
+  using RoutesV6Delta = NodeMapDelta<NodeMapRibV6,
+        DeltaValue<NodeMapRibV6::Node>,
+        MapUniquePointerTraits<NodeMapRibV6>>;
+
   using DeltaValue<RouteTable>::DeltaValue;
+
   RoutesV4Delta getRoutesV4Delta() const {
-    return RoutesV4Delta(getOld() ? getOld()->getRibV4().get() : nullptr,
-                         getNew() ? getNew()->getRibV4().get() : nullptr);
+    std::unique_ptr<NodeMapRibV4> oldRib, newRib;
+    if (getOld()) {
+      oldRib.reset(new NodeMapRibV4());
+      oldRib->addRoutes(*(getOld()->getRibV4()));
+    }
+    if (getNew()) {
+      newRib.reset(new NodeMapRibV4());
+      newRib->addRoutes(*(getNew()->getRibV4()));
+    }
+    return RoutesV4Delta(std::move(oldRib), std::move(newRib));
   }
-  RoutesV6Delta getRoutesV6Delta() const {
-    return RoutesV6Delta(getOld() ? getOld()->getRibV6().get() : nullptr,
-                         getNew() ? getNew()->getRibV6().get() : nullptr);
+  RoutesV6Delta getRoutesV6Delta()  const {
+    std::unique_ptr<NodeMapRibV6> oldRib, newRib;
+    if (getOld()) {
+      oldRib.reset(new NodeMapRibV6());
+      oldRib->addRoutes(*(getOld()->getRibV6()));
+    }
+    if (getNew()) {
+      newRib.reset(new NodeMapRibV6());
+      newRib->addRoutes(*(getNew()->getRibV6()));
+    }
+    return RoutesV6Delta(std::move(oldRib), std::move(newRib));
   }
 };
 

fboss/agent/state/RouteTableRib.cpp

@@ -12,36 +12,39 @@
 #include "fboss/agent/state/NodeMap-defs.h"
 #include "fboss/agent/state/Route.h"
 
+namespace {
+constexpr auto kRoutes = "routes";
+}
+
 namespace facebook { namespace fboss {
 
-template<typename AddrT>
-RouteTableRib<AddrT>::RouteTableRib() {
-}
+FBOSS_INSTANTIATE_NODE_MAP(RouteTableRibNodeMap<folly::IPAddressV4>,
+                           RouteTableRibNodeMapTraits<folly::IPAddressV4>);
+FBOSS_INSTANTIATE_NODE_MAP(RouteTableRibNodeMap<folly::IPAddressV6>,
+                           RouteTableRibNodeMapTraits<folly::IPAddressV6>);
 
 template<typename AddrT>
-RouteTableRib<AddrT>::~RouteTableRib() {
+folly::dynamic RouteTableRib<AddrT>::toFollyDynamic() const {
+  std::vector<folly::dynamic> routesJson;
+  for (const auto& route: rib_) {
+    routesJson.emplace_back(route->value()->toFollyDynamic());
+  }
+  folly::dynamic routes = folly::dynamic::object;
+  routes[kRoutes] = routesJson;
+  return routes;
 }
 
 template<typename AddrT>
-std::shared_ptr<Route<AddrT>> RouteTableRib<AddrT>::longestMatch(
-    const AddrT& nexthop) const {
-  std::shared_ptr<Route<AddrT>> bestMatch;
-  int bestMatchMask = -1;
-  for (const auto& rt : Base::getAllNodes()) {
-    const auto& prefix = rt.first;
-    if (prefix.mask > bestMatchMask
-        && nexthop.inSubnet(prefix.network, prefix.mask)) {
-      bestMatch = rt.second;
-      bestMatchMask = prefix.mask;
-    }
+std::shared_ptr<RouteTableRib<AddrT>>
+RouteTableRib<AddrT>::fromFollyDynamic(const folly::dynamic& routes) {
+  auto rib = std::make_shared<RouteTableRib<AddrT>>();
+  auto routesJson = routes[kRoutes];
+  for (const auto& routeJson: routesJson) {
+    rib->addRoute(Route<AddrT>::fromFollyDynamic(routeJson));
   }
-  return bestMatch;
+  return rib;
 }
 
-FBOSS_INSTANTIATE_NODE_MAP(RouteTableRib<folly::IPAddressV4>,
-                           RouteTableRibTraits<folly::IPAddressV4>);
-FBOSS_INSTANTIATE_NODE_MAP(RouteTableRib<folly::IPAddressV6>,
-                           RouteTableRibTraits<folly::IPAddressV6>);
 template class RouteTableRib<folly::IPAddressV4>;
 template class RouteTableRib<folly::IPAddressV6>;