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: add the bgp_zebra_announce_dest() function #16393

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

bgpd: add the bgp_zebra_announce_dest() function #16393

wants to merge 1 commit into from

Conversation

pguibert6WIND
Copy link
Member

Split the bgp_zebra_announce_table() in two. The bgp_dest pointer is given to the bgp_zebra_announce_dest(), which will install only the selected bgp_path_info.

@frrbot frrbot bot added the bgp label Jul 16, 2024
riw777
riw777 approved these changes Jul 16, 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

@riw777
Copy link
Member

riw777 commented Jul 16, 2024 •
edited
Loading

Failures look related --

Failed: New core[s] found: /tmp/topotests/bgp_gr_functionality_topo1.test_bgp_gr_functionality_topo1-3/r2/bgpd_core-sig_11-pid_8359.dmp

AssertionError: Expected padded packet with length 1497, got packet with length 97 
   assert 'Expected padded packet with length 1497, got packet with length 97' is True

mjstapp
mjstapp reviewed Jul 16, 2024
View reviewed changes
bgpd/bgp_zebra.c
@@ -1722,11 +1727,7 @@ void bgp_zebra_announce_table_all_subtypes(struct bgp *bgp, afi_t afi,
return;

for (dest = bgp_table_top(table); dest; dest = bgp_route_next(dest))
for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next)
if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED) &&
pi->type == ZEBRA_ROUTE_BGP)
Copy link
Contributor

Choose a reason for hiding this comment

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

the tests in the new function doesn't look the same - that seems ... questionable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a parameter to handle this case.

Copy link
Member

Choose a reason for hiding this comment

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

What about pi->subtype and BGP_ROUTE_NORMAL, the new code tests for this as well, when before it doesn't. This looks like it would break STATIC/AGGREGATES/REDISTRIBUTION here right?

@pguibert6WIND
bgpd: add the bgp_zebra_announce_dest() function
3004ecd 
Split the bgp_zebra_announce_table() in two. The bgp_dest
pointer is given to the bgp_zebra_announce_dest(), which will
install only the selected bgp_path_info.

Signed-off-by: Philippe Guibert <[email protected]>
@pguibert6WIND pguibert6WIND force-pushed the bgp_zebra_announce_dest branch from 3e79318 to 3004ecd Compare July 16, 2024 15:40
@donaldsharp donaldsharp self-requested a review July 23, 2024 15:21
donaldsharp
donaldsharp requested changes Jul 24, 2024
View reviewed changes
Copy link
Member

@donaldsharp donaldsharp left a comment

Choose a reason for hiding this comment

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

I'd like a better understanding of the changes first.

bgpd/bgp_zebra.c
@@ -1722,11 +1727,7 @@ void bgp_zebra_announce_table_all_subtypes(struct bgp *bgp, afi_t afi,
return;

for (dest = bgp_table_top(table); dest; dest = bgp_route_next(dest))
for (pi = bgp_dest_get_bgp_path_info(dest); pi; pi = pi->next)
if (CHECK_FLAG(pi->flags, BGP_PATH_SELECTED) &&
pi->type == ZEBRA_ROUTE_BGP)
Copy link
Member

Choose a reason for hiding this comment

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

What about pi->subtype and BGP_ROUTE_NORMAL, the new code tests for this as well, when before it doesn't. This looks like it would break STATIC/AGGREGATES/REDISTRIBUTION here right?

@riw777 riw777 self-requested a review July 30, 2024 14:09
@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.

@github-actions GitHub Actions
Copy link

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

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

@mjstapp mjstapp mjstapp left review comments

@donaldsharp donaldsharp donaldsharp requested changes

@riw777 riw777 Awaiting requested review from riw777

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
4 participants
@pguibert6WIND @riw777 @donaldsharp @mjstapp