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 600ebee221..406982b5eb 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 @@ -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, 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 { diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java index db2b52c295..3a831fec85 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java +++ b/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 diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/CleanupConflictIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CleanupConflictIT.java new file mode 100644 index 0000000000..6805ecbd51 --- /dev/null +++ b/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; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResource.java new file mode 100644 index 0000000000..9675273085 --- /dev/null +++ b/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 + implements Namespaced { + +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResourceStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResourceStatus.java new file mode 100644 index 0000000000..3488981eb1 --- /dev/null +++ b/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; + } +} diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictReconciler.java new file mode 100644 index 0000000000..14d8f8617f --- /dev/null +++ b/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, Cleaner { + + public static final long WAIT_TIME = 500L; + private final AtomicInteger numberOfCleanupExecutions = new AtomicInteger(0); + private final AtomicInteger numberReconcileExecutions = new AtomicInteger(0); + + @Override + public UpdateControl reconcile( + CleanupConflictCustomResource resource, + Context context) { + numberReconcileExecutions.addAndGet(1); + return UpdateControl.noUpdate(); + } + + @Override + public DeleteControl cleanup(CleanupConflictCustomResource resource, + Context 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(); + } +}