PollingEventSource should trigger reconcile when a secondary resource is missing

[scrocquesel]

Bug Report

I'm was using PerResourcePollingEventSource but when having a lot of primary resources, it can lead to a lot of presure on the external system.
I refactor around a PollingEventSource to batch external resources get but the difference of behavior make it is not a drop in replacement.

What did you do?

1. Create an operator that will create an external secondary resource with an ObservedGenerationAwareStatus.
2. Add the primary resource to the cluster and start the operator: External resource is created.
3. Stop the operator, delete the external resource
4. Start the operator. Reconcile is never called. The external resource can't be recreated.

If I create manually the secondary resource and wait for the polling period to grab the new secondary resource, reconcile is triggered, and if the secondary resource is then manually deleted, it will also trigger the reconcile on next polling.

What did you expect to see?

At startup, reconcile should be called for each primary resource not in the map returned by the PollingEventSource supplier.

Ideally, PollingEventSource and PerResourcePollingEventSource should be an implementation details and should not change the way the reconciler works.

[csviri]

Hi @scrocquesel

If I create manually the secondary resource and wait for the polling period to grab the new secondary resource, reconcile is triggered, and if the secondary resource is then manually deleted, it will also trigger the reconcile on next polling.

This is an expected behavior, event should be propagated if a dependent resource is deleted. It's not clear for if you meant this as a problem?

At startup, reconcile should be called for each primary resource not in the map returned by the PollingEventSource supplier.

Do I understand correctly, you mean that basically a triggering event should be propagated on startup, since the dependent resource was deleted meanwhile in the background?

(Sorry just try to understand what do you mean.)

[scrocquesel]

Hi @scrocquesel

If I create manually the secondary resource and wait for the polling period to grab the new secondary resource, reconcile is triggered, and if the secondary resource is then manually deleted, it will also trigger the reconcile on next polling.

This is an expected behavior, event should be propagated if a dependent resource is deleted. It's not clear for if you meant this as a problem?

No this is indeed expected.

At startup, reconcile should be called for each primary resource not in the map returned by the PollingEventSource supplier.

Do I understand correctly, you mean that basically a triggering event should be propagated on startup, since the dependent resource was deleted meanwhile in the background?

(Sorry just try to understand what do you mean.)

Yes, otherwise it is not possible to recreate the missing dependent resource. The only way to trigger reconciliation is to update the primary resource.

[csviri]

@scrocquesel one way to solve this is set generationAwareEventProcessing to false. So on startup everything will be reconciled.

Yes, otherwise it is not possible to recreate the missing dependent resource. The only way to trigger reconciliation is to update the primary resource.

The problem is that this would not even be consistent with the Informers (Neither with PerResourcePollingEventSource) . So if an operator is down, and a k8s resources is deleted meanwhile. On startup the informer does not propagate an event.

Also what you are saying is not equivalent to: on startup there should be an event for every resource?

[scrocquesel]

The problem is that this would not even be consistent with the Informers (Neither with PerResourcePollingEventSource) . So if an operator is down, and a k8s resources is deleted meanwhile. On startup the informer does not propagate an event.

Was pretty sure it was the case for PerResourcePollingEventSource. But i'm not reproducing it with only the PerResourcePollingEventSource . But when used with an InformerEventSource on Secret, the reconcile is triggered at startup while the secret do not change.

