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
Predicates added with For() affects Owns() #2684
Comments
/kind support |
would appreciate if someone can try to reproduce and verify. |
This issue may be hard to diagnose and reproduce without knowing exactly the intentions are with what you are trying to do. Per the documentation for Owns, it watches resources that are generated by the controller. |
So the CustomResource creates Namespaces and puts owner references on it. And you can also specify namespace metadata from CustomResource. So i am trying to watch namespace delete and update events on its labels and annotations. We are following operator-sdk, and till the latest version, operator-sdk supports k8s 1.26 and controller runtime 14.7 |
Who is the owner of the namespace? The controller or the CustomResource? If it is the latter, then I don't believe that it will enqueue request for that namespace. Are you checking that the namespace exists in your reconcile loop?
I'm not sure what this means, but if there is some 1:1 relationship there your schema may be conflicting with how it resolves events.
This will still queue the parent object on the reconciler in your request and you would have to see if the namespace was deleted and/or modified. So if custom resource A's namespace was deleted, you'll get a request to handle the reconciliation for custom resource A and you'll have to see if the owned objects, exists, became modified, to handle that logic. |
the CustomResource, and yes the namespace exists. On the rest of the points, the controller works as expected if i put But if i add any predicate inside To be noted, the operator is very much mature and functionality has been in use for years. I came across this while trying to fine grain the predicates, and writing tests to test the predicate. |
From the docs:
Have you tried to write your own predicate func? I do not know the value of the generation in the metadata of your resources that would help the predicate understand the true/false nature your change, but if your generation of the custom resource doesn't change that update request will not get sent to the reconciler. You are reducing the events being handled by the update event because you added the predicate. That is why it works when you don't put the predicate on because that update event is allowed to go through. |
For the events coming from But it shouldnt have any effect on Namespace changes. Like if someone/some other operator changes Namespace labels or annotations. In my understanding, both have separate scopes. If you use But Adding predicates to For() should not affect Owns() or Watches() scope.
Not yet. I wanted to know first why the overlap is happening between predicate of different resources. |
From the docs:
It acts on the parent object.
This means that it will in fact update the owner object as a reconcile request Owns is just syntactic sugar for the Watches which act on parent resources. If the request is sent but the predicate disallows that from coming into the queue it won't be shown. This is the expected behavior. If I write a predicate to recieve all requests (like not putting one in the For block) I can expect it to recieve events on the customresource when anything that is owned or watches changes and enqueues the parent resource from updated. If the namespace changes, the owned parent resource must be requeued because what is owned by the custom resource is the only scope you have to deal with that logic. |
This is confusing me a bit. Let me explain and present some scenarios on how its behaving, and you can tell me if thats the expected behavior.
|
Your manager gets all events for custom resource including status updates. If you don’t want to handle status updates, you can write a predicate to do this or make your controller more idempotent to handle the status changes. Look at this issue kubernetes-sigs/kubebuilder#618
I agree the controller doesn’t get events for this because nothing changes on the parent resource because that predicate is there now. Think of it as when the Owned resource changes the parent resource is sent to the event queue to be handled by the controller but the controller see the generation didn’t change because this event doesn’t increment the generation because nothing on this object has changed. OldObject == NewObject don’t send. Look at this test to see an example.
To rely on events from other resources you must understand how those resources handle generation change. https://github.com/kubernetes-sigs/controller-runtime/blob/v0.17.1/pkg/predicate/predicate.go#L156 If what you are trying to accomplish is to not get events for status changes on your custom resource, you can write a predicate that checks if the status has changed but because you are patching timestamps, that will be every event it seems. You can make this more granular and only handle the events where oldObject.Status.Somefield != newObject.Status.Somefield |
So that basically means, if i understood it correctly, that the conditions i am trying to achieve are not possible at the moment, when used together?
Since no matter what prediate i put on CustomResource, events coming from other resources will be ignored as the |
You can write your own handler to do the reconcile requests for the events you have for the crb and the namespace.
After you write the handler, you can deal with what gets requeued by handling the logic within the handler. This ensures you have the capability to deal with the events triggered by the owned resources. As far as not dealing with the status changes requeues, if you are not worried about handling them, they should be a no-op and/or if you want to reduce the events the controller is dealing with don't place things in the status where that would requeue. |
we do have alot of custom handlers, which i removed here for better understanding of the issue. The problem I am concerned about is mainly Should the event really go to the For() part if it comes from Owns() or Watches() ?
I considered along those lines, but the status that we put at the moment, i consider it quite useful for such big project that we have, I myself as a user would like to know when was the last time my CustomResource reconciled and under what conditions/state, what was the observed error/state at that time. so i dont have to go through thousands of lines of code to figure out the timestamps, and same for the Operator User/Customer, since we are not expecting them to be experts of Operator Development, i want to make it as easier as it can be. Besides, we are using Conditions from apiMachinery which i suppose is also used by community |
The owned object will never change the owner object and that queue for the event triggered by the owned object has to make sure that we requeue the owner object. Because it doesn't change, if you restrict it with a predicate, it will disallow that requeue to take place with your predicate. The lifecycles of the owned and owner objects are very different and can be handled by different controllers and not just your use case. If you want to treat these resources separately, constructing your operator to deal with these different events, controller-runtime has the tooling available to do that which is stated in this issue and in the docs. What you are suggesting is to break the current behavior to accommodate the use case of not dealing with status changes but allow everything else to go through. Supporting both use cases is something I don't believe controller-runtime has the ability to support as the normal behavior which is in place now is the use case for how informers work and the work queue that gets generated from events of owned objects.
There are many other ways to observe reconciles and even provide the data you are seeking to surface. If your CR is reconciling too much because of the status, that may be a symptom of a bigger problem and something to diagnose while understanding the limitations of predicates and/or other functionality shipped with controller-runtime. If your goal is to get all events from owned objects understanding that it will requeue the owner object and that will in fact not change the owner object, your controller handling that logic can deal with those events by making the controller idempotent in nature so that a lot of these edge cases you are dealing with will not be a problem. If you are not worried about the status in your controller logic, should those things in your status really be there? You can put timestamps in your metadata as labels, so the status requeue will only be for things you need to key on to make the "cluster" reflect the desired state of the CR you are pushing. |
I might have found the middle ground to resolve all of these conditions, i will test it and get back to you. If it works, i can close the issue. |
A predicate in controller-runtime/pkg/builder/controller.go Lines 298 to 307 in bbe3bbe
@MuneebAijaz can you write a minimal reproducer for this, please? |
@alvaroaleman sure, i will get back to you with a small controller with mentioned conditions |
I am watching multiple resources under
SetupWithManager
Two of them being,
Namespaces
andCustomResource
.Controller runs for
CustomResource
and is specified asFor(&v1beta1.CustomResource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
Namespaces as
Owns(&corev1.Namespace{}).
Now, i want events from
CustomResource
when spec gets updates. and fromNamespaces
under all conditions.It seems like, whenever I add
, builder.WithPredicates(predicate.GenerationChangedPredicate{}
or similiar in For(), I stop getting events for Namespaces. But it works as expected in absence of any WithPredicates condition with For()To be noted, i am not using any WithEventFilter()
Is this the expected behaviour? If yes, pls guide with the correct conditions to be applied.
Thanks in advance
Controller Runtime at v0.14.7
k8s.io libraries at v0.26.11
The text was updated successfully, but these errors were encountered: