Skip to content

Commit

Permalink
Attr to distinguish between LFA vs 2nd-shortest path
Browse files Browse the repository at this point in the history 
Summary:
Fib module forwards only smallest metric nexthops to FibAgent. This is inteded
to not program LFA routes and keep them in backup.

However in case of KSP2 we want both shortest and non-shortest paths to be
programmed. Adding `useNonShortestRoute` attribute to nextHop to achieve the
same. Nexthops that have this attribute set will always be programmed

Reviewed By: jstrizich

Differential Revision: D15728459

fbshipit-source-id: 46170e55ce8906fc3d73a44d3a646535f6ae319a
  • Loading branch information
@saifhhasan @facebook-github-bot
saifhhasan authored and facebook-github-bot committed Jun 26, 2019
1 parent 150ad06 commit 7876662
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 54 deletions.
2 changes: 1 addition & 1 deletion openr/common/Util.cpp
View file
Expand Up @@ -426,7 +426,7 @@ getBestNextHopsUnicast(std::vector<thrift::NextHopThrift> const& allNextHops) {
// Find nextHops with the minimum cost
std::vector<thrift::NextHopThrift> bestNextHops;
for (auto const& nextHop : allNextHops) {
if (nextHop.metric == minCost) {
if (nextHop.metric == minCost or nextHop.useNonShortestRoute) {
bestNextHops.emplace_back(nextHop);
}
}
Expand Down
4 changes: 3 additions & 1 deletion openr/common/Util.h
View file
Expand Up @@ -331,12 +331,14 @@ createNextHop(
thrift::BinaryAddress addr,
const std::string& ifName = "",
int32_t metric = 0,
folly::Optional<thrift::MplsAction> maybeMplsAction = folly::none) {
folly::Optional<thrift::MplsAction> maybeMplsAction = folly::none,
bool useNonShortestRoute = false) {
thrift::NextHopThrift nextHop;
nextHop.address = addr;
nextHop.address.ifName = ifName;
nextHop.metric = metric;
nextHop.mplsAction = maybeMplsAction;
nextHop.useNonShortestRoute = useNonShortestRoute;
return nextHop;
}

Expand Down
9 changes: 9 additions & 0 deletions openr/common/tests/UtilTest.cpp
View file
Expand Up @@ -413,6 +413,15 @@ TEST(UtilTest, getBestNextHopsUnicast) {
getBestNextHopsUnicast({path1_2_1, path1_2_2, path1_2_3, path1_3_1});
EXPECT_EQ(
bestNextHops, std::vector<thrift::NextHopThrift>({path1_2_1, path1_3_1}));

auto path1_2_2_updated = path1_2_2;
path1_2_2_updated.useNonShortestRoute = true;
bestNextHops = getBestNextHopsUnicast(
{path1_2_1, path1_2_2_updated, path1_2_3, path1_3_1});
EXPECT_EQ(
bestNextHops,
std::vector<thrift::NextHopThrift>(
{path1_2_1, path1_2_2_updated, path1_3_1}));
}

TEST(UtilTest, getBestNextHopsMpls) {
Expand Down
3 changes: 2 additions & 1 deletion openr/decision/Decision.cpp
View file
Expand Up @@ -1215,7 +1215,8 @@ SpfSolver::SpfSolverImpl::createOpenRKsp2EdRoute(
: firstLink->getNhV6FromNode(myNodeName),
firstLink->getIfaceFromNode(myNodeName),
pathCost,
std::move(mplsAction)));
std::move(mplsAction),
true /* useNonShortestRoute */));
}

return std::move(route);
Expand Down
105 changes: 54 additions & 51 deletions openr/decision/tests/DecisionTest.cpp
View file
Expand Up @@ -109,7 +109,8 @@ const thrift::NextHopThrift labelPopNextHop{
toBinaryAddress(folly::IPAddressV6("::")),
0 /* weight */,
labelPopAction,
0 /* metric */};
0 /* metric */,
false /* useNonShortestRoute */};

// timeout to wait until decision debounce
// (i.e. spf recalculation, route rebuild) finished
Expand All @@ -130,12 +131,14 @@ createNextHopFromAdj(
thrift::Adjacency adj,
bool isV4,
int32_t metric,
folly::Optional<thrift::MplsAction> mplsAction = folly::none) {
folly::Optional<thrift::MplsAction> mplsAction = folly::none,
bool useNonShortestRoute = false) {
return createNextHop(
isV4 ? adj.nextHopV4 : adj.nextHopV6,
adj.ifName,
metric,
std::move(mplsAction));
std::move(mplsAction),
useNonShortestRoute);
}

