diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java index 8fb9a30ae9..9b34794066 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java +++ b/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) which + // can happen if JSON Patch is not a valid request since there was + // a concurrent request which 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()); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalCustomResource.java new file mode 100644 index 0000000000..3f9b61d1a8 --- /dev/null +++ b/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 implements Namespaced {} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalIT.java new file mode 100644 index 0000000000..da166ce2c1 --- /dev/null +++ b/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; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler1.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler1.java new file mode 100644 index 0000000000..b789255cb5 --- /dev/null +++ b/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, + Cleaner { + + @Override + public UpdateControl reconcile( + ConcurrentFinalizerRemovalCustomResource resource, + Context context) { + return UpdateControl.noUpdate(); + } + + @Override + public DeleteControl cleanup( + ConcurrentFinalizerRemovalCustomResource resource, + Context context) + throws Exception { + return DeleteControl.defaultDelete(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler2.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler2.java new file mode 100644 index 0000000000..0b8993a8f5 --- /dev/null +++ b/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, + Cleaner { + + @Override + public UpdateControl reconcile( + ConcurrentFinalizerRemovalCustomResource resource, + Context context) { + return UpdateControl.noUpdate(); + } + + @Override + public DeleteControl cleanup( + ConcurrentFinalizerRemovalCustomResource resource, + Context context) + throws Exception { + return DeleteControl.defaultDelete(); + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalSpec.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalSpec.java new file mode 100644 index 0000000000..d740721f30 --- /dev/null +++ b/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; + } +}