-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Zebra run once #4154
Zebra run once #4154
Conversation
27ca78a
to
d67c8d4
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7276/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base12 Static Analyzer issues remaining.See details at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the solution very much; just had a couple of questions inline
The tests are not coming up consistently on my test box. Add a bit of wait time to test to allow normal bgp when the first attempt doesn't come up. Especially since bgp timeouts are 120 seconds with non datacenter compiles. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Add a ability to count the number of nexthops in a nexthop_group. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
We are effectively calling nexthop_active_update() on every route entry being processed for installation at least 2 times. This is a bit ridiculous. We need to resolve the nexthops when we know a route has changed in some manner, so do so. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
The NEXTHOP_FLAG_FILTERED went away when we started treating static routes like every other route in the system. This was a special case for handling static route code that just didn't get finished cleaning up. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
The nexthop_active_update command looks at each individual nexthop and decides if it has changed. If any nexthop has changed we will set the re->status to ROUTE_ENTRY_CHANGED and ROUTE_ENTRY_NEXTHOPS_CHANGED. Additionally the test for old_nh_num != curr_active makes no sense because suppose we have several events we are processing at the same time and a total ecmp of 16 but 14 are active at the start and 14 are active at the end but different interfaces are up or down. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
We currently run nexthop_active_check multiple times. Make the code run once and figure out state from that. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
Update the nexthop flag output for the route entry dump to include all possible flag states be output. Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
d67c8d4
to
df38b09
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - I'll merge tomorrow in case someone wants to chime in
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7299/ This is a comment from an automated CI system. CLANG Static Analyzer Summary
12 Static Analyzer issues remaining.See details at |
Run nexthop_active_update() one time as part of handling a new route in the system.