Update LoadBalancerReadyCondition on deletion #3871

mnitchev · 2022-11-22T16:24:46Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The LoadBalancerReadyCondition is sometimes explicitly patched and other times it only updates the awsCluster object. Furthermore the condition won't be patched by the patch helper in the AWSClusterReconciler, because it's not in the patch.WithOwnedConditions list. This change puts the condition in the list and also updates the condition reason to Deleted when the LB is not found. Without setting the condition in that case, the reconciler will set condition to Deleting and it will never go back to Deleted.

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 #3870

Special notes for your reviewer:

Checklist:

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

CC @fiunchinho

The LoadBalancerReadyCondition is sometimes explicitly patched and other times it only updates the awsCluster object. Furthermore the condition won't be patched by the patch helper in the AWSClusterReconciler, because it's not in the `patch.WithOwnedConditions` list. This change puts the condition in the list and also updates the condition reason to `Deleted` when the LB is not found. Without setting the condition in that case, the reconciler will set condition to `Deleting` and it will never go back to `Deleted`. Co-authored-by: Jose Armesto <github@armesto.net>

linux-foundation-easycla · 2022-11-22T16:24:53Z

The committers listed above are authorized under a signed CLA.

✅ login: mnitchev / name: Mario Nitchev (430412c)

k8s-ci-robot · 2022-11-22T16:24:54Z

Welcome @mnitchev!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-11-22T16:24:55Z

Hi @mnitchev. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nprokopic · 2022-11-22T18:34:27Z

/ok-to-test

Ankitasw · 2022-11-24T13:46:28Z

/hold
until this discussion is concluded

AverageMarcus · 2022-12-13T10:14:00Z

@Ankitasw what needs to be concluded with the discussion on Slack?

Is there anything that's blocking this PR from moving ahead?

We've been running with these changes included for a couple weeks now (based on the 1.5 branch) and it's been working as expected.

Skarlso · 2022-12-14T08:31:50Z

/lgtm

Final approve left for @richardcase :)

Ankitasw · 2022-12-14T08:45:32Z

controllers/awscluster_controller.go

@@ -171,6 +171,7 @@ func (r *AWSClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 			patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{
 				infrav1.PrincipalCredentialRetrievedCondition,
 				infrav1.PrincipalUsageAllowedCondition,
+				infrav1.LoadBalancerReadyCondition,


Is this change really needed?
Does setting LoadBalancerReadyCondition as done in loadbalancer.go would not suffice?

Just asking this as there are many other conditions which are patched fine without this change, hence not added here.

Our observation was that it didn't work without having the condition in that list. My understanding is that the patch helper will ignore conditions not owned by the controller. At least that's the impression I'm left with by the documentation here.

I'll double check, since it's been a while since I submitted this change.

cc @richardcase if you have any opinions

richardcase · 2023-01-09T12:24:20Z

/test ?

k8s-ci-robot · 2023-01-09T12:24:23Z

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

richardcase · 2023-01-09T12:24:56Z

This change seems reasonable to me. Lets check the e2e pass and then merge:

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

richardcase · 2023-01-09T12:25:10Z

Until the e2e pass:
/hold

richardcase · 2023-01-09T12:26:02Z

/approve

@mnitchev @AverageMarcus - feel free to unhold when the e2e passes.

k8s-ci-robot · 2023-01-09T12:26:12Z

[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

richardcase · 2023-01-09T12:27:52Z

/cherrypick release-1.5

k8s-infra-cherrypick-robot · 2023-01-09T12:27:54Z

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

/cherrypick 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.

AverageMarcus · 2023-01-09T13:53:15Z

E2E passed

/hold cancel

k8s-infra-cherrypick-robot · 2023-01-09T14:02:23Z

@richardcase: #3871 failed to apply on top of branch "release-1.5":

Applying: Update LoadBalancerReadyCondition on deletion
Using index info to reconstruct a base tree...
M	controllers/awscluster_controller.go
M	pkg/cloud/services/elb/loadbalancer.go
M	pkg/cloud/services/elb/loadbalancer_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloud/services/elb/loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/cloud/services/elb/loadbalancer_test.go
Auto-merging pkg/cloud/services/elb/loadbalancer.go
CONFLICT (content): Merge conflict in pkg/cloud/services/elb/loadbalancer.go
Auto-merging controllers/awscluster_controller.go
CONFLICT (content): Merge conflict in controllers/awscluster_controller.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 Update LoadBalancerReadyCondition on deletion
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:

/cherrypick 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.

Apply #3871 "Update LoadBalancerReadyCondition on deletion" to release-1.5

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 22, 2022

k8s-ci-robot added the needs-priority label Nov 22, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 22, 2022

k8s-ci-robot requested review from richardcase and Skarlso November 22, 2022 16:25

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 22, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 22, 2022

Make gci linter happy

cd7273f

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2022

AndiDog mentioned this pull request Dec 12, 2022

New clusters are stuck in deleting on grizzly giantswarm/roadmap#1719

Closed

k8s-ci-robot assigned Skarlso Dec 14, 2022

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

Ankitasw reviewed Dec 14, 2022

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2023

k8s-ci-robot merged commit c0bd2df into kubernetes-sigs:main Jan 9, 2023

mnitchev mentioned this pull request Jan 9, 2023

Apply #3871 "Update LoadBalancerReadyCondition on deletion" to release-1.5 #3967

Merged

4 tasks

k8s-ci-robot added a commit that referenced this pull request Jan 19, 2023

Merge pull request #3967 from giantswarm/set-lb-conditions

5d8786d

Apply #3871 "Update LoadBalancerReadyCondition on deletion" to release-1.5

This was referenced Jan 24, 2023

Upgrade CAPA to v2.x including v1beta2 CRDs giantswarm/roadmap#1832

Closed

Merge latest upstream changes giantswarm/cluster-api-provider-aws#411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update LoadBalancerReadyCondition on deletion #3871

Update LoadBalancerReadyCondition on deletion #3871

mnitchev commented Nov 22, 2022

linux-foundation-easycla bot commented Nov 22, 2022 •

edited

Loading

k8s-ci-robot commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

nprokopic commented Nov 22, 2022

Ankitasw commented Nov 24, 2022

AverageMarcus commented Dec 13, 2022

Skarlso commented Dec 14, 2022

Ankitasw Dec 14, 2022

Ankitasw Dec 14, 2022

mnitchev Dec 15, 2022

Ankitasw Dec 15, 2022

richardcase commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

richardcase commented Jan 9, 2023

richardcase commented Jan 9, 2023

richardcase commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

richardcase commented Jan 9, 2023

k8s-infra-cherrypick-robot commented Jan 9, 2023

AverageMarcus commented Jan 9, 2023

k8s-infra-cherrypick-robot commented Jan 9, 2023

Update LoadBalancerReadyCondition on deletion #3871

Update LoadBalancerReadyCondition on deletion #3871

Conversation

mnitchev commented Nov 22, 2022

linux-foundation-easycla bot commented Nov 22, 2022 • edited Loading

k8s-ci-robot commented Nov 22, 2022

k8s-ci-robot commented Nov 22, 2022

nprokopic commented Nov 22, 2022

Ankitasw commented Nov 24, 2022

AverageMarcus commented Dec 13, 2022

Skarlso commented Dec 14, 2022

Ankitasw Dec 14, 2022

Choose a reason for hiding this comment

Ankitasw Dec 14, 2022

Choose a reason for hiding this comment

mnitchev Dec 15, 2022

Choose a reason for hiding this comment

Ankitasw Dec 15, 2022

Choose a reason for hiding this comment

richardcase commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

richardcase commented Jan 9, 2023

richardcase commented Jan 9, 2023

richardcase commented Jan 9, 2023

k8s-ci-robot commented Jan 9, 2023

richardcase commented Jan 9, 2023

k8s-infra-cherrypick-robot commented Jan 9, 2023

AverageMarcus commented Jan 9, 2023

k8s-infra-cherrypick-robot commented Jan 9, 2023

linux-foundation-easycla bot commented Nov 22, 2022 •

edited

Loading