Resource updates during reconciliation causes status update error

[gyfora]

Bug Report

What did you do?

While running the Flink Kubernetes Operator we started seeing the following errors in the log:

2022-05-06 03:06:04,960 i.j.o.p.e.ReconciliationDispatcher [ERROR Error during error status handling.
io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: PUT at: https://address/apis/flink.apache.org/v1beta1/namespaces/myns/flinkdeployments/flink-job/status. Message: Operation cannot be fulfilled on flinkdeployments.flink.apache.org "flink-job": the object has been modified; please apply your changes to the latest version and try again. Received status: Status(apiVersion=v1, code=409, details=StatusDetails(causes=[], group=flink.apache.org, kind=flinkdeployments, name=flink-job, retryAfterSeconds=null, uid=null, additionalProperties={}), kind=Status, message=Operation cannot be fulfilled on flinkdeployments.flink.apache.org "flink-job": the object has been modified; please apply your changes to the latest version and try again, metadata=ListMeta(_continue=null, remainingItemCount=null, resourceVersion=null, selfLink=null, additionalProperties={}), reason=Conflict, status=Failure, additionalProperties={}).
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.requestFailure(OperationSupport.java:682)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.requestFailure(OperationSupport.java:661)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.assertResponseCode(OperationSupport.java:612)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.handleResponse(OperationSupport.java:555)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.handleResponse(OperationSupport.java:518)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.handleUpdate(OperationSupport.java:342)
        at io.fabric8.kubernetes.client.dsl.base.OperationSupport.handleUpdate(OperationSupport.java:322)
        at io.fabric8.kubernetes.client.dsl.base.BaseOperation.handleUpdate(BaseOperation.java:649)
        at io.fabric8.kubernetes.client.dsl.base.BaseOperation.updateStatus(BaseOperation.java:504)
        at io.fabric8.kubernetes.client.dsl.base.BaseOperation.updateStatus(BaseOperation.java:83)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher$CustomResourceFacade.updateStatus(ReconciliationDispatcher.java:323)
        at java.base/java.util.Optional.ifPresent(Optional.java:183)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleErrorStatusHandler(ReconciliationDispatcher.java:183)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleReconcile(ReconciliationDispatcher.java:111)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleDispatch(ReconciliationDispatcher.java:74)
        at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleExecution(ReconciliationDispatcher.java:50)
        at io.javaoperatorsdk.operator.processing.event.EventProcessor$ControllerExecution.run(EventProcessor.java:349)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:829)

The managed resources never interact with each other, and they only ever update the status (never the spec) of the managed resource.

We could reproduce the issue by sending an update using kubectl apply during a longer reconciliation step.

What did you expect to see?

I would expect status updates to go through even if the user appled a spec change in the meantime, as status has nothing to do with what the user has changed.

These errors can make the operator logic much more fragile.

Environment

Kubernetes cluster type:
minikube

$ kubectl version
1.23.3

[metacosm]

Which version of the SDK are you using? Does your CRD define the status sub-resource?

[gyfora]

SDK version: 2.1.2
https://github.com/apache/flink-kubernetes-operator/blob/main/flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/FlinkDeployment.java

[scrocquesel]

status is part of the revision of the resource. A reconcile loop reconcile a revision and should update the status of the same revision. If the spec change, a conflict occurs and the reconciler will loop on the new revision and should update the status successfully.
May be related to #1156

[gyfora]

I understand the underlying logic here but unfortunately our operator does some quite heavy operations (that can often take minutes even).

Some of these are not retriable under some circumstances and if not persisted to the status require manual user intervention. In these cases we would much prefer to simply update the status for whatever latest revision there is instead of failing because that is much worse from the user perspective.

[scrocquesel]

You should be able to update the status during reconcile loop. Just reuse the resource returned by the fabricio client to do subsequent update and use it in the UpdateControl return result.
If you do the update yourself, you can avoid revision check, see #970 (comment)

[gyfora]

Interesting, I will take a look, maybe for our use-case it is better to update the status manually to avoid these errors and simply return UpdateControl.noUpdate()

[csviri]

SDK version: 2.1.2 https://github.com/apache/flink-kubernetes-operator/blob/main/flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/FlinkDeployment.java

please update to latest version 2.1.4

Some of these are not retriable under some circumstances and if not persisted to the status require manual user intervention. In these cases we would much prefer to simply update the status for whatever latest revision there is instead of failing because that is much worse from the user perspective.

pls see this part: https://javaoperatorsdk.io/docs/patterns-best-practices#managing-state
in this case I would advise to put the state to a ConfigMap or a dedicated CustomResource. There is a view that these states should not be in status, will probably fix this retry problem.

Note that in v3 now we handle the optimsitic locking part differently, it's actually retried in the background with an up to date resource version. So this error what you see here, would normally pass. There are discussion about as mentioned by @scrocquesel
in this issues: #1156

We also support patch in v3 which does not do optimistic locking and won't do in future.

Note that neither way is incorrect (in terms that the eventual consistency is achieved in both cases). Just some approach might be more optimal for different use-cases. There is also a technical reason why the way with optimistic locking has some advantages, see this comment: fabric8io/kubernetes-client#3943 (comment)

Regarding the long reconciliations. Just out of curiosity, it's not something that could be avoided in general to have long reconciliations, like if some API calls are slow, there is nothing to do about it. But a generic rule of thumb is that the actual state is cached (See PollingEventSource and PerResourcePollingEventSource for external resources), not read on reconciliation using the API but just using the cache of event sources. This might help to optimize the situation.

[csviri]

Attached a PR to this issue. From v3 there will be also patch (without optimistic locking in the UpdateControl), but that would also not cover this situation.
Basically without optimistic locking it would be very hard (not even sure if possible, in case some event could be lost) to support to have the latest resource in the local cache.
(Certainly not with the current version of the fabric8 client, it does not repport if for example the informer loses the connection, that would play crucial role in this)

So I would generally advice, to put the state about the external resource to a ConfigMap or into a dedicated custom resource that would just hold the data, and will be read on next reconciliation. (Pls always anticipate errors, so on create or update of ConfigMap/CustomResource make sure to have retry only for that resource to cover (unlikely) network issues.

[metacosm]

Maybe we should add some API support for this external state storage?

[csviri]

Maybe we should add some API support for this external state storage?

yep, we can think about is, probably to dependent resources it makes sense very much.