On Mon, 2 Mar 2026 at 06:54, Ondrej Zajicek <[email protected]> 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/4327fabff1db9b78e5e80bb8d22dc848fbf9a6b9 > > https://gitlab.nic.cz/labs/bird/-/commit/7eda3e381414f97caed5b2cd02d6b4b3b4554ee0 > > https://gitlab.nic.cz/labs/bird/-/commit/0ee9f93bd076c5cc425ceaec9acedbbb7c9021ec > > (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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
