Skip to content

Commit

Permalink
attempt to patchStatus regardless of finalizer (#64)
Browse files Browse the repository at this point in the history
Description of changes:

* ACK reconciler marks a resource as `Managed` before performing the `CREATE` call
* ResourceReference in ACK reconciler is resolved before `READ_ONE` call which precedes `CREATE` call
* When ResourceReference resolution fails, ACK reconciler needs to set `Status.Condition` indicating reference resolution failed
* Hence the status patching needs to be done for unmanaged resources too
* `NotFound` error while status patching of deleted resource is caught and not logged in controller logs

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
vijtrip2 authored Jan 12, 2022
1 parent dff8696 commit 30afbb6
Showing 1 changed file with 14 additions and 9 deletions.
23 changes: 14 additions & 9 deletions pkg/runtime/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -507,12 +507,15 @@ func (r *resourceReconciler) patchResourceStatus(
latest.RuntimeObject(),
client.MergeFrom(desired.DeepCopy().RuntimeObject()),
)
rlog.Exit("kc.Patch (status)", err)
if err != nil {
return err
if err == nil {
rlog.Debug("patched resource status")
} else if apierrors.IsNotFound(err) {
// reset the NotFound error so it is not printed in controller logs
// providing false positive error
err = nil
}
rlog.Debug("patched resource status", "latest", latest)
return nil
rlog.Exit("kc.Patch (status)", err)
return err
}

// deleteResource ensures that the supplied AWSResource's backing API resource
Expand Down Expand Up @@ -714,15 +717,17 @@ func (r *resourceReconciler) HandleReconcileError(
latest acktypes.AWSResource,
err error,
) (ctrlrt.Result, error) {
if ackcompare.IsNotNil(latest) && r.rd.IsManaged(latest) {
if ackcompare.IsNotNil(latest) {
// The reconciliation loop may have returned an error, but if latest is
// not nil, there may be some changes available in the CR's Status
// struct (example: Conditions), and we want to make sure we save those
// changes before proceeding
//
// Attempt to patch the status only if the resource is managed.
// Otherwise, the underlying K8s resource gets deleted and
// patchStatus call will return NotFound error
// PatchStatus even when resource is unmanaged. This helps in setting
// conditions when resolving resource-reference fails, which happens
// before resource is marked as managed.
// It is okay to patch status when resource is not present due to deletion
// because a NotFound error is thrown which will be ignored.
//
// TODO(jaypipes): We ignore error handling here but I don't know if
// there is a more robust way to handle failures in the patch operation
Expand Down

0 comments on commit 30afbb6

Please sign in to comment.