controllerutil: Add DeleteIfExists and TryUpdate / TryUpdateStatus

[adracus]

When developing controllers, other methods that are frequently required and are missing in the controllerutil package are:

• DeleteIfExists: Delete a resource if it exists, otherwise do nothing
• TryUpdate: Update a resource, if a Conflict is returned, retry (with wait.Backoff ?)
• TryUpdateStatus: Same as TryUpdate but for the Status subresource.

This would require making a more general OperationResult type that incorporates all possible OperationResult (like OperationResultDeleted, OperationResultUpdated etc.) and having 'subclasses' per operation, i.e. CreateOrUpdateResultCreated = CreateOrUpdateResult(OperationResultCreated). As such, this would be a partially breaking change.

Since I already make use of these methods, I already implemented them and just quickly tried to incorporate them in the controller-runtime. For reference, please look here.

[DirectXMan12]

Going to address these one at a time, because some of them seem to imply that we have documentation issues:

DeleteIfExists

Caveat: this shouldn't really matter, except for performance reasons -- if you delete, and it doesn't exist, you should get an error and requeue until the cache updates, or you backoff (and then you'll get a requeue when the cache actually updates).

That being said, a IgnoreNotFound helper could be nice for avoiding the requeue, and in conditions where you don't want to log, or where you don't want to auto-requeue (which is actually most conditions, since you don't want to do anything till you get a change notification from the watch cache).

Then, DeleteIfExists becomes if err := client.Delete(ctx, &obj); client.IgnoreNotFound(err) != nil { ... }. I've seen this in other places

/good-first-issue
/priority important-longterm
/kind feature

TryUpdate

This seems like a documentation issue on our part, since returning an error from the reconciler will cause an automatic requeue, with backoff if need. Retrying inside a reconciler is generally an anti-pattern -- it leads to non-obvious head-of-line blocking and weird logic errors. Your controllers should be idempotent, so requeuing shouldn't cause issues.

What's your usecase here that's not solved by requeuing?

TryUpdateStatus

Same as above.

[k8s-ci-robot]

@DirectXMan12:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Going to address these one at a time, because some of them seem to imply that we have documentation issues:

DeleteIfExists

Caveat: this shouldn't really matter, except for performance reasons -- if you delete, and it doesn't exist, you should get an error and requeue until the cache updates, or you backoff (and then you'll get a requeue when the cache actually updates).

That being said, a IgnoreNotFound helper could be nice for avoiding the requeue, and in conditions where you don't want to log, or where you don't want to auto-requeue (which is actually most conditions, since you don't want to do anything till you get a change notification from the watch cache).

Then, DeleteIfExists becomes if err := client.Delete(ctx, &obj); client.IgnoreNotFound(err) != nil { ... }. I've seen this in other places

/good-first-issue
/priority important-longterm
/kind feature

TryUpdate

This seems like a documentation issue on our part, since returning an error from the reconciler will cause an automatic requeue, with backoff if need. Retrying inside a reconciler is generally an anti-pattern -- it leads to non-obvious head-of-line blocking and weird logic errors. Your controllers should be idempotent, so requeuing shouldn't cause issues.

What's your usecase here that's not solved by requeuing?

TryUpdateStatus

Same as above.

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.