fixes for code review

Signed-off-by: Attila Mészáros <[email protected]>

Author: csviri

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java

@@ -274,11 +274,20 @@ public static <P extends HasMetadata> P removeFinalizer(
         r -> r.hasFinalizer(finalizerName));
   }
 
+  /**
+   * @param client KubernetesClient
+   * @param resource to update
+   * @param resourceChangesOperator changes to be done on the resource before update
+   * @param preCondition condition to check if the patch operation still needs to be performed or
+   *     not.
+   * @return updated resource or unchanged if the precondition does not hold.
+   * @param <P> resource type
+   */
   @SuppressWarnings("unchecked")
   public static <P extends HasMetadata> P conflictRetryingPatch(
       KubernetesClient client,
       P resource,
-      UnaryOperator<P> unaryOperator,
+      UnaryOperator<P> resourceChangesOperator,
       Predicate<P> preCondition) {
     if (log.isDebugEnabled()) {
       log.debug("Conflict retrying update for: {}", ResourceID.fromResource(resource));
@@ -289,7 +298,7 @@ public static <P extends HasMetadata> P conflictRetryingPatch(
         if (!preCondition.test(resource)) {
           return resource;
         }
-        return client.resource(resource).edit(unaryOperator);
+        return client.resource(resource).edit(resourceChangesOperator);
       } catch (KubernetesClientException e) {
         log.trace("Exception during patch for resource: {}", resource);
         retryIndex++;

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

@@ -145,7 +145,7 @@ private void submitReconciliationExecution(ResourceState state) {
       Optional<P> maybeLatest = cache.get(resourceID);
       maybeLatest.ifPresent(MDCUtils::addResourceInfo);
       if (!controllerUnderExecution
-          && (maybeLatest.isPresent() || (triggerOnAllEvent() && state.deleteEventPresent()))) {
+          && (maybeLatest.isPresent() || isTriggerOnAllEventAndDeleteEventPresent(state))) {
         var rateLimit = state.getRateLimit();
         if (rateLimit == null) {
           rateLimit = rateLimiter.initState();
@@ -228,7 +228,7 @@ private void handleEventMarking(Event event, ResourceState state) {
       }
     } else if (!state.deleteEventPresent() && !state.processedMarkForDeletionPresent()) {
       state.markEventReceived(triggerOnAllEvent());
-    } else if (triggerOnAllEvent() && state.deleteEventPresent()) {
+    } else if (isTriggerOnAllEventAndDeleteEventPresent(state)) {
       state.markAdditionalEventAfterDeleteEvent();
     } else if (log.isDebugEnabled()) {
       log.debug(
@@ -285,7 +285,7 @@ synchronized void eventProcessingFinished(
       state.markProcessedMarkForDeletion();
       metrics.cleanupDoneFor(resourceID, metricsMetadata);
     } else {
-      if (state.eventPresent() || (triggerOnAllEvent() && state.deleteEventPresent())) {
+      if (state.eventPresent() || isTriggerOnAllEventAndDeleteEventPresent(state)) {
         log.debug("Submitting for reconciliation.");
         submitReconciliationExecution(state);
       } else {
@@ -294,6 +294,10 @@ synchronized void eventProcessingFinished(
     }
   }
 
+  private boolean isTriggerOnAllEventAndDeleteEventPresent(ResourceState state) {
+    return triggerOnAllEvent() && state.deleteEventPresent();
+  }
+
   /**
    * In case retry is configured more complex error logging takes place, see handleRetryOnException
    */
@@ -502,15 +506,16 @@ public void run() {
             log.debug(
                 "Resource not found in the cache, checking for delete event resource: {}",
                 resourceID);
-            var state = resourceStateManager.get(resourceID);
             if (executionScope.isDeleteEvent()) {
+              var state = resourceStateManager.get(resourceID);
               actualResource =
                   (Optional<P>)
                       state
                           .filter(ResourceState::deleteEventPresent)
                           .map(ResourceState::getLastKnownResource);
               if (actualResource.isEmpty()) {
-                throw new IllegalStateException("this should not happen");
+                throw new IllegalStateException(
+                    "ActualResource should be always present, either from cache or delete event.");
               }
             } else {
               log.debug("Skipping execution since delete event received meanwhile");

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

@@ -64,6 +64,7 @@ public ReconciliationDispatcher(Controller<P> controller) {
   }
 
   public PostExecutionControl<P> handleExecution(ExecutionScope<P> executionScope) {
+    validateExecutionScope(executionScope);
     try {
       return handleDispatch(executionScope);
     } catch (Exception e) {
@@ -90,7 +91,6 @@ && shouldNotDispatchToCleanupWhenMarkedForDeletion(originalResource)) {
           originalResource.getMetadata().getFinalizers());
       return PostExecutionControl.defaultDispatch();
     }
-
     Context<P> context =
         new DefaultContext<>(
             executionScope.getRetryInfo(),
@@ -435,6 +435,15 @@ public P conflictRetryingPatch(
     }
   }
 
+  private void validateExecutionScope(ExecutionScope<P> executionScope) {
+    if (!triggerOnAllEvent()
+        && (executionScope.isDeleteEvent() || executionScope.isDeleteFinalStateUnknown())) {
+      throw new OperatorException(
+          "isDeleteEvent or isDeleteFinalStateUnknown cannot be true if not triggerOnAllEvent."
+              + " This indicates an issue with the implementation.");
+    }
+  }
+
   // created to support unit testing
   static class CustomResourceFacade<R extends HasMetadata> {
 

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

@@ -38,7 +38,7 @@ private enum EventingState {
   private EventingState eventing;
   private RateLimitState rateLimit;
   private HasMetadata lastKnownResource;
-  private boolean isDeleteFinalStateUnknown;
+  private boolean isDeleteFinalStateUnknown = false;
 
   public ResourceState(ResourceID id) {
     this.id = id;

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ControllerEventSource.java

@@ -124,9 +124,9 @@ public void onUpdate(T oldCustomResource, T newCustomResource) {
   }
 
   @Override
-  public void onDelete(T resource, boolean b) {
-    super.onDelete(resource, b);
-    eventReceived(ResourceAction.DELETED, resource, null, b);
+  public void onDelete(T resource, boolean deletedFinalStateUnknown) {
+    super.onDelete(resource, deletedFinalStateUnknown);
+    eventReceived(ResourceAction.DELETED, resource, null, deletedFinalStateUnknown);
   }
 
   @Override

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceDeleteEvent.java

@@ -1,8 +1,14 @@
 package io.javaoperatorsdk.operator.processing.event.source.controller;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.javaoperatorsdk.operator.processing.event.ResourceID;
 
+/**
+ * Extends ResourceEvent for informer Delete events, it holds also information if the final stat is
+ * unknown for the deleted resource.
+ */
 public class ResourceDeleteEvent extends ResourceEvent {
 
   private final boolean deletedFinalStateUnknown;
@@ -19,4 +25,18 @@ public ResourceDeleteEvent(
   public boolean isDeletedFinalStateUnknown() {
     return deletedFinalStateUnknown;
   }
+
+  @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;
+    ResourceDeleteEvent that = (ResourceDeleteEvent) o;
+    return deletedFinalStateUnknown == that.deletedFinalStateUnknown;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(super.hashCode(), deletedFinalStateUnknown);
+  }
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java

@@ -687,6 +687,24 @@ void reSchedulesFromErrorHandler() {
     assertThat(res.getRuntimeException()).isEmpty();
   }
 
+  @Test
+  void isDeleteEventCannotBeTrueIfNotTriggerOnAllEvent() {
+    assertThrows(
+        OperatorException.class,
+        () ->
+            reconciliationDispatcher.handleExecution(
+                new ExecutionScope(null, null, true, false).setResource(testCustomResource)));
+  }
+
+  @Test
+  void isDeleteFinalStateUnknownEventCannotBeTrueIfNotTriggerOnAllEvent() {
+    assertThrows(
+        OperatorException.class,
+        () ->
+            reconciliationDispatcher.handleExecution(
+                new ExecutionScope(null, null, false, true).setResource(testCustomResource)));
+  }
+
   @Test
   void reconcilerContextUsesTheSameInstanceOfResourceAsParam() {
     initConfigService(false, false);
@@ -709,9 +727,6 @@ void reconcilerContextUsesTheSameInstanceOfResourceAsParam() {
         .isNotSameAs(testCustomResource);
   }
 
-  @Test
-  void procAllEventModeNoReSchedulesAllowedForDeleteEvent() {}
-
   private ObservedGenCustomResource createObservedGenCustomResource() {
     ObservedGenCustomResource observedGenCustomResource = new ObservedGenCustomResource();
     observedGenCustomResource.setMetadata(new ObjectMeta());