-
Notifications
You must be signed in to change notification settings - Fork 637
✨Create multiple control plane loadbalancers concurrently #5569
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
Conversation
Correct. We're phasing out support of classic load balancers as much as possible.
This was definitely possible a few releases ago, but since Go dropped older ciphers that classic ELBs supported, we've since defaulted to NLBs. I added the support for a second LB, and supporting classic there was never the intention. So I think this test coverage is sufficient, and the change is a pretty easy speed improvement. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: cfffb0adac6701c3d65da975687cef7d94153bd2
|
|
This is still running after 33 hours! Wonder if we can kick it somehow. /test pull-cluster-api-provider-aws-build-docker |
|
/test pull-cluster-api-provider-aws-build-docker |
What type of PR is this?
/kind feature
What this PR does / why we need it:
LoadBalancers take many minutes to become available after creation. We currently wait synchronously for a loadbalancer to become available immediately after creation. This increases total cluster installation time when creating multiple control plane loadbalancers as we don't create a loadbalancer until the previous one is fully initialised. It would be more time-efficient to create them all first, then wait for them to become available.
This PR splits loadbalancer creation into 2 phases:
We ensure that we do the waiting in the reconcile phase after all getOrCreates have executes.
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 #5568
Special notes for your reviewer:
This PR doesn't introduce any parallel execution in CAPA. It simply changes the order of operations so that loadbalancer initialisations can proceed concurrently in AWS.
Apart from a minor refactor to enable re-use, the existing
ensure two load balancers are reconciledtest remains unchanged and continues to pass.We add a new test,
ensure two load balancers are created concurrently. This is also a general creation test, which did not previously exist. Concurrency is asserted by asserting that bothWaitUntilLoadBalancerAvailableWithContextcalls happen after bothCreateLoadBalancercalls.We don't add a test for classic load balancers, as support for these already seems to be problematic. Note that 2 classic loadbalancers cannot work because
getOrCreateClassicLoadBalancer(formerlyreconcileClassicLoadBalancer) does not take an lbSpec argument, therefore it hardcodes the primary load balancer. You could presumably mix a classic primary and v2 secondary LB, but I did not want to add coverage for an esoteric test case.I believe I also spotted a latent bug in classic load balancer reconciliation where it will always reconcile a HealthCheck even if it is already set. I added a comment about this but did not attempt to fix it because it is unrelated.
Checklist:
Release note: