bgpv2: Adding preflight and neighbor reconcilers using CiliumBGPNodeConfig CRD. #30108
Conversation
This file contains 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
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
harsimran-pabla
added
area/bgp
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
from
9a52d0e to
aaed377
Compare
rastislavs
reviewed
Jan 5, 2024
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
from
aaed377 to
6909d7d
Compare
|
/test |
YutaroHayakawa
requested changes
Jan 17, 2024
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
2 times, most recently
from
921387b to
de3abe0
Compare
|
Thanks @YutaroHayakawa for the review. I think all of them are addressed. One significant change is new logger package under bgpv1/, I think it makes sense to put all the k/v variables in one place so logging is consistent across BGP sub-system. Let me know if this makes sense. cc @rastislavs |
YutaroHayakawa
requested changes
Jan 22, 2024
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
2 times, most recently
from
b934469 to
4ccdda7
Compare
YutaroHayakawa
requested changes
Jan 24, 2024
There was a problem hiding this comment.
Did a third review. I added a comment for the issue I found while I was working for this PR #30382.
Since we derived new reconcilers from the v1 reconciler, it obviously contains these issues we had in the bgpv1. I started to wonder how much of the issue we want to solve in this PR.
One option is, we only fix "critical" issues and let rest of the fixes to go into the follow-up if we can do that later. The cons of this approach is it is very easy to deprioritize the follow-up once this feature starts to "work". In that sense, we should solve any concern in this PR. WDYT?
Introducing preflight and neighbor reconciler using CiliumBGPNodeConfig and CiliumBGPPeerConfig resources. Reconcilers are added in reconcilerv2 pkg. Preflight reconciler is used to create, delete and update BGP instances. It differs from BGPv1 preflight reconciler. It overrides RouterID and listening port from CiliumBGPNodeConfig, if they are set. Neighbor reconciler is used to create, delete and update BGP peer configuration per instance. It differs from BGPv1 reconciler in terms to metadata storage and how peer configuration is created. In case of v2 reconcilers, peer configuration is fetched from CiliumBGPPeerConfig CRD resource. These reconcilers will need to be registered with BGP cell, this will be done in future changes. Signed-off-by: harsimran pabla <[email protected]>
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
2 times, most recently
from
6bdea43 to
ce0c626
Compare
YutaroHayakawa
requested changes
Jan 26, 2024
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
from
ce0c626 to
4efe1d7
Compare
This change introduces new API ReconcileInstances to BGPRouteManager. This API will be called by controller when BGPv2 is enabled and reconciliation is triggered. New BGPv2 equivalent methods are added to workdiff and BGPRouteManager. This is done because new configuration is based on CiliumBGPNodeConfig instead of CiliumBGPPeeringPolicy. Signed-off-by: harsimran pabla <[email protected]>
harsimran-pabla
force-pushed
the
hpabla/bgp/v2/manager
branch
from
4efe1d7 to
7c9150f
Compare
rastislavs
approved these changes
Jan 29, 2024
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
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 change introduces scaffolding for BGPv2 reconcilers, starting with preflight and neighbor reconcilers. At later stage, other reconcilers will be added.
Change includes two commits