Predicates added with For() affects Owns()

[MuneebAijaz]

I am watching multiple resources under SetupWithManager
Two of them being, Namespaces and CustomResource.

Controller runs for CustomResource and is specified as
For(&v1beta1.CustomResource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
Namespaces as
Owns(&corev1.Namespace{}).

Now, i want events from CustomResource when spec gets updates. and from Namespaces 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

[troy0820]

/kind support

[MuneebAijaz]

would appreciate if someone can try to reproduce and verify.
its sort of a headache

[troy0820]

This issue may be hard to diagnose and reproduce without knowing exactly the intentions are with what you are trying to do.
Is this namespace you are watching, get created by the controller? Are you trying to watch the namespace the custom resource is in for events of the namespace deletion? Does this namespace have an owner reference on it? Do you have to stay at v0.14.7? Can you move to a newer version to take advantage of MatchEveryOwner? What events do you expect to get from namespaces?

Per the documentation for Owns, it watches resources that are generated by the controller.
https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.17.1/pkg/builder#Builder.Owns

[MuneebAijaz]

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

[troy0820]

So the CustomResource creates Namespaces and puts owner references on it.

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?

And you can also specify namespace metadata from CustomResource

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.

So i am trying to watch namespace delete and update events on its labels and annotations.

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.

[MuneebAijaz]

Who is the owner of the namespace? The controller or the CustomResource?

the CustomResource, and yes the namespace exists.

On the rest of the points, the controller works as expected if i put Owns(&corev1.Namespace{} with For(&v1beta1.CustomResource{}).

But if i add any predicate inside For() (no matter what i do with Owns(Namespace)), it stops giving me events for Namespace.
for example
For(&v1beta1.CustomResource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).

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.

[troy0820]

From the docs:

GenerationChangedPredicate implements a default update predicate function on Generation change.
This predicate will skip update events that have no change in the object's metadata.generation field. The metadata.generation field of an object is incremented by the API server when writes are made to the spec field of an object. This allows a controller to ignore update events where the spec is unchanged, and only the metadata and/or status fields are changed.
For CustomResource objects the Generation is only incremented when the status subresource is enabled.

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.

[MuneebAijaz]

You are reducing the events being handled by the update event because you added the predicate.

For the events coming from CustomResource, yes.

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 WithEventFilter(), only then it should affect all the Watches/Owns/For resources.

But Adding predicates to For() should not affect Owns() or Watches() scope.

Have you tried to write your own predicate func?

Not yet. I wanted to know first why the overlap is happening between predicate of different resources.

[troy0820]

From the docs:

// 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}).

It acts on the parent object.

But it shouldnt have any effect on Namespace changes. Like if someone/some other operator changes Namespace labels or annotations.

This means that it will in fact update the owner object as a reconcile request
https://github.com/kubernetes-sigs/controller-runtime/blob/v0.14.7/pkg/handler/enqueue_owner.go <-- this shows that and comments that out to describe its functionality.

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.

[MuneebAijaz]

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.

1. In this, CustomResource is re-queued, when controller gets an event from CRB or Namespace changes. But the drawback here is, CustomResource will get stuck in a loop with itself, because we add timestamps in CustomResource status, that means every patch to CustomResource status will generate an event to controller.

Owns(&corev1.Namespace{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, 
           predicate.LabelChangedPredicate{}, predicate.AnnotationChangedPredicate{}))).
Owns(&rbacv1.ClusterRoleBinding{}).
For(&v1beta2.CustomResource{}).

2. In this, CustomResource is re-queued when controller gets delete event from CRB or Namespace changes. The drawback is here is that, controller stops getting reconciled for update events for Namespace and ClusterRoleBinding. (this seems like a bug which i am concerned about. because in my understanding, predicate in FOR() is affecting OWNS() of Namespace.)

Owns(&corev1.Namespace{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, 
           predicate.LabelChangedPredicate{}, predicate.AnnotationChangedPredicate{}))).
