-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fixing the nodebalancer config rebuilds to include ids of preexisting nodebalancer nodes #192
Fixing the nodebalancer config rebuilds to include ids of preexisting nodebalancer nodes #192
Conversation
To succeed all the CI steps I had to drop the support for 1.20 which is unable to build it after bumping k8s deps and tidying the modules. Not really sure what we should do when the node_controller or service_controller would not be able to register the informers, for now I'm just logging the error. See the implementation of the change which introduced these errors: kubernetes/client-go@ecdc8bf |
… nodebalancer nodes to avoid rebuilds.
e5f931c
to
50ecce4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments here or there.
One more important point from is that I'd ideally like to see this reflected in (unit) tests - what was the behaviour like before this change and what thing we're trying to achieve here. Just to avoid regressions.
Can we have the CI tests pass for this PR? Also, not sure if we have tested building the image and running it on a k8s cluster. I am seeing pods crashlooping with this branch and hence checking. Looks like complaining due to the change in main.go
|
Is it still the case? I believe it was some glitch during rebasing, now the changes in main.go are those which are really needed to bump linodego up and subsequently k8s deps. |
Thanks @avestuk for simplification. Co-authored-by: Alex Vest <avestuk@gmail.com>
Co-authored-by: Alex Vest <avestuk@gmail.com>
… copies which existed due to _test.go in name, that prevents the code to be imported
E2E tests passed.
Had to do a manifest change
|
Can we also add the following diff to this PR so that helm installs work fine after the change?
|
Also, once I switch to leases instead of endpoints, I see following error on all CCM pods:
One can test helm install working fine or not by following these steps.
|
Not sure why its not happening in the cluster you are testing. Can we add these changes as well, this seems to fix my issue:
|
…leader election since the new k8s api introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Currently nodebalancer configs are rebuilt when nodes change, but that causes nodebalancer to reload and that can have some impact on performance. For nodes which didn't have changed this is not necessary, but we are not sending node ids. This PR bumps linodego to 1.30 (to allow sending these ids) and before rebuilding the the nodebalancer config it does API request to obtain ids of current nodes.
Here is a recorded POST request after this change. Node
192.168.242.85:31059
was added while the others were kept form the original config, so they have ids set.General:
Pull Request Guidelines: