-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Builder.Watches + EnqueueRequestsFromMapFunc = invalid kind #1330
Comments
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 |
It looks like you need to use It looks like #1182 added explicit checks for this, and that PR was released in v0.7.0. |
So I see you're mapping pods to requests. In your |
@joelanford Thanks for the tip, and showing how the 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:
I'm not sure if that helps or adds more confusion, but hopefully it explains the reasoning behind the approach. |
That smells more like a webhook than a controller. |
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 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. |
The problem with
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).
This is why in our original design and implementation we used a custom event handler in the |
Why is |
Two reasons:
|
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. |
Okay, I’ll test out the |
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 |
@joelanford In our current operator, nothing. In the controller's 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.
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. |
@aharbis Got it! I missed that part in your comment. I think another option here is to skip the use of the It looks like that's basically what you already have. That should still work. |
Oh, neat. I didn't realize that API was still available, but that makes sense. I did get the If it becomes too much of nuisance to have the CR instances trigger reconciliation in the short-term, we can switch back to the 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. |
BTW, this function |
I'm working on migrating our operator to Operator SDK
v0.19.4
, and along with that comes acontroller-runtime
bump tov0.6.1
. We use a custom event handler, which I've usedEnqueueRequestsFromMapFunc
to register viaWatches
. Here's the basic structure:However, somewhere along the way of the controller being created and this registered, I get back a very generic error:
If I switch out this custom
Watches
for the standardFor
andOwns
combination, the controller is created and registered successfully. So, I seem to have narrowed the error down to the usage ofEnqueueRequestsFromMapFunc
.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!
The text was updated successfully, but these errors were encountered: