Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BgpUtils-fix-for-clean-up-for-FRR #49

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 78 additions & 128 deletions src/terragraph-e2e/e2e/minion/BgpUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,18 @@ const std::string kRunVtyshDaemonCommand{"-d bgpd -c"};
const std::string kVtyshShowVersionCommand{"show version"};

// vtysh commands to get the BGP summary
const std::string kVtyshQuaggaGetBgpSummaryCommand{"show ipv6 bgp summary"};
const std::string kVtyshFrrGetBgpSummaryCommand{
"show bgp ipv6 unicast summary"};
"show bgp ipv6 unicast summary json"};

// vtysh commands to get received routes from a BGP neighbor
// (requires neighbor ipv6 passed into {})
const std::string kVtyshQuaggaGetBgpReceivedRoutesFormat{
"show ipv6 bgp neighbor {} received-routes"};
const std::string kVtyshFrrGetBgpReceivedRoutesFormat{
"show bgp ipv6 unicast neighbor {} routes"};
"show bgp ipv6 unicast neighbor {} routes json"};

// vtysh commands to get advertised routes from a BGP neighbor
// (requires neighbor ipv6 passed into {})
const std::string kVtyshQuaggaGetBgpAdvertisedRoutesFormat{
"show ipv6 bgp neighbor {} advertised-routes"};
const std::string kVtyshFrrGetBgpAdvertisedRoutesFormat{
"show bgp ipv6 unicast neighbor {} advertised-routes"};
"show bgp ipv6 unicast neighbor {} advertised-routes json"};
// -------------------------------------------------------------------------- //

