Builder.Watches + EnqueueRequestsFromMapFunc = invalid kind

[aharbis]

I'm working on migrating our operator to Operator SDK v0.19.4, and along with that comes a controller-runtime bump to v0.6.1. We use a custom event handler, which I've used EnqueueRequestsFromMapFunc to register via Watches. Here's the basic structure:

func (r *TestReconciler) SetupWithManager(mgr ctrl.Manager) error {
    return ctrl.NewControllerManagedBy(mgr).
        Watches(
            &source.Kind{Type: &corev1.Pod{}},
            &handler.EnqueueRequestsFromMapFunc{
                ToRequests: handler.ToRequestsFunc(func(a handler.MapObject) []reconcile.Request {
                    return []reconcile.Request{
                        {
                            NamespacedName: types.NamespacedName{
                                Name:      a.Meta.GetName(),
                                Namespace: a.Meta.GetNamespace(),
                            },
                        },
                    }
                }),
            }).
        Complete(r)
}

However, somewhere along the way of the controller being created and this registered, I get back a very generic error:

expected pointer, but got invalid kind

If I switch out this custom Watches for the standard For and Owns combination, the controller is created and registered successfully. So, I seem to have narrowed the error down to the usage of EnqueueRequestsFromMapFunc.

Looking at this example from the documentation, I'm using the correct syntax/structure as far as I can tell.

I'm not sure where to go from here, so hoping someone here will be able to help out. Thanks!

[aharbis]

If I switch out this custom Watches for the standard For and Owns combination, the controller is created and registered successfully.

For clarity, this was purely for proof of concept and troubleshooting. Since we use a custom event handler, we are reliant upon registering it with EnqueueRequestsFromMapFunc. I'm not aware of a mechanism to register a custom event handler with For/Owns, but if that's feasible it might be a viable workaround.

[joelanford]

It looks like you need to use For. Otherwise you're passing a nil pointer for the primary source.Kind{}

It looks like #1182 added explicit checks for this, and that PR was released in v0.7.0.

[joelanford]

So I see you're mapping pods to requests. In your Reconcile method, what GVK are you reconciling? You should watch that type with a For call.

[aharbis]

@joelanford Thanks for the tip, and showing how the For links in to the impl. of doWatch(). I'll give that a shot and see how it behaves in tandem with Watches.

To answer your second question, this controller in question is not the primary controller for the custom resource. Technically this controller does not reconcile any GVK. Instead, its purpose is to watch for Pod events and take some action based on the event, where an action might be invoking a REST API against the running Pod.

The GVK that owns the Pod (via a StatefulSet) is reconciled by another controller. The reason we are using a second controller for this is that the operations this controller performs can take minutes, and we do not want to block the execution of the main controller reconciliation.

So in short, we have two controllers here:

1. Primary controller uses a For/Owns pair to reconcile our primary resource, which in-turn creates and manages a StatefulSet.
2. Secondary controller that watches for Pod events, and uses Reconcile as a means to orchestrate operations against that Pod, and its peers.

I'm not sure if that helps or adds more confusion, but hopefully it explains the reasoning behind the approach.

[coderanger]

That smells more like a webhook than a controller.

[aharbis]

For some clarification, we do technically have a second GVK that the secondary controller uses as a means of configuration and status reporting, but there's no reconciled object like a Pod as the result. The CR is read at beginning of Reconcile to get the config, a goroutine is spun off to perform the operation, and the status subresource on the CR is updated during processing.

So there is a GVK here, but it's really just a means of storing configuration and reporting status. It's not a standard CR -> StatefulSet -> Pods relationship, as our other CRD.

[aharbis]

The problem with For/Owns in this use case is that we need to respond to Pod events by triggering reconciliation of a GVK that technically does not own (even indirectly) the Pod. Arrows indicate ownership:

PrimaryCRD --> SecondaryCRD
    |
    v
StatefulSet
    |
    v
   Pod

The Pod is owned by a StatefulSet, which is owned / managed by our PrimaryCRD and controller. The purpose of this secondary controller is to watch for Pod events in the namespace, filter down to Pods we care about, and trigger "reconciliation" of the SecondaryCRD instance (which as mentioned above is purely for config/status).

For defines the type of Object being reconciled, and configures the ControllerManagedBy to respond to create / delete / update events by reconciling the object. This is the equivalent of calling Watches(&source.Kind{Type: apiType}, &handler.EnqueueRequestForObject{})

For does not fit, because we are not reconciling Pod instances in this controller.

Owns defines types of Objects being generated by the ControllerManagedBy, and configures the ControllerManagedBy to respond to create / delete / update events by reconciling the owner object. This is the equivalent of calling Watches(&source.Kind{Type: }, &handler.EnqueueRequestForOwner{OwnerType: apiType, IsController: true})

Owns does not fit, because this controller does not generate Pods (directly or indirectly), or any other resource for that matter.

This is why in our original design and implementation we used a custom event handler in the Watch() to map Pod events to reconcile requests of the SecondaryCRD instance.

[coderanger]

Why is For(SecondaryCRD) not correct? If that's the object being reconciled then it's the root type for that controller.

[aharbis]

Two reasons:

1. We want Pod events to trigger the secondary controller’s Reconcile method.
2. We have no reason to run Reconcile unless a Pod event has occurred.

[coderanger]

Right, you do For(SecondaryCRD) to set the root type and then use a watch map function on Nodes, with the function mapping them back to the responsible SecondaryCRD so it can be reconciled.

[aharbis]

Okay, I’ll test out the For in combination with my original Watches and see how it behaves. I’m concerned that it will generate more reconcile requests than we currently get, but it’s something we might be able to work around.

[joelanford]

I agree with trying @coderanger's suggestion. Also, what happens if the secondary CRD changes? Do you need to re-reconcile all of the existing pods?

If you end up needing to stick with your existing setup, I'm still curious about my original question. In your map function, you're getting a pod event, and then creating a request with the name and namespace of that pod. And then it sounds like you're using that request to lookup that pod in your reconcile method? That's exactly what the EnqueueRequestForObject handler used by For() does.

[aharbis]

Also, what happens if the secondary CRD changes? Do you need to re-reconcile all of the existing pods?

@joelanford In our current operator, nothing. In the controller's add() function, we had only:

	err = c.Watch(&source.Kind{Type: &corev1.Pod{}}, getPodEventHandler())
	if err != nil {
		return err
	}

There was technically no link between the SecondaryCRD and the controller, from the perspective of the watches.

In your map function, you're getting a pod event, and then creating a request with the name and namespace of that pod. And then it sounds like you're using that request to lookup that pod in your reconcile method?

I should clarify that the "basic" map function I showed in the top comment was mostly to demonstrate a minimal failing case; in our operator there's a more intelligent map function implemented which does the pod event filtering, resource lookups, etc.

The pod event handler traverses the relationship tree (ref: comment) to find the SecondaryCRD name/namespace, and a reconcile request is generated with that metadata.

The SecondaryCRD does not directly own the Pod (nor StatefulSet), hence we use the custom map function to traverse the ownership tree and find the CR instance to reconcile.

[joelanford]

@aharbis Got it! I missed that part in your comment. I think another option here is to skip the use of the Builder and directly create and register the controller with the manager, and then add watches with controller.New() and then c.Watch() in the Controller interface.

It looks like that's basically what you already have. That should still work.

[aharbis]

Oh, neat. I didn't realize that API was still available, but that makes sense.

I did get the For and Watches combination to work, but I suspect that updates to the SecondaryCRD instance would trigger reconciliation. While this is "new" behavior compared to our current controller implementation, it may not be a bad thing in the long run if we add more functionality to that controller that would warrant CR instance update reconciliation.

If it becomes too much of nuisance to have the CR instances trigger reconciliation in the short-term, we can switch back to the controller.New() and c.Watch() implementation we have currently.

We've got a couple of paths forward, I appreciate the help from both of you @coderanger / @joelanford! I'm happy to close this issue now.

[abdennour]

BTW, this function ToRequestsFunc was removed from the handler package. Who could help if we upgraded the package ?