feat: external load balancer garbage collection (part 1) - proposal

[richardcase]

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This is the proposal for the new garbage collection functionality. It was written post implementation to make sure we captured the intent of the change and to aid with the review.

Even though its written post implementation it may merge first as the gc work has been split into smaller PRs to aid review.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Relates #1718

Special notes for your reviewer:

This part 1 of 4 of changes (i.e. a stack) to implement the garbage collection. This is work to split up the original pr #3518

Checklist:

• squashed commits
• includes documentation

[k8s-ci-robot]

@richardcase: This issue is currently awaiting triage.

If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Skarlso]

tag

[Skarlso]

First initial read-through. :D

I will review the structure next, but I think this is looking good so far. :)

[pydctw]

Initial review - looks good with minor nits.

One question - I have noticed the term, a child or a tenant cluster is used. Is there a reason that these terms are used instead of a workload cluster, which is commonly used in CAPI (i.e. CAPI quickstart guide)?

[dlipovetsky]

When I looked at the original PR, I formed a different model, perhaps by mistake. At the time, I said:

The garbage collection controller always applies its finalizer, and always removes it, but it decides whether to perform garbage collection based on the annotation.
-- #3518 (comment)

This proposal describes a different mental model: The annotation controls whether the finalizer is applied.

I find this model more complex, because it allows these scenarios:

1. User creates cluster without the gc annotation.
2. Controller applies the gc finalizer.
3. User wants to disable the gc, so they add the gc annotation, its value set to false.
4. User deletes the cluster.

Observe that the gc runs during step 4, even though the user might believe that what they do in step 3 disables the gc.

1. User creates cluster with the gc annotation, its value set to false .
2. Controller does not apply the gc finalizer.
3. User wants to enable the gc, so they remove the gc annotation, or modify its value to true.
4. User deletes the cluster.

Observe that the gc does not run during step 4, even though the user might believe that what they do in step 3 enables the gc.

1. User creates cluster with the gc annotation, its value set to false.
2. Controller does not apply the gc finalizer.
3. User wants to enable the gc, so they add the gc finalizer.
4. User deletes the cluster.

Observe that the gc does run during step 4, just as the user wants, even though the annotation's value is false.

I think these scenarios violate the Principle of Least Surprise.

---

What do you think of the model I had in mind, where the controller always applies the gc finalizer, but, at the time it reconciles the cluster delete, evaluates the annotation and decides how to act on the gc finalizer, i.e. to clean up AWS resources, or to skip them.

I think another way of distinguishing the two models is this: In the proposal, the user decides whether to enable the gc when they create the cluster. In the model I describe, the user decides whether to enable the gc at any time before they delete the cluster.

[richardcase]

One question - I have noticed the term, a child or a tenant cluster is used. Is there a reason that these terms are used instead of a workload cluster, which is commonly used in CAPI (i.e. CAPI quickstart guide)?

Thanks @pydctw . No reason other than we use that term a lot.....i have changed it to workload.

[richardcase]

In the model I describe, the user decides whether to enable the gc at any time before they delete the cluster.

I agree this is a better model.

I think we could simplify the implementation further. As we aren't using a separate controller and the GC logic is being added to the existing reconciliation of the awsc & awsmcp then we don't specifically need another finalizer. If the gc feature is enabled, in the reconcileDelete of the 2 controllers we will call the gc service. The gc service will then decide at that point if it should do GC based on the existence of the annotation.

Although having a separate finalizer does allow us to remove it after doing gc so that if reconcileDelete occurs again we don't try and do gc again.

[richardcase]

@dlipovetsky - i have updated the proposal with the suggested model changes. Any chance you could take a look?

[dlipovetsky]

I think we could simplify the implementation further. As we aren't using a separate controller and the GC logic is being added to the existing reconciliation of the awsc & awsmcp then we don't specifically need another finalizer.

Agreed!

If the gc feature is enabled, in the reconcileDelete of the 2 controllers we will call the gc service. The gc service will then decide at that point if it should do GC based on the existence of the annotation.

👍

Although having a separate finalizer does allow us to remove it after doing gc so that if reconcileDelete occurs again we don't try and do gc again.

While we can save a few "discover" API calls if we skip, I would prefer to call the gc every time; it has to be idempotent, since the controller could crash at any time.

[dlipovetsky]

@dlipovetsky - i have updated the proposal with the suggested model changes. Any chance you could take a look?

Looks very good! Thank you for incorporating the feedback.

I think not using a separate finalizer would be an improvement, so I'll wait to see if you want to make that change before giving an lgtm.

[Skarlso]

That was some very good thinking @dlipovetsky. I shall endeavour to be more vigilant. :)

[richardcase]

@dlipovetsky @Skarlso - i have now removed the use of the additional finalizer based on the discussion above.

[Skarlso]

I'll re-read the entire thing in a bit with that in mind.

[Skarlso]

Seems like you have the same ci problem as this PR: #3483

[richardcase]

/retest

[richardcase]

/test pull-cluster-api-provider-aws-test

[dlipovetsky]

Thanks for a great proposal!

/lgtm

I just had one more thought: Would we ever want the user to choose which resources are garbage collected? If so, we may want to include the resource category in the annotation key. For example,

external-resource-gc.aws.cluster.x-k8s.io/elb: true
external-resource-gc.aws.cluster.x-k8s.io/ebs: false

Since this is an experimental feature, I think we can make this change later, if necessary.

[richardcase]

I just had one more thought: Would we ever want the user to choose which resources are garbage collected?

Yes i think we would and have created #3541 to look at this type customization.

[richardcase]

Thanks everyone for taking the time to review.

And special thanks to @dlipovetsky for his diligence in the review and helping to drive us to a better overall solution 🙇

/approve

And we will want all GC change in 1.5 so:

/cherry-pick release-1.5

[k8s-infra-cherrypick-robot]

@richardcase: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

In response to this:

Thanks everyone for taking the time to review.

And special thanks to @dlipovetsky for his diligence in the review and helping to drive us to a better overall solution 🙇

/approve

And we will want all GC change in 1.5 so:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: richardcase

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 [richardcase]

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

[k8s-infra-cherrypick-robot]

@richardcase: new pull request created: #3625

In response to this:

Thanks everyone for taking the time to review.

And special thanks to @dlipovetsky for his diligence in the review and helping to drive us to a better overall solution 🙇

/approve

And we will want all GC change in 1.5 so:

/cherry-pick release-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Skarlso]

Very nice work! Sorry I didn't get to re-reading it. :/