Skip to content

Commit

Permalink
(28/N): Remove static MPLS routes support in DecisionRouteUpdate
Browse files Browse the repository at this point in the history 
Summary:
PrefixManager does NOT publish static MPLS label routes into Decision for processing.

The inter-thread queue usage is the following:

```
  // PrefixManager -> Decision
  ReplicateQueue<DecisionRouteUpdate> staticRouteUpdatesQueue;
  auto decisionStaticRouteUpdatesQueueReader =
      staticRouteUpdatesQueue.getReader("decision");
```

Reviewed By: TangoRoxy

Differential Revision: D41751550

fbshipit-source-id: 64be42df4c8c449ecd041f07c362b20630dcc9ca
  • Loading branch information
@facebook-github-bot
Xiang Xu authored and facebook-github-bot committed Dec 7, 2022
1 parent 4265b9e commit 4d460ae
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 192 deletions.
47 changes: 21 additions & 26 deletions openr/decision/Decision.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -832,34 +832,29 @@ Decision::processPublication(thrift::Publication&& thriftPub) {

void
Decision::processStaticRoutesUpdate(DecisionRouteUpdate&& routeUpdate) {
// update static unicast routes
if (routeUpdate.unicastRoutesToUpdate.size() or
routeUpdate.unicastRoutesToDelete.size()) {
// store as local storage
spfSolver_->updateStaticUnicastRoutes(
routeUpdate.unicastRoutesToUpdate, routeUpdate.unicastRoutesToDelete);

// Create set of changed prefixes
std::unordered_set<folly::CIDRNetwork> changedPrefixes{
routeUpdate.unicastRoutesToDelete.cbegin(),
routeUpdate.unicastRoutesToDelete.cend()};
for (const auto& [prefix, ribUnicastEntry] :
routeUpdate.unicastRoutesToUpdate) {
changedPrefixes.emplace(prefix);
}

// only apply prefix updates, no full DB rebuild
pendingUpdates_.applyPrefixStateChange(
std::move(changedPrefixes), thrift::PrefixDatabase().perfEvents());
/*
* ATTN: static route processing ONLY applies to unicast routes
*/
CHECK(routeUpdate.mplsRoutesToUpdate.empty());
CHECK(routeUpdate.mplsRoutesToDelete.empty());

// store as local storage
spfSolver_->updateStaticUnicastRoutes(
routeUpdate.unicastRoutesToUpdate, routeUpdate.unicastRoutesToDelete);

// Create set of changed prefixes
std::unordered_set<folly::CIDRNetwork> changedPrefixes{
routeUpdate.unicastRoutesToDelete.cbegin(),
routeUpdate.unicastRoutesToDelete.cend()};
for (const auto& [prefix, ribUnicastEntry] :
routeUpdate.unicastRoutesToUpdate) {
changedPrefixes.emplace(prefix);
}

// update static MPLS routes
if (routeUpdate.mplsRoutesToUpdate.size() or
routeUpdate.mplsRoutesToDelete.size()) {
spfSolver_->updateStaticMplsRoutes(
routeUpdate.mplsRoutesToUpdate, routeUpdate.mplsRoutesToDelete);
pendingUpdates_.setNeedsFullRebuild(); // Mark for full DB rebuild
}
// only apply prefix updates, no full DB rebuild
pendingUpdates_.applyPrefixStateChange(
std::move(changedPrefixes), thrift::PrefixDatabase().perfEvents());

rebuildRoutesDebounced_();

auto prefixType = routeUpdate.prefixType;
Expand Down
34 changes: 0 additions & 34 deletions openr/decision/SpfSolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,33 +140,6 @@ SpfSolver::updateStaticUnicastRoutes(
}
}

void
SpfSolver::updateStaticMplsRoutes(
const std::unordered_map<int32_t, RibMplsEntry>& mplsRoutesToUpdate,
const std::vector<int32_t>& mplsRoutesToDelete) {
// Process MPLS routes to add or update
XLOG_IF(INFO, mplsRoutesToUpdate.size())
<< "Adding/Updating " << mplsRoutesToUpdate.size()
<< " static mpls routes.";
for (const auto& [label, mplsRoute] : mplsRoutesToUpdate) {
staticMplsRoutes_.insert_or_assign(label, mplsRoute);

XLOG(DBG1) << "> " << label
<< ", NextHopsCount = " << mplsRoute.nexthops.size();
for (auto const& nh : mplsRoute.nexthops) {
XLOG(DBG2) << " via " << toString(nh);
}
}

XLOG_IF(INFO, mplsRoutesToDelete.size())
<< "Deleting " << mplsRoutesToDelete.size() << " static mpls routes.";
for (const auto& topLabel : mplsRoutesToDelete) {
staticMplsRoutes_.erase(topLabel);

XLOG(DBG1) << "> " << std::to_string(topLabel);
}
}

std::optional<RibUnicastEntry>
SpfSolver::createRouteForPrefixOrGetStaticRoute(
const std::string& myNodeName,
Expand Down Expand Up @@ -544,13 +517,6 @@ SpfSolver::buildRouteDb(
}
}

//
// Add MPLS static routes
//
for (const auto& [_, mplsEntry] : staticMplsRoutes_) {
routeDb.addMplsRoute(RibMplsEntry(mplsEntry));
}

auto deltaTime = std::chrono::duration_cast<std::chrono::milliseconds>(
std::chrono::steady_clock::now() - startTime);
XLOG(INFO) << "Decision::buildRouteDb took " << deltaTime.count() << "ms.";
Expand Down
6 changes: 1 addition & 5 deletions openr/decision/SpfSolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,18 +122,14 @@ class SpfSolver {
~SpfSolver();

//
// util function to update IP/MPLS static route
// util function to update IP static route
//

void updateStaticUnicastRoutes(
const std::unordered_map<folly::CIDRNetwork, RibUnicastEntry>&
unicastRoutesToUpdate,
const std::vector<folly::CIDRNetwork>& unicastRoutesToDelete);

void updateStaticMplsRoutes(
const std::unordered_map<int32_t, RibMplsEntry>& mplsRoutesToUpdate,
const std::vector<int32_t>& mplsRoutesToDelete);

// Build route database using given prefix and link states for a given
// router, myNodeName
// Returns std::nullopt if myNodeName doesn't have any prefix database
Expand Down
127 changes: 0 additions & 127 deletions openr/decision/tests/DecisionTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4325,113 +4325,6 @@ TEST_F(DecisionTestFixture, BasicOperations) {
std::sort(routeDb.mplsRoutes()->begin(), routeDb.mplsRoutes()->end());
routeDelta = findDeltaRoutes(routeDb, routeDbBefore);
EXPECT_TRUE(checkEqualRoutesDelta(routeDbDelta, routeDelta));

// construct new static mpls route add
thrift::RouteDatabaseDelta input;
thrift::NextHopThrift nh, nh1;
*nh.address() = toBinaryAddress(folly::IPAddressV6("::1"));
*nh1.address() = toBinaryAddress(folly::IPAddressV6("::2"));
nh.mplsAction() = createMplsAction(thrift::MplsActionCode::POP_AND_LOOKUP);
nh1.mplsAction() = createMplsAction(thrift::MplsActionCode::POP_AND_LOOKUP);
thrift::MplsRoute route;
route.topLabel() = 32011;
route.nextHops() = {nh};
input.mplsRoutesToUpdate()->push_back(route);

// record number of mplsRoute as reference
const auto mplsRouteCnt =
decision->getDecisionRouteDb("").get()->mplsRoutes()->size();

// Update 32011 and make sure only that is updated
sendStaticRoutesUpdate(input);
auto routesDelta = routeUpdatesQueueReader.get().value();
routesDelta.perfEvents.reset();
EXPECT_EQ(routesDelta.toThrift(), input);

// Update 32012 and make sure only that is updated
route.topLabel() = 32012;
input.mplsRoutesToUpdate() = {route};
sendStaticRoutesUpdate(input);
routesDelta = routeUpdatesQueueReader.get().value();
routesDelta.perfEvents.reset();
EXPECT_EQ(routesDelta.toThrift(), input);

auto mplsRoutes = *decision->getDecisionRouteDb("").get()->mplsRoutes();
EXPECT_THAT(mplsRoutes, testing::SizeIs(mplsRouteCnt + 2));
EXPECT_THAT(
mplsRoutes,
testing::Contains(AllOf(
Truly([&](auto route) {
return route.topLabel() == 32011 or route.topLabel() == 32012;
}),
ResultOf(getMplsNextHops, testing::UnorderedElementsAre(nh)))));

// Test our consolidating logic, we first update 32011 then delete 32011
// making sure only delete for 32011 is emitted.
route.topLabel() = 32011;
route.nextHops() = {nh, nh1};
input.mplsRoutesToUpdate() = {route};
input.mplsRoutesToDelete()->clear();
sendStaticRoutesUpdate(input);

input.mplsRoutesToUpdate()->clear();
input.mplsRoutesToDelete() = {32011};
sendStaticRoutesUpdate(input);

routesDelta = routeUpdatesQueueReader.get().value();
routesDelta.perfEvents.reset();
EXPECT_EQ(routesDelta.mplsRoutesToDelete.at(0), 32011);
EXPECT_EQ(routesDelta.mplsRoutesToUpdate.size(), 0);

mplsRoutes = *decision->getDecisionRouteDb("").get()->mplsRoutes();
EXPECT_THAT(mplsRoutes, testing::SizeIs(mplsRouteCnt + 1));
EXPECT_THAT(
mplsRoutes,
testing::Contains(AllOf(
Truly([&](auto route) { return route.topLabel() == 32012; }),
ResultOf(getMplsNextHops, testing::UnorderedElementsAre(nh)))));

// test our consolidating logic, we first delete 32012 then update 32012
// making sure only update for 32012 is emitted.
input.mplsRoutesToUpdate()->clear();
input.mplsRoutesToDelete() = {32012};
sendStaticRoutesUpdate(input);

route.topLabel() = 32012;
route.nextHops() = {nh, nh1};
input.mplsRoutesToUpdate() = {route};
input.mplsRoutesToDelete()->clear();
sendStaticRoutesUpdate(input);

routesDelta = routeUpdatesQueueReader.get().value();
routesDelta.perfEvents.reset();
EXPECT_EQ(1, routesDelta.mplsRoutesToUpdate.size());
ASSERT_EQ(1, routesDelta.mplsRoutesToUpdate.count(32012));
EXPECT_THAT(
routesDelta.mplsRoutesToUpdate.at(32012).nexthops,
testing::UnorderedElementsAre(nh, nh1));

mplsRoutes = *decision->getDecisionRouteDb("").get()->mplsRoutes();
EXPECT_THAT(mplsRoutes, testing::SizeIs(mplsRouteCnt + 1));
EXPECT_THAT(
mplsRoutes,
testing::Contains(AllOf(
Truly([&](auto route) { return route.topLabel() == 32012; }),
ResultOf(getMplsNextHops, testing::UnorderedElementsAre(nh, nh1)))));

routeDb = dumpRouteDb({"1"})["1"];
bool foundLabelRoute{false};
for (auto const& mplsRoute : *routeDb.mplsRoutes()) {
if (*mplsRoute.topLabel() != 32012) {
continue;
}

EXPECT_THAT(
*mplsRoute.nextHops(), testing::UnorderedElementsAreArray({nh, nh1}));
foundLabelRoute = true;
break;
}
EXPECT_TRUE(foundLabelRoute);
}

/**
Expand All @@ -4441,7 +4334,6 @@ TEST_F(DecisionTestFixture, BasicOperations) {
* Types of information updated
* - Adjacencies (with MPLS labels)
* - Prefixes
* - MPLS Static routes
*/
TEST_F(DecisionTestFixture, InitialRouteUpdate) {
// Send adj publication
Expand All @@ -4454,19 +4346,6 @@ TEST_F(DecisionTestFixture, InitialRouteUpdate) {
{}),
false /*prefixPubExists*/);

// Send static MPLS routes
thrift::RouteDatabaseDelta staticRoutes;
{
thrift::NextHopThrift nh;
nh.address() = toBinaryAddress(folly::IPAddressV6("::1"));
nh.mplsAction() = createMplsAction(thrift::MplsActionCode::POP_AND_LOOKUP);
thrift::MplsRoute mplsRoute;
mplsRoute.topLabel() = 32011;
mplsRoute.nextHops() = {nh};
staticRoutes.mplsRoutesToUpdate()->push_back(mplsRoute);
}
sendStaticRoutesUpdate(staticRoutes);

// Send prefix publication
sendKvPublication(createThriftPublication(
{createPrefixKeyValue("1", 1, addr1),
Expand All @@ -4480,12 +4359,6 @@ TEST_F(DecisionTestFixture, InitialRouteUpdate) {
EXPECT_EQ(1, routeDbDelta.unicastRoutesToUpdate.size());
EXPECT_EQ(0, routeDbDelta.mplsRoutesToDelete.size());
EXPECT_EQ(0, routeDbDelta.unicastRoutesToDelete.size());
// self mpls route, node 2 mpls route and static MPLS route
auto mplsRoutesToUpdateCount = routeDbDelta.mplsRoutesToUpdate.size();
while (mplsRoutesToUpdateCount != 3) {
routeDbDelta = recvRouteUpdates();
mplsRoutesToUpdateCount += routeDbDelta.mplsRoutesToUpdate.size();
}
}

/*
Expand Down

0 comments on commit 4d460ae

Please sign in to comment.