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

Bgp musings #15563

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

Bgp musings #15563

wants to merge 3 commits into from
+23 −17

Conversation

donaldsharp
Copy link
Member

a) Make attribute hash faster? convert jhash_1word to jhash_3words so we make less calls
b) Extend show bgp attribute-info to dump more data
c) Gather some data around how many times path_info_cmp is called and how many times we double enqueue and defer enqueuing.

@frrbot frrbot bot added the bgp label Mar 16, 2024
ton31337
ton31337 reviewed Mar 18, 2024
View reviewed changes
bgpd/bgp_route.c Outdated
@@ -12594,6 +12599,12 @@ DEFUN(show_ip_bgp_afi_safi_statistics, show_ip_bgp_afi_safi_statistics_cmd,
json = json_object_new_object();
json_object_object_add(json, get_afi_safi_str(afi, safi, true),
json_afi_safi);
json_object_int_add(json, "bgpBestPathCalls",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be this in bgp_table_stats_single()? Because what if we have show bgp vrf all statistics json? Also, is this only for JSON output?

Copy link
Member Author

Choose a reason for hiding this comment

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

No because it's not per afi/safi, it's per struct bgp. I intentionally only made this as a json output because most operators will not care about this.

Copy link
Member

Choose a reason for hiding this comment

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

Not afi/safi, but per-instance (BGP, let's say lots of VRFs), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it's per router bgp XXX in the cli

Copy link
Member

Choose a reason for hiding this comment

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

So this means that show bgp vrf all statistics json won't work as expected, correct?

riw777
riw777 approved these changes Mar 19, 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.

looks good

ton31337
ton31337 requested changes Mar 19, 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.

I quickly tested this branch, and I got a bgpd crash when using show bgp vrf all statistics json.

@ton31337
Copy link
Member

Still have a crash:

ton# sh ip bgp vrf all statistics json 
vtysh: error reading from bgpd: Resource temporarily unavailable (11)Warning: closing connection to bgpd because of an I/O error!
ton#

BGP[450029]: show_ip_bgp_afi_safi_statistics+0x11c     5588ad755d4c     7ffd3a7d8e00 /usr/lib/frr/bgpd (mapped at 0x5588ad5e9000)
BGP[450029]: cmd_execute_command_real+0x1be     7f53e64ad7be     7ffd3a7d8e60 /usr/local/lib/libfrr.so.0 (mapped at 0x7f53e6410000)
BGP[450029]: cmd_execute_command+0x61           7f53e64ad8b1     7ffd3a7d8ed0 /usr/local/lib/libfrr.so.0 (mapped at 0x7f53e6410000)
BGP[450029]: cmd_execute+0xd0                   7f53e64adb50     7ffd3a7d8f20 /usr/local/lib/libfrr.so.0 (mapped at 0x7f53e6410000)

@mwinter-osr
Copy link
Member

CI:rerun Rerun after fixing git access on CI infra

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

@donaldsharp
Copy link
Member Author

@ton31337 -> I don't disagree that you are getting a crash. But it's not in code that I wrote. There is no handler written for vrf all..... hence the crash. Outside the scope of this PR( not that it shouldn't be fixed )

ton31337
ton31337 approved these changes Oct 18, 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.

LGTM, but can we fix frrbot styling issues?

@donaldsharp
bgpd: Optimize the usage of jhash
a7f2236 
There are a bunch of jhash_1word uses.  Let's convert
to jhash_3word and get rid of some calls.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
@donaldsharp
bgpd: Further extend show bgp attribute-info
e2090bf 
Fill out a bit more data about what is being held.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
@donaldsharp
bgpd: add some counters not displayed yet
1115fee 
Add some counters to keep track how often stuff is done.
This is mainly for us developers.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
@riw777
Copy link
Member

riw777 commented Oct 29, 2024

ospf convergence failure ... rerunning that single test

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

@riw777 riw777 riw777 approved these changes

@ton31337 ton31337 ton31337 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@donaldsharp @ton31337 @mwinter-osr @riw777