First of all, thank you very much for taking on this very thorough rework, I really appreciate it.
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.
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'.
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.
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.
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.
- 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