feat: use patch without optimistic locking to remove finalizer

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java

@@ -3,6 +3,7 @@
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.function.Function;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -308,13 +309,14 @@ private R updateCustomResource(R resource) {
     return customResourceFacade.replaceResourceWithLock(resource);
   }
 
-  private R removeFinalizer(R resource) {
+  private R removeFinalizer(R originalResource) {
     log.debug(
         "Removing finalizer on resource: {} with version: {}",
-        getUID(resource),
-        getVersion(resource));
+        getUID(originalResource),
+        getVersion(originalResource));
+    var resource = cloneResource(originalResource);
     resource.removeFinalizer(configuration().getFinalizerName());
-    return customResourceFacade.replaceResourceWithLock(resource);
+    return customResourceFacade.patchResource(resource, originalResource);
   }
 
 
@@ -354,19 +356,27 @@ public R updateStatus(R resource) {
       return (R) hasMetadataOperation.replaceStatus(resource);
     }
 
+    public R patchResource(R resource, R originalResource) {
+      return handlePatch(resource, originalResource, (r) -> r.edit((or) -> resource));
+    }
+
     public R patchStatus(R resource, R originalResource) {
-      log.trace("Updating status for resource: {}", resource);
+      return handlePatch(resource, originalResource, (r) -> r.editStatus((or) -> resource));
+    }
+
+    private R handlePatch(R resource, R originalResource, Function<Resource<R>, R> unaryOperator) {
+      log.trace("Updating resource: {}", resource);
       String resourceVersion = resource.getMetadata().getResourceVersion();
       // don't do optimistic locking on patch
       originalResource.getMetadata().setResourceVersion(null);
       resource.getMetadata().setResourceVersion(null);
       try (var bis = new ByteArrayInputStream(
           Serialization.asJson(originalResource).getBytes(StandardCharsets.UTF_8))) {
-        return resourceOperation
+        var resOps = resourceOperation
             .inNamespace(resource.getMetadata().getNamespace())
             // will be simplified in fabric8 v6
-            .load(bis)
-            .editStatus(r -> resource);
+            .load(bis);
+        return unaryOperator.apply(resOps);
       } catch (IOException e) {
         throw new IllegalStateException(e);
       } finally {

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

@@ -231,7 +231,7 @@ void removesDefaultFinalizerOnDeleteIfSet() {
     reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource));
 
     assertEquals(0, testCustomResource.getMetadata().getFinalizers().size());
-    verify(customResourceFacade, times(1)).replaceResourceWithLock(any());
+    verify(customResourceFacade, times(1)).patchResource(any(), any());
   }
 
   @Test

operator-framework/src/test/java/io/javaoperatorsdk/operator/CleanupConflictIT.java

