reconciler/managed: make more resilient to error conditions

[sttts]

Description of your changes

Two cases where the manager reconciler bricked an object forever:

1. when external.Create returned a conflict error. This was an oversight in reconciler/managed: only debug log transient conflict errors PT.2 #540 when introducing less noisy conflict handling.
2. on non-API errors when the critical failed/succeeded annotations were applied. Think of network issues. These are not API errors and hence they stopped the retry loop.

There is still a risk that an object becomes bricked, e.g.:

• when the ctx is cancelled after some timeout (30s or so per reconcile?). I.e. if the error persists for long enough.
• when the controller crashes before apply the critical annotations.

I have:

• Read and followed Crossplane's [contribution process].
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Careful code reading and review. This is hard to test other than under real-life condition under heavy load.

[luxas]

Change seems sensible.

However, when I read the code, I noticed that we don't propagate the possible error of UpdateCriticalAnnotations to the managed resource's status, we only emit an event

			// We handle annotations specially here because it's
			// critical that they are persisted to the API server.
			// If we don't add the external-create-failed annotation
			// the reconciler will refuse to proceed, because it
			// won't know whether or not it created an external
			// resource.
			meta.SetExternalCreateFailed(managed, time.Now())
			if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil {
				log.Debug(errUpdateManagedAnnotations, "error", err)
				record.Event(managed, event.Warning(reasonCannotUpdateManaged, errors.Wrap(err, errUpdateManagedAnnotations)))

				// We only log and emit an event here rather
				// than setting a status condition and returning
				// early because presumably it's more useful to
				// set our status condition to the reason the
				// create failed.
			}

			managed.SetConditions(xpv1.Creating(), xpv1.ReconcileError(errors.Wrap(err, errReconcileCreate)))
			return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)

the if err := r.managed.UpdateCriticalAnnotations(ctx, managed); err != nil { creates a new err variable, and does not overwrite the create error (https://go.dev/play/p/DXtx63QBA3v), so xpv1.ReconcileError(errors.Wrap(err, errReconcileCreate)) will contain only the info of that the create failed. maybe that is what we want, but I was wondering that if these annotations are so critical to add, in order to avoid dead objects, that maybe is worth propagating to status as well?

[luxas]

This seems common enough that it could be encapsulated somewhere; to our own RetryOnError func?
Or when would we want to retry although the context was cancelled?

[luxas]

There is still a risk that an object becomes bricked, e.g.:
when the ctx is cancelled after some timeout (30s or so per reconcile?). I.e. if the error persists for long enough.
when the controller crashes before apply the critical annotations.

Isn't there any solution to retry without the annotations? What does it take to do a reverse-lookup of the external resource to see if it indeed was created before the controller crashed, and then delete or update it on a new reconcile?

[github-actions]

Successfully created backport PR for release-1.14:

• [Backport release-1.14] reconciler/managed: make more resilient to error conditions #652

[github-actions]

Successfully created backport PR for release-1.15:

• [Backport release-1.15] reconciler/managed: make more resilient to error conditions #653

[negz]

What does it take to do a reverse-lookup of the external resource to see if it indeed was created before the controller crashed, and then delete or update it on a new reconcile?

@luxas as far as I know, there's no way to do this. Especially no way that will work for every external system we want to reconcile. I opened crossplane/docs#688 to (finally) document what these annotations are doing and why.