aws: delete CLBs after migration to NLB

[johngmyers]

No description provided.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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:

• OWNERS [hakman]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rifelpet]

I think the reason we didn't implement this initially was that when migrating from CLB to NLB, we can't ensure that all traffic has migrated to the NLB in regards to DNS TTLs and the newly created NLB being ready for traffic.

If the kops CLI is performing this deletion at the same time it is creating the NLB then this could be disruptive to apiserver traffic. Is that not still the case?

[johngmyers]

My testing shows that the CLB has all its targets removed once the first apply_cluster is done. So there's no point in keeping it.

If we can fix it so the existing instances stay registered, then we can uncomment the code here that delays deleting the CLB until it has no target instances.

[hakman]

Is that not still the case?

NLB is "created" first and deletions are done later. By created I mean it starts creating it and takes 2 mins. For sure there will be some downtime.
The difference is that it's been 2 years, and CLB is now marked as deprecated.
It is still manual process to switch, so operators should realise that there may be a certain downtime.

[hakman]

/hold in case there is some extra feedback

[hakman]

We could also add a WaitUntilLoadBalancerExists() or WaitUntilLoadBalancerAvailable() for the NLB, to make sure it exists (like is done for None DNS).

[johngmyers]

If the CLB has all its targets deregistered, there's no point. There's going to be an interruption whether we tear down the CLB or not.

[rifelpet]

The instances are deregistered from the CLB because the UpdateAutoscalingGroup call no longer includes the CLB. I wonder if we could avoid that by keeping CLBs in the UpdateAutoscalingGroup call if we know we'll also be deleting that CLB. I suppose that depends on whether AWS allows you to a delete a CLB still attached to an ASG vs needing to remove it from the ASG first. It may not be worth the effort to implement this

[hakman]

It may not be worth the effort to implement this

That was my reasoning. CLB is more or less deprecated by AWS and (hopefully) fewer people continue to use it. We don't force operators to switch to NLB for API, so it should be reasonable for them to schedule a maintenance window with some downtime.

[rifelpet]

and (hopefully) fewer people continue to use it

All stable kops versions use it by default for new clusters (until #14410 is released in kops 1.26) so we can assume there are many kops clusters still using CLBs. If the 1.26 release notes mention that CLBs are deprecated then it would be nice if the migration path didn't incur apiserver downtime. Though docs/cluster_spec.md does mention that the migration will involve downtime, so maybe that is acceptable.

[johngmyers]

Reducing or eliminating the downtime could be done in a subsequent PR. This PR doesn't make it worse, as it leaves a path to keeping the CLB around longer if need be.

[rifelpet]

Makes sense

/hold cancel