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.

Sign up for GitHub

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

Jump to bottom

bgpd: show json output changes to optimize various show commands #17431

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

bgpd: show json output changes to optimize various show commands #17431

wants to merge 6 commits into from
+287 −122

Conversation

krishna-samy
Copy link
Contributor

This PR includes multiple commits that optimize the show json output for various BGP show commands in scaled setup.

@frrbot frrbot bot added the bgp label Nov 14, 2024
ton31337
ton31337 requested changes Nov 14, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Please write a topotest to verify this new CLI command + documentation.

bgpd/bgp_evpn_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
bgpd/bgp_vty.c Outdated Show resolved Hide resolved
@krishna-samy krishna-samy force-pushed the bgpd_json_commits branch 3 times, most recently from c32f90a to b2af790 Compare November 16, 2024 05:57
Pdoijode and others added 2 commits November 15, 2024 22:48
@Pdoijode @krishna-samy
bgpd: [EVPN GR] Added JSON support for "show bgp router"
7decdc5 
Introduced JSON support of "show bgp router" command

VTY:
mlx-3700-19# show bgp router
BGP started gracefully at Tue Nov 14 21:18:34 2023
Graceful restart completed at Tue Nov 14 21:18:37 2023
Number of BGP instances (including default): 1

JSON:
mlx-3700-19# show bgp router json
{
  "bgpStartedAt":"Tue Nov 14 21:18:34 2023\n",
  "bgpStartedGracefully":"Yes",
  "grComplete":"Yes",
  "grCompletedAt":"Tue Nov 14 21:18:37 2023\n",
  "bgpInMaintenanceMode":"No",
  "bgpInstanceCount":1
}

Issue:3624937
Signed-off-by: Pooja Jagadeesh Doijode <[email protected]>
@ashred-lnx @krishna-samy
bgpd: show bgp l2vpn evpn route detail json stringify changes
9c55301 
Json object could lead to out-of-memory in scaled bgp l2vpn evpn route setup.

Changes:

- not use pretty print and stringify smaller json objects to reduce memory
usage
- free memory after ouput of json_rd
- minor formatting of json output

Commands supported with this Json stringify:

show bgp l2vpn evpn route detail json
show bgp l2vpn evpn route detail type 2 json
show bgp l2vpn evpn route detail type 2 self-originate json
show bgp l2vpn evpn route detail self-originate json
show bgp l2vpn evpn route json
show bgp l2vpn evpn route type 2 json
show bgp l2vpn evpn route type 2 self-originate json
show bgp l2vpn evpn route self-originate json

Ticket:#3513249

Issue:3513249

Signed-off-by: Ashwini Reddy <[email protected]>
Signed-off-by: Sindhu Parvathi Gopinathan's <[email protected]>
@krishna-samy krishna-samy force-pushed the bgpd_json_commits branch 5 times, most recently from cc3769b to 819db39 Compare November 16, 2024 09:21
@ton31337
Copy link
Member

Please fix frrbot yet.

ton31337
ton31337 requested changes Nov 16, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Still, the documentation is missing for this new CLI command.

@krishna-samy krishna-samy force-pushed the bgpd_json_commits branch 2 times, most recently from 6c0726e to 631ba8f Compare November 16, 2024 14:03
@krishna-samy
Copy link
Contributor Author

Please write a topotest to verify this new CLI command + documentation.

Done

@ton31337
Copy link
Member

Lots of failed existing tests?

Sindhu Parvathi Gopinathan and others added 4 commits November 18, 2024 18:58
@krishna-samy
bgpd: neighbors received-routes/advertised-routes stringify changes
4838bac 
JSON object could lead to out-of-memory in scaled bgp
neighbors received-routes/advertised-routes setup.

Changes:

not use pretty print and stringify smaller json objects to reduce memory
usage
free memory after ouput of json object

Commands supported with this Json stringify:

```
show bgp vrf <vrf-id> neighbors <nbr-id> advertised-routes json
show bgp vrf <vrf-id> neighbors <nbr-id> advertised-routes detail json
show bgp vrf <vrf-id> neighbors <nbr-id> advertised-routes <prefix> json
show bgp vrf <vrf-id> neighbors <nbr-id> received-routes json
show bgp vrf <vrf-id> neighbors <nbr-id> received-routes detail json
show bgp vrf <vrf-id> neighbors <nbr-id> received-routes <prefix> json
```

Ticket:#3513253, #3513254

Issue:3513253, 3513254

Signed-off-by: Sindhu Parvathi Gopinathan's <[email protected]>
@ashred-lnx @krishna-samy
bgpd: Nexthop Stringify changes
c8290e9 
Issue: 3576989

Ticket# 3576989

Signed-off-by: Sindhu Parvathi Gopinathan's <[email protected]>

Signed-off-by: Ashwini Reddy's <[email protected]>
@krishna-samy
bgpd: add topotest for show bgp router json command
355d5e2 
This commit has changes to validate show bgp router json command

Signed-off-by: Krishnasamy R <[email protected]>
@krishna-samy
bgpd: add documentation for show bgp router json command
4d8207c 
Signed-off-by: Krishnasamy R <[email protected]>
@krishna-samy
Copy link
Contributor Author

Lots of failed existing tests?

Fixed the json format issue

@krishna-samy krishna-samy marked this pull request as ready for review November 19, 2024 05:25
@krishna-samy
Copy link
Contributor Author

CI:rerun

mjstapp
mjstapp reviewed Nov 19, 2024
View reviewed changes
bgpd/bgp_evpn_vty.c
if (json)
if (json) {
if (first) {
vty_out(vty, "\"%s\":", rd_str);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by these literal json outputs: why do we want to hard-code these kinds of components in the output? if we're not managing the json object hierarchy correctly, I'd rather we fix that than stick some hard-coded tokens in the output like 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.

Some of the parameters were identified to stringify them in the output so that the overall memory consumption is reduced when the json command was executed in a high scale setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

we talked about this issue at the dev meeting last week, and I thought we had agreement that we wouldn't hard-code json-ish strings in the output stream - but these are still present?

@krishna-samy
Copy link
Contributor Author

krishna-samy commented Nov 22, 2024 •
edited
Loading

Still, the documentation is missing for this new CLI command.

Hi @ton31337, This is taken care. Please take a look!

ton31337
ton31337 reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

One simple question: are here any breaking changes in the JSON outputs?

@krishna-samy
Copy link
Contributor Author

krishna-samy commented Nov 25, 2024 •
edited
Loading

One simple question: are here any breaking changes in the JSON outputs?

@ton31337 - Did you mean any breakage in the json output format?
Some of the json output are stringified. But the structure, or content of the JSON data are not changed. so there would not be any breakage.

riw777
riw777 reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

waiting on @mjstapp 's question about hard coding output strings ... otherwise good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ton31337 ton31337 ton31337 approved these changes

@riw777 riw777 Awaiting requested review from riw777

@mjstapp mjstapp Awaiting requested review from mjstapp

Assignees
No one assigned
Labels
bgp master size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@krishna-samy @ton31337 @riw777 @mjstapp @Pdoijode @ashred-lnx