@@ -0,0 +1,85 @@
+package io.javaoperatorsdk.operator;
+
+import java.time.Duration;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import io.fabric8.kubernetes.api.model.ObjectMeta;
+import io.javaoperatorsdk.operator.junit.LocalOperatorExtension;
+import io.javaoperatorsdk.operator.sample.cleanupconflict.CleanupConflictCustomResource;
+import io.javaoperatorsdk.operator.sample.cleanupconflict.CleanupConflictReconciler;
+
+import static io.javaoperatorsdk.operator.sample.cleanupconflict.CleanupConflictReconciler.WAIT_TIME;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class CleanupConflictIT {
+
+  private static final String ADDITIONAL_FINALIZER = "myfinalizer";
+  public static final String TEST_RESOURCE_NAME = "test1";
+
+  @RegisterExtension
+  LocalOperatorExtension operator =
+      LocalOperatorExtension.builder().withReconciler(new CleanupConflictReconciler())
+          .build();
+
+  @Test
+  void cleanupRemovesFinalizerWithoutConflict() throws InterruptedException {
+    var testResource = operator.create(CleanupConflictCustomResource.class, createTestResource());
+    await().untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(CleanupConflictReconciler.class)
+          .getNumberReconcileExecutions()).isEqualTo(1);
+    });
+
+    // adding second finalizer later so there is no issue with the remove patch
+    // that remove the finalizer by index so order matters
+    testResource = operator.get(CleanupConflictCustomResource.class, TEST_RESOURCE_NAME);
+    testResource.getMetadata().getFinalizers().add(ADDITIONAL_FINALIZER);
+    testResource = operator.replace(CleanupConflictCustomResource.class, testResource);
+
+    operator.delete(CleanupConflictCustomResource.class, testResource);
+    Thread.sleep(WAIT_TIME / 2);
+    testResource = operator.get(CleanupConflictCustomResource.class, TEST_RESOURCE_NAME);
+    testResource.getMetadata().getFinalizers().remove(ADDITIONAL_FINALIZER);
+    testResource.getMetadata().setResourceVersion(null);
+    operator.replace(CleanupConflictCustomResource.class, testResource);
+
+    await().pollDelay(Duration.ofMillis(WAIT_TIME * 2)).untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(CleanupConflictReconciler.class)
+          .getNumberOfCleanupExecutions()).isEqualTo(1);
+    });
+  }
+
+  @Test
+  void cleanupRemovesFinalizerOrderRemovalIssue() throws InterruptedException {
+    var testResource = createTestResource();
+    testResource.getMetadata().setFinalizers(List.of(ADDITIONAL_FINALIZER));
+    testResource = operator.create(CleanupConflictCustomResource.class, testResource);
+    await().untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(CleanupConflictReconciler.class)
+          .getNumberReconcileExecutions()).isEqualTo(1);
+    });
+
+    operator.delete(CleanupConflictCustomResource.class, testResource);
+    Thread.sleep(WAIT_TIME / 2);
+    testResource = operator.get(CleanupConflictCustomResource.class, TEST_RESOURCE_NAME);
+    testResource.getMetadata().getFinalizers().remove(ADDITIONAL_FINALIZER);
+    testResource.getMetadata().setResourceVersion(null);
+    operator.replace(CleanupConflictCustomResource.class, testResource);
+
+    await().pollDelay(Duration.ofMillis(WAIT_TIME * 2)).untilAsserted(() -> {
+      assertThat(operator.getReconcilerOfType(CleanupConflictReconciler.class)
+          .getNumberOfCleanupExecutions()).isEqualTo(2);
+    });
+    assertThat(operator.get(CleanupConflictCustomResource.class, TEST_RESOURCE_NAME)).isNull();
+  }
+
+  private CleanupConflictCustomResource createTestResource() {
+    CleanupConflictCustomResource cr = new CleanupConflictCustomResource();
+    cr.setMetadata(new ObjectMeta());
+    cr.getMetadata().setName(TEST_RESOURCE_NAME);
+    return cr;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResource.java

@@ -0,0 +1,18 @@
+package io.javaoperatorsdk.operator.sample.cleanupconflict;
+
+import io.fabric8.kubernetes.api.model.Namespaced;
+import io.fabric8.kubernetes.client.CustomResource;
+import io.fabric8.kubernetes.model.annotation.Group;
+import io.fabric8.kubernetes.model.annotation.Kind;
+import io.fabric8.kubernetes.model.annotation.ShortNames;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+@Group("sample.javaoperatorsdk")
+@Version("v1")
+@Kind("CleanupConflictCustomResource")
+@ShortNames("ccc")
+public class CleanupConflictCustomResource
+    extends CustomResource<Void, CleanupConflictCustomResourceStatus>
+    implements Namespaced {
+
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResourceStatus.java

@@ -0,0 +1,15 @@
+package io.javaoperatorsdk.operator.sample.cleanupconflict;
+
+public class CleanupConflictCustomResourceStatus {
+
+  private Integer value = 0;
+
+  public Integer getValue() {
+    return value;
+  }
+
+  public CleanupConflictCustomResourceStatus setValue(Integer value) {
+    this.value = value;
+    return this;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictReconciler.java

@@ -0,0 +1,42 @@
+package io.javaoperatorsdk.operator.sample.cleanupconflict;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+import io.javaoperatorsdk.operator.api.reconciler.*;
+
+@ControllerConfiguration
+public class CleanupConflictReconciler
+    implements Reconciler<CleanupConflictCustomResource>, Cleaner<CleanupConflictCustomResource> {
+
+  public static final long WAIT_TIME = 500L;
+  private final AtomicInteger numberOfCleanupExecutions = new AtomicInteger(0);
+  private final AtomicInteger numberReconcileExecutions = new AtomicInteger(0);
+
+  @Override
+  public UpdateControl<CleanupConflictCustomResource> reconcile(
+      CleanupConflictCustomResource resource,
+      Context<CleanupConflictCustomResource> context) {
+    numberReconcileExecutions.addAndGet(1);
+    return UpdateControl.noUpdate();
+  }
+
+  @Override
+  public DeleteControl cleanup(CleanupConflictCustomResource resource,
+      Context<CleanupConflictCustomResource> context) {
+    numberOfCleanupExecutions.addAndGet(1);
+    try {
+      Thread.sleep(WAIT_TIME);
+    } catch (InterruptedException e) {
+      throw new RuntimeException(e);
+    }
+    return DeleteControl.defaultDelete();
+  }
+
+  public int getNumberOfCleanupExecutions() {
+    return numberOfCleanupExecutions.intValue();
+  }
+
+  public int getNumberReconcileExecutions() {
+    return numberReconcileExecutions.intValue();
+  }
+}