frr
frr copied to clipboard
FRRouting
tools: fix unsetting of bfd profile in frr-reload
Given the following FRR configuration file:
[...]
router bgp 65000
neighbor 10.0.0.1 remote-as 65000
neighbor 10.0.0.1 bfd profile bfd-profile-1
[...]
The running configuration is:
[...]
router bgp 65000
neighbor 10.0.0.1 remote-as 65000
neighbor 10.0.0.1 bfd
neighbor 10.0.0.1 bfd profile bfd-profile-1
[...]
New FRR configuration file:
[...]
router bgp 65000
neighbor 10.0.0.1 remote-as 65000
[...]
Running configuration:
[...]
router bgp 65000
neighbor 10.0.0.1 remote-as 65000
neighbor 10.0.0.1 bfd
[...]
Despite the user's desire to disable BFD altogether, the reloader was just unsetting the custom BFD profile falling back to the default built-in profile.
Continuous Integration Result: FAILED
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3393/
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 i386 part 9: Failed (click for details)
Topotests Ubuntu 18.04 i386 part 9: Unknown LogSuccessful on other platforms/tests
- Topotests Ubuntu 18.04 arm8 part 6
- Topotests Ubuntu 18.04 amd64 part 0
- Topotests Ubuntu 18.04 arm8 part 1
- Topotests debian 10 amd64 part 6
- Addresssanitizer topotests part 2
- Topotests Ubuntu 18.04 amd64 part 7
- Topotests Ubuntu 18.04 amd64 part 5
- Topotests debian 10 amd64 part 5
- Topotests debian 10 amd64 part 0
- Addresssanitizer topotests part 3
- Topotests Ubuntu 18.04 arm8 part 2
- Topotests Ubuntu 18.04 arm8 part 7
- Topotests Ubuntu 18.04 amd64 part 4
- CentOS 7 rpm pkg check
- Addresssanitizer topotests part 9
- Fedora 29 rpm pkg check
- Topotests Ubuntu 18.04 i386 part 2
- Addresssanitizer topotests part 8
- Topotests debian 10 amd64 part 9
- Topotests Ubuntu 18.04 arm8 part 8
- Topotests Ubuntu 18.04 i386 part 7
- Topotests Ubuntu 18.04 amd64 part 3
- Addresssanitizer topotests part 6
- Topotests Ubuntu 18.04 amd64 part 1
- Topotests Ubuntu 18.04 i386 part 5
- Topotests Ubuntu 18.04 i386 part 0
- Ubuntu 18.04 deb pkg check
- Topotests Ubuntu 18.04 amd64 part 2
- Topotests debian 10 amd64 part 7
- Topotests Ubuntu 18.04 i386 part 8
- Topotests debian 10 amd64 part 8
- Addresssanitizer topotests part 0
- Topotests Ubuntu 18.04 arm8 part 4
- Debian 10 deb pkg check
- Topotests Ubuntu 18.04 arm8 part 9
- IPv6 protocols on Ubuntu 18.04
- Topotests Ubuntu 18.04 amd64 part 6
- Topotests debian 10 amd64 part 3
- Debian 9 deb pkg check
- Addresssanitizer topotests part 1
- Topotests Ubuntu 18.04 arm8 part 3
- Topotests Ubuntu 18.04 i386 part 4
- Topotests debian 10 amd64 part 4
- Topotests Ubuntu 18.04 i386 part 3
- IPv4 protocols on Ubuntu 18.04
- Addresssanitizer topotests part 4
- Topotests debian 10 amd64 part 2
- Topotests Ubuntu 18.04 amd64 part 9
- Addresssanitizer topotests part 7
- Static analyzer (clang)
- Ubuntu 16.04 deb pkg check
- Ubuntu 20.04 deb pkg check
- Topotests debian 10 amd64 part 1
- Topotests Ubuntu 18.04 amd64 part 8
- Addresssanitizer topotests part 5
- Topotests Ubuntu 18.04 i386 part 1
- Topotests Ubuntu 18.04 i386 part 6
- Topotests Ubuntu 18.04 arm8 part 0
- IPv4 ldp protocol on Ubuntu 18.04
- Topotests Ubuntu 18.04 arm8 part 5
Continuous Integration Result: SUCCESSFUL
Continuous Integration Result: SUCCESSFUL
Congratulations, this patch passed basic tests
Tested-by: NetDEF / OpenSourceRouting.org CI System
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3398/
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.
Continuous Integration Result: SUCCESSFUL
Continuous Integration Result: SUCCESSFUL
Congratulations, this patch passed basic tests
Tested-by: NetDEF / OpenSourceRouting.org CI System
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3404/
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.
Continuous Integration Result: FAILED
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3405/
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 i386 part 9: Failed (click for details)
Topotests Ubuntu 18.04 i386 part 9: Unknown LogSuccessful on other platforms/tests
- Addresssanitizer topotests part 2
- Topotests Ubuntu 18.04 amd64 part 5
- Topotests Ubuntu 18.04 i386 part 6
- IPv4 ldp protocol on Ubuntu 18.04
- Topotests debian 10 amd64 part 5
- Ubuntu 16.04 deb pkg check
- Topotests Ubuntu 18.04 amd64 part 4
- Topotests Ubuntu 18.04 i386 part 1
- Addresssanitizer topotests part 9
- Topotests Ubuntu 18.04 i386 part 2
- Topotests Ubuntu 18.04 arm8 part 1
- Topotests debian 10 amd64 part 6
- Topotests Ubuntu 18.04 arm8 part 6
- Addresssanitizer topotests part 3
- Topotests Ubuntu 18.04 amd64 part 7
- Topotests Ubuntu 18.04 i386 part 7
- Topotests Ubuntu 18.04 arm8 part 2
- Topotests Ubuntu 18.04 arm8 part 7
- Topotests debian 10 amd64 part 0
- Addresssanitizer topotests part 7
- Topotests Ubuntu 18.04 arm8 part 8
- Topotests Ubuntu 18.04 i386 part 0
- Addresssanitizer topotests part 6
- Topotests debian 10 amd64 part 9
- CentOS 7 rpm pkg check
- Fedora 29 rpm pkg check
- Topotests debian 10 amd64 part 7
- Topotests Ubuntu 18.04 amd64 part 3
- Topotests Ubuntu 18.04 amd64 part 0
- Topotests debian 10 amd64 part 8
- IPv6 protocols on Ubuntu 18.04
- Topotests Ubuntu 18.04 amd64 part 2
- Topotests Ubuntu 18.04 arm8 part 4
- Topotests Ubuntu 18.04 i386 part 5
- Topotests Ubuntu 18.04 arm8 part 9
- Ubuntu 18.04 deb pkg check
- Ubuntu 20.04 deb pkg check
- Debian 9 deb pkg check
- Debian 10 deb pkg check
- IPv4 protocols on Ubuntu 18.04
- Addresssanitizer topotests part 1
- Topotests Ubuntu 18.04 i386 part 4
- Topotests Ubuntu 18.04 amd64 part 1
- Topotests Ubuntu 18.04 amd64 part 9
- Topotests Ubuntu 18.04 i386 part 8
- Addresssanitizer topotests part 8
- Topotests Ubuntu 18.04 i386 part 3
- Topotests Ubuntu 18.04 amd64 part 6
- Addresssanitizer topotests part 4
- Topotests Ubuntu 18.04 arm8 part 3
- Topotests debian 10 amd64 part 1
- Topotests Ubuntu 18.04 arm8 part 5
- Addresssanitizer topotests part 5
- Topotests debian 10 amd64 part 4
- Topotests debian 10 amd64 part 2
- Topotests Ubuntu 18.04 arm8 part 0
- Topotests Ubuntu 18.04 amd64 part 8
- Static analyzer (clang)
- Addresssanitizer topotests part 0
- Topotests debian 10 amd64 part 3
Continuous Integration Result: FAILED
Continuous Integration Result: FAILED
See below for issues. CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3450/
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
IPv6 protocols on Ubuntu 18.04: Failed (click for details)
Successful on other platforms/tests
- Topotests Ubuntu 18.04 i386 part 3
- Topotests Ubuntu 18.04 amd64 part 6
- Topotests Ubuntu 18.04 i386 part 8
- Addresssanitizer topotests part 0
- Topotests Ubuntu 18.04 amd64 part 2
- Topotests debian 10 amd64 part 8
- Topotests debian 10 amd64 part 4
- Topotests debian 10 amd64 part 9
- Topotests Ubuntu 18.04 arm8 part 4
- Addresssanitizer topotests part 4
- Topotests Ubuntu 18.04 arm8 part 9
- Topotests debian 10 amd64 part 3
- Debian 9 deb pkg check
- Addresssanitizer topotests part 1
- Topotests Ubuntu 18.04 i386 part 4
- Topotests Ubuntu 18.04 arm8 part 3
- Addresssanitizer topotests part 9
- Topotests Ubuntu 18.04 amd64 part 8
- Topotests debian 10 amd64 part 2
- IPv4 protocols on Ubuntu 18.04
- Topotests Ubuntu 18.04 arm8 part 0
- Static analyzer (clang)
- Topotests Ubuntu 18.04 arm8 part 2
- Topotests debian 10 amd64 part 0
- Ubuntu 16.04 deb pkg check
- Topotests Ubuntu 18.04 amd64 part 9
- Topotests Ubuntu 18.04 arm8 part 7
- Addresssanitizer topotests part 7
- Topotests Ubuntu 18.04 i386 part 6
- Ubuntu 20.04 deb pkg check
- Topotests Ubuntu 18.04 i386 part 1
- Topotests Ubuntu 18.04 amd64 part 5
- Topotests Ubuntu 18.04 arm8 part 5
- Debian 10 deb pkg check
- IPv4 ldp protocol on Ubuntu 18.04
- Addresssanitizer topotests part 5
- Topotests debian 10 amd64 part 1
- Topotests Ubuntu 18.04 arm8 part 1
- Topotests Ubuntu 18.04 amd64 part 7
- Topotests Ubuntu 18.04 arm8 part 6
- Topotests Ubuntu 18.04 amd64 part 0
- Topotests debian 10 amd64 part 6
- Topotests Ubuntu 18.04 i386 part 5
- Addresssanitizer topotests part 3
- Topotests Ubuntu 18.04 i386 part 0
- Addresssanitizer topotests part 2
- Fedora 29 rpm pkg check
- Topotests Ubuntu 18.04 amd64 part 4
- CentOS 7 rpm pkg check
- Topotests Ubuntu 18.04 i386 part 9
- Topotests debian 10 amd64 part 7
- Topotests Ubuntu 18.04 amd64 part 3
- Topotests Ubuntu 18.04 i386 part 2
- Topotests debian 10 amd64 part 5
- Addresssanitizer topotests part 8
- Topotests Ubuntu 18.04 arm8 part 8
- Topotests Ubuntu 18.04 i386 part 7
- Addresssanitizer topotests part 6
- Topotests Ubuntu 18.04 amd64 part 1
- Ubuntu 18.04 deb pkg check
Continuous Integration Result: SUCCESSFUL
Congratulations, this patch passed basic tests
Tested-by: NetDEF / OpenSourceRouting.org CI System
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3456/
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.
From yesterday's technical meeting, the FRR community would prefer that the turn on/off of BFD be an affirmative action instead of an implied action of the neighbor 10.0.0.1 bfd profile bfd-profile-1. This way reload would work properly and be more in line with the rest of FRR behavior. This of course means that the prefered solution on our part would be to make this change in the code instead of this one.
Hi @donaldsharp, just wanted to make sure what you mean by affirmative action. Does that mean that you need to turn on bfd before you set a profile? And the same way when you issue 'no neighbor x.x.x.x bfd profile bfd-profile' you also need to use 'no neighbor x.x.x.x bfd' as well?
Hi @donaldsharp, just wanted to make sure what you mean by affirmative action. Does that mean that you need to turn on bfd before you set a profile? And the same way when you issue 'no neighbor x.x.x.x bfd profile bfd-profile' you also need to use 'no neighbor x.x.x.x bfd' as well?
Right.
Hi, so just testing this idea here, if it is good enough I can make a PR. So isn't this all just a matter of adding a check for the peer->bfd_config when setting anything else?
diff --git a/bgpd/bgp_bfd.c b/bgpd/bgp_bfd.c
index a859b7a..d75a6f7 100644
--- a/bgpd/bgp_bfd.c
+++ b/bgpd/bgp_bfd.c
@@ -586,6 +586,12 @@ DEFUN(neighbor_bfd_profile, neighbor_bfd_profile_cmd,
if (!peer)
return CMD_WARNING_CONFIG_FAILED;
+ //BFD not yet enabled
+ if (!peer->bfd_config) {
+ vty_out(vty, "%% Enable bfd for this neighbor first\n"); <--- just an example warning
+ return CMD_WARNING_CONFIG_FAILED;
+ }
+
if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
bgp_group_configure_bfd(peer);
else
Similar to the case when you would try to set 'neighbor x.x.x.x bfd' before setting 'neighbor x.x.x.x remote-as xxxxx'? This check would probably go to neighbor_bfd_check_controlplane_failure and neighbor_bfd_profile to make sure the user enables the general bfd option first.
Also I don't quite understand these lines in bgp_bfd.c in no_neighbor_bfd_profile function:
if (CHECK_FLAG(peer->sflags, PEER_STATUS_GROUP))
bgp_group_configure_bfd(peer);
else
bgp_peer_configure_bfd(peer, true);
Why configuring bfd again when using 'no neighbor x.x.x.x bfd profile
Thanks.
@mruprich
so just testing this idea here, if it is good enough I can make a PR. So isn't this all just a matter of adding a check for the peer->bfd_config when setting anything else?
Yes, your idea looks good. I don't recall what is the behavior for IS-IS or OSPFd but I believe that after much discussion the correct thing is to expect neighbor A.B.C.D bfd and neighbor A.B.C.D bfd profile WORD. The shortcut is a convenience that is caused more pain than good.
Also I don't quite understand these lines in bgp_bfd.c in no_neighbor_bfd_profile function: code block Why configuring bfd again when using 'no neighbor x.x.x.x bfd profile ' command?
When you remove the BFD configuration from a peer group, it is possible that a peer from the group has its own specific BFD configuration. In that case you want to keep the BFD configuration applied.
@rzalamena
Thanks,
seems like ospf needs this as well but with ISIS I am a bit confused. Seems like with ISIS you can use the 'isis bfd' and 'isis bfd profile
# conf t
(config) # int eth0
(config-if) # ip router isis myisis
(config-if) # isis bfd profile myprofile
(config-if) # do sh run
!
interface eth0
ip router isis myisis
isis bfd profile myprof
exit
(config-if) # isis bfd
(config-if) # do sh run
interface eth0
ip router isis myisis
isis bfd
isis bfd profile myprof
exit
(config-if) # no isis bfd
(config-if) # do sh run
interface eth0
ip router isis myisis
isis bfd profile myprof
exit
I would expect that the profile would be removed with 'no isis bfd', but frr does not seem to care. Is this ok? I do not feel qualified to say yes or no in this case.
@mruprich
I would expect that the profile would be removed with 'no isis bfd', but frr does not seem to care. Is this ok? I do not feel qualified to say yes or no in this case.
IS-IS is behaving differently and should be eventually fixed (so not ok). Looking at the YANG model I can see that it doesn't use the presence container, so the profile leaf is independent of enabled leaf. Ideally all BFD options should be underneath a presence container and if the presence container is gone so should all configuration (doesn't make sense to keep BFD configurations in memory if the feature is disabled).
IS-IS yang model ( https://github.com/FRRouting/frr/blob/master/yang/frr-isisd.yang#L541 ):
container bfd-monitoring {
leaf enabled {
type boolean;
default "false";
description
"Monitor IS-IS peers on this circuit.";
}
leaf profile {
type string;
description
"Let BFD use a pre-configured profile.";
}
}
PIM YANG model for reference ( https://github.com/FRRouting/frr/blob/master/yang/frr-pim.yang#L364 ):
container bfd {
presence
"Enable BFD support on the interface.";
leaf profile {
type string;
description
"Use a preconfigure BFD profile.";
}
}
The BFD protocol integration grew organically and with help of different developers so these inconsistencies are just beginning to be fixed. Some of the daemons don't even have YANG northbound yet (like OSPF).
@rzalamena Thanks! I am probably going to take it one step at a time, leaving the isis for the last one since that one seems the most complicated.
I created a PR for bgp - https://github.com/FRRouting/frr/pull/11951
Looking again at the ospf, this has already been added in this commit: https://github.com/FRRouting/frr/commit/f3fd7196ebe752ebe7464ebcc1cafd833bbc5e54
It just did not appear in the latest release, that is why I missed it. I am going to take a look at IS-IS as well but that will take more time.
@cgoncalves thanks for the PR and for bringing attention to the problem, but the correct approach is to expect the bfd line before bfd profile for 2 reasons:
- YANG model consistency: the
bfdpresence container needs to be created first, then we can start saving BFD related configurations (even though BGP doesn't have YANG northbound yet). -
frr-reload.pyalready expectsbfdline to exist, otherwise it'd append anno neighbor A.B.C.D bfdto the configuration update and remove thebfd profileconfiguration anyway (because of the presence container removal).
I'm closing this PR because I think this won't go forward.