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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

add kube-vip as a service load balancer #432

Merged
merged 10 commits into from
Jan 29, 2024

Conversation

gereonvey
Copy link
Contributor

Proposed Changes

Thanks a lot for your work @timothystewart6, love to follow your tutorials! In the video for this project you mentioned that you didn't get kube-vip to work for services...

This PR allows to use kube-vip as loadbalancer for services, replacing MetalLB.

You can choose if you want to use MetalLB or kube-vip as the loadbalancer for services, just by changing variable service_lb_type to either metallb or kube-vip. The IP range for the service loadbalancer can be specified via the service_lb_ip_range variable.

Breaking changes for existing inventories:

  • metal_lb_ip_range has been renamed to service_lb_ip_range to reflect that the IP range is independent of the choice of loadbalancer for services

Checklist

  • Tested locally
  • Ran site.yml playbook
  • Ran reset.yml playbook
  • Did not add any unnecessary changes
  • Ran pre-commit install at least once before committing
  • 馃殌

@timothystewart6
Copy link
Contributor

timothystewart6 commented Jan 27, 2024

Thank you for this! I love the idea of adding support for using kube-vips service load balancer. Is there any way we could make this a non breaking change?

Something like

metal_lb_ip_range

kube_vip_lb_ip_range

I know that service_lb_ip_range and service_lb_type more scalable however I'd like to not break the contract / api if we really don't have to because it also breaks the documentation :)

We just did something similar in #414 (making changes backwards compatible). Let me know what you think!

Thanks!

@timothystewart6
Copy link
Contributor

Also, if you are up to it, it would be great to have a molecule test for this. It will be a lot of copy and paste but would be helpful to know if it works and if future changes break it. You can also see #414 for an example test. Single nodes is fine!

@gereonvey gereonvey marked this pull request as draft January 28, 2024 16:03
kube_vip_cloud_provider_tag_version: "main"

# kube-vip ip range for load balancer
kube_vip_lb_ip_range: "192.168.30.80-192.168.30.90"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking instead of having service_lb_type we could just check to see if kube_vip_lb_ip_range had values or metal_lb_ip_range had values and infer which one to use based on that?
e.g. in this scenario kube-vip would be used.

# metallb ip range for load balancer
#metal_lb_ip_range: "192.168.30.80-192.168.30.90"

# kube-vip ip range for load balancer
kube_vip_lb_ip_range: "192.168.30.80-192.168.30.90"

Then the default group vars could be:

# metallb ip range for load balancer
metal_lb_ip_range: "192.168.30.80-192.168.30.90"

# kube-vip ip range for load balancer
#kube_vip_lb_ip_range: "192.168.30.80-192.168.30.90"

This way someone would only need to uncomment kube-vip and comment metal lb and everything would work, we don't break the contract, and we don't need a type. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the configuration according to your suggestion

@gereonvey
Copy link
Contributor Author

I added a molecule scenario for kube-vip, based on single-node (similar to the calico scenario). Unfortunately I can't test this, it seems I can't get Vagrant to work on my laptop.

@gereonvey gereonvey marked this pull request as ready for review January 28, 2024 20:21
@timothystewart6
Copy link
Contributor

timothystewart6 commented Jan 28, 2024

Thank you! I just approved it for CI, so it will now run remote. I should write up a guide at some point for configuring molecule tests locally. Thank you for the test too!

@timothystewart6
Copy link
Contributor

Looks like lint. You can fix it by installing pre-commit and it will fix it for you.

@gereonvey
Copy link
Contributor Author

Looks like lint. You can fix it by installing pre-commit and it will fix it for you.

Sorry for that, forgot to run it before commit... installed it now.
Also I noticed that the new molecule scenario needs to be listed in the test workflow.

@timothystewart6
Copy link
Contributor

timothystewart6 commented Jan 28, 2024

The tests are passing but this is because metallb is still getting installed.

https://github.com/techno-tim/k3s-ansible/actions/runs/7688568449/job/20950037294#step:13:222

I think you might need to add an override for metallb to unset the metallb IP range since it's in the default all group vars

# Make sure that our IP ranges do not collide with those of the other scenarios
apiserver_endpoint: "192.168.30.225"
# Use kube-vip instead of MetalLB
kube_vip_lb_ip_range: "192.168.30.110-192.168.30.119"
Copy link
Contributor

Choose a reason for hiding this comment

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

may need to add metal_lb_ip_range: "" to unset this since the default is set.

@gereonvey
Copy link
Contributor Author

The tests are passing but this is because metallb is still getting installed.

What makes you believe that metallb is getting installed? The task is being skipped.

TASK [k3s_server : Deploy metallb manifest] ************************************
  skipping: [control1] => {"changed": false, "false_condition": "kube_vip_lb_ip_range is not defined", "skip_reason": "Conditional result was False"}

In fact, if you scroll further you see how kube-vip is being deployed.

https://github.com/techno-tim/k3s-ansible/actions/runs/7688568449/job/20950037294#step:13:222

I think you might need to add an override for metallb to unset the metallb IP range since it's in the default all group vars

No, the whole logic is relying on kube_vip_lb_ip_range being defined or not.

@timothystewart6
Copy link
Contributor

Thank you! You're right! I am not sure what I was looking at. Thank you for this!

@timothystewart6 timothystewart6 merged commit bcd37a6 into techno-tim:master Jan 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants