tools: fix unsetting of bfd profile in frr-reload #10570
Conversation
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: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topotests Ubuntu 18.04 i386 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3393/artifact/TOPO9U18I386/ErrorLog/ Topotests Ubuntu 18.04 i386 part 9: No useful log foundSuccessful on other platforms/tests
|
cgoncalves
force-pushed
the
reloader_bfd_profile
branch
from
23fcca7
to
1be31fc
Compare
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-PULLREQ2-3398/ This is a comment from an automated CI system. |
cgoncalves
force-pushed
the
reloader_bfd_profile
branch
2 times, most recently
from
b5456c9
to
6709aec
Compare
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-PULLREQ2-3404/ This is a comment from an automated CI system. |
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: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 9: Failed (click for details)Topotests Ubuntu 18.04 i386 part 9: Unknown Log URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3405/artifact/TOPO9U18I386/ErrorLog/ Topotests Ubuntu 18.04 i386 part 9: No useful log foundSuccessful on other platforms/tests
|
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.
Signed-off-by: Carlos Goncalves <cgoncalves@redhat.com>
cgoncalves
force-pushed
the
reloader_bfd_profile
branch
from
6709aec
to
15141f5
Compare
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: SuccessfulBasic Tests: FailedIPv6 protocols on Ubuntu 18.04: Failed (click for details)Successful on other platforms/tests
|
|
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-PULLREQ2-3456/ This is a comment from an automated CI system. |
There was a problem hiding this comment.
This PR fixes a problem with hand made configurations using neighbor A.B.C.D bfd profile FOO without previous statement neighbor A.B.C.D bfd (which is convenient), so it seems valid to me.
Regarding the Python code and how it was written I don't feel confident to evaluate that, other than that it looks fine to me (just giving someone else with more Python/frr-reload knowledge time to evaluate this).
|
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 |
|
Hi @donaldsharp, |
Right. |
|
Hi, 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: Why configuring bfd again when using 'no neighbor x.x.x.x bfd profile ' command? Thanks. |
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
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 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 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 |
|
@cgoncalves thanks for the PR and for bringing attention to the problem, but the correct approach is to expect the
I'm closing this PR because I think this won't go forward. |
Given the following FRR configuration file:
The running configuration is:
New FRR configuration file:
Running configuration:
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.