On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
Hello
I finally got to finish reviewing and merging this patchset, sorry for delay.
Here are the modified versions:
https://gitlab.nic.cz/labs/bird/-/commits/dynamic-nbrs-2
https://gitlab.nic.cz/labs/bird/-/commit/4327fabff1db9b78e5e80bb8d22dc848fbf...
https://gitlab.nic.cz/labs/bird/-/commit/7eda3e381414f97caed5b2cd02d6b4b3b45...
https://gitlab.nic.cz/labs/bird/-/commit/0ee9f93bd076c5cc425ceaec9acedbbb7c9...
(They are backported to BIRD v2, i will port them to BIRD v3 soon)
Hi Ondrej, First of all, thank you very much for taking on this very thorough rework, I really appreciate it. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
Please take a look at these patches and reply if you are okay with them after my modifications. If you have any questions, feel free to ask.
Overall, I have no objections to the changes to my original patches. I think the added consistency with the existing pieces is a clear improvement, so thank you for that. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
The most visible change is naming, using neighbor instead of peer. That is consistent with how equivalent structures are named in other protocols in peer.
I went back and forth on the naming for a while, at the start of the implementation, so I am entirely fine settling for 'neighbor'. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
Otherwise in the first two patches (Nest and RADv) there are just minor changes:
Nest: - Neighbor entry formatting change - Parser grammar for neighbor entries - Filter tests - Minor fixes - Some documentation
Sorry for missing on adding the filter tests in the first place, it is really something I should have included myself. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
RAdv: - Moving neighbor prune to separate timer unrelated to ifaces - More idiomatic log messages, incluing TBF for warning - Reusing existing 'lifetime' attribute for neighbor entries (it still stores expire time but plan to change it in the future)
Moving neighbor prune to separate timer is a good idea, my original reuse of the timer was a little too "hacky", in hindsight. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
With the BGP patch the situation was more complicated, i noticed several bugs in our dynamic BGP code in BIRD 3 and some parts of your patch looked like a workaround for some of these issues.
- Refactoring BGP spawning code to avoid duplication
- No disable and re-spawn of existing spawned protocols, this should not be necessary. We just claim existing sessions in this case.
- Simplified some parts of channel handling
Yes, I had to workaround some of the original dynamic BGP behavior (especially when it came to session duplication), so my implementation came out a lot more complicated than I would have liked. Thank you very much for reworking the spawning behavior, I have seen the patch and the automatic peering implementation is much cleaner now. On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <santiago@crfreenet.org> wrote:
- I removed the 'persist' option. We just keep track whether the session is claimed (there is existing neighbor entry) or unclaimed. This is just makeshift now, there is a good question how to properly handle unclaimed sessions, but i think it should be consistent with how we handle dynamic BPG sessions from incoming connections, so i would prefer to keep it simple now.
I am ok with dropping the persist option. The main intent was always to keep the behavior aligned with the dynamic BGP one (this is why I set persist=on as the default option). The automatic teardown was a cool option but not essential to the feature and I can see the benefit of keeping things as simple as possible. Thanks again for the work on this. Let me know if you need anything or have further comments for the BIRD 3 port. Best Regards, Matteo