2022-02-01 20:17:38,013 INFO  [io.jav.ope.Operator] (Quarkus Main Thread) Operator SDK 2.0.2 (commit: ba03d9e) built on Thu Jan 20 11:22:23 CET 2022 starting...
2022-02-01 20:17:38,013 INFO  [io.jav.ope.Operator] (Quarkus Main Thread) Client version: 5.10.2
2022-02-01 20:17:38,014 DEBUG [io.jav.ope.api.con.ExecutorServiceManager] (Quarkus Main Thread) Initialized ExecutorServiceManager executor: class java.util.concurrent.ThreadPoolExecutor, timeout: 10
2022-02-01 20:17:38,038 DEBUG [io.jav.ope.pro.eve.EventSourceManager] (Quarkus Main Thread) Starting event sources.
2022-02-01 20:17:38,039 DEBUG [io.fab.kub.cli.inf.imp.DefaultSharedIndexInformer] (Quarkus Main Thread) informer: ready to run resync and reflector for class com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.User with resync 0
2022-02-01 20:17:38,040 DEBUG [io.fab.kub.cli.inf.imp.DefaultSharedIndexInformer] (Quarkus Main Thread) informer#Controller: resync skipped due to 0 full resync period class com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.User
2022-02-01 20:17:38,041 WARN  [io.fab.kub.cli.int.VersionUsageUtils] (Quarkus Main Thread) The client is using resource type 'users' with unstable version 'v1alpha1'
2022-02-01 20:17:38,056 DEBUG [io.fab.kub.cli.inf.cac.Reflector] (Quarkus Main Thread) Listing items (1) for resource class com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.User v981115
2022-02-01 20:17:38,058 DEBUG [io.jav.ope.pro.eve.sou.con.ControllerResourceEventSource] (Quarkus Main Thread) Event received for resource: test
2022-02-01 20:17:38,061 DEBUG [io.jav.ope.pro.eve.sou.con.ControllerResourceEventSource] (Quarkus Main Thread) Skipping event handling resource a0c018aa-5fc1-41b8-a89a-b2ed82293a1b with version: 950969
2022-02-01 20:17:38,061 DEBUG [io.jav.ope.pro.eve.sou.pol.PerResourcePollingEventSource] (Timer-1) Event source not yet started. Will not run for: CustomResourceID{name='test', namespace='default'}
2022-02-01 20:17:38,062 DEBUG [io.fab.kub.cli.inf.cac.Reflector] (Quarkus Main Thread) Starting watcher for resource class com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.User v981115
2022-02-01 20:17:38,066 DEBUG [io.fab.kub.cli.dsl.int.AbstractWatchManager] (Quarkus Main Thread) Watching https://kubernetes.docker.internal:6443/apis/foo.bar/v1alpha1/users?resourceVersion=981115&watch=true...
2022-02-01 20:17:38,115 DEBUG [io.fab.kub.cli.dsl.int.WatcherWebSocketListener] (OkHttp https://kubernetes.docker.internal:6443/...) WebSocket successfully opened
2022-02-01 20:17:38,116 DEBUG [io.jav.ope.pro.eve.sou.con.ControllerResourceEventSource] (Quarkus Main Thread) Registered 'userreconciler' Controller -> io.fabric8.kubernetes.client.informers.impl.DefaultSharedIndexInformer@494f7637 for any namespace
2022-02-01 20:17:38,116 DEBUG [io.fab.kub.cli.inf.imp.DefaultSharedIndexInformer] (Quarkus Main Thread) informer: ready to run resync and reflector for class io.fabric8.kubernetes.api.model.Secret with resync 0
2022-02-01 20:17:38,117 DEBUG [io.fab.kub.cli.inf.imp.DefaultSharedIndexInformer] (Quarkus Main Thread) informer#Controller: resync skipped due to 0 full resync period class io.fabric8.kubernetes.api.model.Secret
2022-02-01 20:17:38,126 DEBUG [io.fab.kub.cli.inf.cac.Reflector] (Quarkus Main Thread) Listing items (1) for resource class io.fabric8.kubernetes.api.model.Secret v981115
2022-02-01 20:17:38,128 DEBUG [io.jav.ope.pro.eve.EventProcessor] (Quarkus Main Thread) Received event: Event{relatedCustomResource=CustomResourceID{name='test', namespace='default'}}
2022-02-01 20:17:38,128 DEBUG [io.jav.ope.pro.eve.EventProcessor] (Quarkus Main Thread) Skipping event: Event{relatedCustomResource=CustomResourceID{name='test', namespace='default'}} because the event processor is not started
2022-02-01 20:17:38,129 DEBUG [io.fab.kub.cli.inf.cac.Reflector] (Quarkus Main Thread) Starting watcher for resource class io.fabric8.kubernetes.api.model.Secret v981115
2022-02-01 20:17:38,129 DEBUG [io.fab.kub.cli.dsl.int.AbstractWatchManager] (Quarkus Main Thread) Watching https://kubernetes.docker.internal:6443/api/v1/secrets?labelSelector=app.kubernetes.io%2Fmanaged-by%3Drabbitmq-messaging-topology-operator&resourceVersion=981115&watch=true...
2022-02-01 20:17:38,166 DEBUG [io.fab.kub.cli.dsl.int.WatcherWebSocketListener] (OkHttp https://kubernetes.docker.internal:6443/...) WebSocket successfully opened
2022-02-01 20:17:38,183 DEBUG [io.jav.ope.pro.eve.EventProcessor] (Quarkus Main Thread) Executing events for custom resource. Scope: ExecutionScope{ resource id: CustomResourceID{name='test', namespace='default'}, version: 950969}
2022-02-01 20:17:38,185 DEBUG [io.jav.ope.pro.eve.ReconciliationDispatcher] (EventHandler-userreconciler) Handling dispatch for resource test
2022-02-01 20:17:38,187 DEBUG [io.jav.ope.pro.eve.ReconciliationDispatcher] (EventHandler-userreconciler) Reconciling resource test with version: 950969 with execution scope: ExecutionScope{ resource id: CustomResourceID{name='test', namespace='default'}, version: 950969}
2022-02-01 20:17:38,301 INFO  [io.quarkus] (Quarkus Main Thread) rabbitmq-messaging-topology-operator 1.0.0-SNAPSHOT on JVM (powered by Quarkus 2.6.3.Final) started in 4.147s. Listening on: http://0.0.0.0:8080
2022-02-01 20:17:38,302 INFO  [io.quarkus] (Quarkus Main Thread) Profile dev activated. Live Coding activated.
2022-02-01 20:17:38,302 INFO  [io.quarkus] (Quarkus Main Thread) Installed features: [cdi, kubernetes, kubernetes-client, openshift-client, operator-sdk, rest-client, rest-client-jackson, smallrye-context-propagation, smallrye-health, vertx]
2022-02-01 20:17:38,307 INFO  [com.foo.rab.UserReconciler] (EventHandler-userreconciler) Create new user doe-test
2022-02-01 20:17:38,313 INFO  [com.foo.rab.rab.com.CommandUser] (EventHandler-userreconciler) Creating user { name='doe-test', password='', tags=''}
2022-02-01 20:17:38,358 DEBUG [io.jav.ope.pro.eve.EventProcessor] (EventHandler-userreconciler) Event processing finished. Scope: ExecutionScope{ resource id: CustomResourceID{name='test', namespace='default'}, version: 950969}, PostExecutionControl: PostExecutionControl{onlyFinalizerHandled=false, updatedCustomResource=CustomResource{kind='User', apiVersion='foo.bar/v1alpha1', metadata=ObjectMeta(annotations={kubectl.kubernetes.io/last-applied-configuration={"apiVersion":"foo.bar/v1alpha1","kind":"User","metadata":{"annotations":{},"name":"test","namespace":"default"},"spec":{"username":"doe"}}
}, clusterName=null, creationTimestamp=2022-01-30T00:12:43Z, deletionGracePeriodSeconds=null, deletionTimestamp=null, finalizers=[users.foo.bar/finalizer], generateName=null, generation=6, labels=null, managedFields=[ManagedFieldsEntry(apiVersion=foo.bar/v1alpha1, fieldsType=FieldsV1, fieldsV1=FieldsV1(additionalProperties={f:metadata={f:annotations={.={}, f:kubectl.kubernetes.io/last-applied-configuration={}}}, f:spec={.={}, f:username={}}}), manager=kubectl-client-side-apply, operation=Update, subresource=null, time=2022-01-30T00:12:43Z, additionalProperties={}), ManagedFieldsEntry(apiVersion=foo.bar/v1alpha1, fieldsType=FieldsV1, fieldsV1=FieldsV1(additionalProperties={f:metadata={f:finalizers={.={}, v:"users.foo.bar/finalizer"={}}}}), manager=okhttp, operation=Update, subresource=null, time=2022-01-30T00:12:43Z, additionalProperties={}), ManagedFieldsEntry(apiVersion=foo.bar/v1alpha1, fieldsType=FieldsV1, fieldsV1=FieldsV1(additionalProperties={f:status={.={}, f:conditions={}, f:observedGeneration={}, f:observedSpec={.={}, f:username={}}}}), manager=okhttp, operation=Update, subresource=status, time=2022-01-30T16:38:25Z, additionalProperties={})], name=test, namespace=default, ownerReferences=[], resourceVersion=950969, selfLink=null, uid=a0c018aa-5fc1-41b8-a89a-b2ed82293a1b, additionalProperties={}), spec=com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.UserSpec@13f3691b, status=com.foo.rabbitmqmessagingtopologyoperator.api.v1alpha1.UserStatus@320f8d2e}, runtimeException=null}

2022-02-01 20:17:38,359 DEBUG [io.jav.ope.pro.eve.EventProcessor] (EventHandler-userreconciler) Cleanup for successful execution for resource: test

Also what you are saying is not equivalent to: on startup there should be an event for every resource?

As you said, this is the expected behavior when generationAwareEventProcessing is false. It shouldn't be a big overhead to do it even when it is true.

Somehow, we must reconcile with the dependent resource that has been deleted when the operator was down.

Modified resource when the operator is down works. As far as I understand, it triggers reconcile not on startup but when the next polling occured. I guess that as the cache is empty, it detect that there is now an object and it is different.
Maybe for missing dependent resource at startup, we should make a difference between nothing in the cache and an empty Optional.

[scrocquesel]

// we only propagate event if the resource was previously in cache
if (cachedValue.isPresent()) {
getEventHandler().handleEvent(new Event(relatedResourceID));
}

I think that it should make the difference between knowing the resource doesn't exist (poller return Optional.empty() and when it is unkown like at startup and the cache is really empty. Something like

if (cachedValue.isPresent()) {
    if (cachedValue.get().isPresent()) {
      // we know for sure the resource doesn't exist because we poll for it
      getEventHandler().handleEvent(new Event(relatedResourceID));
   }
}else {
 // previous state is unknown, we never poll for it
 getEventHandler().handleEvent(new Event(relatedResourceID));
}

That is the cache should be a protected UpdatableCache<Optional<T>>.

I did a quick and dirty dev so it works as I expected for PerResourcePollingEventSource : scrocquesel@8f80ac3

But I don't know all the picture and there may be cons I don't see like memory overhead.

[csviri]

But when used with an InformerEventSource on Secret, the reconcile is triggered at startup while the secret do not change.

yes, because the secret was there. If it would be deleted meanwhile, it would not trigger the reconciliation.

Modified resource when the operator is down works. As far as I understand, it triggers reconcile not on startup but when the next polling occured. I guess that as the cache is empty, it detect that there is now an object and it is different.
Maybe for missing dependent resource at startup, we should make a difference between nothing in the cache and an empty Optional.

I did a quick and dirty dev so it works as I expected for PerResourcePollingEventSource : scrocquesel/java-operator-sdk@8f80ac3

The problem is that you don't know if it was deleted or it's just not created yet. The modified works because it's there, basically that is reported. TBH I don't see how this should help in general, you need to know the resourceID already, but isn't it comming from the polled resource / poller?

In case the operator is replicated (we don't support it yet), this should not be the problem.

But this could be an issue also with Informers on deleted objects, so probably generationAwareEventProcessing should be turned off by default.

Will create a PR to not delay the PollingEventSource on startup, but for now I think what you should do is just set this to false.

Anyways, thanks very much to pointing this out!

[csviri]

This is loosely related: #901

[sclorng]

The problem is that you don't know if it was deleted or it's just not created yet.

From the reconcile pov, it make no difference. We are not doing reconciliation based on add/delete/modify delta operation. And it will not be possible to pass such an operation when calling reconcile. to me, handleDelete and handleEvent are only for managing the cache and decide if the reconcile should be triggered. I see three use case:

• we know for sure something change (being added, modified, deleted) and it should trigger an event.
• we know for sure nothing change (equals return true or the resource was known to be not present and is still not present) and it should not trigger an event.
• In between, when uncertain because the cache entry was empty and it cannot decide from a previous seen state, it should trigger the reconcile and let the reconcilier do its logic.

As you said, maybe the easiest way is to trigger reconcile for all resources at startup to allow the cache to be populated with a reconciliated observed state of dependent resources.
I was focusing on delete but the same happen with modified. At startup, we cannot be sure if the dependent resource has been modified/added manually or not during shutdown.

[csviri]

@sclorng I completely agree, form the comment before.

@sclorng @scrocquesel So again not sure we are understand each other,(happy to make a call on our discord server or somewhere to discuss, them make a summery under this issues), but:

Made a change under this PR: ​#901

So from now on startup PollingEventSource will synchronize, that means that if you check the resource on the reconciliation on startup, if the resource is not there it means it was not created yet, see:
http://github.com/java-operator-sdk/java-operator-sdk/blob/5bd303efb55b2206a87d6ab74878f418f4717a5d/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java#L47-L47

Also added this method:
http://github.com/java-operator-sdk/java-operator-sdk/blob/52267272b4e6ed7e71e644653751c8dd22506bf4/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/polling/PollingEventSource.java#L67-L67

So in my opinion the workflow should be the following (after this feature is merged, will be released today to tomorrow):

1. Reconcile all the resources on starup
2. In Reconciler implementation check if the PollingEventSource has the resource in cache. If not create the resource.
3. If the resource was created, put the just created resource in the cache manually. (So if reconciled before it's polled, we know it's already there) This manual step is very annoying now, will address this in the upcoming DependentResources feature.

Do you agree with this, does this solve the problem that we are in kinda intermediate state? If yes will add it also into (java)docs for now.

Sorry, this was a relatively new stuff, I exactly anticipated feedback like this. So thank you again. And happy to discuss also in a call in depth.

[sclorng]

3. If the resource was created, put the just created resource in the cache manually. (So if reconciled before it's polled, we know it's already there) This manual step is very annoying now, will address this in the upcoming DependentResources feature.

For the manual step, this is the same issue as #870. Even if we don't put it in the cache, it will latter poll and eventually reconcile to sync. The important thing to document is that the cache should be called once per unit of work for each resource as it can't keep track of what's being done during this unit of work. UoW = reconcile or cleanup.

Do you agree with this, does this solve the problem that we are in kinda intermediate state? If yes will add it also into (java)docs for now.
Agree with the logic.

Wouldn't it be better to lazy populate the cache ? When calling getAssociated if the cache has not yet been getStateAndFillCache, then we should call the method ? To me, this is what happen for PerResourcePolling.

[csviri]

For the manual step, this is the same issue as #870. Even if we don't put it in the cache, it will latter poll and eventually reconcile to sync. The important thing to document is that the cache should be called once per unit of work for each resource as it can't keep track of what's being done during this unit of work. UoW = reconcile or cleanup.

Yes, it is very similar, but it serves also a different purpose. In #870 it is mainly to not propagate the event. On polling event source this is an aspect too. Bu in PollingEventSource is an also more probabilistic situation in place. If you don't populate itmanually, there can be a new reconciliation between the current reconciliation and the next poll (note the time period) that will actually propagate the cache. If that happens the reconciler will again look into the cache and the cache will be still empty.

Wouldn't it be better to lazy populate the cache ? When calling getAssociated if the cache has not yet been getStateAndFillCache, then we should call the method ? To me, this is what happen for PerResourcePolling.

For PolliongEventSource is a little bit different situtation. Since one poll does not handle multiple resources. Imaginge that you have hundreads of resources, on startup PollingEventSource would be called 100x times (the poll method). But the whole point of the polling event source is to optimize this for API's that allow that. So no, it does not make sense there.

[sclorng]

For PolliongEventSource is a little bit different situtation. Since one poll does not handle multiple resources. Imaginge that you have hundreads of resources, on startup PollingEventSource would be called 100x times (the poll method). But the whole point of the polling event source is to optimize this for API's that allow that. So no, it does not make sense there.

I was thinking of doing that only the first call to the cache. Then, a flag would say it has poll at least once so subsequent call will rely on this state (or the update one by the timer). So it is just delaying the first getStateAndFillCache until it is really needed. Then, all others call will be done by the timer only.
I'm thinking about this when you have more than one dependent resource and you will only need the cache of PollingEventSource if one of the other resource are in a given state. So maybe, when you really need it, the timer will have trigger.

[csviri]

I was thinking of doing that only the first call to the cache. Then, a flag would say it has poll at least once so subsequent call will rely on this state (or the update one by the timer). So it is just delaying the first getStateAndFillCache until it is really needed. Then, all others call will be done by the timer only. I'm thinking about this when you have more than one dependent resource and you will only need the cache of PollingEventSource if one of the other resource are in a given state. So maybe, when you really need it, the timer will have trigger.

Well, yes that is an interesting idea, it could be optimized this way. Not sure if in real life scenarios helps that much, so you eventually will have a custom resource in that state. And on an operator restart it would be good to poll before first sync as done now in that PR. But not against it. Feel free to create a PR, maybe a separate issue, to handle that.