Consider using generics for the reconcile.Reconciler interface

[alvaroaleman]

Most reconcilers reconcile some kubernetes object. Any such reconciler needs to start off with a check if the object still exists and do nothing if not. We could avoid this check by doing it in controller-runtime and adding a new interfaceReconcile[o ctrlclient.Object](context.Context, o)(reconcile.Result, error) that people can use instead of the current one. We can not remove the current one, because not all reconcilers act on k8s objects.

The main challenge here is that there are also reconcilers that do not reconcile a kubernetes object and that need to be able to use some kind of arbitrary key.

/cc @sbueringer @vincepri

[vincepri]

This is interesting. We might need to rework how the Controller handles the internal workqueue and instead of adding requests, it also passes in the object that's being reconciled, if any.

[alvaroaleman]

This is interesting. We might need to rework how the Controller handles the internal workqueue and instead of adding requests, it also passes in the object that's being reconciled, if any

No, the workqueue needs to store identifiers only to be able to de-duplicate.

We need a controller implementation or maybe a reconciler wrapper that takes a client, is configured with the right object, returns if it is not found or calls the typed reconciler if it is found.

[vincepri]

Ah so you don't want the events' objects, but rather do a new Get call?

[alvaroaleman]

Yeah exactly because at the time the object gets reconciled it might have been deleted. This is really just about elimnating the Get(ctx, myObject); if apierrors.IsNotFound(err) { return nil }.

We have a downstream wrapper to do this and I thought that something like that would make a lot of sense to be upstreamed.

[sbueringer]

Sounds good to me!

[jonathan-innis]

We came up with this implementation for our usage: https://github.com/aws/karpenter-core/blob/main/pkg/operator/controller/typed.go which is potentially a step further than you want to go with the Finalize() method but has the same ideas

[nathanperkins]

Why does this need generics? Would func Reconcile(context.Context, o client.Object)(reconcile.Result, error) work?

(edit: I misread the original post so the comments below aren't valid)

I would prefer this change isn't made:

1. It's a breaking change to the API and a change in behavior, so it means that everybody pulling in this change via upgrade will have to understand and fix it.
1. Some reconcilers have special logic handling for when an object is deleted.
1. The pattern that it cleans up is clear and not a lot of code. It's like 5 lines.

If we like this change, it could be a separate interface so that people can opt in. And you can mark the old one deprecated if you like.

[alvaroaleman]

@nathanperkins the issue is titled "Consider using generics", so this is up for debate. We are unlikely to break the Reconciler interface simply because there are reconcilers that do not act on a k8s object. I will update the issue to clarify that.

Some reconcilers have special logic handling for when an object is deleted.

Do you mind elaborating on what kind of deletion hendling would be broken by this?

[nathanperkins]

(edit: I misread the original post so the comments below aren't valid)

If you change the function signature, it's a breaking API change. All the current reconcilers which use ctrl.Request will have to be updated to meet the new signature. It also means that any repo which wants to upgrade controller-runtime but imports a module that uses old controller-runtime will have to wait for the dependency to upgrade.

Any breaking change in API should be carefully considered for whether the benefit (removing a few lines of boiler plate) outweighs the risk (every downstream reconciler has to be changed and the upgrade story is complicated).

You can get around most of the issues by introducing a new interface or by using major versions v1 and beyond

Do you mind elaborating on what kind of deletion hendling would be broken by this?

Any controller which has to maintain some internal state or cache about the objects it is reconciling. One example is if you use mapping functions, you might cache the information you need to make mapping decisions. This may be preferable to using client.List in the mapping function since client.List can fail and the mapping functions don't have any error handling or retries.

[alvaroaleman]

You can get around most of the issues by introducing a new interfac

I know, that is what the issue body states.

Any controller which has to maintain some internal state or cache about the objects it is reconciling. One example is if you use mapping functions, you might cache the information you need to make mapping decisions. This may be preferable to using client.List in the mapping function since client.List can fail and the mapping functions don't have any error handling or retries.

Okay, interesting approach. How do you ensure that data exists prior to the handler being used? It also means that the cached mapping data might be arbitrarily outdated or not?

[nathanperkins]

I know, that is what the issue body states.

My bad, I don't know how I misread that. Sorry about that. I don't have any problems with a new interface.

I generally avoid generics unless absolutely necessary. But I suppose it allows us to write the reconcilers without having to do a type assertion?

Would this automatically fulflll the interface without having to explicitly specify the generic type?

func (r *Reconciler) Reconcile(context.Context, corev1.Node) (ctrl.Result, error)

If so, that sounds cool to me.

Okay, interesting approach. How do you ensure that data exists prior to the handler being used? It also means that the cached mapping data might be arbitrarily outdated or not?

If the cached data is missing or out of date, it's usually because the object has changed and there would already be a request queued for it. In that case, mapping an event to that object would be redundant.

[alvaroaleman]

I generally avoid generics unless absolutely necessary. But I suppose it allows us to write the reconcilers without having to do a type assertion?

Yes, exactly

Would this automatically fulflll the interface without having to explicitly specify the generic type?

func (r *Reconciler) Reconcile(context.Context, corev1.Node) (ctrl.Result, error)

It would need to be *corev1.Node, not corev1.Node as the client.Object is implemented by the pointer type only but other than that, yeah.