Add validation for GC tasks annotation #4485

Fedosin · 2023-09-04T15:06:33Z

What type of PR is this?
/kind support

What this PR does / why we need it:
This PR adds validation for the GC config annotation.

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 #

Special notes for your reviewer:
As a part of this work 2 constants ExternalResourceGCAnnotation and ExternalResourceGCTasksAnnotation were moved from exp/api/v1beta2 to api/v1beta2.
Checklist:

squashed commits
includes documentation
adds unit tests
adds or updates e2e tests

Release note:

None

api/v1beta2/awscluster_webhook.go

Ankitasw · 2023-09-05T05:48:19Z

/test ?

k8s-ci-robot · 2023-09-05T05:48:20Z

@Ankitasw: 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
/test pull-cluster-api-provider-aws-e2e-eks-gc
/test pull-cluster-api-provider-aws-e2e-eks-testing

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.

Ankitasw · 2023-09-05T05:48:31Z

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

Ankitasw

Also, I am not aware but does this require any changes in E2E GC tests as well? cc @richardcase

Ankitasw · 2023-09-05T07:21:21Z

@richardcase since this is API change, shall we include this as part of v1beta3 changes?

Fedosin · 2023-09-05T08:10:44Z

I think we can avoid introducing a breaking change if we move ExternalResourceGCTasksAnnotation constant only. A new version (2.3) hasn't been released yet, so we can modify that part safely.

cmd/clusterawsadm/gc/gc.go

Ankitasw · 2023-09-05T10:18:47Z

exp/api/v1beta2/types.go

