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

banswamy · 2022-12-14T20:34:51Z

Signed-off-by: bmswamy [email protected]

Prerequisites

I have read the Contributing Guidelines.
I have read the Code of Conduct.
If this is a non-trivial change, I have already opened an accompanying Issue.
If applicable, I have included documentation updates alongside my code changes.

Description

BgpUtils fix for clean up for FRR

Test Plan

Tested the below commands
show bgp ipv6 unicast summary json
show bgp ipv6 unicast neighbor {ip} routes json
show bgp ipv6 unicast neighbor {ip} advertised-routes json
Tested the above commands Json information as per task
I have added the tested logs below :

LOGS :

I1214 17:41:15.980036 3782 Broker.cpp:348] Disconnecting from controller on url tcp://[2002:db8:0:1::1]:7007
I1214 17:41:15.980118 3782 Broker.cpp:364] Connecting to controller on url tcp://[2002:db8:0:1::1]:7007
E1214 17:41:16.462097 3793 BgpUtils.cpp:215] bgp summary
E1214 17:41:16.462143 3793 BgpUtils.cpp:220] 2620:10d:c089:603::2
E1214 17:41:16.462155 3793 BgpUtils.cpp:224] 1
E1214 17:41:16.462167 3793 BgpUtils.cpp:228] 65123
E1214 17:41:16.462177 3793 BgpUtils.cpp:232] 01:33:27
E1214 17:41:16.462186 3793 BgpUtils.cpp:236] Established
E1214 17:41:16.462194 3793 BgpUtils.cpp:237] *************************************************************************************************
E1214 17:41:16.935168 3793 BgpUtils.cpp:547] Recived Routes
E1214 17:41:16.935207 3793 BgpUtils.cpp:549] print network:2620:10d:c089:800::/56
E1214 17:41:16.935225 3793 BgpUtils.cpp:557] Next hop printing:2620:10d:c089:603::2
E1214 17:41:16.935236 3793 BgpUtils.cpp:561] ********************************************************************************
E1214 17:41:16.935245 3793 BgpUtils.cpp:547] Recived Routes
E1214 17:41:16.935254 3793 BgpUtils.cpp:549] print network:::/0
E1214 17:41:16.935264 3793 BgpUtils.cpp:557] Next hop printing:2620:10d:c089:603::2
E1214 17:41:16.935274 3793 BgpUtils.cpp:561] ********************************************************************************
E1214 17:41:17.526739 3793 BgpUtils.cpp:582] advertisedRoutes**
E1214 17:41:17.526778 3793 BgpUtils.cpp:585] print network:2620:10d:c089:9520::/60
E1214 17:41:17.526791 3793 BgpUtils.cpp:591] Next hop printing:2620:10d:c089:603::4
E1214 17:41:17.526803 3793 BgpUtils.cpp:594] ********************************************************************************
I1214 17:41:17.527005 3793 StatusApp.cpp:1515] Reporting status to controller

Signed-off-by: bmswamy <[email protected]>

elludraon

Code does not compile.
You did not handle any errors and this will crash in a lot of places.
Please format your code properly like surrounding code. 80 chars per line, 2 spaces to indent, inline {} braces, etc...

Will do a real review after basics are addressed.

elludraon · 2022-12-14T23:15:37Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

-    receivedRoutesFormat = kVtyshQuaggaGetBgpReceivedRoutesFormat;
-  } else /* if (versionOutput.value().rfind("FRRouting", 0) == 0) */ {
+
+  //FRROUTING


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

Addressed the review comments

elludraon · 2022-12-14T23:15:44Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

-  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


Addressed the review comments

elludraon · 2022-12-14T23:16:48Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

+ // dynamic object for  bgpSummary
+ folly::dynamic bgpSummaryInfo = folly::dynamic::object;
+
+ bgpSummaryInfo = folly::parseJson(bgpSummary)["peers"];


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

Addressed the review comments

elludraon · 2022-12-14T23:18:08Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

 }

+// Vtysh Bgp Recived Routes Info List


Addressed the review comments

elludraon · 2022-12-14T23:18:15Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

-  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) {


fix typo "Recived"

80 chars per line

Addressed the review comments

elludraon · 2022-12-14T23:24:20Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

-      }
+// Vtysh Bgp Advertised Routes Info List
+std::vector<thrift::BgpRouteInfo>
+BgpUtils::createVtyshBgpAdvertisedRouteInfoList(const std::string &advertisedRoutes, const std::string &key, const std::string &value) {


80 chars per line

Addressed the review comments

elludraon · 2022-12-14T23:24:33Z

src/terragraph-e2e/e2e/minion/BgpUtils.h


-  /**
+   /**


delete extra leading space

Addressed the review comments

elludraon · 2022-12-14T23:24:44Z

src/terragraph-e2e/e2e/minion/BgpUtils.h

   * 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.


Addressed the review comments

elludraon · 2022-12-14T23:25:05Z

src/terragraph-e2e/e2e/minion/BgpUtils.h

-      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);


extra paren, this code does not compile

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

elludraon · 2022-12-14T23:25:29Z

src/terragraph-e2e/e2e/minion/BgpUtils.h

-  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);


extra paren, this code does not compile

sorry for this while I am doing the alignment it happened

Signed-off-by: bmswamy <[email protected]>

elludraon

Some comments were not properly addressed and formatting is still all over the place.

elludraon · 2022-12-16T19:03:11Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

+ try {
+    bgpSummaryInfo = folly::parseJson(bgpSummary)["peers"];
+  } catch (const std::exception& ex) {
+    throw std::invalid_argument("Could not parse bgp summary ");


Please do not re-throw an error... it is not caught. It will crash the program.

elludraon · 2022-12-16T19:03:23Z

src/terragraph-e2e/e2e/minion/BgpUtils.cpp

-    advertisedRoutesFormat = kVtyshQuaggaGetBgpAdvertisedRoutesFormat;
-    receivedRoutesFormat = kVtyshQuaggaGetBgpReceivedRoutesFormat;
-  } else /* if (versionOutput.value().rfind("FRRouting", 0) == 0) */ {
+  if (versionOutput.value().rfind("FRRouting", 0) == 0) {


This comment was not addressed.

Signed-off-by: bmswamy <[email protected]>

BgpUtils-fix-for-clean-up-for-FRR

23bc296

Signed-off-by: bmswamy <[email protected]>

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 14, 2022

elludraon requested changes Dec 14, 2022

View reviewed changes

Fix for E2E minion BgpUtils clean-up for FRR

c210d3d

Signed-off-by: bmswamy <[email protected]>

elludraon requested changes Dec 16, 2022

View reviewed changes

banswamy added 2 commits December 19, 2022 09:31

fix(BgpUtils):E2E minion BgpUtils clean-up for FRR

9a347de

Signed-off-by: bmswamy <[email protected]>

fix for BgpUtils:E2E minion BgpUtils clean-up for FRR

92a9a98

Signed-off-by: bmswamy <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

banswamy commented Dec 14, 2022 •

edited

Loading

elludraon left a comment

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon Dec 14, 2022

banswamy Dec 15, 2022

elludraon left a comment

elludraon Dec 16, 2022

elludraon Dec 16, 2022

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

Are you sure you want to change the base?

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

Conversation

banswamy commented Dec 14, 2022 • edited Loading

Prerequisites

Description

Test Plan

elludraon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elludraon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banswamy commented Dec 14, 2022 •

edited

Loading