// Note: use unordered_set bcoz paths in a route can be in arbitrary order
Expand Down Expand Up @@ -1348,25 +1351,25 @@ TEST_P(SimpleRingTopologyFixture, Ksp2EdEcmp) {
// validate router 1
EXPECT_EQ(
routeMap[make_pair("1", toString(v4Enabled ? addr4V4 : addr4))],
NextHops({createNextHopFromAdj(adj12, v4Enabled, 20, push4),
createNextHopFromAdj(adj13, v4Enabled, 20, push4)}));
NextHops({createNextHopFromAdj(adj12, v4Enabled, 20, push4, true),
createNextHopFromAdj(adj13, v4Enabled, 20, push4, true)}));
EXPECT_EQ(
routeMap[make_pair("1", std::to_string(adjacencyDb4.nodeLabel))],
NextHops({createNextHopFromAdj(adj12, false, 20, labelSwapAction4),
createNextHopFromAdj(adj13, false, 20, labelSwapAction4)}));

EXPECT_EQ(
routeMap[make_pair("1", toString(v4Enabled ? addr3V4 : addr3))],
NextHops({createNextHopFromAdj(adj13, v4Enabled, 10),
createNextHopFromAdj(adj12, v4Enabled, 30, push34)}));
NextHops({createNextHopFromAdj(adj13, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj12, v4Enabled, 30, push34, true)}));
EXPECT_EQ(
routeMap[make_pair("1", std::to_string(adjacencyDb3.nodeLabel))],
NextHops({createNextHopFromAdj(adj13, false, 10, labelPhpAction)}));

EXPECT_EQ(
routeMap[make_pair("1", toString(v4Enabled ? addr2V4 : addr2))],
NextHops({createNextHopFromAdj(adj12, v4Enabled, 10),
createNextHopFromAdj(adj13, v4Enabled, 30, push24)}));
NextHops({createNextHopFromAdj(adj12, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj13, v4Enabled, 30, push24, true)}));
EXPECT_EQ(
routeMap[make_pair("1", std::to_string(adjacencyDb2.nodeLabel))],
NextHops({createNextHopFromAdj(adj12, false, 10, labelPhpAction)}));
Expand All @@ -1378,25 +1381,25 @@ TEST_P(SimpleRingTopologyFixture, Ksp2EdEcmp) {

EXPECT_EQ(
routeMap[make_pair("2", toString(v4Enabled ? addr4V4 : addr4))],
NextHops({createNextHopFromAdj(adj24, v4Enabled, 10),
createNextHopFromAdj(adj21, v4Enabled, 30, push43)}));
NextHops({createNextHopFromAdj(adj24, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj21, v4Enabled, 30, push43, true)}));
EXPECT_EQ(
routeMap[make_pair("2", std::to_string(adjacencyDb4.nodeLabel))],
NextHops({createNextHopFromAdj(adj24, false, 10, labelPhpAction)}));

EXPECT_EQ(
routeMap[make_pair("2", toString(v4Enabled ? addr3V4 : addr3))],
NextHops({createNextHopFromAdj(adj21, v4Enabled, 20, push3),
createNextHopFromAdj(adj24, v4Enabled, 20, push3)}));
NextHops({createNextHopFromAdj(adj21, v4Enabled, 20, push3, true),
createNextHopFromAdj(adj24, v4Enabled, 20, push3, true)}));
EXPECT_EQ(
routeMap[make_pair("2", std::to_string(adjacencyDb3.nodeLabel))],
NextHops({createNextHopFromAdj(adj21, false, 20, labelSwapAction3),
createNextHopFromAdj(adj24, false, 20, labelSwapAction3)}));

EXPECT_EQ(
routeMap[make_pair("2", toString(v4Enabled ? addr1V4 : addr1))],
NextHops({createNextHopFromAdj(adj21, v4Enabled, 10),
createNextHopFromAdj(adj24, v4Enabled, 30, push13)}));
NextHops({createNextHopFromAdj(adj21, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj24, v4Enabled, 30, push13, true)}));
EXPECT_EQ(
routeMap[make_pair("2", std::to_string(adjacencyDb1.nodeLabel))],
NextHops({createNextHopFromAdj(adj21, false, 10, labelPhpAction)}));
Expand All @@ -1408,25 +1411,25 @@ TEST_P(SimpleRingTopologyFixture, Ksp2EdEcmp) {

EXPECT_EQ(
routeMap[make_pair("3", toString(v4Enabled ? addr4V4 : addr4))],
NextHops({createNextHopFromAdj(adj34, v4Enabled, 10),
createNextHopFromAdj(adj31, v4Enabled, 30, push42)}));
NextHops({createNextHopFromAdj(adj34, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj31, v4Enabled, 30, push42, true)}));
EXPECT_EQ(
routeMap[make_pair("3", std::to_string(adjacencyDb4.nodeLabel))],
NextHops({createNextHopFromAdj(adj34, false, 10, labelPhpAction)}));

