Use PATCH without optimistick locking to remove finalizer this way will avoid conflicts

[csviri]

For remove this makes sense in all cases.
(For add this could cause some issues, its hard to avoid "double adding" without optimistic locking" atm)

2022-05-24 14:19:26,505 i.j.o.p.e.ReconciliationDispatcher [ERROR] [flink.basic-session-job-example] Error during event processing ExecutionScope{ resource id: ResourceID{name='basic-session-job-example', namespace='flink'}, version: 854260} failed.
io.fabric8.kubernetes.client.KubernetesClientException: Failure executing: PUT at: https://127.0.0.1:55618/apis/flink.apache.org/v1beta1/namespaces/flink/flinksessionjobs/basic-session-job-example. Message: Operation cannot be fulfilled on flinksessionjobs.flink.apache.org "basic-session-job-example": 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=flinksessionjobs, name=basic-session-job-example, retryAfterSeconds=null, uid=null, additionalProperties={}), kind=Status, message=Operation cannot be fulfilled on flinksessionjobs.flink.apache.org "basic-session-job-example": 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.HasMetadataOperation.lambda$replace$1(HasMetadataOperation.java:195)
	at io.fabric8.kubernetes.client.dsl.base.HasMetadataOperation.replace(HasMetadataOperation.java:200)
	at io.fabric8.kubernetes.client.dsl.base.HasMetadataOperation.replace(HasMetadataOperation.java:141)
	at io.fabric8.kubernetes.client.dsl.base.HasMetadataOperation.replace(HasMetadataOperation.java:43)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher$CustomResourceFacade.replaceResourceWithLock(ReconciliationDispatcher.java:344)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.removeFinalizer(ReconciliationDispatcher.java:317)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleCleanup(ReconciliationDispatcher.java:283)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleDispatch(ReconciliationDispatcher.java:78)
	at io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.handleExecution(ReconciliationDispatcher.java:55)
	at io.javaoperatorsdk.operator.processing.event.EventProcessor$ControllerExecution.run(EventProcessor.java:356)
	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)

[popdaniel942]

Hello.
I am using the latest version of the operator and whenever I delete a resource and the cleanup method is being called, the first time I get the error described above with "the object has been modified; please apply your changes to the latest version and try again." and then the cleanup is called again and everything works fine.
Is this related to the issue described here?

[csviri]

Hi @popdaniel942 , yes this is related to that. Will investigate that, it should not happen in general (or just rarely when the resource is updated while the reconciliation running), so this issue will mitigate that or rather eliminate that problem in all cases. But will investigate why is that happening anyway.

[csviri]

@popdaniel942 there is integration test on our side, that checks this, so it does not happen at least in normal flows:
https://github.com/java-operator-sdk/java-operator-sdk/blob/49ee0a03f90fdeb72a5f5cdd8d1477ef4329eeb3/operator-framework/src/test/java/io/javaoperatorsdk/operator/ControllerExecutionIT.java#L69-L71

If you could tell more in what situation this heppens to you would be very helpful. Isn't you code open source? thx

[popdaniel942]

@csviri unfortunately the code isn't open source...I tried the simplest way possible to use the operator and I still get problem. I used version 3.0.0. The crds are generated from Java code. The reconciliation works, but the cleanup fails with the above error and then retries and it works.
Controller:
@ControllerConfiguration( name = "resource", namespaces = Constants.WATCH_CURRENT_NAMESPACE)
public class ResourceController implements Reconciler, ErrorStatusHandler, Cleaner {

private static final Logger LOGGER = LoggerFactory.getLogger(ResourceController.class);

@OverRide
public DeleteControl cleanup(Resourceresource, Context context) {
LOGGER.info("Delete resources for " + resource.getMetadata().getName());

return DeleteControl.defaultDelete();

}

@OverRide
public UpdateControl reconcile(Resource resource, Context context) {
LOGGER.info("Create or Update resources for " + resource.getMetadata().getName());
resource.setStatus(new ResourceStatus());

return UpdateControl.updateStatus(resource);

}

@OverRide
public ErrorStatusUpdateControl updateErrorStatus(Resource resource, Context context,
Exception e) {
LOGGER.error(e.getMessage());
resource.setStatus(new ResourceStatus());

return ErrorStatusUpdateControl.updateStatus(resource);

}

Resource:
@Version("v1")
@group("resourceGroup")
public class Resource extends CustomResource<ResourceSpec, ResourceStatus> implements Namespaced {

@NotNull
private ResourceSpec spec;

@Override
protected ResourceStatus initStatus() {
    return new ResourceStatus();
}

}

[popdaniel942]

apparently with 3.0.1 this example works

[csviri]

@popdaniel942 thank you! we were able to reproduce this on an other project too, I'm on it! :)

[popdaniel942]

In our full code it still doesn't work with 3.0.1. But we don't change anything in the resource In the cleanup. All we do is call some rest methods. And when DeleteControl.defaultDelete() is called it fails. And after the retry it works.

[csviri]

Yes, this happens because the framework tried to remove the finalizer during a default delete. And the resource it sees in cache might have a different version than on the server. This is strange because once a resource is marked for deletion it cannot be changed, only finalizer removed. So if there are no more finalizers used, it's weird. But also since the cleanup is triggered is means that the resource (in memory) is already marked for deletion. Looking into it..

[csviri]

@popdaniel942 could you pls turn on debug level logs, with MDC values for resource name+namespace+version, and thread id present, something like this:

appender.console.layout.pattern = %style{%d}{yellow} %tid %style{%-30c{1.}}{cyan} %highlight{[%-5level] [%X{resource.namespace}.%X{resource.name}.%X{resource.resourceVersion}] %msg%n%throwable}

the styles are not important :D

and send us the log files pls? thx!

Also isn't some controller on the cluster that adds an additional finalizer?

[csviri]

@popdaniel942 are you using UpdateControl.patchStatus to update the resource?

[popdaniel942]

Right now we are using UpdateControl.updateStatus but we got the error with both. What is the difference between them? Also I see that when the cleanup is made there is a foregroungDeletion finalizer being added and that the resourceVersion gets changed. Also in some cases, if we run the code in debug mode with breakpoints on the cleanup method we get the error, but if we run normally without debug mode, we don't get the error