frr
frr copied to clipboard
FRRouting
bgpd: Checking if bfd is enabled before setting other commands
When using 'bfd profile' or 'bfd check-control-plane-failure' for a neighbor, make sure that bfd is enabled first for this neighbor.
Signed-off-by: Michal Ruprich [email protected]
Continuous Integration Result: FAILED
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7434/
This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.
Get source / Pull Request: Successful
Building Stage: Successful
Basic Tests: Failed
Topotests Ubuntu 18.04 amd64 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP1U1804AMD64-7434/test
Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7434/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt
Topotests debian 10 amd64 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1DEB10AMD64-7434/test
Topology Tests failed for Topotests debian 10 amd64 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7434/artifact/TOPO1DEB10AMD64/ErrorLog/log_topotests.txt
Topotests Ubuntu 18.04 i386 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1U18I386-7434/test
Topology Tests failed for Topotests Ubuntu 18.04 i386 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7434/artifact/TOPO1U18I386/ErrorLog/log_topotests.txt
Topotests Ubuntu 18.04 arm8 part 1: Failed (click for details)
Topotests Ubuntu 18.04 arm8 part 1: No useful log foundSuccessful on other platforms/tests
- Ubuntu 20.04 deb pkg check
- Topotests Ubuntu 18.04 arm8 part 4
- Topotests Ubuntu 18.04 arm8 part 9
- Topotests debian 10 amd64 part 3
- Debian 10 deb pkg check
- IPv6 protocols on Ubuntu 18.04
- Debian 9 deb pkg check
- Addresssanitizer topotests part 1
- Topotests Ubuntu 18.04 i386 part 4
- Ubuntu 18.04 deb pkg check
- Topotests Ubuntu 18.04 i386 part 3
- IPv4 protocols on Ubuntu 18.04
- Topotests Ubuntu 18.04 amd64 part 6
- Topotests Ubuntu 18.04 i386 part 8
- Addresssanitizer topotests part 8
- Topotests Ubuntu 18.04 amd64 part 9
- Addresssanitizer topotests part 7
- Topotests debian 10 amd64 part 4
- Addresssanitizer topotests part 4
- Topotests Ubuntu 18.04 arm8 part 5
- Addresssanitizer topotests part 5
- Topotests Ubuntu 18.04 i386 part 9
- Topotests Ubuntu 18.04 arm8 part 3
- Topotests Ubuntu 18.04 amd64 part 8
- Topotests debian 10 amd64 part 2
- Topotests Ubuntu 18.04 arm8 part 0
- Static analyzer (clang)
- Ubuntu 16.04 deb pkg check
- Addresssanitizer topotests part 0
- Topotests Ubuntu 18.04 i386 part 6
- Addresssanitizer topotests part 2
- Topotests Ubuntu 18.04 amd64 part 5
- IPv4 ldp protocol on Ubuntu 18.04
- Topotests Ubuntu 18.04 amd64 part 4
- Addresssanitizer topotests part 9
- Topotests Ubuntu 18.04 arm8 part 6
- Topotests Ubuntu 18.04 amd64 part 7
- Topotests debian 10 amd64 part 6
- Topotests Ubuntu 18.04 i386 part 2
- Topotests debian 10 amd64 part 5
- Topotests Ubuntu 18.04 arm8 part 2
- Topotests debian 10 amd64 part 0
- Topotests Ubuntu 18.04 arm8 part 7
- Addresssanitizer topotests part 3
- Topotests Ubuntu 18.04 arm8 part 8
- Topotests debian 10 amd64 part 9
- Topotests Ubuntu 18.04 i386 part 7
- Fedora 29 rpm pkg check
- Addresssanitizer topotests part 6
- CentOS 7 rpm pkg check
- Topotests Ubuntu 18.04 amd64 part 2
- Topotests debian 10 amd64 part 7
- Topotests Ubuntu 18.04 amd64 part 3
- Topotests Ubuntu 18.04 amd64 part 0
- Topotests Ubuntu 18.04 i386 part 5
- Topotests debian 10 amd64 part 8
- Topotests Ubuntu 18.04 i386 part 0
@mruprich looks like the tests were expecting the convenience: https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_profiles_topo1/r2/bgpd.conf#L8
router bgp 100
! ...
neighbor 172.16.1.1 bfd profile fasttx
! ...
!
Same here: https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_topo3/r1/bgpd.conf#L5
router bgp 100
! ...
neighbor 2001:db8:1::2 bfd profile fast-tx
! ...
!
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-7456/
This is a comment from an automated CI system. For questions and feedback in regards to this CI system, please feel free to email Martin Winter - mwinter (at) opensourcerouting.org.
Get source / Pull Request: Successful
Building Stage: Successful
Basic Tests: Failed
Topotests Ubuntu 18.04 amd64 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TP1U1804AMD64-7456/test
Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7456/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt
Topotests debian 10 amd64 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1DEB10AMD64-7456/test
Topology Tests failed for Topotests debian 10 amd64 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7456/artifact/TOPO1DEB10AMD64/ErrorLog/log_topotests.txt
Topotests Ubuntu 18.04 arm8 part 1: Failed (click for details)
Topotests Ubuntu 18.04 arm8 part 1: No useful log foundTopotests Ubuntu 18.04 i386 part 1: Failed (click for details)
Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO1U18I386-7456/test
Topology Tests failed for Topotests Ubuntu 18.04 i386 part 1 see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-7456/artifact/TOPO1U18I386/ErrorLog/log_topotests.txt
Successful on other platforms/tests
- Topotests Ubuntu 18.04 amd64 part 9
- Topotests Ubuntu 18.04 i386 part 9
- Topotests debian 10 amd64 part 2
- Addresssanitizer topotests part 8
- Static analyzer (clang)
- Addresssanitizer topotests part 6
- Topotests Ubuntu 18.04 i386 part 5
- Ubuntu 20.04 deb pkg check
- Topotests Ubuntu 18.04 i386 part 0
- Ubuntu 18.04 deb pkg check
- Topotests Ubuntu 18.04 amd64 part 8
- Topotests Ubuntu 18.04 arm8 part 5
- Topotests debian 10 amd64 part 7
- Topotests Ubuntu 18.04 arm8 part 0
- Topotests Ubuntu 18.04 arm8 part 6
- Addresssanitizer topotests part 0
- Topotests debian 10 amd64 part 6
- Topotests debian 10 amd64 part 3
- Topotests Ubuntu 18.04 amd64 part 6
- Topotests Ubuntu 18.04 arm8 part 3
- Debian 9 deb pkg check
- Addresssanitizer topotests part 4
- Addresssanitizer topotests part 1
- Topotests Ubuntu 18.04 i386 part 4
- Topotests debian 10 amd64 part 5
- Topotests debian 10 amd64 part 4
- Topotests Ubuntu 18.04 i386 part 2
- Topotests Ubuntu 18.04 amd64 part 3
- Topotests Ubuntu 18.04 arm8 part 7
- Addresssanitizer topotests part 7
- Topotests Ubuntu 18.04 arm8 part 8
- Topotests Ubuntu 18.04 i386 part 7
- Topotests Ubuntu 18.04 amd64 part 5
- IPv4 ldp protocol on Ubuntu 18.04
- Ubuntu 16.04 deb pkg check
- Topotests Ubuntu 18.04 amd64 part 2
- Addresssanitizer topotests part 5
- Topotests Ubuntu 18.04 i386 part 6
- Topotests Ubuntu 18.04 i386 part 8
- Topotests Ubuntu 18.04 amd64 part 0
- Topotests Ubuntu 18.04 i386 part 3
- Topotests debian 10 amd64 part 8
- Debian 10 deb pkg check
- Topotests Ubuntu 18.04 arm8 part 4
- IPv6 protocols on Ubuntu 18.04
- Topotests Ubuntu 18.04 amd64 part 7
- Topotests Ubuntu 18.04 arm8 part 9
- Addresssanitizer topotests part 2
- Topotests debian 10 amd64 part 0
- Topotests Ubuntu 18.04 arm8 part 2
- Topotests debian 10 amd64 part 9
- Addresssanitizer topotests part 3
- Addresssanitizer topotests part 9
- Topotests Ubuntu 18.04 amd64 part 4
- CentOS 7 rpm pkg check
- Fedora 29 rpm pkg check
- IPv4 protocols on Ubuntu 18.04
@mruprich sorry I should have mentioned, those tests have more than one router missing the neighbor A.B.C.D bfd configuration:
- https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_profiles_topo1/r3/bgpd.conf#L5
- https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_profiles_topo1/r4/bgpd.conf#L9
- (this file is missing 3 lines) https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_topo3/r1/bgpd.conf#L5
- (this file is missing 2 lines) https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_topo3/r2/bgpd.conf#L5
- (this file is missing 3 lines) https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_topo3/r3/bgpd.conf#L6
- (this file is missing 2 lines) https://github.com/FRRouting/frr/blob/master/tests/topotests/bfd_topo3/r4/bgpd.conf#L5
One last thing: please use topotests or tests in the commit subject instead of bgpd.
where are we on this? did we decide we cannot have the order dependence in the configuration? if so, then we probably need to close this?
Update I'm working on bringing static route monitoring with BFD back and so I adapted the code to follow this new rule set.
In the upcoming PR I update the frr-bfdd.yang to provide a BFD integration grouping template for protocols to use which has this concept of keeping the BFD configuration but disabling its usage. With this change the order of the BFD commands is irrelevant, the neighbor A.B.C.D bfd command would just set the enable leaf of the YANG model.
Here is the work-in-progress YANG file snippet:
grouping bfd-monitoring {
description
"BFD monitoring template for protocol integration.";
leaf enable {
description
"Administrative state.";
type boolean;
default false;
}
leaf source {
type inet:ip-address;
description
"Source address to use for liveness check.
When source is not set and and multi-hop is `true` the source
address will be automatic selected through Next Hop Tracking (NHT).";
}
leaf multi-hop {
description
"Use multi hop session instead of single hop.";
type boolean;
default false;
}
leaf profile {
description
"BFD pre configured profile.";
type frr-bfdd:profile-ref;
}
}
Even though this alone will not 'unbreak' whatever the current BGP/BFD state, it means this PR should be probably be closed because currently BGP/BFD integration doesn't save configuration if BFD is not enabled.
@riw777 Hi, I was kinda hoping that after the NAK in comments above would get some follow-up, I even asked for one so I have no idea how to proceed now.
@eqvinox can you get back to this one? @mruprich David got sick for a few weeks and this one fell through the holes
Sorry @mruprich I think I should've answered this before.
@eqvinox pointed out what needs to be done:
If the user issues neighbor 2001:db8:2::2 bfd profile slowtx as a command, that configuration must be saved (while having no effect) even if BFD is not enabled for the neighbor. If peer->bfd_config does not exist, it must be created at this point even if BFD is disabled. (That also means checking for peer->bfd_config cannot be used to check if BFD is enabled on a peer.)
More explicitly this means: change BGP BFD integration code to always accept and save BFD configuration even though it is not enabled (and by enabled I mean neighbor A.B.C.D bfd). Currently, if I'm not mistaken, BGP will only keep BFD configuration if it is enabled, because the disabled check is basically if (peer->bfd_config == NULL).
NOTE on staticd and other YANG enabled daemons this is easier to do because a copy of the BFD configuration is always available in the northbound (outside the daemon's data structure). On BGP on the other hand there is no YANG northbound yet and the whole configuration keeping needs to be done in the peer data structure which is a bit more verbose code-wise.
IMHO, the fix @rzalamena is suggesting as part of the yang model would be a better idea in the long run ... thoughts?