Owns(&rbacv1.ClusterRoleBinding{}).
For(&v1beta2.CustomResource{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).

3. In this, CustomResource is re-queued as expected but issues comes up on updates for resources like ClusterRoleBinding, when there's no spec struct in the resource. In that case i stop getting update events for such resources and controller doesnt get reconciled. Because GenerationChangedPredicate acts on difference in spec.

Owns(&corev1.Namespace{}, builder.WithPredicates(predicate.Or( predicate.LabelChangedPredicate{}, 
           predicate.AnnotationChangedPredicate{}))).
Owns(&rbacv1.ClusterRoleBinding{}).
For(&v1beta2.CustomResource{}).
WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{})).

[troy0820]

In this, CustomResource is re-queued, when controller gets an event from CRB or Namespace changes. But the drawback here is, CustomResource will get stuck in a loop with itself, because we add timestamps in CustomResource status, that means every patch to CustomResource status will generate an event to controller.

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

In this, CustomResource is re-queued when controller gets delete event from CRB or Namespace changes. The drawback is here is that, controller stops getting reconciled for update events for Namespace and ClusterRoleBinding. (this seems like a bug which i am concerned about. because in my understanding, predicate in FOR() is affecting OWNS() of Namespace.)

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.

controller-runtime/pkg/predicate/predicate_test.go

Line 289 in 5615941

Describe("When checking a GenerationChangedPredicate", func() {

In this, CustomResource is re-queued as expected but issues comes up on updates for resources like ClusterRoleBinding, when there's no spec struct in the resource. In that case i stop getting update events for such resources and controller doesnt get reconciled. Because GenerationChangedPredicate acts on difference in spec

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

[MuneebAijaz]

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?

1. to get update events from namespace metadata
2. to get update events from resources like CRB which dont have spec
3. to ignore update events from CustomResource on status changes

Since no matter what prediate i put on CustomResource, events coming from other resources will be ignored as the CustomResource didnt change itself

[troy0820]

You can write your own handler to do the reconcile requests for the events you have for the crb and the namespace.

controller-runtime/pkg/handler/enqueue_mapped.go

Line 41 in b9f2982

func EnqueueRequestsFromMapFunc(fn MapFunc) EventHandler {

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.

[MuneebAijaz]

You can write your own handler to do the reconcile requests for the events you have for the crb and the namespace.

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 understand that it does at the moment, but change of the Owner object shouldnt be checked in events coming from Owned objects, no matter what predicate I put in For() (in my opinion). As the lifecycle of Owned and Owner resources can be very different, i can treat all of the resources separately and very precisely if this link can be broken. Based on that, i'd like to see the possibility of considering this as a feature request.

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.

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
https://pkg.go.dev/k8s.io/apimachinery@v0.26.11/pkg/apis/meta/v1#Condition

[troy0820]

I understand that it does at the moment, but change of the Owner object shouldnt be checked in events coming from Owned objects, no matter what predicate I put in For() (in my opinion). As the lifecycle of Owned and Owner resources can be very different, i can treat all of the resources separately and very precisely if this link can be broken. Based on that, i'd like to see the possibility of considering this as a feature request.

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.
#2351 (comment) talks about the pipeline.

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.

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.

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.

[MuneebAijaz]

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.

[alvaroaleman]

A predicate in For should not be global and only affect events that directly come from the object in For. Other objects that have their own handler should ignores settings that were passed in For. The only way to have a predicate for everything is to use WithEventFilter. From what I can tell from the code, that is what is happening at the moment:

controller-runtime/pkg/builder/controller.go

Lines 298 to 307 in bbe3bbe

	hdler := handler.EnqueueRequestForOwner(
	blder.mgr.GetScheme(), blder.mgr.GetRESTMapper(),
	blder.forInput.object,
	opts...,
	)
	allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...)
	allPredicates = append(allPredicates, own.predicates...)
	if err := blder.ctrl.Watch(src, hdler, allPredicates...); err != nil {
	return err
	}

@MuneebAijaz can you write a minimal reproducer for this, please?

[MuneebAijaz]

@alvaroaleman sure, i will get back to you with a small controller with mentioned conditions