feat: retry remove finalizer (#1249)

Author: csviri

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

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) {}
 

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());
   }

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<R> retryEventSource() {
   private void handleRetryOnException(
       ExecutionScope<R> 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<Long> 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<R> 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() {

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

@@ -6,26 +6,26 @@
 
 final class PostExecutionControl<R extends HasMetadata> {
 
-  private final boolean onlyFinalizerHandled;
+  private final boolean finalizerRemoved;
   private final R updatedCustomResource;
   private final boolean updateIsStatusPatch;
   private final Exception runtimeException;
 
   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;
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> onlyFinalizerAdded(
       R updatedCustomResource) {
-    return new PostExecutionControl<>(true, updatedCustomResource, false, null);
+    return new PostExecutionControl<>(false, updatedCustomResource, false, null);
   }
 
   public static <R extends HasMetadata> PostExecutionControl<R> defaultDispatch() {
@@ -42,6 +42,11 @@ public static <R extends HasMetadata> PostExecutionControl<R> customResourceUpda
     return new PostExecutionControl<>(false, updatedCustomResource, false, null);
   }
 
+  public static <R extends HasMetadata> PostExecutionControl<R> customResourceFinalizerRemoved(
+      R updatedCustomResource) {
+    return new PostExecutionControl<>(true, updatedCustomResource, false, null);
+  }
+
   public static <R extends HasMetadata> PostExecutionControl<R> 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;
+  }
 }