-
Notifications
You must be signed in to change notification settings - Fork 5
Floating IP support for NodeDNSRecordSet #44
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
Floating IP support for NodeDNSRecordSet #44
Conversation
jcodybaker
left a comment
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.
This looks great! I'd swear you've been slinging Go for a long while.
| "node": nodeName, | ||
| }) | ||
|
|
||
| node, err := c.kubeCS.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{}) |
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.
This is a small nit--only a matter of efficiency and pattern. Feel free to leave this as is if changing it turns into a mess.
It'd be ideal to pull this from the nodeinformer in the node match controller. That will pull the latest node resource from our local memory, rather than fetching from the Kubernetes API. I made a similar call in my (closed) first attempt at this which might be relevant. In that case I was getting all nodes, but there is also a Get() option I believe
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.
Thanks for the review and feedback. I agree that we should get the data from cache where we can. It wasn't very straight forward as the node match controllers are properties of FloatingIPPools and the floating ip controller needs some way to find the node in the various node match controller instances.
I took the approach to just iterate through all the node match controllers. I assume that most deployments wont have many FloatingIPPools so this iteration isn't a big deal. I felt this was the more straight forward approach.
An alternative is to keep a NodeNameToPool mapping in the floating ip controller. This would involve passing in a NodeNameToPoolUpdater function when creating the FloatingIPPool and node match controller. I didn't feel like that was as straight forward, and ended up with having to worry about ensuring the mapping was always up to date. Not that its particularly hard, but it was more state to manage, where the other approach keeps all the state in the node match controller.
Happy to switch to use the mapping approach if you think it's best.
|
Hey Joe, LGTM. I think you should be good to merge provide you've tried it and it works as expected. If you're not able to merge, tag me on Slack and I'll merge it. |
This PR adds to support to NodeDNSRecordSet allowing it to use the assigned Reserved IP Addresses in the DNS record it managed. This was done with the main changes:
There was also a small optimization to the ip controller's handling of node updates. Now when the FloatingIPPool is configured with a dnsRecordSet a DNS update is not made if the assignment of the Reserved IP Address does not change. Prior to this change any change in match status to a node triggered a DNS update regardless if the node was assigned a Reserved IP Address. So for example if I had 3 matching nodes, but only 2 IP addresses, and the node that did not have an Reserved IP Address was cordoned a DNS update would have been even though the IP addresses in the DNS record would not have changed.
The following scenarios were tested in validated in a live environment:
FloatingIPPool
NodeDNSRecordSet