@@ -25,11 +25,11 @@ import (
 const (
 	// ExternalResourceGCAnnotation is the name of an annotation that indicates if
 	// external resources should be garbage collected for the cluster.
-	ExternalResourceGCAnnotation = "aws.cluster.x-k8s.io/external-resource-gc"
+	ExternalResourceGCAnnotation = infrav1.ExternalResourceGCAnnotation


I think this would be duplication, either we keep these constants in exp/api/v1beta2, or in api/v1beta2

Now these constants are in api/v1beta2, but I kept them as duplicates in exp/api/v1beta2 to prevent a breaking change in the API. People who used the constants from exp can continue to do so, but now the correct location is api/v1beta2.

We might need more opinions here
cc @richardcase @Skarlso @dlipovetsky

It's OK not to duplicate it. If we remove it, make sure this gets into a minor release, though, because it might be a breaking change. Although, honestly, I doubt too many people call this from anywhere outside of CAPA.

alright! I'll remove it from exp completely since we don't consider it as a breaking change. for sure, it will make things simpler.

dlipovetsky · 2023-09-05T16:48:11Z

This is a follow-up to #4466

To prevent a breaking change we keep the constants in exp/api/v1beta2, and they will have the same values as ones in api/v1beta2

api/v1beta2/awscluster_webhook.go

pkg/cloud/services/gc/cleanup.go

Fedosin · 2023-09-07T08:57:40Z

I moved this validation code to a separate function.

Skarlso · 2023-09-07T12:33:30Z

/lgtm

@Ankitasw final word :)

Ankitasw · 2023-09-08T07:27:31Z

/approve

k8s-ci-robot · 2023-09-08T07:28:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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~~ [Ankitasw]

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

Ankitasw · 2023-09-12T15:21:28Z

/cherry-pick release-2.2

k8s-infra-cherrypick-robot · 2023-09-12T15:22:03Z

@Ankitasw: #4485 failed to apply on top of branch "release-2.2":

Applying: Move GC annotation constants from exp/api/v1beta2 to api/v1beta2
Using index info to reconstruct a base tree...
M	exp/api/v1beta2/types.go
M	pkg/cloud/services/gc/cleanup.go
M	pkg/cloud/services/gc/cleanup_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/services/gc/cleanup_test.go
CONFLICT (content): Merge conflict in pkg/cloud/services/gc/cleanup_test.go
Auto-merging pkg/cloud/services/gc/cleanup.go
CONFLICT (content): Merge conflict in pkg/cloud/services/gc/cleanup.go
Auto-merging exp/api/v1beta2/types.go
CONFLICT (content): Merge conflict in exp/api/v1beta2/types.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Move GC annotation constants from exp/api/v1beta2 to api/v1beta2
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-2.2

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 requested review from richardcase and Skarlso September 4, 2023 15:06

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2023

Fedosin changed the title ~~Gc config validation~~ Add validation for GC tasks annotation Sep 4, 2023

Ankitasw reviewed Sep 5, 2023

View reviewed changes

api/v1beta2/awscluster_webhook.go Outdated Show resolved Hide resolved

Ankitasw reviewed Sep 5, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

Fedosin force-pushed the gc_config_validation branch from 2ea9f5b to cf90cec Compare September 5, 2023 08:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2023

Ankitasw added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. and removed kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Sep 5, 2023

Ankitasw reviewed Sep 5, 2023

View reviewed changes

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

Fedosin force-pushed the gc_config_validation branch from cf90cec to 64ee831 Compare September 5, 2023 09:10

Ankitasw reviewed Sep 5, 2023

View reviewed changes

Move GC annotation constants from exp/api/v1beta2 to api/v1beta2

7dc146b

To prevent a breaking change we keep the constants in exp/api/v1beta2, and they will have the same values as ones in api/v1beta2

Fedosin force-pushed the gc_config_validation branch from 64ee831 to 22d1ea4 Compare September 6, 2023 14:18

Skarlso reviewed Sep 7, 2023

View reviewed changes

api/v1beta2/awscluster_webhook.go Outdated Show resolved Hide resolved

Skarlso reviewed Sep 7, 2023

View reviewed changes

pkg/cloud/services/gc/cleanup.go Show resolved Hide resolved

Add validation for GC tasks annotation

fe3457d

Fedosin force-pushed the gc_config_validation branch from 22d1ea4 to fe3457d Compare September 7, 2023 08:57

k8s-ci-robot assigned Skarlso Sep 7, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2023

Fedosin mentioned this pull request Sep 7, 2023

Graduate the garbage Collection feature #4397

Open

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2023

k8s-ci-robot merged commit 15ae5f1 into kubernetes-sigs:main Sep 8, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for GC tasks annotation #4485

Add validation for GC tasks annotation #4485

Fedosin commented Sep 4, 2023 •

edited

Loading

Ankitasw commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

Ankitasw commented Sep 5, 2023

Ankitasw left a comment •

edited

Loading

Ankitasw commented Sep 5, 2023

Fedosin commented Sep 5, 2023

Ankitasw Sep 5, 2023

Fedosin Sep 5, 2023

Ankitasw Sep 6, 2023 •

edited

Loading

Skarlso Sep 6, 2023

Fedosin Sep 6, 2023

Skarlso Sep 6, 2023

dlipovetsky commented Sep 5, 2023

Fedosin commented Sep 7, 2023

Skarlso commented Sep 7, 2023

Ankitasw commented Sep 8, 2023

k8s-ci-robot commented Sep 8, 2023

Ankitasw commented Sep 12, 2023

k8s-infra-cherrypick-robot commented Sep 12, 2023

Add validation for GC tasks annotation #4485

Add validation for GC tasks annotation #4485

Conversation

Fedosin commented Sep 4, 2023 • edited Loading

Ankitasw commented Sep 5, 2023

k8s-ci-robot commented Sep 5, 2023

Ankitasw commented Sep 5, 2023

Ankitasw left a comment • edited Loading

Choose a reason for hiding this comment

Ankitasw commented Sep 5, 2023

Fedosin commented Sep 5, 2023

Ankitasw Sep 5, 2023

Choose a reason for hiding this comment

Fedosin Sep 5, 2023

Choose a reason for hiding this comment

Ankitasw Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Skarlso Sep 6, 2023

Choose a reason for hiding this comment

Fedosin Sep 6, 2023

Choose a reason for hiding this comment

Skarlso Sep 6, 2023

Choose a reason for hiding this comment

dlipovetsky commented Sep 5, 2023

Fedosin commented Sep 7, 2023

Skarlso commented Sep 7, 2023

Ankitasw commented Sep 8, 2023

k8s-ci-robot commented Sep 8, 2023

Ankitasw commented Sep 12, 2023

k8s-infra-cherrypick-robot commented Sep 12, 2023

Fedosin commented Sep 4, 2023 •

edited

Loading

Ankitasw left a comment •

edited

Loading

Ankitasw Sep 6, 2023 •

edited

Loading