Custom event filter for controllers #457

lburgazzoli · 2021-07-22T07:36:10Z

Fixes #404

lburgazzoli · 2021-07-22T07:52:58Z

@metacosm this PR is incomplete as it lacks some tests, however I'd like to discuss if it goes in the right direction because there are some behavioral changes:

the custom resource cache did not perform a copy of the resource in the cacheResource method, which could lead unexpected results (but I don't know it this was expected)
the latest generation value was cached as part of the CustomEventSource after a successful invocation of the event handler and I'm not sure about the reasoning behind that logic as I would assume that any retry logic should not be affected by the revision.

metacosm · 2021-07-22T07:59:33Z

...in/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java


-  @Override
-  public void eventSourceDeRegisteredForResource(String customResourceUid) {


Why was this method removed?

because the method was only about to remove the generation from the local cache, but since the cache does not exists anymore the method is useless (and the interface provide a default no-op method)

lburgazzoli · 2021-08-18T09:16:04Z

@csviri @metacosm what do you think about the PR (in general) ?

csviri · 2021-08-18T09:48:45Z

@csviri @metacosm what do you think about the PR (in general) ?

@lburgazzoli will try to take a look soon, like the idea of predicate very much! Will check deeper later.

lburgazzoli · 2021-08-18T09:50:09Z

@csviri @metacosm what do you think about the PR (in general) ?

@lburgazzoli will try to take a look soon, like the idea of predicate very much! Will check deeper later.

At the moment it is like a POC so, lot of things may be wrong :)

lburgazzoli · 2021-09-06T12:00:55Z

@csviri @metacosm I need to add some tests but it should be ready for review

metacosm · 2021-09-09T07:33:31Z

How do you propose to configure the predicate?

lburgazzoli · 2021-09-09T07:47:06Z

How do you propose to configure the predicate?

This is an advanced functionality which I don't think it should be made configurable as it depends on the specific spec/status and behavior of the operator so I think this should not be exposed as a configuration option (IMHO, the "generation aware" configuration option should not be exposed too)

metacosm · 2021-09-09T07:55:58Z

How do you propose to configure the predicate?

This is an advanced functionality which I don't think it should be made configurable as it depends on the specific spec/status and behavior of the operator so I think this should not be exposed as a configuration option (IMHO, the "generation aware" configuration option should not be exposed too)

I'm confused: I thought the whole point of this PR was to be able to configure the behavior with more than just a property or annotation value. Could you please explain the use case in greater details, then?

csviri · 2021-09-09T08:04:52Z

hi @lburgazzoli @metacosm sorry busy days, will try to take a look today

lburgazzoli · 2021-09-09T08:13:49Z

How do you propose to configure the predicate?

This is an advanced functionality which I don't think it should be made configurable as it depends on the specific spec/status and behavior of the operator so I think this should not be exposed as a configuration option (IMHO, the "generation aware" configuration option should not be exposed too)

I'm confused: I thought the whole point of this PR was to be able to configure the behavior with more than just a property or annotation value. Could you please explain the use case in greater details, then?

Oh sorry I though that with wow do you propose to configure the predicate you meant how to use a property to configure a predicate.

So far what I did is to use ControllerConfiguration upon registration so like

var operator = new Operator();
operator.register(new MyController(), new MyConfiguration() {
    @Override
    public CustomResourcePredicate<MyResource> getPredicate() {
        return ...;
    }
});

metacosm · 2021-09-09T09:20:41Z

OK, so that should part of the configuration overriding mechanism, then…

lburgazzoli · 2021-09-09T09:43:28Z

OK, so that should part of the configuration overriding mechanism, then…

will have a look later this week or beginning next week

lburgazzoli · 2021-09-09T13:59:44Z

OK, so that should part of the configuration overriding mechanism, then…

will have a look later this week or beginning next week

I've added support to the conf override, now I wonder if it still make sense to expose the option to configure the generation awarness

metacosm

Please rebase your PR so that all commits follow the conventional commit message format: https://www.conventionalcommits.org/en/v1.0.0/

metacosm · 2021-09-10T06:59:13Z

OK, so that should part of the configuration overriding mechanism, then…

will have a look later this week or beginning next week

I've added support to the conf override, now I wonder if it still make sense to expose the option to configure the generation awarness

I'm not sure either but then again we can't break the API before v2. That said, the other question I have is how someone using Quarkus where the controller is automatically registered would be able to specify their predicates and have it taken into account…

lburgazzoli · 2021-09-13T05:56:50Z

I'm not sure either but then again we can't break the API before v2. That said, the other question I have is how someone using Quarkus where the controller is automatically registered would be able to specify their predicates and have it taken into account…

What about something like:

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface Controller {
    Class<CustomResource> eventFilter default GenrationAwareFilter.class;
}

@Controller(
      eventFilter = MyCustomResourcePredicate.class
)

As alternative the controller should be able to set some config option in its init method (that would be my preferred option)

csviri · 2021-09-13T07:33:28Z

I'm not sure either but then again we can't break the API before v2. That said, the other question I have is how someone using Quarkus where the controller is automatically registered would be able to specify their predicates and have it taken into account…

What about something like:
@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.TYPE})
public @interface Controller {
    Class<CustomResource> eventFilter default GenrationAwareFilter.class;
}
@Controller(
      eventFilter = MyCustomResourcePredicate.class
)
As alternative the controller should be able to set some config option in its init method (that would be my preferred option)

