feat: external load balancer garbage collection (part 3) - add gc to reconciliation #3633

richardcase · 2022-07-29T11:43:37Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This change allows the new garbage collection service to be used. Its enabled via a new feature flag ExternalResourceGC which is disabled by default. If the feature flag is enabled then the gc service is called in reconcileDelete of the controllers for AWSCluster and AWSManagedControlPlane. The actual gc service does the work of clean-up.

New commands have been added to clusterawsadm to allow users to opt-in/out an already existing cluster from garbage collection. This can also be accomplished adding the annotation manually.

Additionally, with the new mocks folder introduced with the gc service the existing mocks have been deleted and tests/controllers updated.

NOTE: the e2e tests will be in part 4

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 3 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

This change uses the new garbage collection service enables this during the reconciliation of `AWSCluster` and `AWSManagedControlPlane`. Its enabled via a new feature flag `ExternalResourceGC` which is disabled by default. If the feature flag is enabled then the the gc service is called in `reconcileDelete` for the infra clusters. The actual gc service does the work of cleanup. New commands have been added to `clusterawsadm` to allow users to opt-in/out an already existing cluster from garbage collection. Additionally, with the new mocks folder introduced with the gc service the existing mocks have been deleted and tests/controllers updated. Signed-off-by: Richard Case <[email protected]>

k8s-ci-robot · 2022-07-29T11:43:45Z

@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.

richardcase · 2022-07-29T11:45:05Z

We want this in 1.5 so:

/cherry-pick release-1.5

k8s-infra-cherrypick-robot · 2022-07-29T11:45:06Z

@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:

We want this 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 · 2022-07-29T12:00:59Z

Love the -33000 :D

Skarlso

A couple of nits and remarks and two outstanding questions around the documentation. Overall, I have just one question about the patch helper. I assume it handles out-of-sync resources like it just throws an error if the resource happened to be at a different revision. So it Gets the resource first I assume?

cmd/clusterawsadm/cmd/gc/disable.go

cmd/clusterawsadm/cmd/gc/enable.go

cmd/clusterawsadm/cmd/gc/disable.go

docs/book/src/topics/external-resource-gc.md

cmd/clusterawsadm/cmd/gc/disable.go

cmd/clusterawsadm/gc/gc_test.go

feature/feature.go

dlipovetsky · 2022-08-02T23:33:13Z

main.go

+	externalResourceGC := false
+	if feature.Gates.Enabled(feature.ExternalResourceGC) {
+		setupLog.Info("enabling external resource garbage collection")
+		externalResourceGC = true
+	}
+


I think this belongs in the enableGates function, but I understand why it does not, at the moment: We need do this before we initialize AWSClusterReconciler, which we do before we call enableGates.

cmd/clusterawsadm/gc/disable.go

Various changes as a result of review feedback. Signed-off-by: Richard Case <[email protected]>

k8s-ci-robot · 2022-08-03T11:05:31Z

@richardcase: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-aws-apidiff-main	`7ade599`	link	false	`/test pull-cluster-api-provider-aws-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

richardcase · 2022-08-03T12:42:15Z

We can ignore the apidiff changes.

richardcase · 2022-08-03T12:42:25Z

/test ?

k8s-ci-robot · 2022-08-03T12:42:26Z

@richardcase: The following commands are available to trigger required jobs:

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

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-aws-apidiff-main
/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-blocking
/test pull-cluster-api-provider-aws-e2e-clusterclass
/test pull-cluster-api-provider-aws-e2e-conformance
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
/test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-aws-apidiff-main
pull-cluster-api-provider-aws-build
pull-cluster-api-provider-aws-test
pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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.

richardcase · 2022-08-03T12:42:45Z

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

Skarlso · 2022-08-03T14:37:27Z

/lgtm

richardcase · 2022-08-03T14:37:31Z

The e2e have passed:

richardcase · 2022-08-03T14:46:05Z

@pydctw @dlipovetsky - i will wait for another lgtm before merging this.

pydctw · 2022-08-03T15:27:58Z

/lgtm

richardcase · 2022-08-03T16:23:39Z

/approve

k8s-ci-robot · 2022-08-03T16:23:53Z

[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 · 2022-08-03T16:26:40Z

@richardcase: new pull request created: #3646

In response to this:

We want this 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 added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 29, 2022

k8s-ci-robot requested review from dthorsen and shivi28 July 29, 2022 11:43

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 29, 2022

Skarlso reviewed Aug 2, 2022

View reviewed changes

pydctw reviewed Aug 2, 2022

View reviewed changes

cmd/clusterawsadm/cmd/gc/disable.go Outdated Show resolved Hide resolved

cmd/clusterawsadm/gc/gc_test.go Outdated Show resolved Hide resolved

feature/feature.go Outdated Show resolved Hide resolved

dlipovetsky reviewed Aug 2, 2022

View reviewed changes

cmd/clusterawsadm/gc/disable.go Outdated Show resolved Hide resolved

refactor: changes from review

7ade599

Various changes as a result of review feedback. Signed-off-by: Richard Case <[email protected]>

k8s-ci-robot assigned Skarlso Aug 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2022

k8s-ci-robot assigned pydctw Aug 3, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2022

k8s-ci-robot merged commit a87fc10 into kubernetes-sigs:main Aug 3, 2022

k8s-ci-robot added this to the v1.x milestone Aug 3, 2022

k8s-infra-cherrypick-robot mentioned this pull request Aug 3, 2022

[release-1.5] feat: external load balancer garbage collection (part 3) - add gc to reconciliation #3646

Merged

dlipovetsky mentioned this pull request Aug 4, 2022

refactor: Group setup by feature flag #3647

Merged

4 tasks

feat: external load balancer garbage collection (part 3) - add gc to reconciliation #3633

feat: external load balancer garbage collection (part 3) - add gc to reconciliation #3633

Uh oh!

Conversation

richardcase commented Jul 29, 2022

Uh oh!

k8s-ci-robot commented Jul 29, 2022

Uh oh!

richardcase commented Jul 29, 2022

Uh oh!

k8s-infra-cherrypick-robot commented Jul 29, 2022

Uh oh!

Skarlso commented Jul 29, 2022

Uh oh!

Skarlso left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dlipovetsky Aug 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

k8s-ci-robot commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

Skarlso commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

pydctw commented Aug 3, 2022

Uh oh!

richardcase commented Aug 3, 2022

Uh oh!

k8s-ci-robot commented Aug 3, 2022

Uh oh!

k8s-infra-cherrypick-robot commented Aug 3, 2022

Uh oh!

Uh oh!