oxidecomputer / oxidecomputer/maghemite
RIB Path shutdown needs rework
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 94
- Forks
- 6
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 12
Description
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
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.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by tracing the RIB Path struct, BFD FSM transitions and peer deletion, static-route installation, and BGP bestpath handling. Define separate state for BFD next-hop status and BGP graceful shutdown, then verify the reported peer-removal and new-static-route races are covered by tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- networking
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100