@lburgazzoli @metacosm just to add my two cents.

should not be this a list of predicates in the annotation? So it's easy to compose more predicates.
we could consider having the generation awareness as a boolean (as it is now ) and just add the predicate in the background. The only reason to do this is just to have this nice explicit api about generations, since those are quite central concepts of event processing. While the other predicates will usually solve some special cases I guess.

csviri · 2021-09-19T13:05:34Z

...in/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java

+      // if the finalizer has not been set, the operator logic will add it so there will be
+      // events generated by the operator itself that won't lead to a new generation so such
+      // events should not be filtered
+      final CustomResourceEventFilter<T> finalizerPredicate =


Why are we checking this with predicate? It would be much simpler and easy to read with just checking directly the field with in a method.

So that the same logic is applied everywhere it's needed instead of having variations of the same code.

csviri · 2021-09-19T13:10:04Z

Back to the main topic of this PR:

In general I think if we could solve these cases elegantly with this predicates as now, it would be a nice way to implement things like generation awareness. On the other hand I see currently two issues:

The oldObject (thus the previouse version of CR) is used in the filter api, which might not be awailable in the future.
I still don't see the clear vision / use cases, if somebody wan't to test a new version of an operator it's typically we can already do in a separate namespace. Remember that if we introduce this API for filters we will have to support it. But without clear vision it could make some confusion. Is there something like this supported in controller runtime?

lburgazzoli · 2021-09-20T05:36:29Z

Back to the main topic of this PR:

In general I think if we could solve these cases elegantly with this predicates as now, it would be a nice way to implement things like generation awareness. On the other hand I see currently two issues:
* The oldObject (thus the previouse version of CR) is used in the filter api, which might not be awailable in the future.

I'm not sure about this point but it can eventually be done or maybe we can clearly document that the oldObject may be null in case of a cacheless controller. Eventually also the generation generation awarness can be delegated to the user by using the obeservedGeneration patter (implemented by a number of Kubernetes controllers) in which, in essence you have to store the generation the controller is act upon in the status.

As a side note, removing the oldObject would make the java sdk to drift from the design of the golang sdk (https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/)

* I still don't see the clear vision / use cases, if somebody wan't to test a new version of an operator it's typically we can already do in a separate namespace. Remember that if we introduce this API for filters we will have to support it. But without clear vision it could make some confusion. Is there something like this supported in controller runtime?

