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.

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

fix: update peers on node update #1723

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sebltm
Copy link

@sebltm sebltm commented Aug 22, 2024

Fixes #676 by reloading the peer configuration when nodes are updated

@aauren
Copy link
Collaborator

aauren commented Aug 24, 2024

Thanks @sebltm!

I won’t be able to test this out in my setup for a week or so do to IRL complications, but I’ll give it a go as soon as I’m able to.

FWIW on the surface this seems super simple and straight forward, if it’s really this simple I’ll be face-palming that we didn’t do this earlier 🙂

@aauren
Copy link
Collaborator

aauren commented Sep 22, 2024

After reviewing your change and putting it through its paces a bit, I think that this is definitely a good start, but that there is more to the story here.

From what I can tell, putting in the nrc.OnNodeUpdate(newObj) will update remote nodes, but it would not work cohesively as that won't cause the node to reload its own annotations and settings. There are a bunch of settings for the node itself that are only considered either when the Routes controller is being created (https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller.go#L1424-L1682) or when it is being started (https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/network_routes_controller.go#L1074-L1371)

Even in the functions that nrc.OnNodeUpdate() calls, there are a few bits of logic that specifically exclude the node that it is running on from being considered: https://github.com/cloudnativelabs/kube-router/blob/master/pkg/controllers/routing/bgp_peers.go#L56

I think that we're going to need a more holistic overhaul of the controller before #676 is resolved.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node Annotations are Only Evaluated on Startup and not Actively Watched During Runtime
2 participants