EXPECT_EQ(
routeMap[make_pair("3", toString(v4Enabled ? addr2V4 : addr2))],
NextHops({createNextHopFromAdj(adj31, v4Enabled, 20, push2),
createNextHopFromAdj(adj34, v4Enabled, 20, push2)}));
NextHops({createNextHopFromAdj(adj31, v4Enabled, 20, push2, true),
createNextHopFromAdj(adj34, v4Enabled, 20, push2, true)}));
EXPECT_EQ(
routeMap[make_pair("3", std::to_string(adjacencyDb2.nodeLabel))],
NextHops({createNextHopFromAdj(adj31, false, 20, labelSwapAction2),
createNextHopFromAdj(adj34, false, 20, labelSwapAction2)}));

EXPECT_EQ(
routeMap[make_pair("3", toString(v4Enabled ? addr1V4 : addr1))],
NextHops({createNextHopFromAdj(adj31, v4Enabled, 10),
createNextHopFromAdj(adj34, v4Enabled, 30, push12)}));
NextHops({createNextHopFromAdj(adj31, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj34, v4Enabled, 30, push12, true)}));
EXPECT_EQ(
routeMap[make_pair("3", std::to_string(adjacencyDb1.nodeLabel))],
NextHops({createNextHopFromAdj(adj31, false, 10, labelPhpAction)}));
Expand All @@ -1438,24 +1441,24 @@ TEST_P(SimpleRingTopologyFixture, Ksp2EdEcmp) {

EXPECT_EQ(
routeMap[make_pair("4", toString(v4Enabled ? addr3V4 : addr3))],
NextHops({createNextHopFromAdj(adj43, v4Enabled, 10),
createNextHopFromAdj(adj42, v4Enabled, 30, push31)}));
NextHops({createNextHopFromAdj(adj43, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj42, v4Enabled, 30, push31, true)}));
EXPECT_EQ(
routeMap[make_pair("4", std::to_string(adjacencyDb3.nodeLabel))],
NextHops({createNextHopFromAdj(adj43, false, 10, labelPhpAction)}));

EXPECT_EQ(
routeMap[make_pair("4", toString(v4Enabled ? addr2V4 : addr2))],
NextHops({createNextHopFromAdj(adj42, v4Enabled, 10),
createNextHopFromAdj(adj43, v4Enabled, 30, push21)}));
NextHops({createNextHopFromAdj(adj42, v4Enabled, 10, folly::none, true),
createNextHopFromAdj(adj43, v4Enabled, 30, push21, true)}));
EXPECT_EQ(
routeMap[make_pair("4", std::to_string(adjacencyDb2.nodeLabel))],
NextHops({createNextHopFromAdj(adj42, false, 10, labelPhpAction)}));

EXPECT_EQ(
routeMap[make_pair("4", toString(v4Enabled ? addr1V4 : addr1))],
NextHops({createNextHopFromAdj(adj42, v4Enabled, 20, push1),
createNextHopFromAdj(adj43, v4Enabled, 20, push1)}));
NextHops({createNextHopFromAdj(adj42, v4Enabled, 20, push1, true),
createNextHopFromAdj(adj43, v4Enabled, 20, push1, true)}));
EXPECT_EQ(
routeMap[make_pair("4", std::to_string(adjacencyDb1.nodeLabel))],
NextHops({createNextHopFromAdj(adj42, false, 20, labelSwapAction1),
Expand Down Expand Up @@ -2243,63 +2246,63 @@ TEST_F(ParallelAdjRingTopologyFixture, Ksp2EdEcmp) {

EXPECT_EQ(
routeMap[make_pair("1", toString(addr4))],
NextHops({createNextHopFromAdj(adj12_1, false, 22, push4),
createNextHopFromAdj(adj13_1, false, 22, push4)}));
NextHops({createNextHopFromAdj(adj12_1, false, 22, push4, true),
createNextHopFromAdj(adj13_1, false, 22, push4, true)}));

EXPECT_EQ(
routeMap[make_pair("1", toString(addr3))],
NextHops({createNextHopFromAdj(adj13_1, false, 11),
createNextHopFromAdj(adj12_1, false, 33, push34)}));
NextHops({createNextHopFromAdj(adj13_1, false, 11, folly::none, true),
createNextHopFromAdj(adj12_1, false, 33, push34, true)}));
EXPECT_EQ(
routeMap[make_pair("1", toString(addr2))],
NextHops({createNextHopFromAdj(adj12_1, false, 11),
createNextHopFromAdj(adj12_3, false, 20)}));
NextHops({createNextHopFromAdj(adj12_1, false, 11, folly::none, true),
createNextHopFromAdj(adj12_3, false, 20, folly::none, true)}));

// validate router 2

EXPECT_EQ(
routeMap[make_pair("2", toString(addr4))],
NextHops({createNextHopFromAdj(adj24_1, false, 11),
createNextHopFromAdj(adj21_1, false, 33, push43)}));
NextHops({createNextHopFromAdj(adj24_1, false, 11, folly::none, true),
createNextHopFromAdj(adj21_1, false, 33, push43, true)}));