This issue/PR is not related to testing or canary deployment (that issue was the one about using label selector #453) but it is about letting the user be able to install advanced filter to reduce the reconcile events.

csviri · 2021-09-20T06:55:53Z

I'm not sure about this point but it can eventually be done or maybe we can clearly document that the oldObject may be null in case of a cacheless controller. Eventually also the generation generation awarness can be delegated to the user by using the obeservedGeneration patter (implemented by a number of Kubernetes controllers) in which, in essence you have to store the generation the controller is act upon in the status.

If we document that old object might be null, not sure if that would make sense even to put it there - Or we could also replicate the 3 methods in the filter (Create, Update, Delete), (if we are actually going to use informers in the background.)

This interface I guess comes from the fact that there is an informer used in the background and the old object is always cached.
(I mean this https://sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering/ )
So not sure if these filters are such tightly coupled with informers, it could be added to rabric8 client, and we could just make an adaptor for it (can't see it there atm).

The main problems with the oldObject. If the operator is restarted we don't have the old object anymore which is then a little bit smell, although also it's true that on restart we usually want to reconcile.

But on large clusters where there are thousands of custom resources we don't want to cache all the objects since it can happen that there might be just no related events coming for a very long time. In that case it might be more efficient to just read the CR from the k8s API. The plan for the future is that we will provide a configurable cache for this (like Caffein). So in this case the old object won't be available by definition.
We should have actually a discussion about from above, also if we should have multiple options for caches etc, to cache or not cache the objects. So very interested in your opinion @lburgazzoli also. But the design of this predicates also depends on that outcome.

lburgazzoli · 2021-09-20T07:14:17Z

I'm not sure about this point but it can eventually be done or maybe we can clearly document that the oldObject may be null in case of a cacheless controller. Eventually also the generation generation awarness can be delegated to the user by using the obeservedGeneration patter (implemented by a number of Kubernetes controllers) in which, in essence you have to store the generation the controller is act upon in the status.

If we document that old object might be null, not sure if that would make sense even to put it there - Or we could also replicate the 3 methods in the filter (Create, Update, Delete), (if we are actually going to use informers in the background.)

This interface I guess comes from the fact that there is an informer used in the background and the old object is always cached.
(I mean this sdk.operatorframework.io/docs/building-operators/golang/references/event-filtering )
So not sure if these filters are such tightly coupled with informers, it could be added to rabric8 client, and we could just make an adaptor for it (can't see it there atm).

The main problems with the oldObject. If the operator is restarted we don't have the old object anymore which is then a little bit smell, although also it's true that on restart we usually want to reconcile.

But on large clusters where there are thousands of custom resources we don't want to cache all the objects since it can happen that there might be just no related events coming for a very long time. In that case it might be more efficient to just read the CR from the k8s API. The plan for the future is that we will provide a configurable cache for this (like Caffein). So in this case the old object won't be available by definition.
We should have actually a discussion about from above, also if we should have multiple options for caches etc, to cache or not cache the objects. So very interested in your opinion @lburgazzoli also. But the design of this predicates also depends on that outcome.

Fine with me, maybe we can have an ad-hoc meeting for this discussion ?

csviri · 2021-09-20T07:28:11Z

Fine with me, maybe we can have an ad-hoc meeting for this discussion ?

Definitely, that would be the best IMHO.

lburgazzoli · 2021-09-20T12:51:06Z

Converted to a draft until we get consensus

csviri · 2021-09-21T12:58:44Z

...in/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilter.java

+  /**
+   * Determines whether the change between the old version of the resource and the new one needs to
+   * be propagated to the controller or not.
+   *


Should we have filter on Creates(Deletes)?

lburgazzoli · 2021-09-29T07:02:11Z

@csviri @metacosm pr is now ready for review

csviri · 2021-09-29T07:27:17Z

@lburgazzoli will take a look today, after finished my current issue

csviri · 2021-09-29T13:28:37Z

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/CustomResourceCache.java

    try {
      lock.lock();
      final var uid = getUID(resource);
      if (predicate.test(resources.get(uid))) {
        if (passthrough != predicate) {
          log.trace("Update cache after condition is true: {}", getName(resource));
        }
-        resources.put(uid, resource);
+        // defensive copy
+        resources.put(getUID(resource), clone(resource));


why are we cloning here?

The reason is that as example, if for some reason the process that applies the finalizer fails when updating the object, then the cache would still reporting the object as having the finalizer which is not correct.

it probably won't matter so if it should be removed, I ca do it in a subsequent pr

csviri · 2021-09-29T14:20:38Z

...n/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilters.java

+      (configuration, oldResource, newResource) -> {
+        if (configuration.useFinalizer()) {
+          final var finalizer = configuration.getFinalizer();
+          boolean oldFinalizer = oldResource == null || oldResource.hasFinalizer(finalizer);


It would be good to write unit tests for these methods, since there is non-trivial logic. But it can be a separate PR.

metacosm reviewed Jul 22, 2021

View reviewed changes

lburgazzoli mentioned this pull request Aug 19, 2021

Custom event filter for controllers #404

Closed

lburgazzoli force-pushed the gh-404 branch 2 times, most recently from 097933b to b29157c Compare August 20, 2021 11:29

lburgazzoli force-pushed the gh-404 branch from b29157c to 713d839 Compare September 6, 2021 07:58

lburgazzoli marked this pull request as ready for review September 6, 2021 08:03

lburgazzoli marked this pull request as draft September 6, 2021 10:50

lburgazzoli force-pushed the gh-404 branch 3 times, most recently from ebca1db to 326a3ad Compare September 6, 2021 12:00

lburgazzoli marked this pull request as ready for review September 6, 2021 12:00

lburgazzoli force-pushed the gh-404 branch from 326a3ad to 63e024e Compare September 9, 2021 13:58

metacosm reviewed Sep 10, 2021

View reviewed changes

lburgazzoli force-pushed the gh-404 branch from 63e024e to 6c62e31 Compare September 13, 2021 05:50

csviri reviewed Sep 19, 2021

View reviewed changes

lburgazzoli marked this pull request as draft September 20, 2021 12:50

csviri reviewed Sep 21, 2021

View reviewed changes

lburgazzoli and others added 10 commits September 29, 2021 06:47

feat: custom event filter for controllers operator-framework#404

d0582ca

feat: add and/or combination methods on CustomResourceEventFilters

ff9ba8d

fix: make filters take configuration into account

da3437d

fix: make and/or method work if other filter is null

3d849dc

refactor: use useFinalizer filter logic

ad84c2f

refactor: simplify logic using and operator

c136f09

refactor: rename test method to acceptChange to be more explicit

9e02044

docs: document CustomResourceEventFilter

c0b8f5a

refactor: rename useFinalizer to finalizerNeededAndApplied

5d60b4a

chore: adapt to latest changes

8dd8302

lburgazzoli force-pushed the gh-404 branch from 93ad879 to 8dd8302 Compare September 29, 2021 06:00

lburgazzoli marked this pull request as ready for review September 29, 2021 07:01

metacosm approved these changes Sep 29, 2021

View reviewed changes

csviri reviewed Sep 29, 2021

View reviewed changes

csviri approved these changes Sep 29, 2021

View reviewed changes

csviri reviewed Sep 29, 2021

View reviewed changes

csviri merged commit 0dc0de1 into operator-framework:master Sep 29, 2021

lburgazzoli deleted the gh-404 branch September 29, 2021 14:30

shawkins mentioned this pull request Oct 8, 2021

Status update triggers reconciliation with finalizers disabled #595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom event filter for controllers #457

Custom event filter for controllers #457

lburgazzoli commented Jul 22, 2021 •

edited

Loading

lburgazzoli commented Jul 22, 2021

metacosm Jul 22, 2021

lburgazzoli Jul 22, 2021

lburgazzoli commented Aug 18, 2021 •

edited

Loading

csviri commented Aug 18, 2021

lburgazzoli commented Aug 18, 2021

lburgazzoli commented Sep 6, 2021

metacosm commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

metacosm commented Sep 9, 2021

csviri commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021 •

edited

Loading

metacosm commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

metacosm left a comment

metacosm commented Sep 10, 2021

lburgazzoli commented Sep 13, 2021 •

edited

Loading

csviri commented Sep 13, 2021

csviri Sep 19, 2021

metacosm Sep 21, 2021

csviri commented Sep 19, 2021 •

edited

Loading

lburgazzoli commented Sep 20, 2021

csviri commented Sep 20, 2021 •

edited

Loading

lburgazzoli commented Sep 20, 2021

csviri commented Sep 20, 2021

lburgazzoli commented Sep 20, 2021

csviri Sep 21, 2021

lburgazzoli Sep 29, 2021

lburgazzoli commented Sep 29, 2021

csviri commented Sep 29, 2021

csviri Sep 29, 2021

lburgazzoli Sep 29, 2021

csviri Sep 29, 2021 •

edited

Loading


		@Override
		public void eventSourceDeRegisteredForResource(String customResourceUid) {

Custom event filter for controllers #457

Custom event filter for controllers #457

Conversation

lburgazzoli commented Jul 22, 2021 • edited Loading

lburgazzoli commented Jul 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli commented Aug 18, 2021 • edited Loading

csviri commented Aug 18, 2021

lburgazzoli commented Aug 18, 2021

lburgazzoli commented Sep 6, 2021

metacosm commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

metacosm commented Sep 9, 2021

csviri commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021 • edited Loading

metacosm commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

lburgazzoli commented Sep 9, 2021

metacosm left a comment

Choose a reason for hiding this comment

metacosm commented Sep 10, 2021

lburgazzoli commented Sep 13, 2021 • edited Loading

csviri commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Sep 19, 2021 • edited Loading

lburgazzoli commented Sep 20, 2021

csviri commented Sep 20, 2021 • edited Loading

lburgazzoli commented Sep 20, 2021

csviri commented Sep 20, 2021

lburgazzoli commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lburgazzoli commented Sep 29, 2021

csviri commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

lburgazzoli commented Jul 22, 2021 •

edited

Loading

lburgazzoli commented Aug 18, 2021 •

edited

Loading

lburgazzoli commented Sep 9, 2021 •

edited

Loading

lburgazzoli commented Sep 13, 2021 •

edited

Loading

csviri commented Sep 19, 2021 •

edited

Loading

csviri commented Sep 20, 2021 •

edited

Loading

csviri Sep 29, 2021 •

edited

Loading