From 5cf7d41683dfe8a73b58c9d65dffcf6b145207df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 17:42:40 +0200 Subject: [PATCH 1/6] fix: retry finalizer on http 422 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this can happen if there are multiple finalizers present and multiple reconilers competent to remove their finalizer, since we remove the finalizer with JSON Patch Signed-off-by: Attila Mészáros --- .../operator/processing/event/ReconciliationDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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..7f9a3c0d01 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 @@ -382,7 +382,7 @@ public P conflictRetryingPatch( log.trace("Exception during patch for resource: {}", resource); retryIndex++; // only retry on conflict (HTTP 409), otherwise fail - if (e.getCode() != 409) { + if (e.getCode() != 409 && e.getCode() != 422) { throw e; } if (retryIndex >= MAX_UPDATE_RETRY) { From 3bc86547e89cd1cd9d4374119b8873cbe4892404 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 28 Apr 2025 18:03:47 +0200 Subject: [PATCH 2/6] comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../processing/event/ReconciliationDispatcher.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 7f9a3c0d01..0bb6ffddf8 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,7 +381,12 @@ public P conflictRetryingPatch( } catch (KubernetesClientException e) { log.trace("Exception during patch for resource: {}", resource); retryIndex++; - // only retry on conflict (HTTP 409), otherwise fail + // 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; } From f3196e77dd57eedbc5e512cd6b4a1c3555228753 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 29 Apr 2025 17:28:13 +0200 Subject: [PATCH 3/6] integration test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../event/ReconciliationDispatcher.java | 5 ++ ...currentFinalizerRemovalCustomResource.java | 13 ++++ .../ConcurrentFinalizerRemovalIT.java | 68 +++++++++++++++++++ ...ConcurrentFinalizerRemovalReconciler1.java | 29 ++++++++ ...ConcurrentFinalizerRemovalReconciler2.java | 29 ++++++++ .../ConcurrentFinalizerRemovalSpec.java | 15 ++++ 6 files changed, 159 insertions(+) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler1.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalReconciler2.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/concurrentfinalizerremoval/ConcurrentFinalizerRemovalSpec.java 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 0bb6ffddf8..7a1ef70ade 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 @@ -397,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..28bd0843ca --- /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; + } +} From 0b756cede213225ff9ed3a6d3017e1c48500f86f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 29 Apr 2025 17:30:38 +0200 Subject: [PATCH 4/6] format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- .../ConcurrentFinalizerRemovalIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 28bd0843ca..da166ce2c1 100644 --- 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 @@ -21,7 +21,7 @@ class ConcurrentFinalizerRemovalIT { LocallyRunOperatorExtension extension = LocallyRunOperatorExtension.builder() // should work without a retry, thus not retry the whole reconciliation but to retry - // finalizer removal only. + // finalizer removal only. .withReconciler( new ConcurrentFinalizerRemovalReconciler1(), o -> From e55328299cc1c748b24f708a5f7d47c35f898e9a Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 30 Apr 2025 12:39:12 +0200 Subject: [PATCH 5/6] fix: wording --- .../operator/processing/event/ReconciliationDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7a1ef70ade..417e50a102 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,7 +381,7 @@ public P conflictRetryingPatch( } catch (KubernetesClientException e) { log.trace("Exception during patch for resource: {}", resource); retryIndex++; - // only retry on conflict (409) and unprocessable content (422) what + // 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 already removed another finalizer: // List element removal from a list is by index in JSON Patch From bdd3e56e0d97a4aa0b506f2d550317e4be4009b0 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 30 Apr 2025 12:39:28 +0200 Subject: [PATCH 6/6] fix: wording Co-authored-by: Martin Stefanko --- .../operator/processing/event/ReconciliationDispatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 417e50a102..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 @@ -383,7 +383,7 @@ public P conflictRetryingPatch( retryIndex++; // 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 already removed another finalizer: + // 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.