-
Notifications
You must be signed in to change notification settings - Fork 20
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] [improvement] move apiserver loadbalancing logic from linodemachine controller to linodecluster controller #457
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #457 +/- ##
==========================================
- Coverage 65.33% 64.53% -0.81%
==========================================
Files 80 81 +1
Lines 4192 4196 +4
==========================================
- Hits 2739 2708 -31
- Misses 1243 1278 +35
Partials 210 210 ☔ View full report in Codecov by Sentry. |
Changes look good to me, can you rebase off main and squash @amold1 ? 🙏 |
cloud/services/loadbalancers.go
Outdated
portsToBeAdded = append(portsToBeAdded, map[string]int{"configID": *portConfig.NodeBalancerConfigID, "port": portConfig.Port}) | ||
} | ||
|
||
logger.Info("abir", "portsToBeAdded", portsToBeAdded) |
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.
whoops, missed leftover debugging
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.
oops... removed!
rebased and checks running now - I'll "squash and merge" on approval |
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.
Working for me on latest changes 👍
What this PR does / why we need it:
For APIServer LoadBalancing, we are currently creating NB or DNS entries from the LinodeMachine controller causing inconsistencies during the creation/deletion process leading to either failed attempts to create a cluster or some DNS Entries being left behind. We had also turned off concurrency to mitigate these issues but that hasn't proven to be a good solution.
This PR now moves all of the logic to the LinodeCluster controller allowing it to accurately access what DNS entries or NB backends need to be added or deleted.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs: