fix: Remove Finalizer from resource if create returns error #185
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm 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:
Approvers can indicate their approval by writing |
|
/test ec2-controller-test |
pkg/runtime/reconciler.go
Outdated
| @@ -638,6 +638,9 @@ func (r *resourceReconciler) createResource( | |||
| latest, err = rm.Create(ctx, desired) | |||
| rlog.Exit("rm.Create", err) | |||
| if err != nil { | |||
| if _, ok := ackerr.AWSError(err); ok { | |||
| r.setResourceUnmanaged(ctx, rm, desired) | |||
There was a problem hiding this comment.
do you want to handle the error here?
There was a problem hiding this comment.
You're right! addressed
The reason marking a resource as managed (putting the finalizer) before attempting a create is a general practice in kubernetes. The main reason we do it is to protect against deletion protection. If we don't put the finalizer, there is no deletion protection against the resource. The current adoption logic expects the resource to not be managed (no finalizer) to trigger an adoption. If the initial creation attempt of a resource fails due to any AWS error, any subsequent reconciliations attempting to adopt an existing resource will not succeed. These changes set the resource as unmanaged if for any reason there is an error during the create call, which will allow the adoption logic to run in subsequent reconciliations.
michaelhtm
force-pushed
the
fix/unmanageresourceiferror
branch
from
979cd39 to
40f2bbb
Compare
|
@michaelhtm: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
/test s3-controller-test |
Issue #2406#issuecomment-2860631351
Description of changes:
The reason marking a resource as managed (putting the finalizer)
before attempting a create is a general practice in kubernetes. The main reason we
do it is to protect against deletion protection.
If we don't put the finalizer, there is no deletion protection against the resource.
The current adoption logic expects the resource to not be managed
(no finalizer) to trigger an adoption.
If the initial creation attempt of a resource fails due to any AWS
error, any subsequent reconciliations attempting to adopt an existing
resource will not succeed.
These changes set the resource as unmanaged if for any reason there is
an error during the create call, which will allow the adoption logic to
run in subsequent reconciliations.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.