fix: do not update cache if the execution failed

[metacosm]

Should fix #310

[ppatierno]

@metacosm I have tested this running the CustomService based example but removing the Namespaced implementation from it which drives the exception I raised in #310.
The fix doesn't work because that code is never reached in the eventProcessingFinished, because the following condition is true and return:

if (retry != null && postExecutionControl.exceptionDuringExecution()) {
        handleRetryOnException(executionScope);
        return;
      }

It seems to be right because in the end due to an exception, the operator sdk has to retry.
The problem I see is that on retry, it's lost the reason why the previous execution was failed.
Could it be useful to save the postExecutionControl instance in the current executionScope as "previous exception in case of retry" so that on next handleDispatch (or handleCreateOrUpdate) we have that in the execution scope and we can evaluate the customResourceUpdatedDuringExecution at the right time?
It's just the first idea came to my mind, maybe there is a different approach for you who knows the framework better than me.

[ppatierno]

@metacosm actually I tried a different way of thinking an opened this PR #320

[csviri]

The problem is that we add the finalizer to the cached object, and even if the update fails the object itself remains changed.
IMHO this last PR would fix this particular problem, but I can imagine that this could cause problems elsewhere, since we are using just the instance from cache everywhere.
So the proper solution would be to clone the object when reading it from cache, so the cache always returns a clone of the custom resource not the instance in the cache itself. So even there is an exception in the dispatcher, we make sure that we use an instance that is not changed by us, but a version that represents the state in the API.

[csviri]

#321
@ppatierno @metacosm what do you think?
@ppatierno could you test please if this solves the problem?

[metacosm]

@csviri I'd like to revisit with you the decision to use a cache because I'm still not convinced we need one and I'm afraid it might cause more issues than it will solve…

[ppatierno]

At the beginning I thought that the cache was need for handling events for resources with newer generation only then I saw that this kind of skip is made by using this lastGenerationProcessedSuccessfully map. I would know why the resource cache is needed as well.

[csviri]

@ppatierno @metacosm we can organize a call, happy to discuss the details. Its needed also for the generation aware processing also, since we are doing optimistic version control, but that is a details. The main idea is:

If we execute the controller, the custom resource is always received as a parameter in the controller, this naturally needed, since we would like to update its status, but mainly we always reconcile based on specs of a custom resource. Since we are not just listening to the changes of the custom resource itself, but we can have other event sources ( where we execute the controller if we receive event from different source - and want to provide the CR also in this case ), for this case we use the cache with the latest resource, since we are already watching for he changes for that resource its quite natural to cache it (not to call always the K8S Api for a fresh one).

[csviri]

@ppatierno does the last PR fix your issue? I would say we should merge that in case does. Event if we want to change the behavior with caches that is something for a long term.

[ppatierno]

@csviri yes I tested it yesterday and it fixed this case.