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

BGP-CP: Remove NodeSpecer and ControlPlaneState #27285

Merged
merged 6 commits into from Aug 16, 2023
Merged

BGP-CP: Remove NodeSpecer and ControlPlaneState #27285

merged 6 commits into from Aug 16, 2023

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Aug 4, 2023

This PR removes the NodeSpecer abstraction and the ControlPlaneState structure from the BGP control plane.

Now that LocalNodeStore is available it can be used in place of these two constructs.

Additionally, since all ConfigReconcilers are now part of the Hive/Cell dependency injection graph, each ConfigReconciler can declare their own depedencies at time of construction. This provides much greater flexibility as a ConfigReconciler can now utilize any entities available in the DIG to perform their necessary reconciliation.

Remove NodeSpecer and ControlPlaneState from BGP-CP. Rely on Hive/Cell for further ConfigReconciler dependencies.

@ldelossa
pkg/node: expose 'newTestLocalNodeStore' for testing
a25e595 
This commit makes the previously private 'newTestLocalNodeStore'
constructor public and also updates the constructor to receive a mock
LocalNode structure.

The returned 'LocalNodeStore' will then "emit" the mock LocalNode when
invoked.

This is useful for unit tests which include the 'LocalNodeStore'
structure as a depedency, avoiding the need to create a Hive for the
unit test.

These changes will be used in subsequent commits.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@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 Aug 4, 2023
@ldelossa ldelossa added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 4, 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 Aug 4, 2023
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 4, 2023

/test

@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from 0a58aa6 to 68fc1b8 Compare August 4, 2023 20:58
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 4, 2023

/test

@joestringer
Copy link
Member

Is this changing a user-facing interface or will it have user-facing consequences? If yes, release note seems fine though I'm not sure whether users will understand it. If no, this should probably be labelled with release-note/misc. Note that even if it's misc, that's not saying the PR is any less valuable, it's just that we use the major/minor/bug/misc as a way to order the release notes from most user-facing and prominent for all users to know about to least user-facing and prominent.

@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from 68fc1b8 to 6724985 Compare August 5, 2023 03:23
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 5, 2023

@joestringer no user facing changes. I'll update the label to misc.

@ldelossa ldelossa added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Aug 5, 2023
@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from 6724985 to e27e4ed Compare August 5, 2023 12:11
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 5, 2023

/test

rastislavs
rastislavs reviewed Aug 7, 2023
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment •
edited

Choose a reason for hiding this comment

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

found just some nits, overall a great change!

pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved
pkg/bgpv1/manager/manager.go Outdated Show resolved Hide resolved
pkg/bgpv1/agent/annotations.go Show resolved Hide resolved
@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from e27e4ed to ad19b90 Compare August 7, 2023 17:27
harsimran-pabla
harsimran-pabla reviewed Aug 7, 2023
View reviewed changes
pkg/bgpv1/agent/controller.go Show resolved Hide resolved
pkg/bgpv1/agent/controller.go Outdated Show resolved Hide resolved
pkg/bgpv1/manager/manager.go Show resolved Hide resolved
pkg/bgpv1/manager/reconcile.go Show resolved Hide resolved
pkg/bgpv1/agent/annotations.go Outdated Show resolved Hide resolved
pkg/bgpv1/agent/controller.go Show resolved Hide resolved
@ldelossa
bgpv1,annotations: introduce ResolveRouterID method
48c2111 
Introduce a helper method for resolving the RouterID with a parsed
BGP-CP annotation.

This will be used in subsequent commits

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa
bgpv1: use LocalNodeStore to eliminiate ControlPlaneState and NodeSpecer
273f10c 
This commit removes usages of NodeSpecer and ControlPlaneState from the
BGP Control Plane.

With the inclusion of LocalNodeStore we no longer need the NodeSpecer
abstraction nor the ControlPlaneState.

Moving forward all ConfigReconcilers will get a read-only node.LocalNode object
to inspect the current state of the Cilium node.

If a ConfigReconciler needs more information then this they can now
"request" depedencies via the Hive/Cell depedency injection graph (DIG).

All ConfigReconcilers are part of the DIG and thus can declare
depedencies via their constructor's parameters.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa
bgpv1: remove NodeSpecer and ControlPlaneState references
705473d 
This commit removes all references to NodeSpecer and ControlPlaneState.

The previous commit removed them from the BGP CP.

See the previous commit for further explanation.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa
bpgv1,tests: make tests work with NodeSpecer and ControlPlaneState omit
058459f 
This commit updates the BGP-CP unit tests to work correct after the
removal of NodeSpecer and ControlPlaneState structures.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch 2 times, most recently from 32862d1 to 5f20b29 Compare August 8, 2023 01:14
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 8, 2023

