Intoduce tokio/async-based BFD implementation#795
Open
jgallagher wants to merge 23 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a tokio/async-based implementation of BFD. It's heavily based on the existing implementation both in API shape and names, but has some intentional architectural differences (primarily to support pretty extensive unit testing). The non-test code is handwritten, but I leaned pretty heavily on Claude to help with writing tests (although I went over all of them by hand - it has a tendency to write flaky-in-a-not-obvious-way tests when sockets are involved, so hopefully I caught all of those) and used it to compare this implementation against the existing one. The primary motivator for moving to async is fixing #787, which it does: running the reproducer against this implementation results in the
DELETEs returning more or less immediately (under 1ms).The biggest architectural changes are:
send_to()getting stuck or something) used to mean we'd have unbounded memory growth now means we have a lot of dropped packets instead.Some less-architectural-but-still-intentional changes:
NonZeroU8, both because I was concerned about multiplying by 0 and because the RFC says such packetsMUSTbe rejected. We now do so, and also return a 400 if we're asked to configure a session with a threshold of 0. This might be the wrong call and I'm happy to discuss - there was a "panic on startup if we can't re-add all peers" path, so I made that path bump any persisted 0s to 1s, but maybe I should do the same in the API handler instead of returning a 400, in case there are Nexus configs persisted that have a threshold of 0?as u32that wrapped around on large values with.saturating_*()methods).SessionCounters::control_packets_receivedon incoming packets - I think this was just an oversight before?Some things that I left unchanged but I'm not sure are correct:
TODO-correctnesscomments on these with my notes but didn't want to change that behavior as part of a big rework.detection_multipliervalue (also tagged with aTODO-correctnessin the state machine).UdpSocket::send_to(). Could that leave us in a constant loop of closing/opening sockets if sends are failing for some reason not related to the local socket?Upstate, we'll leave behind anexthop_shudownof false for that address? I don't know whether this is okay or gets cleaned up by something else.required_min_rxalso be aNonZerotype? If that gets set to 0 we'll also have a "multiply by 0" in the calculation of the recv deadline.This PR is only the async implementation with no modifications to the main
mgdbinary except the change of detection threshold toNonZeroU8mentioned above. I'll open a second PR that swaps the implementations over.