Track restart count across resource versions

[gyfora]

Bug Report

Currently restart attempts in the controller are tracked/counted per resource and resource version. This seems to be good in most cases however in operator implementations that change the status/spec of the managed resource manually during reconciliation (through fabric8 client) it can cause situations where the controller loops infinitely with practically 0 timeout. (as there is no timeout at the first retry at all, regardless of the retry config).

I believe we should somehow track the attempts accross versions such as by the resource id itself (and expire attempts after some time or successful controller run)

ResourceID{name='my-resource', namespace='flink-test'}, version: 15427577412} failed
ResourceID{name='my-resource'', namespace='flink-test'}, version: 15427577640} failed

What did you expect to see?

Resource strategy applied correctly

What did you see instead? Under which circumstances?

Infinite retry loop with 0 timeout.

[gyfora]

cc @csviri , we discussed this symptom earlier, I managed to track down the root cause as explained here

[gyfora]

I think a reasonable strategy would be to by default track the attempts per spec version (resource generation) and add some logic to expire the attempt count after some time for a given generation.

[csviri]

@gyfora maybe I don't see something but it does not seems the resource is tracked by resource version, it is tracked by ResourceID(name, namespace). see: https://github.com/java-operator-sdk/java-operator-sdk/blob/2aa8dfe7399f918b58d24606822758e164c8c54e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ResourceStateManager.java#L12-L12

What happens probably is that you change a resource a new event arrives and for that the retry is basically ignored. See:

https://github.com/java-operator-sdk/java-operator-sdk/blob/2aa8dfe7399f918b58d24606822758e164c8c54e/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java#L302-L306

The rational behind this, that regardless of retry state, if a new event arrived the state of the system changed basically, or a state of the resource that might caused the error. Also the reconciler is triggered when a new event arrived, event if the retry is exhausted, since this event (or the change that induced the event) might be just a fix for the problem.

IMO, first of there should be a rate limiter set:
https://javaoperatorsdk.io/docs/features#rate-limiting
To prevent self DOS attacking, we did not put a default rate limiter, since that is quite use case specific (but we might reconsider that)

We might think how to prevent such scenario, but we have to rethink the retry mechanism.

[gyfora]

We change the resource indeed but only the status. But status events are normally "ignored" as we only reconcile on new spec generations so this doesn't sound to be consistent with the generationAware setting.

[gyfora]

I am not sure why status changes should fix any problems as the operator is the one managing the status :)

[csviri]

We discussed the problem with @gyfora in a call, and it seems this is the case what I mentioned here:

#2046 (comment)

And the only straightforward solution for this now is the rate limiter.
Will let this issue open, until we receive feedback that indeed that was the case, and solution helped.