Set nexthop to bestpaths loopback address

Summary: Set the proper nodes loopback address from which we learnt the best path. This ensures that when we migrate from openr to spr pod, there won't be any nexthop differences seen by bgp monitor. Even though these differences wont' cause any traffic issue, showing proper nexthop from which we learnt will be cleaner to debug and fbossdeploy bgp monitor difference issue is also addressed automatically.

Reviewed By: saifhhasan

Differential Revision: D15838802

fbshipit-source-id: 548408ca0a369e9c27722dfe989cfd28b2fefb87

openr/decision/Decision.cpp

@@ -919,6 +919,7 @@ SpfSolver::SpfSolverImpl::createBGPRoute(
     thrift::IpPrefix const& prefix,
     std::unordered_map<std::string, thrift::PrefixEntry> const& nodePrefixes,
     bool const isV4) {
+  std::string bestNode;
   std::unordered_set<std::string> nodes;
   thrift::MetricVector const* bestVector = nullptr;
   std::string const* bestData = nullptr;
@@ -940,6 +941,7 @@ SpfSolver::SpfSolverImpl::createBGPRoute(
     case MetricVectorUtils::CompareResult::TIE_WINNER:
       bestVector = &(metricVector);
       bestData = &(prefixEntry.data);
+      bestNode = name;
       FOLLY_FALLTHROUGH;
     case MetricVectorUtils::CompareResult::TIE_LOOSER:
       nodes.emplace(name);
@@ -959,14 +961,23 @@ SpfSolver::SpfSolverImpl::createBGPRoute(
     return folly::none;
   }
   CHECK_NOTNULL(bestData);
+  auto bestNexthop = getLoopbackVias({bestNode}, isV4);
+  if (bestNexthop.size() != 1) {
+    LOG(ERROR)
+        << "Cannot find the best paths loopback address. Skipping route for prefix: "
+        << toString(prefix);
+    return folly::none;
+  }
+
   return thrift::UnicastRoute{FRAGILE,
                               prefix,
                               {},
                               thrift::AdminDistance::EBGP,
                               getLoopbackVias(nodes, isV4),
                               thrift::PrefixType::BGP,
                               *bestData,
-                              bgpDryRun_ /* doNotProgram */};
+                              bgpDryRun_, /* doNotProgram */
+                              bestNexthop.at(0)};
 }
 
 std::vector<thrift::NextHopThrift>

openr/decision/tests/DecisionTest.cpp

@@ -623,7 +623,8 @@ TEST(BGPRedistribution, BasicOperation) {
       {createNextHop(addr1.prefixAddress)},
       thrift::PrefixType::BGP,
       data1,
-      false);
+      false,
+      createNextHop(addr1.prefixAddress));
   EXPECT_THAT(routeDb.value().unicastRoutes, testing::SizeIs(2));
   EXPECT_THAT(routeDb.value().unicastRoutes, testing::Contains(route1));
 
@@ -669,7 +670,8 @@ TEST(BGPRedistribution, BasicOperation) {
       {createNextHop(addr2.prefixAddress)},
       thrift::PrefixType::BGP,
       data2,
-      false);
+      false,
+      createNextHop(addr2.prefixAddress));
 
   routeDb = spfSolver.buildPaths("1");
   EXPECT_THAT(routeDb.value().unicastRoutes, testing::SizeIs(2));

openr/if/Network.thrift

@@ -94,4 +94,6 @@ struct UnicastRoute {
   5: optional PrefixType prefixType
   6: optional binary data
   7: bool doNotInstall = false
+
+  41: optional NextHopThrift bestNexthop
 }