Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bgpv1: pass router state to gobgp #26194

Merged
merged 2 commits into from Jun 27, 2023
Merged

bgpv1: pass router state to gobgp #26194

merged 2 commits into from Jun 27, 2023

Conversation

harsimran-pabla
Copy link
Contributor

@harsimran-pabla harsimran-pabla commented Jun 13, 2023 •
edited by YutaroHayakawa

This change passes additional context regarding the control plane state to GoBGP.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 13, 2023
@harsimran-pabla harsimran-pabla added area/bgp release-note/misc This PR makes changes that have no direct user impact. labels Jun 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 13, 2023
@harsimran-pabla harsimran-pabla force-pushed the hpabla/bgp_refactor_cee branch 2 times, most recently from abf4590 to e6e40b2 Compare June 20, 2023 15:47
@harsimran-pabla harsimran-pabla marked this pull request as ready for review June 20, 2023 15:47
@harsimran-pabla harsimran-pabla requested a review from a team as a code owner June 20, 2023 15:47
ldelossa
ldelossa approved these changes Jun 20, 2023
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look fine to me.

Just have to remember not to hold onto a ControlPlaneState, as that data is always pushed down on reconcile.

I say this just because the constructor almost looks like its getting ready to hold onto the state, but the intention is just to use it as flags for construction purposes right? I.e. the ControlPlaneState shouldn't be referenced after initial BGP Speaker initialization, cuz it may contain stale data.

@harsimran-pabla
Copy link
Contributor Author

@ldelossa Yes, the idea is only to use it in the constructor and not hold on to it.

@harsimran-pabla
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 21, 2023
@harsimran-pabla
Copy link
Contributor Author

Not a new feature, should be eligible for merge.

cc @ldelossa

@ldelossa
Copy link
Contributor

@harsimran-pabla theres two stuck tests here, can you rebase and push?

@harsimran-pabla
Copy link
Contributor Author

/test

@ldelossa
Copy link
Contributor

/test

@harsimran-pabla
bgpv1: pass vrouter in neighbor calls and cstate to gobgp
4e73aa2 
Pass CiliumBGPVirtualRouter in NeighborRequest and cstate while
initializing gobgp server. These changes are done to pass extra context
to gobgp.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
@harsimran-pabla
bgpv1: use types.NeighborRequest in getPeerConfig
b29d036 
types.NeighborRequest contains additional information which can be used
to create Peer request.

Signed-off-by: harsimran pabla <hpabla@isovalent.com>
@brb
Copy link
Member

brb commented Jun 27, 2023

@harsimran-pabla @ldelossa Please rebase against the latest main branch to avoid ci-e2e failures.

@harsimran-pabla
Copy link
Contributor Author

Thanks @brb, currently this PR is on top of commit fbbdcab from main, I think it contains KPR related fixes.

@ldelossa
Copy link
Contributor

/test

@ldelossa ldelossa merged commit 2ef6aa7 into cilium:main Jun 27, 2023
65 checks passed
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

@rastislavs rastislavs rastislavs approved these changes

Assignees
No one assigned
Labels
area/bgp ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@harsimran-pabla @ldelossa @brb @rastislavs