Retry on enablement failure

[fwiesel]

No description provided.

[fwiesel]

We need to retry, if there is some transient error. My understanding is your ( @notandy ) preference is not to let the error get up to the controller loop, so I return now a retry and requeue the reconciliation manually.

[notandy]

Alternatively, if gophercloud is encapsulating all openstack related errors, you could just check if the bubbling up error is an gophercloud error to retry instead, and throw everything else.

[fwiesel]

I went with the RetryError.

[notandy]

minor nitpick

[fwiesel]

I start to think, returning the wish to requeue the reconciliation is not a good idea....
It started off with not needing to change the signature of an internal function, but if I continue going down that path, then I end up with something like

type errorRequeue struct {
	RequeueAfter time.Duration
}

func (e errorRequeue) Error() string {
	return "someone didn't handle the errorRetry case"
}

func requeueAfter(t time.Duration) errorRequeue {
	return errorRequeue{RequeueAfter: t}
}

func RetryOnConflict(backoff wait.Backoff, fn func() (ctrl.Result, error)) (ctrl.Result, error) {
	err := retry.OnError(backoff, k8serrors.IsConflict, func() error {
		result, err := fn()
		if err != nil {
			return err
		}
		if result.IsZero() {
			return nil
		}

		return requeueAfter(result.RequeueAfter)
	})

	var requeueError errorRequeue
	if errors.As(err, &requeueError); requeueError.RequeueAfter > 0 {
		return ctrl.Result{RequeueAfter: requeueError.RequeueAfter}, nil
	}

	return ctrl.Result{}, nil
}

And somehow it doesn't sit right with me.

[notandy]

You're right, if you really want to propagate a duration, it makes no sense anymore to marshal it into an error.
I am fine with the current pr although, but it's up to you if you want to build it differently.