Do not retry updates on conflict

[ludydoo]

When the reconciliation loops tries to update a CR, and the result is a Conflict, the reconciliation loop should not retry because this error will never succeed. The reconciliation loop needs to be restarted.

Additionally, there was a situation where the resource would be deleted, but the Updater would still try to update the deleted resource, resulting in errors.

[joelanford]

Does this change result in conflict errors consistently showing up in the logs during uninstall? In general, I think that's reasonable because it is what is happening, but I've seen that conflict error make many, many users nervous in lots of different operators.

It seems like if it is showing up, we would want to quickly see if its possible to eliminate the conflict issue altogether.

[ludydoo]

Does this change result in conflict errors consistently showing up in the logs during uninstall

@joelanford

Yes, we've seen it happen pretty much consistently. These changes should fix that, basically by not performing an update on a Gone resource after the controllerutils.WaitForDeletion. I think it's also a misleading apiServer error (precondition failed) rather than a 404 not found.

[ludydoo]

@joelanford let me know if there's anything I can do in terms of tweaking/changing stuff !

[joelanford]

I'm being pulled in a bunch of different directions, so I'll volunteer @everettraven (if he's willing) to take a look as well.

[everettraven]

I'll add it to my queue 😄

[everettraven]

The changes look good to me and seem reasonable. Thanks @ludydoo for taking the time to make this contribution!

[ludydoo]

@joelanford would this be good to go ?

[porridge]

This is a bit awkward, because these two new comment lines applies to both retry.OnError calls. However the above 3 lines only apply to the first retry.OnError call.

I can suggest two solutions:

1. move these two lines to a separate comment above this one. However this takes it further away from the second OnError call, which is not great.
2. create a helper function such as:

func retryOnRetryableUpdateError(backoff wait.Backoff, fn func() error) {
  return retry.OnError(backoff, isRetryableUpdateError, fn)
}

and then we could put this comment on this function, which would be the most appropriate place. We could optionally also inline isRetryableUpdateError in that case.

[ludydoo]

Sounds good :)

[porridge]

Introducing a new variable here that is ready by the defer closure (and not easy to grok straight away), I think it might be cleaner to introduce a method on the updater such as SkipAllUpdates() or DoNotApply() that would:

• cause Apply(...) to just return nil without doing anything, and
• (optional) cause the updater to panic() on further Ensure... calls to make sure that SkipUpdates is closely followed by a return from Reconcile()

[ludydoo]

I like this a lot 💯

[porridge]

Suggested change

	log.Info("Resource is already terminated, skipping deletion")
	log.Info("Resource is already terminated, skipping deletion.")

[porridge]

This is slightly different from the case described in the PR description, right?
I'm guessing the problem here was that on deletions both the uninstallUpdater and the regular u updater would fire, and the latter was both useless and would cause errors since the resource would typically be already gone after deleting the finalizer?
If I got this right, please mention this in the PR description.

[porridge]

Looks nice, but please add tests for CancelUpdates to the updater package for completeness.

[varshaprasad96]

/lgtm
/approve

The changes look good to me. Agree on adding a test for CancelUpdates() to keep the code coverage same. We can merge this for now to move fwd, and will create an issue for followup as its straightforward.