/test

@rastislavs rastislavs mentioned this pull request Aug 8, 2023
Merged
@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from 5f20b29 to c98595a Compare August 8, 2023 14:17
@ldelossa ldelossa marked this pull request as ready for review August 8, 2023 14:42
@ldelossa ldelossa requested review from a team as code owners August 8, 2023 14:42
@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 8, 2023

/test

YutaroHayakawa
YutaroHayakawa approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

LGTM overall 👍 Thanks for your big effort! This makes code much simpler and easy to change.

One non-blocking suggestion. How about splitting each reconcilers into separated files? That makes the code easy to read, reduce the risk of conflict when we introduce a new reconciler, etc.

@ldelossa
Copy link
Contributor Author

ldelossa commented Aug 9, 2023

@YutaroHayakawa was thinking this as well, but we probably want to keep the code a bit similar for backporting reasons (if we need to).

We can do this split in a follow up pull-request? I do agree.

rastislavs
rastislavs approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM

@ldelossa ldelossa requested a review from joamaki August 9, 2023 14:00
@ldelossa
localnode: consider annotations in LocalNodeStore updates
551016d 
Prior to this PR only label changes made to the Kubernetes Node object
would propagate to LocalNodeStore updates.

This commit adds Annotations to this mix as well.

This is useful as the BGP-CP now observes the LocalNodeStore and
requires events to occur when annotations are updated.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/bgpv1-configreconciler-hivecell branch from c98595a to 551016d Compare August 9, 2023 16:02
@ldelossa
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor

I have a local rebase of #27100 with this PR. @joamaki @aditighag PTAL and merge when you have a moment.

@aditighag
Copy link
Member

aditighag commented Aug 10, 2023 •
edited

I don't have much context for the endpoint related changes. As this is a BGP focused change, feel free to go ahead with the merge based on the existing reviews. If there is anything specific you want me to look at in the endpoint manager change, please ping me.

@aditighag aditighag removed their request for review August 10, 2023 23:10
@ldelossa
Copy link
Contributor Author

@aditighag i think endpoint team was required due to the first and last commit, which arent exactly BGP related, rather, we now trigger LocalNodeStore updates on annotation changes. Can you review just those two commits?

@aditighag aditighag self-requested a review August 11, 2023 16:53
aditighag
aditighag requested changes Aug 14, 2023
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Thanks for highlighting the commits that are not BGP specific. I've left some review comments. Mainly, it's not clear to me how some of the changes may affect other subsystems subscribing to node events.

pkg/endpointmanager/host.go Show resolved Hide resolved
pkg/endpointmanager/host.go Show resolved Hide resolved
pkg/k8s/watchers/node.go Show resolved Hide resolved
aditighag
aditighag approved these changes Aug 15, 2023
View reviewed changes
pkg/endpointmanager/host.go Show resolved Hide resolved
pkg/endpointmanager/host.go Show resolved Hide resolved
pkg/k8s/watchers/node.go Show resolved Hide resolved
@ldelossa ldelossa merged commit 6a517a0 into cilium:main Aug 16, 2023
59 checks passed
@ldelossa ldelossa deleted the ldelossa/bgpv1-configreconciler-hivecell branch August 16, 2023 14:21
ldelossa added a commit to ldelossa/cilium that referenced this pull request Aug 16, 2023
@ldelossa
bgpv1: fix manager_test.go build error
ec62c9b 
Due to the merge of cilium#27182 before cilium#27285 a bug was introduced in
pkg/bgpv1/manager/manager_test.go.

This commit fixes the issue.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
ldelossa added a commit that referenced this pull request Aug 16, 2023
@ldelossa
bgpv1: fix manager_test.go build error
73b5e46 
Due to the merge of #27182 before #27285 a bug was introduced in
pkg/bgpv1/manager/manager_test.go.

This commit fixes the issue.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
vonzepp added a commit to nebius/cilium that referenced this pull request Feb 8, 2024
@vonzepp
Accept ipamOption.IPAMDelegatedPlugin if bgpControlPlane.enabled
fcf1d60 
required for 1.14.6 only, alternative is to backport cilium#27285
@vonzepp vonzepp mentioned this pull request Feb 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aditighag aditighag aditighag approved these changes

@nebril nebril nebril approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@rastislavs rastislavs rastislavs approved these changes

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

@joamaki joamaki Awaiting requested review from joamaki

Assignees
No one assigned
Labels
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
8 participants
@ldelossa @joestringer @danehans @aditighag @nebril @YutaroHayakawa @rastislavs @harsimran-pabla