giantswarm / roadmap

Giant Swarm Product Roadmap
https://github.com/orgs/giantswarm/projects/273
Apache License 2.0
3 stars 0 forks source link

Change kube-vip LB IP pool logic #3637

Closed vxav closed 1 week ago

vxav commented 1 month ago

Current setup

I don't get why we do that. It means we have a 10-IPs IPAM pool in the MC but the WC as a 1-IP pool 🤷. So the first load balancer will work fine (most often nginx) while subsequent LBs will be stuck on pending.

Proposition Collect the actual IP pool from the LB IP pool in the MC.

> kg globalinclusterippools.ipam.cluster.x-k8s.io cassino-svc-lb-ips -ojsonpath='{.spec.addresses[0]}'
10.109.147.221-10.109.147.230

And patch the Helmrelease with a range-global instead.

EDIT: Actually now I remember. We use an IPaddressclaim to block the IP on the MC side. If we set a range in the WC it could (potentially) be set in multiple WCs. Ideally we would need something like ipaddressrangeclaim or create a claim for each IP. Somehow. In the meantime, the solution Could be:

vxav commented 1 month ago

Untested : https://github.com/giantswarm/cluster-vsphere/pull/266

anvddriesch commented 2 weeks ago

I made some small changes to the PR and now several ips can be assigned.

anvddriesch commented 2 weeks ago

the standard test was green when I ran it last week. However, as of now all providers fail and the reason is related to some already-exists issue with cert exporter configmaps in the default-apps. This leads me to believe that its related to the default apps changes still being made.

vxav commented 2 weeks ago

Thanks a lot! I'll test this manually as the E2E tests won't be testing what we're changing here

anvddriesch commented 2 weeks ago

I created some random lb services on a test cluster and we see the expected behavior. Three single ip cidrs are added to the helm relesase for lb ip space. Creating four lb services in a row resulted in these three ips being assigned to the first three and then the fourth one not getting an assignment.

vxav commented 2 weeks ago

Ok that's good. I think for now we can stick with this being an immutable field and keep 3 as default like you suggested here as it would be more work than worth it to make it a dynamic value. wdyt?

anvddriesch commented 1 week ago

changes were merged