EXPECT_EQ(
routeMap[make_pair("2", toString(addr3))],
NextHops({createNextHopFromAdj(adj21_1, false, 22, push3),
createNextHopFromAdj(adj24_1, false, 22, push3)}));
NextHops({createNextHopFromAdj(adj21_1, false, 22, push3, true),
createNextHopFromAdj(adj24_1, false, 22, push3, true)}));
EXPECT_EQ(
routeMap[make_pair("2", toString(addr1))],
NextHops({createNextHopFromAdj(adj21_1, false, 11),
createNextHopFromAdj(adj21_3, false, 20)}));
NextHops({createNextHopFromAdj(adj21_1, false, 11, folly::none, true),
createNextHopFromAdj(adj21_3, false, 20, folly::none, true)}));

// validate router 3

EXPECT_EQ(
routeMap[make_pair("3", toString(addr4))],
NextHops({createNextHopFromAdj(adj34_1, false, 11),
createNextHopFromAdj(adj34_3, false, 20)}));
NextHops({createNextHopFromAdj(adj34_1, false, 11, folly::none, true),
createNextHopFromAdj(adj34_3, false, 20, folly::none, true)}));
EXPECT_EQ(
routeMap[make_pair("3", toString(addr2))],
NextHops({createNextHopFromAdj(adj31_1, false, 22, push2),
createNextHopFromAdj(adj34_1, false, 22, push2)}));
NextHops({createNextHopFromAdj(adj31_1, false, 22, push2, true),
createNextHopFromAdj(adj34_1, false, 22, push2, true)}));
EXPECT_EQ(
routeMap[make_pair("3", toString(addr1))],
NextHops({createNextHopFromAdj(adj31_1, false, 11),
createNextHopFromAdj(adj34_1, false, 33, push12)}));
NextHops({createNextHopFromAdj(adj31_1, false, 11, folly::none, true),
createNextHopFromAdj(adj34_1, false, 33, push12, true)}));

// validate router 4

EXPECT_EQ(
routeMap[make_pair("4", toString(addr3))],
NextHops({createNextHopFromAdj(adj43_1, false, 11),
createNextHopFromAdj(adj43_3, false, 20)}));
NextHops({createNextHopFromAdj(adj43_1, false, 11, folly::none, true),
createNextHopFromAdj(adj43_3, false, 20, folly::none, true)}));
EXPECT_EQ(
routeMap[make_pair("4", toString(addr2))],
NextHops({createNextHopFromAdj(adj42_1, false, 11),
createNextHopFromAdj(adj43_1, false, 33, push21)}));
NextHops({createNextHopFromAdj(adj42_1, false, 11, folly::none, true),
createNextHopFromAdj(adj43_1, false, 33, push21, true)}));
EXPECT_EQ(
routeMap[make_pair("4", toString(addr1))],
NextHops({createNextHopFromAdj(adj42_1, false, 22, push1),
createNextHopFromAdj(adj43_1, false, 22, push1)}));
NextHops({createNextHopFromAdj(adj42_1, false, 22, push1, true),
createNextHopFromAdj(adj43_1, false, 22, push1, true)}));
}

/**
Expand Down
3 changes: 3 additions & 0 deletions openr/if/Network.thrift
View file
Expand Up @@ -63,6 +63,9 @@ struct NextHopThrift {

// Metric (aka cost) associated with this nexthop
51: i32 metric = 0

// Use non-shortest route (usually false but enabled for KSP2_ED_ECMP)
52: bool useNonShortestRoute = 0
}

struct MplsRoute {
Expand Down

0 comments on commit 7876662

Please sign in to comment.