// -- exabgp ---------------------------------------------------------------- //
Expand Down Expand Up @@ -153,11 +148,9 @@ BgpUtils::fetchVtyshBgpStatus() {
return bgpStatus;
}
std::string summaryCommand, advertisedRoutesFormat, receivedRoutesFormat;
if (versionOutput.value().rfind("Quagga", 0) == 0) {
summaryCommand = kVtyshQuaggaGetBgpSummaryCommand;
advertisedRoutesFormat = kVtyshQuaggaGetBgpAdvertisedRoutesFormat;
receivedRoutesFormat = kVtyshQuaggaGetBgpReceivedRoutesFormat;
} else /* if (versionOutput.value().rfind("FRRouting", 0) == 0) */ {

//FRROUTING
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of these new comments are unnecessary... please delete them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

if (versionOutput.value().rfind("FRRouting", 0) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the whole "Get version" section too, since there is no more if/else. This code is wrong as is because you don't have an "else" block and it would crash if this condition doesn't trigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments.

summaryCommand = kVtyshFrrGetBgpSummaryCommand;
advertisedRoutesFormat = kVtyshFrrGetBgpAdvertisedRoutesFormat;
receivedRoutesFormat = kVtyshFrrGetBgpReceivedRoutesFormat;
Expand All @@ -172,67 +165,62 @@ BgpUtils::fetchVtyshBgpStatus() {
}

std::string bgpSummary = bgpSummaryOutput.value();
std::vector<std::string> summaryHeaders, summaryEntries;
int neighborCount = parseVtyshBgpTable(
bgpSummary, "Neighbor", summaryHeaders, summaryEntries);

// Iterate through neighbors and create BgpInfo per neighbor
for (int i = 0; i < neighborCount; ++i) {
// BGP Headers:
// Neighbor V AS MsgRcvd MsgSent TblVer InQ OutQ Up/Down State/PfxRcd
// dynamic object for bgpSummary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

folly::dynamic bgpSummaryInfo = folly::dynamic::object;

bgpSummaryInfo = folly::parseJson(bgpSummary)["peers"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to handle errors in parseJson() or else this code will crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments


int startRowIdx = i * summaryHeaders.size();
// Iterate through neighbors and create BgpInfo per neighbor
for (const auto &summary : bgpSummaryInfo.items()) {
// Bgp Neighbor Information
thrift::BgpInfo neighbor;
// State/PfxRcd Header
auto stateOrPfxToInt = folly::tryTo<int>(summaryEntries[startRowIdx + 9]);

neighbor.ipv6Address = summaryEntries[startRowIdx]; // Neighbor Header
// Need to check if the stateOrPfx is an int since it can either
// be a string referring to its state or the actual neighbor prefix
neighbor.online = stateOrPfxToInt.hasValue();
// AS Header
neighbor.asn = folly::to<int>(summaryEntries[startRowIdx + 2]);
neighbor.upDownTime = summaryEntries[startRowIdx + 8]; // Up/Down Header
// State/PfxRcd Header
neighbor.stateOrPfxRcd = summaryEntries[startRowIdx + 9];

// Neighbor ipv6 Address
std::string ipv6Address = summary.first.asString();
neighbor.ipv6Address = ipv6Address;

// online
neighbor.online = bgpSummaryInfo[ipv6Address.c_str()]["pfxRcd"].asBool();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look up bgpSummaryInfo[ipv6Address.c_str()] once instead of repeatedly calling this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments


// asn
neighbor.asn = bgpSummaryInfo[ipv6Address.c_str()]["remoteAs"].asInt();

// Up Down Time
neighbor.upDownTime = bgpSummaryInfo[ipv6Address.c_str()]["peerUptime"].asString();

// state
neighbor.stateOrPfxRcd = bgpSummaryInfo[ipv6Address.c_str()]["state"].asString();

// Run 'bgp neighbor {} advertised-routes' command
auto advertisedRoutesOutput = runVtyshCmd(
folly::sformat(advertisedRoutesFormat, neighbor.ipv6Address));

if (advertisedRoutesOutput.hasValue()) { // ran successfully
std::vector<std::string> advertisedHeaders, advertisedEntries;
int advertisedRoutesCount = parseVtyshBgpTable(
advertisedRoutesOutput.value(),
"Network",
advertisedHeaders,
advertisedEntries);
neighbor.advertisedRoutes =
createVtyshBgpRouteInfoList(advertisedRoutesCount, advertisedEntries);
} else { // command failed
LOG(ERROR) << "vtysh BGP advertised routes command failed for neighbor "
<< neighbor.ipv6Address << ": "
<< advertisedRoutesOutput.error().str();

neighbor.advertisedRoutes = createVtyshBgpAdvertisedRouteInfoList(
advertisedRoutesOutput.value(), "advertisedRoutes", "nextHopGlobal");
}
else { // command failed
LOG(ERROR) << "vtysh BGP advertised routes command failed for neighbor "
<< neighbor.ipv6Address << ": "
<< advertisedRoutesOutput.error().str();
}

// Run 'bgp neighbor {} received-routes' command
auto receivedRoutesOutput = runVtyshCmd(
folly::sformat(receivedRoutesFormat, neighbor.ipv6Address));
if (receivedRoutesOutput.hasValue()) { // ran successfully
std::vector<std::string> receivedHeaders, receivedEntries;
int receivedRoutesCount = parseVtyshBgpTable(
receivedRoutesOutput.value(),
"Network",
receivedHeaders,
receivedEntries);
neighbor.receivedRoutes =
createVtyshBgpRouteInfoList(receivedRoutesCount, receivedEntries);
} else { // command failed
LOG(ERROR) << "vtysh BGP received routes command failed for neighbor "
<< neighbor.ipv6Address << ": "
<< receivedRoutesOutput.error().str();
}

if (receivedRoutesOutput.hasValue())
{ // ran successfully

neighbor.receivedRoutes = createVtyshBgpRecivedRouteInfoList(receivedRoutesOutput.value(), "routes", "peerId");
}
else
{ // command failed
LOG(ERROR) << "vtysh BGP received routes command failed for neighbor "
<< neighbor.ipv6Address << ": "
<< receivedRoutesOutput.error().str();
}
bgpStatus.insert(std::make_pair(neighbor.ipv6Address, neighbor));
}

Expand Down Expand Up @@ -377,83 +365,45 @@ BgpUtils::runVtyshCmd(const std::string& command) {
return SysUtils::runCommand(commandVec);
}

int
BgpUtils::parseVtyshBgpTable(
const std::string& bgpTableOutput,
const std::string& firstHeader,
std::vector<std::string>& headers,
std::vector<std::string>& entries) {
// Split output line by line
std::vector<folly::StringPiece> bgpTableLines;
folly::split('\n', bgpTableOutput, bgpTableLines);

int rowCount = 0;
bool inTable = false;

for (const auto& line : bgpTableLines) {
// Split line by whitespace
std::string lineStr = line.toString();
std::istringstream lineStrStream(lineStr);
std::vector<std::string> lineEntries{
std::istream_iterator<std::string>{lineStrStream},
std::istream_iterator<std::string>{}};

if (inTable) {
// Table is finished when the next line is empty
if (lineStr.empty()) {
inTable = false;
continue;
}
// Vtysh Bgp Advertised Routes Info List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete, function comments only go in header files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

std::vector<thrift::BgpRouteInfo>
BgpUtils::createVtyshBgpAdvertisedRouteInfoList(const std::string &advertisedRoutes, const std::string &key, const std::string &value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80 chars per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments


entries.insert(entries.end(), lineEntries.begin(), lineEntries.end());
} else {
// Not in the table
if (!lineEntries.empty() && lineEntries[0] == firstHeader) {
// We are at the start of the table
headers.insert(headers.end(), lineEntries.begin(), lineEntries.end());
inTable = true;
} else if (line.startsWith("Total number")) {
// We are at the section that states the number of neighbors/prefixes
rowCount = folly::to<int>(lineEntries.back());
break;
} else if (line.startsWith("Displayed") && lineEntries.size() == 7) {
rowCount = folly::to<int>(lineEntries[1]);
break;
}
// Network and Next Hop
std::vector<thrift::BgpRouteInfo> routeInfo;
folly::dynamic advertisedRoutesInfo = folly::dynamic::object;
advertisedRoutesInfo = folly::parseJson(advertisedRoutes)[key.c_str()];
thrift::BgpRouteInfo bgpRouteInfo;
for (const auto &routes : advertisedRoutesInfo.items()) {
// Advertised Routes Information
std::string network = routes.first.asString();
bgpRouteInfo.network = network;
bgpRouteInfo.nextHop = advertisedRoutesInfo[network.c_str()][value.c_str()].asString();
routeInfo.push_back(bgpRouteInfo);
}
}

return rowCount;
return routeInfo;
}

// Vtysh Bgp Recived Routes Info List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

std::vector<thrift::BgpRouteInfo>
BgpUtils::createVtyshBgpRouteInfoList(
int rowCount, const std::vector<std::string>& entries) {
// Since each row can have a variable number of entries
// (due to the variable path length), if the next entry is a 'i', 'e' or '?'
// then it marks the end of the row.
std::vector<thrift::BgpRouteInfo> routeInfo;
unsigned int rowStartIdx = 0;
for (int i = 0; i < rowCount; ++i) {
// Skip the first entry on each row since it is the status code
BgpUtils::createVtyshBgpRecivedRouteInfoList(const std::string &recivedRoutes, const std::string &key, const std::string &value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • fix typo "Recived"
  • 80 chars per line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments


// Network and Next Hop
std::vector<thrift::BgpRouteInfo> routeInfo;
folly::dynamic recivedRoutesInfo = folly::dynamic::object;
recivedRoutesInfo = folly::parseJson(recivedRoutes)[key.c_str()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle parseJson() errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

thrift::BgpRouteInfo bgpRouteInfo;
bgpRouteInfo.network = entries[rowStartIdx + 1];
bgpRouteInfo.nextHop = entries[rowStartIdx + 2];
routeInfo.push_back(bgpRouteInfo);

// Look for a row termination token
while (rowStartIdx < entries.size() &&
entries[rowStartIdx] != "i" &&
entries[rowStartIdx] != "e" &&
entries[rowStartIdx] != "?") {
++rowStartIdx;
for (const auto &routes : recivedRoutesInfo.items()) {
// Recived Routes Information
std::string network = routes.first.asString();
bgpRouteInfo.network = network;
std::string nextHop = recivedRoutesInfo[network.c_str()][0][value.c_str()].asString();
bgpRouteInfo.nextHop = nextHop;
routeInfo.push_back(bgpRouteInfo);
}

// Pick the token right after the row termination token
++rowStartIdx;
}

return routeInfo;
return routeInfo;
}

std::unordered_map<std::string, std::vector<thrift::BgpRouteInfo>>
Expand Down
19 changes: 8 additions & 11 deletions src/terragraph-e2e/e2e/minion/BgpUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,21 +54,18 @@ class BgpUtils {
const std::string& command);

/**
* Reads a vtysh BGP info table by the first header and returns the number of
* rows. 'firstHeader' is the first table header that identifies the table.
* Creates a list of thrift::BgpRouteInfo by taking the 'Network' and
* 'Next Hop' values of the vtysh json entries it Recived Routes.
*/
static int parseVtyshBgpTable(
const std::string& bgpTableOutput,
const std::string& firstHeader,
std::vector<std::string>& headers,
std::vector<std::string>& entries);
static std::vector<thrift::BgpRouteInfo> createVtyshBgpRecivedRouteInfoList(
(const std::string& recivedRoutesInfo,const std::string& key,const std::string& value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra paren, this code does not compile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for this I made changes while I am doing the alignment it happened


/**
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete extra leading space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

* Creates a list of thrift::BgpRouteInfo by taking the 'Network' and
* 'Next Hop' values of the vtysh table entries it receives.
* 'Next Hop' values of the vtysh json entries it Advertised Routes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grammar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the review comments

*/
static std::vector<thrift::BgpRouteInfo> createVtyshBgpRouteInfoList(
int numOfRows, const std::vector<std::string>& entries);
static std::vector<thrift::BgpRouteInfo> createVtyshBgpAdvertisedRouteInfoList(
(const std::string& recivedRoutesInfo,const std::string& key,const std::string& value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra paren, this code does not compile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for this while I am doing the alignment it happened


/**
* Creates a list of thrift::BgpRouteInfo for each neighbor by parsing
Expand Down