Evaluate using generics for source/predicate/handler chain

[alvaroaleman]

Predicates currently take a client.Object and return a bool. With generics, we might be able to have typed generics, which would let ppl avoid a type assertion inside their predicate when they want to access anything other than metadata.

We currently type to client.Object, but this is both too much in the case of a source.Channel that could take literally everything that might not actually be a client.Object and too little in the case of everything else, as we lose the type information.

[nikola-jokic]

/assign

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[alvaroaleman]

/assign

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[alvaroaleman]

I did look into this. It works great for just the source/predicate/handler chain, but due to golang/go#49085 it makes the UX of the builder terrible, as you can not directly pass a type and handler into Watches and friends but instead need to wrap them.

[nikola-jokic]

I can give it another try if you'd like ☺️

[alvaroaleman]

I can give it another try if you'd like

Its not about giving it a try. I don't think it is possible to do this without that golang issue being solved or accepting that the UX of the builder will become quite a bit worse. Let me try to find the branch I did this in (its been a couple of months) so it can be shown

[Danil-Grigorev]

Hi, I added a PR which propagates type information to the handler - #2698 Also faced with the same issue as @alvaroaleman mentioned. One way to tackle this is to pass prepared source.Kind to all builder methods instead of object (For, Owns, Watches), and deprecate, or count non-functioning the OnlyMetadata option for the new methods introduced to support source.Kind. Which does not seem to be required to set PartialObjectMetadata watches anyway.

Another option is to wrap client.Object in a struct[T]{object T} (or similar) and pass this around the builder methods instead. Or just make builder methods wrappers and get rid of chaining.

[alvaroaleman]

Yeah a wrapper around generic source/predicate/handler is the only way to make this work in the builder.

Just to be clear on this, we will not be merging any code unless we are sure with worsening the builder UX like that.

[alvaroaleman]

I guess to extend to the prior message, I would really love to see us using generics there somehow because that would mean that you can build arbitrary controllers that might not even act on kube resources. But not at the price of significantly worsening the experience when building kube controllers, as that will remain the 98% use case of controller-runtime.

[Danil-Grigorev]

I think the builder pattern can be preserved. Here is the flow I came up with:

ctrl.
	NewControllerManagedBy(manager).         // Create the Controller
	With(ctrl.Object(&appsv1.ReplicaSet{})). // ReplicaSet is the Application API
	Own(ctrl.Object(&corev1.Pod{})).         // ReplicaSet owns Pods created by it
               Watch(ctrl.Object(&corev1.ConfigMap{})). // ConfigMaps are being watched
	Complete(&ReplicaSetReconciler{Client: manager.GetClient()})

where ctrl.Object is exposing this

// WatchObject is an interface on an object wrapper with preserved type information
type WatchObject interface {
	GetObject() client.Object
	SetSource(cache.Cache) source.SyncingSource
}

type watchObject[T client.Object] struct {
	object T
}

// SetSource returns inner client.Object
func (w watchObject[T]) GetObject() client.Object {
	return w.object
}

// SetSource sets and returns the source.SyncingSource on the object
func (w watchObject[T]) SetSource(cache cache.Cache) source.SyncingSource {
	return source.ObjectKind(cache, w.object)
}

// Object constructs a wrapper on a generic object with stored type information
func Object[T client.Object](obj T) WatchObject {
	return watchObject[T]{object: obj}
}

and both OwnsInput and ForInput have this stored instead of client.Object. WDYT @nikola-jokic @alvaroaleman? Playing with this here

[Danil-Grigorev]

I looked into this a bit further… The biggest problem with integrating generics is an enormous amount of code duplication, to preserve current API definitions without changes introduced by generics. Essentially everything: predicates, cache.Informers, enqueue mappers, object handlers, event handlers, source.Kind have to have a generic counterpart or just generic version of themselves. Which has a cascade effect on other types across the code.

TLDR: I would personally just prefer having generic predicates and eventHandler, and ensuring the type safety for the objects coming from cache (for starters), and focus on that. WDYT?

The problem I’m facing is again, the builder, or rather the options for each watch input. I’m rotating this pseudocode design in mind at it’s most raw form:

// internal source.Handler:
func NewEventHandler[T any](ctx context.Context, queue workqueue.RateLimitingInterface, 
    handler handler.EventHandler[T], predicates []predicate.Predicate[T]) *EventHandler[T]

// handler:
func EnqueueRequestsFromMapFunc[T any](fn MapFunc[T]) EventHandler[T]

// Predicate:
func NewPredicateFuncs[T any](filter func(object T) bool) Funcs[T]

// Builder:
func (blder *Builder) Add(…) *Builder
func Watch[T any](object T, eventHandler handler.EventHandler[T], opts ...Option[T])

// Usage:
predicate := predicate.NewPredicateFuncs(func(pod *corev1.Pod) bool { return true }))
mapHandler := handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, pod *corev1.Pod) []reconcile.Request …
b.Add(Watch(&corev1.Pod{}, mapHandler, predicate))

With this approach the first occurrence of &corev1.Pod dictates the type for the rest of the declaration, and the predicates or event handlers provided will already expect *corev1.Pod as the source object, reducing amount of business logic required to setup a controller, in theory.

But the options do not fit. If I specify type in it, then everything storing the struct to which the option should be applied have to inherit this type, and the Builder too, which is incorrect. A controller almost always operates on different objects in tandem.

I think that if the current UX brings enough improvement and it makes sense to add in some (similar or other) for, it would be good to add these changes piece by piece. Refactoring whole source/predicate/handler chain causes changes in the whole project. If we agree on that, we can split it in smaller parts and implement incrementally.

[alvaroaleman]

But the options do not fit. If I specify type in it, then everything storing the struct to which the option should be applied have to inherit this type, and the Builder too, which is incorrect

Exactly, that is the unsolved problem. I do not want to force everyone to use a wrapper before calling builder.For/Owns/Watches, as that will lead to an enormous amount of code churn.

[alvaroaleman]

Okay so one approach I can think of that would not require changes to exported parts of the builder is to change the source.Source interface to not take Handler and Predicates in Start, but get them at creation time. That way, everything generic is inside the Source implementation. This would mean that folks that directly call controller.Watch need to change things but not folks that use the builder, which presumably is the majority.

[Danil-Grigorev]

I tried that, that works, have a very much WIP implementation for this in this branch. It is simpler than the original form I was trying to pursue, thanks for the pointer. I agree, it seems to me that the 2 approaches should co-exist, as forcing the majority to move their code to generics may cause confusion.

[alvaroaleman]

@Danil-Grigorev would you be up to write a small doc that describes the changes in public interfaces needed for this and the motivation?

[alvaroaleman]

I created #2783 for this which I think is the most compatible way we can do this, let me know what you think.