fix: retry finalizer removal on http 422

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

@@ -381,8 +381,13 @@ public P conflictRetryingPatch(
       } catch (KubernetesClientException e) {
         log.trace("Exception during patch for resource: {}", resource);
         retryIndex++;
-        // only retry on conflict (HTTP 409), otherwise fail
-        if (e.getCode() != 409) {
+        // only retry on conflict (409) and unprocessable content (422) what
+        // can happen if JSON Patch is not a valid request since there was
+        // a concurrent request already removed another finalizer:
+        // List element removal from a list is by index in JSON Patch
+        // so if addressing a second finalizer but first is meanwhile removed
+        // it is a wrong request.
+        if (e.getCode() != 409 && e.getCode() != 422) {
           throw e;
         }
         if (retryIndex >= MAX_UPDATE_RETRY) {
@@ -392,6 +397,11 @@ public P conflictRetryingPatch(
                   + ") retry attempts to patch resource: "
                   + ResourceID.fromResource(resource));
         }
+        log.debug(
+            "Retrying patch for resource name: {}, namespace: {}; HTTP code: {}",
+            resource.getMetadata().getName(),
+            resource.getMetadata().getNamespace(),
+            e.getCode());
         resource =
             customResourceFacade.getResource(
                 resource.getMetadata().getNamespace(), resource.getMetadata().getName());

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalCustomResource.java

@@ -0,0 +1,13 @@
+package io.javaoperatorsdk.operator.baseapi.concurrentfinalizerremoval;
+
+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.ShortNames;
+import io.fabric8.kubernetes.model.annotation.Version;
+
+@Group("sample.javaoperatorsdk")
+@Version("v1")
+@ShortNames("cfr")
+public class ConcurrentFinalizerRemovalCustomResource
+    extends CustomResource<ConcurrentFinalizerRemovalSpec, Void> implements Namespaced {}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalIT.java

@@ -0,0 +1,68 @@
+package io.javaoperatorsdk.operator.baseapi.concurrentfinalizerremoval;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
+import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
+import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.awaitility.Awaitility.await;
+
+class ConcurrentFinalizerRemovalIT {
+
+  private static final Logger log = LoggerFactory.getLogger(ConcurrentFinalizerRemovalIT.class);
+  public static final String TEST_RESOURCE_NAME = "test";
+
+  @RegisterExtension
+  LocallyRunOperatorExtension extension =
+      LocallyRunOperatorExtension.builder()
+          // should work without a retry, thus not retry the whole reconciliation but to retry
+          // finalizer removal only.
+          .withReconciler(
+              new ConcurrentFinalizerRemovalReconciler1(),
+              o ->
+                  o.withRetry(GenericRetry.noRetry()).withFinalizer("reconciler1.sample/finalizer"))
+          .withReconciler(
+              new ConcurrentFinalizerRemovalReconciler2(),
+              o ->
+                  o.withRetry(GenericRetry.noRetry()).withFinalizer("reconciler2.sample/finalizer"))
+          .build();
+
+  @Test
+  void concurrentFinalizerRemoval() {
+    for (int i = 0; i < 10; i++) {
+      var resource = extension.create(createResource());
+      await()
+          .untilAsserted(
+              () -> {
+                var res =
+                    extension.get(
+                        ConcurrentFinalizerRemovalCustomResource.class, TEST_RESOURCE_NAME);
+                assertThat(res.getMetadata().getFinalizers()).hasSize(2);
+              });
+      resource.getMetadata().setResourceVersion(null);
+      extension.delete(resource);
+
+      await()
+          .untilAsserted(
+              () -> {
+                var res =
+                    extension.get(
+                        ConcurrentFinalizerRemovalCustomResource.class, TEST_RESOURCE_NAME);
+                assertThat(res).isNull();
+              });
+    }
+  }
+
+  public ConcurrentFinalizerRemovalCustomResource createResource() {
+    ConcurrentFinalizerRemovalCustomResource res = new ConcurrentFinalizerRemovalCustomResource();
+    res.setMetadata(new ObjectMetaBuilder().withName(TEST_RESOURCE_NAME).build());
+    res.setSpec(new ConcurrentFinalizerRemovalSpec());
+    res.getSpec().setNumber(0);
+    return res;
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler1.java

@@ -0,0 +1,29 @@
+package io.javaoperatorsdk.operator.baseapi.concurrentfinalizerremoval;
+
+import io.javaoperatorsdk.operator.api.reconciler.Cleaner;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
+import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
+import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
+
+@ControllerConfiguration
+public class ConcurrentFinalizerRemovalReconciler1
+    implements Reconciler<ConcurrentFinalizerRemovalCustomResource>,
+        Cleaner<ConcurrentFinalizerRemovalCustomResource> {
+
+  @Override
+  public UpdateControl<ConcurrentFinalizerRemovalCustomResource> reconcile(
+      ConcurrentFinalizerRemovalCustomResource resource,
+      Context<ConcurrentFinalizerRemovalCustomResource> context) {
+    return UpdateControl.noUpdate();
+  }
+
+  @Override
+  public DeleteControl cleanup(
+      ConcurrentFinalizerRemovalCustomResource resource,
+      Context<ConcurrentFinalizerRemovalCustomResource> context)
+      throws Exception {
+    return DeleteControl.defaultDelete();
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler2.java

@@ -0,0 +1,29 @@
+package io.javaoperatorsdk.operator.baseapi.concurrentfinalizerremoval;
+
+import io.javaoperatorsdk.operator.api.reconciler.Cleaner;
+import io.javaoperatorsdk.operator.api.reconciler.Context;
+import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;
+import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
+import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
+import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
+
+@ControllerConfiguration
+public class ConcurrentFinalizerRemovalReconciler2
+    implements Reconciler<ConcurrentFinalizerRemovalCustomResource>,
+        Cleaner<ConcurrentFinalizerRemovalCustomResource> {
+
+  @Override
+  public UpdateControl<ConcurrentFinalizerRemovalCustomResource> reconcile(
+      ConcurrentFinalizerRemovalCustomResource resource,
+      Context<ConcurrentFinalizerRemovalCustomResource> context) {
+    return UpdateControl.noUpdate();
+  }
+
+  @Override
+  public DeleteControl cleanup(
+      ConcurrentFinalizerRemovalCustomResource resource,
+      Context<ConcurrentFinalizerRemovalCustomResource> context)
+      throws Exception {
+    return DeleteControl.defaultDelete();
+  }
+}

operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalSpec.java

@@ -0,0 +1,15 @@
+package io.javaoperatorsdk.operator.baseapi.concurrentfinalizerremoval;
+
+public class ConcurrentFinalizerRemovalSpec {
+
+  private int number;
+
+  public int getNumber() {
+    return number;
+  }
+
+  public ConcurrentFinalizerRemovalSpec setNumber(int number) {
+    this.number = number;
+    return this;
+  }
+}