-
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
Advertise FIB installed routes to bgp peers #4770
Conversation
Adding the design document |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: FailedOpenBSD 6 amd64 build: Failed (click for details)OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8503/artifact/CI011BUILD/config.status/config.statusMake failed for OpenBSD 6 amd64 build:
Successful on other platforms
Warnings Generated during build:Checkout code: Successful with additional warningsOpenBSD 6 amd64 build: Failed (click for details)OpenBSD 6 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-8503/artifact/CI011BUILD/config.status/config.statusMake failed for OpenBSD 6 amd64 build:
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous 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-8507/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 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.
First round of review has been done here. I have not had a chance to actually test this code yet to look at behaviors. Will do so after these code comments have been addressed
Additionally we need to update the documentation to reflect these changes |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous 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-8561/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base1 Static Analyzer issues remaining.See details at |
On C we see that we have 24 routes:
On C let's clear neighbors:
This seems broken |
Additionally I saw some very very weird behavior associated with applying a route-map to the |
This has been sitting for a while; I suspect we should close this and wait for a new PR to fix this if someone wants to take it on. |
I will continue to look at the PR |
Verified the issue is not observed when the patch is applied to latest build Logs: |
ci:rerun |
The CI tests are not getting triggered when new changes are submitted |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Basic CI tests logs are observed but NetDEF-CI logs is not shown |
ci:rerun |
Continuous Integration Result: SUCCESSFULContinuous 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-10222/ This is a comment from an automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
clang_check |
CI results are fine ( #4770 (comment) ) |
c:
|
The document is updated in the file doc/user/bgp.rst Logs added in the file : fib_test_logs |
💚 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-15030/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base2 Static Analyzer issues remaining.See details at |
All the review comments are addressed |
@kssoman looked at the PR and looks good now. Please fix the conflict and I will get this in. |
Issue: The bgp routes learnt from peers which are not installed in kernel are advertised to peers. This can cause routers to send traffic to these destinations only to get dropped. The fix is to provide a configurable option "bgp suppress-fib-pending". When the option is enabled, bgp will advertise routes only if it these are successfully installed in kernel. Fix (Part1) : * Added message ZEBRA_ROUTE_NOTIFY_REQUEST used by client to request FIB install status for routes * Added AFI/SAFI to ZAPI messages * Modified the functions zapi_route_notify_decode(), zsend_route_notify_owner() and route_notify_internal() to include AFI, SAFI as parameters Signed-off-by: kssoman <somanks@gmail.com>
* Added CLI command "[no] bgp suppress-fib-pending" to enable and disable suppress-fib-pending * Send ZEBRA_ROUTE_NOTIFY_REQUEST to zebra when "bgp suppress-fib-pending" is enabled or disabled * Define BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME which is the delay added to update group timer. * Added error codes Signed-off-by: kssoman <somanks@gmail.com>
* Process FIB update in bgp_zebra_route_notify_owner() and call group_announce_route() if route is installed * When bgp update is received for a route which is not installed earlier (flag BGP_NODE_FIB_INSTALLED is not set) and suppress fib is enabled set the flag BGP_NODE_FIB_INSTALL_PENDING to indicate fib install is pending for the route. The route will be advertised when zebra send ZAPI_ROUTE_INSTALLED status. * The advertisement delay (BGP_DEFAULT_UPDATE_ADVERTISEMENT_TIME) is added to allow more routes to be sent in single update message. This is required since zebra sends route notify message for each route. The delay will be applied to update group timer which advertises routes to peers. Signed-off-by: kssoman <somanks@gmail.com>
Added test case for bgp suppress-fib-pending Updated document Signed-off-by: kssoman <somanks@gmail.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1604AMD64-15240/test Topology Tests failed for Topo tests part 1 on Ubuntu 16.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15240/artifact/TP1U1604AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 2 on Ubuntu 18.04 amd64: Failed (click for details)Topo tests part 2 on Ubuntu 18.04 amd64: No useful log foundIPv4 protocols on Ubuntu 18.04: Failed (click for details)Topo tests part 1 on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-15248/test Topology Tests failed for Topo tests part 1 on Ubuntu 18.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15248/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Topo tests part 1 on Ubuntu 16.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1604AMD64-15248/test Topology Tests failed for Topo tests part 1 on Ubuntu 16.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15248/artifact/TP1U1604AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
Changes updated to latest code The changes were working fine on 10/29/2020 as shown in previous results When test_ebgp_ecmp_topo2 is run seperately, there is no error, looks like timing issue with the test case The error in test_ebgp_ecmp_topo2 is seen in other PRs also therefore it looks like test case issue |
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-15279/test Topology Tests failed for Topo tests part 1 on Ubuntu 18.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15279/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedIPv6 protocols on Ubuntu 18.04: Failed (click for details)Topo tests part 1 on Ubuntu 16.04 i386: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1604I386-15291/test Topology Tests failed for Topo tests part 1 on Ubuntu 16.04 i386:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15291/artifact/TP1U1604I386/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
The error in test_ebgp_ecmp_topo2 is not related to the changes in this PR and likely timing issue in the test case |
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 18.04 arm8: Failed (click for details)Topo tests part 1 on Ubuntu 18.04 arm8: No useful log foundSuccessful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
ci:rerun |
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopo tests part 1 on Ubuntu 18.04 amd64: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-15342/test Topology Tests failed for Topo tests part 1 on Ubuntu 18.04 amd64:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-15342/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
ci:rerun |
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-15361/ This is a comment from an automated CI system. Warnings Generated during build:Debian 10 amd64 build: Successful with additional warningsDebian Package lintian failed for Debian 10 amd64 build:
|
The test run looks ok now, PR can probably be committed |
Test results
fib_test.txt