From 51a37fad93fce3811145e4bfba3a534b3a1ef793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Mon, 30 May 2022 17:39:22 +0200 Subject: [PATCH] feat: retry remove finalizer (#1249) --- .../micrometer/MicrometerMetrics.java | 4 +- .../operator/api/monitoring/Metrics.java | 2 +- .../processing/event/EventMarker.java | 14 ++- .../processing/event/EventProcessor.java | 92 +++++++++++------ .../event/PostExecutionControl.java | 19 +++- .../event/ReconciliationDispatcher.java | 95 ++++++++++++------ .../ControllerResourceEventSource.java | 2 +- .../source/controller/ResourceEvent.java | 28 +++++- .../javaoperatorsdk/operator/TestUtils.java | 4 + .../processing/event/EventProcessorTest.java | 63 +++++++++++- .../event/ReconciliationDispatcherTest.java | 98 ++++++++++++++++--- .../operator/CleanupConflictIT.java | 57 +++++++++++ .../CleanupConflictCustomResource.java | 18 ++++ .../CleanupConflictCustomResourceStatus.java | 15 +++ .../CleanupConflictReconciler.java | 42 ++++++++ 15 files changed, 466 insertions(+), 87 deletions(-) create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/CleanupConflictIT.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResource.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictCustomResourceStatus.java create mode 100644 operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/cleanupconflict/CleanupConflictReconciler.java diff --git a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java index 25058e198e..c3bbb219b7 100644 --- a/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java +++ b/micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java @@ -61,8 +61,8 @@ public void receivedEvent(Event event) { } @Override - public void cleanupDoneFor(ResourceID customResourceUid) { - incrementCounter(customResourceUid, "events.delete"); + public void cleanupDoneFor(ResourceID resourceID) { + incrementCounter(resourceID, "events.delete"); } @Override diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java index b1be115bc4..a1e643ac6f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/monitoring/Metrics.java @@ -15,7 +15,7 @@ default void reconcileCustomResource(ResourceID resourceID, RetryInfo retryInfo) default void failedReconciliation(ResourceID resourceID, Exception exception) {} - default void cleanupDoneFor(ResourceID customResourceUid) {} + default void cleanupDoneFor(ResourceID resourceID) {} default void finishedReconciliation(ResourceID resourceID) {} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventMarker.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventMarker.java index a26c9c7829..0b5a372ead 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventMarker.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventMarker.java @@ -6,6 +6,7 @@ import java.util.stream.Collectors; import static io.javaoperatorsdk.operator.processing.event.EventMarker.EventingState.NO_EVENT_PRESENT; +import static io.javaoperatorsdk.operator.processing.event.EventMarker.EventingState.PROCESSED_MARK_FOR_DELETION; /** * Manages the state of received events. Basically there can be only three distinct states relevant @@ -18,8 +19,9 @@ class EventMarker { public enum EventingState { - /** Event but NOT Delete event present */ EVENT_PRESENT, NO_EVENT_PRESENT, + /** Resource has been marked for deletion, and cleanup already executed successfully */ + PROCESSED_MARK_FOR_DELETION, /** Delete event present, from this point other events are not relevant */ DELETE_EVENT_PRESENT, } @@ -53,11 +55,21 @@ public void unMarkEventReceived(ResourceID resourceID) { setEventingState(resourceID, NO_EVENT_PRESENT); break; + case PROCESSED_MARK_FOR_DELETION: + throw new IllegalStateException("Cannot unmark processed marked for deletion."); case DELETE_EVENT_PRESENT: throw new IllegalStateException("Cannot unmark delete event."); } } + public void markProcessedMarkForDeletion(ResourceID resourceID) { + setEventingState(resourceID, PROCESSED_MARK_FOR_DELETION); + } + + public boolean processedMarkForDeletionPresent(ResourceID resourceID) { + return getEventingState(resourceID) == PROCESSED_MARK_FOR_DELETION; + } + public void markDeleteEventReceived(Event event) { markDeleteEventReceived(event.getRelatedCustomResourceID()); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java index ebbd289fb8..6597131990 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java @@ -121,10 +121,10 @@ public void handleEvent(Event event) { } private void handleMarkedEventForResource(ResourceID resourceID) { - if (!eventMarker.deleteEventPresent(resourceID)) { - submitReconciliationExecution(resourceID); - } else { + if (eventMarker.deleteEventPresent(resourceID)) { cleanupForDeletedEvent(resourceID); + } else if (!eventMarker.processedMarkForDeletionPresent(resourceID)) { + submitReconciliationExecution(resourceID); } } @@ -157,18 +157,50 @@ private void submitReconciliationExecution(ResourceID resourceID) { } private void handleEventMarking(Event event) { - if (event instanceof ResourceEvent - && ((ResourceEvent) event).getAction() == ResourceAction.DELETED) { - log.debug("Marking delete event received for: {}", event.getRelatedCustomResourceID()); - eventMarker.markDeleteEventReceived(event); - } else if (!eventMarker.deleteEventPresent(event.getRelatedCustomResourceID())) { - log.debug("Marking event received for: {}", event.getRelatedCustomResourceID()); - eventMarker.markEventReceived(event); + final var relatedCustomResourceID = event.getRelatedCustomResourceID(); + if (event instanceof ResourceEvent) { + var resourceEvent = (ResourceEvent) event; + if (resourceEvent.getAction() == ResourceAction.DELETED) { + log.debug("Marking delete event received for: {}", relatedCustomResourceID); + eventMarker.markDeleteEventReceived(event); + } else { + if (eventMarker.processedMarkForDeletionPresent(relatedCustomResourceID) + && isResourceMarkedForDeletion(resourceEvent)) { + log.debug( + "Skipping mark of event received, since already processed mark for deletion and resource marked for deletion: {}", + relatedCustomResourceID); + return; + } + // Normally when eventMarker is in state PROCESSED_MARK_FOR_DELETION it is expected to + // receive a Delete event or an event where resource is marked for deletion. In a rare edge + // case however it can happen that the finalizer related to the current controller is + // removed, but also the informers websocket is disconnected and later reconnected. So + // meanwhile the resource could be deleted and recreated. In this case we just mark a new + // event as below. + markEventReceived(event); + } + } else if (!eventMarker.deleteEventPresent(relatedCustomResourceID) || + !eventMarker.processedMarkForDeletionPresent(relatedCustomResourceID)) { + markEventReceived(event); + } else if (log.isDebugEnabled()) { + log.debug( + "Skipped marking event as received. Delete event present: {}, processed mark for deletion: {}", + eventMarker.deleteEventPresent(relatedCustomResourceID), + eventMarker.processedMarkForDeletionPresent(relatedCustomResourceID)); } } - private RetryInfo retryInfo(ResourceID customResourceUid) { - return retryState.get(customResourceUid); + private void markEventReceived(Event event) { + log.debug("Marking event received for: {}", event.getRelatedCustomResourceID()); + eventMarker.markEventReceived(event); + } + + private boolean isResourceMarkedForDeletion(ResourceEvent resourceEvent) { + return resourceEvent.getResource().map(HasMetadata::isMarkedForDeletion).orElse(false); + } + + private RetryInfo retryInfo(ResourceID resourceID) { + return retryState.get(resourceID); } void eventProcessingFinished( @@ -199,6 +231,8 @@ void eventProcessingFinished( metrics.finishedReconciliation(resourceID); if (eventMarker.deleteEventPresent(resourceID)) { cleanupForDeletedEvent(executionScope.getResourceID()); + } else if (postExecutionControl.isFinalizerRemoved()) { + eventMarker.markProcessedMarkForDeletion(resourceID); } else { postExecutionControl .getUpdatedCustomResource() @@ -247,13 +281,13 @@ TimerEventSource retryEventSource() { private void handleRetryOnException( ExecutionScope executionScope, Exception exception) { RetryExecution execution = getOrInitRetryExecution(executionScope); - var customResourceID = executionScope.getResourceID(); - boolean eventPresent = eventMarker.eventPresent(customResourceID); - eventMarker.markEventReceived(customResourceID); + var resourceID = executionScope.getResourceID(); + boolean eventPresent = eventMarker.eventPresent(resourceID); + eventMarker.markEventReceived(resourceID); if (eventPresent) { - log.debug("New events exists for for resource id: {}", customResourceID); - submitReconciliationExecution(customResourceID); + log.debug("New events exists for for resource id: {}", resourceID); + submitReconciliationExecution(resourceID); return; } Optional nextDelay = execution.nextDelay(); @@ -263,8 +297,8 @@ private void handleRetryOnException( log.debug( "Scheduling timer event for retry with delay:{} for resource: {}", delay, - customResourceID); - metrics.failedReconciliation(customResourceID, exception); + resourceID); + metrics.failedReconciliation(resourceID, exception); retryEventSource().scheduleOnce(executionScope.getResource(), delay); }, () -> log.error("Exhausted retries for {}", executionScope)); @@ -288,22 +322,22 @@ private RetryExecution getOrInitRetryExecution(ExecutionScope executionScope) return retryExecution; } - private void cleanupForDeletedEvent(ResourceID customResourceUid) { - log.debug("Cleaning up for delete event for: {}", customResourceUid); - eventMarker.cleanup(customResourceUid); - metrics.cleanupDoneFor(customResourceUid); + private void cleanupForDeletedEvent(ResourceID resourceID) { + log.debug("Cleaning up for delete event for: {}", resourceID); + eventMarker.cleanup(resourceID); + metrics.cleanupDoneFor(resourceID); } - private boolean isControllerUnderExecution(ResourceID customResourceUid) { - return underProcessing.contains(customResourceUid); + private boolean isControllerUnderExecution(ResourceID resourceID) { + return underProcessing.contains(resourceID); } - private void setUnderExecutionProcessing(ResourceID customResourceUid) { - underProcessing.add(customResourceUid); + private void setUnderExecutionProcessing(ResourceID resourceID) { + underProcessing.add(resourceID); } - private void unsetUnderExecution(ResourceID customResourceUid) { - underProcessing.remove(customResourceUid); + private void unsetUnderExecution(ResourceID resourceID) { + underProcessing.remove(resourceID); } private boolean isRetryConfigured() { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java index 820254f9e3..6fddd5ad93 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/PostExecutionControl.java @@ -6,7 +6,7 @@ final class PostExecutionControl { - private final boolean onlyFinalizerHandled; + private final boolean finalizerRemoved; private final R updatedCustomResource; private final boolean updateIsStatusPatch; private final Exception runtimeException; @@ -14,10 +14,10 @@ final class PostExecutionControl { private Long reScheduleDelay = null; private PostExecutionControl( - boolean onlyFinalizerHandled, + boolean finalizerRemoved, R updatedCustomResource, boolean updateIsStatusPatch, Exception runtimeException) { - this.onlyFinalizerHandled = onlyFinalizerHandled; + this.finalizerRemoved = finalizerRemoved; this.updatedCustomResource = updatedCustomResource; this.updateIsStatusPatch = updateIsStatusPatch; this.runtimeException = runtimeException; @@ -25,7 +25,7 @@ private PostExecutionControl( public static PostExecutionControl onlyFinalizerAdded( R updatedCustomResource) { - return new PostExecutionControl<>(true, updatedCustomResource, false, null); + return new PostExecutionControl<>(false, updatedCustomResource, false, null); } public static PostExecutionControl defaultDispatch() { @@ -42,6 +42,11 @@ public static PostExecutionControl customResourceUpda return new PostExecutionControl<>(false, updatedCustomResource, false, null); } + public static PostExecutionControl customResourceFinalizerRemoved( + R updatedCustomResource) { + return new PostExecutionControl<>(true, updatedCustomResource, false, null); + } + public static PostExecutionControl exceptionDuringExecution( Exception exception) { return new PostExecutionControl<>(false, null, false, exception); @@ -76,11 +81,15 @@ public boolean updateIsStatusPatch() { public String toString() { return "PostExecutionControl{" + "onlyFinalizerHandled=" - + onlyFinalizerHandled + + finalizerRemoved + ", updatedCustomResource=" + updatedCustomResource + ", runtimeException=" + runtimeException + '}'; } + + public boolean isFinalizerRemoved() { + return finalizerRemoved; + } } 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 0555a17621..a76fcff8e4 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 @@ -10,10 +10,12 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.KubernetesResourceList; import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.dsl.MixedOperation; import io.fabric8.kubernetes.client.dsl.Resource; import io.fabric8.kubernetes.client.dsl.internal.HasMetadataOperationsImpl; import io.fabric8.kubernetes.client.utils.Serialization; +import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.ObservedGenerationAware; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; @@ -35,6 +37,8 @@ */ class ReconciliationDispatcher { + public static final int MAX_FINALIZER_REMOVAL_RETRY = 10; + private static final Logger log = LoggerFactory.getLogger(ReconciliationDispatcher.class); private final Controller controller; @@ -61,35 +65,41 @@ public PostExecutionControl handleExecution(ExecutionScope executionScope) private PostExecutionControl handleDispatch(ExecutionScope executionScope) throws Exception { - R resource = executionScope.getResource(); - log.debug("Handling dispatch for resource {}", getName(resource)); + R originalResource = executionScope.getResource(); + var resourceForExecution = cloneResource(originalResource); + log.debug("Handling dispatch for resource {}", getName(originalResource)); - final var markedForDeletion = resource.isMarkedForDeletion(); - if (markedForDeletion && shouldNotDispatchToCleanup(resource)) { + final var markedForDeletion = originalResource.isMarkedForDeletion(); + if (markedForDeletion && shouldNotDispatchToCleanupWhenMarkedForDeletion(originalResource)) { log.debug( "Skipping cleanup of resource {} because finalizer(s) {} don't allow processing yet", - getName(resource), - resource.getMetadata().getFinalizers()); + getName(originalResource), + originalResource.getMetadata().getFinalizers()); return PostExecutionControl.defaultDispatch(); } - Context context = new DefaultContext<>(executionScope.getRetryInfo(), controller, resource); + Context context = + new DefaultContext<>(executionScope.getRetryInfo(), controller, originalResource); if (markedForDeletion) { - return handleCleanup(resource, context); + return handleCleanup(resourceForExecution, context); } else { - return handleReconcile(executionScope, resource, context); + return handleReconcile(executionScope, resourceForExecution, originalResource, context); } } - private boolean shouldNotDispatchToCleanup(R resource) { - // we don't dispatch to cleanup if the controller is configured to use a finalizer but that - // finalizer is not present (which means it's already been removed) - return !controller.useFinalizer() || (controller.useFinalizer() - && !resource.hasFinalizer(configuration().getFinalizerName())); + private boolean shouldNotDispatchToCleanupWhenMarkedForDeletion(R resource) { + var alreadyRemovedFinalizer = controller.useFinalizer() + && !resource.hasFinalizer(configuration().getFinalizerName()); + if (alreadyRemovedFinalizer) { + log.warn("This should not happen. Marked for deletion & already removed finalizer: {}", + ResourceID.fromResource(resource)); + } + return !controller.useFinalizer() || alreadyRemovedFinalizer; } private PostExecutionControl handleReconcile( - ExecutionScope executionScope, R originalResource, Context context) throws Exception { + ExecutionScope executionScope, R resourceForExecution, R originalResource, + Context context) throws Exception { if (controller.useFinalizer() && !originalResource.hasFinalizer(configuration().getFinalizerName())) { /* @@ -101,8 +111,6 @@ private PostExecutionControl handleReconcile( var updatedResource = updateCustomResourceWithFinalizer(originalResource); return PostExecutionControl.onlyFinalizerAdded(updatedResource); } else { - var resourceForExecution = - cloneResource(originalResource); try { return reconcileExecution(executionScope, resourceForExecution, originalResource, context); } catch (Exception e) { @@ -278,10 +286,10 @@ private PostExecutionControl handleCleanup(R resource, Context context) { if (useFinalizer) { // note that we don't reschedule here even if instructed. Removing finalizer means that // cleanup is finished, nothing left to done - if (deleteControl.isRemoveFinalizer() - && resource.hasFinalizer(configuration().getFinalizerName())) { - R customResource = removeFinalizer(resource); - return PostExecutionControl.customResourceUpdated(customResource); + final var finalizerName = configuration().getFinalizerName(); + if (deleteControl.isRemoveFinalizer() && resource.hasFinalizer(finalizerName)) { + R customResource = removeFinalizer(resource, finalizerName); + return PostExecutionControl.customResourceFinalizerRemoved(customResource); } } log.debug( @@ -308,20 +316,41 @@ private R updateCustomResource(R resource) { return customResourceFacade.replaceResourceWithLock(resource); } - private R removeFinalizer(R resource) { - log.debug( - "Removing finalizer on resource: {} with version: {}", - getUID(resource), - getVersion(resource)); - resource.removeFinalizer(configuration().getFinalizerName()); - return customResourceFacade.replaceResourceWithLock(resource); - } - - ControllerConfiguration configuration() { return controller.getConfiguration(); } + public R removeFinalizer(R resource, String finalizer) { + if (log.isDebugEnabled()) { + log.debug("Removing finalizer on resource: {}", ResourceID.fromResource(resource)); + } + int retryIndex = 0; + while (true) { + try { + var removed = resource.removeFinalizer(finalizer); + if (!removed) { + return resource; + } + return customResourceFacade.replaceResourceWithLock(resource); + } catch (KubernetesClientException e) { + log.trace("Exception during finalizer removal for resource: {}", resource); + retryIndex++; + // only retry on conflict (HTTP 409), otherwise fail + if (e.getCode() != 409) { + throw e; + } + if (retryIndex >= MAX_FINALIZER_REMOVAL_RETRY) { + throw new OperatorException( + "Exceeded maximum (" + MAX_FINALIZER_REMOVAL_RETRY + + ") retry attempts to remove finalizer '" + finalizer + "' for resource " + + ResourceID.fromResource(resource)); + } + resource = customResourceFacade.getResource(resource.getMetadata().getNamespace(), + resource.getMetadata().getName()); + } + } + } + // created to support unit testing static class CustomResourceFacade { @@ -332,6 +361,10 @@ public CustomResourceFacade( this.resourceOperation = resourceOperation; } + public R getResource(String namespace, String name) { + return resourceOperation.inNamespace(namespace).withName(name).get(); + } + public R replaceResourceWithLock(R resource) { log.debug( "Trying to replace resource {}, version: {}", diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java index 233cecb644..83afcc0be2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerResourceEventSource.java @@ -62,7 +62,7 @@ public void eventReceived(ResourceAction action, T resource, T oldResource) { controller.getEventSourceManager().broadcastOnResourceEvent(action, resource, oldResource); if (filter.acceptChange(controller, oldResource, resource)) { getEventHandler().handleEvent( - new ResourceEvent(action, ResourceID.fromResource(resource))); + new ResourceEvent(action, ResourceID.fromResource(resource), resource)); } else { log.debug("Skipping event handling resource {} with version: {}", getUID(resource), getVersion(resource)); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceEvent.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceEvent.java index 15424fadb4..ba9ea72cd9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceEvent.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceEvent.java @@ -1,16 +1,22 @@ package io.javaoperatorsdk.operator.processing.event.source.controller; +import java.util.Objects; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.processing.event.Event; import io.javaoperatorsdk.operator.processing.event.ResourceID; public class ResourceEvent extends Event { private final ResourceAction action; + private final HasMetadata resource; public ResourceEvent(ResourceAction action, - ResourceID resourceID) { + ResourceID resourceID, HasMetadata resource) { super(resourceID); this.action = action; + this.resource = resource; } @Override @@ -25,4 +31,24 @@ public ResourceAction getAction() { return action; } + public Optional getResource() { + return Optional.ofNullable(resource); + } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + if (!super.equals(o)) + return false; + ResourceEvent that = (ResourceEvent) o; + return action == that.action; + } + + @Override + public int hashCode() { + return Objects.hash(super.hashCode(), action); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java index e8361c49cf..a7b6b46d17 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/TestUtils.java @@ -3,6 +3,7 @@ import java.util.HashMap; import java.util.UUID; +import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition; import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinitionBuilder; @@ -48,5 +49,8 @@ public static TestCustomResource testCustomResource(ResourceID id) { return resource; } + public static void markForDeletion(HasMetadata customResource) { + customResource.getMetadata().setDeletionTimestamp("2019-8-10"); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java index f2868b8c62..6b7798d766 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java @@ -23,6 +23,7 @@ import io.javaoperatorsdk.operator.processing.retry.GenericRetry; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; +import static io.javaoperatorsdk.operator.TestUtils.markForDeletion; import static io.javaoperatorsdk.operator.TestUtils.testCustomResource; import static org.assertj.core.api.Assertions.assertThat; import static org.awaitility.Awaitility.await; @@ -275,6 +276,52 @@ void notUpdatesEventSourceHandlerIfResourceUpdated() { any()); } + @Test + void notReschedulesAfterTheFinalizerRemoveProcessed() { + TestCustomResource customResource = testCustomResource(); + markForDeletion(customResource); + ExecutionScope executionScope = new ExecutionScope(customResource, null); + PostExecutionControl postExecutionControl = + PostExecutionControl.customResourceFinalizerRemoved(customResource); + + eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); + + verify(reconciliationDispatcherMock, timeout(50).times(0)).handleExecution(any()); + } + + @Test + void skipEventProcessingIfFinalizerRemoveProcessed() { + TestCustomResource customResource = testCustomResource(); + markForDeletion(customResource); + ExecutionScope executionScope = new ExecutionScope(customResource, null); + PostExecutionControl postExecutionControl = + PostExecutionControl.customResourceFinalizerRemoved(customResource); + + eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); + eventProcessorWithRetry.handleEvent(prepareCREvent(customResource)); + + verify(reconciliationDispatcherMock, timeout(50).times(0)).handleExecution(any()); + } + + /** + * Cover corner case when a delete event missed and a new resource with same ResourceID is created + */ + @Test + void newResourceAfterMissedDeleteEvent() { + TestCustomResource customResource = testCustomResource(); + markForDeletion(customResource); + ExecutionScope executionScope = new ExecutionScope(customResource, null); + PostExecutionControl postExecutionControl = + PostExecutionControl.customResourceFinalizerRemoved(customResource); + var newResource = testCustomResource(); + newResource.getMetadata().setName(customResource.getMetadata().getName()); + + eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl); + eventProcessorWithRetry.handleEvent(prepareCREvent(newResource)); + + verify(reconciliationDispatcherMock, timeout(50).times(1)).handleExecution(any()); + } + private ResourceID eventAlreadyUnderProcessing() { when(reconciliationDispatcherMock.handleExecution(any())) .then( @@ -291,11 +338,19 @@ private ResourceEvent prepareCREvent() { return prepareCREvent(new ResourceID(UUID.randomUUID().toString(), TEST_NAMESPACE)); } - private ResourceEvent prepareCREvent(ResourceID uid) { - TestCustomResource customResource = testCustomResource(uid); - when(controllerResourceEventSourceMock.get(eq(uid))).thenReturn(Optional.of(customResource)); + private ResourceEvent prepareCREvent(HasMetadata hasMetadata) { + when(controllerResourceEventSourceMock.get(eq(ResourceID.fromResource(hasMetadata)))) + .thenReturn(Optional.of(hasMetadata)); + return new ResourceEvent(ResourceAction.UPDATED, + ResourceID.fromResource(hasMetadata), hasMetadata); + } + + private ResourceEvent prepareCREvent(ResourceID resourceID) { + TestCustomResource customResource = testCustomResource(resourceID); + when(controllerResourceEventSourceMock.get(eq(resourceID))) + .thenReturn(Optional.of(customResource)); return new ResourceEvent(ResourceAction.UPDATED, - ResourceID.fromResource(customResource)); + ResourceID.fromResource(customResource), customResource); } private Event nonCREvent(ResourceID relatedCustomResourceUid) { 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 fabf36b529..c53648b4d1 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 @@ -12,23 +12,35 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatchers; +import org.mockito.stubbing.Answer; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.api.model.ObjectMeta; import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.javaoperatorsdk.operator.MockKubernetesClient; +import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.api.config.Cloner; import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration; import io.javaoperatorsdk.operator.api.config.RetryConfiguration; -import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.api.reconciler.Cleaner; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.DeleteControl; +import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusHandler; +import io.javaoperatorsdk.operator.api.reconciler.ErrorStatusUpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.RetryInfo; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; import io.javaoperatorsdk.operator.processing.Controller; import io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.CustomResourceFacade; import io.javaoperatorsdk.operator.sample.observedgeneration.ObservedGenCustomResource; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; +import static io.javaoperatorsdk.operator.TestUtils.markForDeletion; +import static io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.MAX_FINALIZER_REMOVAL_RETRY; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -193,6 +205,74 @@ void callsDeleteIfObjectHasFinalizerAndMarkedForDelete() { verify(reconciler, times(1)).cleanup(eq(testCustomResource), any()); } + @Test + void removesDefaultFinalizerOnDeleteIfSet() { + testCustomResource.addFinalizer(DEFAULT_FINALIZER); + markForDeletion(testCustomResource); + + var postExecControl = + reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); + + assertThat(postExecControl.isFinalizerRemoved()).isTrue(); + verify(customResourceFacade, times(1)).replaceResourceWithLock(testCustomResource); + } + + @Test + void retriesFinalizerRemovalWithFreshResource() { + testCustomResource.addFinalizer(DEFAULT_FINALIZER); + markForDeletion(testCustomResource); + var resourceWithFinalizer = TestUtils.testCustomResource(); + resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); + when(customResourceFacade.replaceResourceWithLock(testCustomResource)) + .thenThrow(new KubernetesClientException(null, 409, null)) + .thenReturn(testCustomResource); + when(customResourceFacade.getResource(any(), any())).thenReturn(resourceWithFinalizer); + + var postExecControl = + reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); + + assertThat(postExecControl.isFinalizerRemoved()).isTrue(); + verify(customResourceFacade, times(2)).replaceResourceWithLock(any()); + verify(customResourceFacade, times(1)).getResource(any(), any()); + } + + @Test + void throwsExceptionIfFinalizerRemovalRetryExceeded() { + testCustomResource.addFinalizer(DEFAULT_FINALIZER); + markForDeletion(testCustomResource); + when(customResourceFacade.replaceResourceWithLock(any())) + .thenThrow(new KubernetesClientException(null, 409, null)); + when(customResourceFacade.getResource(any(), any())) + .thenAnswer((Answer) invocationOnMock -> createResourceWithFinalizer()); + + var postExecControl = + reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); + + assertThat(postExecControl.isFinalizerRemoved()).isFalse(); + assertThat(postExecControl.getRuntimeException()).isPresent(); + assertThat(postExecControl.getRuntimeException().get()) + .isInstanceOf(OperatorException.class); + verify(customResourceFacade, times(MAX_FINALIZER_REMOVAL_RETRY)).replaceResourceWithLock(any()); + verify(customResourceFacade, times(MAX_FINALIZER_REMOVAL_RETRY - 1)).getResource(any(), + any()); + } + + @Test + void throwsExceptionIfFinalizerRemovalClientExceptionIsNotConflict() { + testCustomResource.addFinalizer(DEFAULT_FINALIZER); + markForDeletion(testCustomResource); + when(customResourceFacade.replaceResourceWithLock(any())) + .thenThrow(new KubernetesClientException(null, 400, null)); + + var res = + reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); + + assertThat(res.getRuntimeException()).isPresent(); + assertThat(res.getRuntimeException().get()).isInstanceOf(KubernetesClientException.class); + verify(customResourceFacade, times(1)).replaceResourceWithLock(any()); + verify(customResourceFacade, never()).getResource(any(), any()); + } + @Test void doesNotCallDeleteOnControllerIfMarkedForDeletionWhenNoFinalizerIsConfigured() { final ReconciliationDispatcher dispatcher = @@ -223,16 +303,7 @@ void doesNotAddFinalizerIfConfiguredNotTo() { assertEquals(0, testCustomResource.getMetadata().getFinalizers().size()); } - @Test - void removesDefaultFinalizerOnDeleteIfSet() { - testCustomResource.addFinalizer(DEFAULT_FINALIZER); - markForDeletion(testCustomResource); - - reconciliationDispatcher.handleExecution(executionScopeWithCREvent(testCustomResource)); - assertEquals(0, testCustomResource.getMetadata().getFinalizers().size()); - verify(customResourceFacade, times(1)).replaceResourceWithLock(any()); - } @Test void doesNotRemovesTheSetFinalizerIfTheDeleteNotMethodInstructsIt() { @@ -565,10 +636,13 @@ private ObservedGenCustomResource createObservedGenCustomResource() { return observedGenCustomResource; } - private void markForDeletion(CustomResource customResource) { - customResource.getMetadata().setDeletionTimestamp("2019-8-10"); + TestCustomResource createResourceWithFinalizer() { + var resourceWithFinalizer = TestUtils.testCustomResource(); + resourceWithFinalizer.addFinalizer(DEFAULT_FINALIZER); + return resourceWithFinalizer; } + private void removeFinalizers(CustomResource customResource) { customResource.getMetadata().getFinalizers().clear(); } 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..65f783336f --- /dev/null +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CleanupConflictIT.java @@ -0,0 +1,57 @@ +package io.javaoperatorsdk.operator; + +import java.time.Duration; + +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 = "javaoperatorsdk.io/additionalfinalizer"; + public static final String TEST_RESOURCE_NAME = "test1"; + + @RegisterExtension + LocalOperatorExtension operator = + LocalOperatorExtension.builder().withReconciler(new CleanupConflictReconciler()) + .build(); + + @Test + void cleanupRemovesFinalizerWithoutConflict() throws InterruptedException { + var testResource = createTestResource(); + testResource.addFinalizer(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(1); + }); + } + + 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(); + } +}