Both BFD and BGP use the same field of the RIB Path struct to indicate "shutdown", even though that means two semantically different things.
For BGP, we have a partial implementation of graceful shutdown which uses this flag to indicate that the peer advertised the path to us with the Graceful-Shutdown community on it. The actual semantics of gshut is not to make this path unusable in the face of all others, but to lower the preference by treating the path as if other attributes (local pref or med, I forget offhand) are set to an extremely unfavorable value. It's entirely possible and reasonable for operator policy to configure those attributes to extremely unfavorable values on a different path, and this implementation influences how bestpath works in those circumstances.
For BFD, this is simply an indicator of whether the last BFD FSM transition moved into or out of the Up state. The point of this is to give a simple go/no-go indicator to the RIB as to whether BFD has indicated the path is down. One problem in the implementation is that BFD does not restore the next-hop shutdown status to false when the corresponding peer is deleted. Another problem is that static routes unconditionally install routes with Path.shutdown set to false. Both of these cause bugs due to race conditions:
- If a BFD peer goes down, all routes installed via that next-hop are shutdown=true. If a new static route is configured via that next-hop, it becomes the only route with that next-hop where shutdown=false. This can last indefinitely, or until the BFD peer comes back up.
- If a BFD peer is removed, the next-hop shutdown status is not cleared. If the BFD peer is down when it is removed, all routes currently in the RIB with that next-hop will point to a
Path where shutdown is true indefinitely, or until either the routes are deleted/reinstalled or the BFD peer is A) reconfigured, B) brought to Up state, C) deleted.
For the time being, there are no ties between BFD and BGP (BFD operating solely for static routes) but this is likely a temporary state until customer requirements demand such an integration. At such a time, it will not be feasible for BGP and BFD to share a single bool to indicate shutdown status -- as the BGP FSM will need to react to it, and there would be no way for bestpath to distinguish between "BGP gshut is set" and "BFD is down for this BGP peer".
Overall, this needs a rewrite of the type as a whole so at the very least these two items do not share state.
Both BFD and BGP use the same field of the RIB
Pathstruct to indicate "shutdown", even though that means two semantically different things.For BGP, we have a partial implementation of graceful shutdown which uses this flag to indicate that the peer advertised the path to us with the Graceful-Shutdown community on it. The actual semantics of gshut is not to make this path unusable in the face of all others, but to lower the preference by treating the path as if other attributes (local pref or med, I forget offhand) are set to an extremely unfavorable value. It's entirely possible and reasonable for operator policy to configure those attributes to extremely unfavorable values on a different path, and this implementation influences how bestpath works in those circumstances.
For BFD, this is simply an indicator of whether the last BFD FSM transition moved into or out of the
Upstate. The point of this is to give a simple go/no-go indicator to the RIB as to whether BFD has indicated the path is down. One problem in the implementation is that BFD does not restore the next-hop shutdown status tofalsewhen the corresponding peer is deleted. Another problem is that static routes unconditionally install routes withPath.shutdownset tofalse. Both of these cause bugs due to race conditions:Pathwhere shutdown is true indefinitely, or until either the routes are deleted/reinstalled or the BFD peer is A) reconfigured, B) brought toUpstate, C) deleted.For the time being, there are no ties between BFD and BGP (BFD operating solely for static routes) but this is likely a temporary state until customer requirements demand such an integration. At such a time, it will not be feasible for BGP and BFD to share a single bool to indicate shutdown status -- as the BGP FSM will need to react to it, and there would be no way for bestpath to distinguish between "BGP gshut is set" and "BFD is down for this BGP peer".
Overall, this needs a rewrite of the type as a whole so at the very least these two items do not share state.