code improvements handling delete event too

Author: csviri

operator-framework/src/main/java/com/github/containersolutions/operator/ControllerUtils.java

@@ -9,7 +9,7 @@
 class ControllerUtils {
 
     static String getDefaultFinalizer(ResourceController controller) {
-        return getAnnotation(controller).defaultFinalizer();
+        return getAnnotation(controller).finalizerName();
     }
 
     static <R extends CustomResource> Class<R> getCustomResourceClass(ResourceController controller) {

operator-framework/src/main/java/com/github/containersolutions/operator/api/Controller.java

@@ -23,6 +23,6 @@
 
     Class<? extends CustomResourceDoneable<? extends CustomResource>> customResourceDoneableClass();
 
-    String defaultFinalizer() default DEFAULT_FINALIZER;
+    String finalizerName() default DEFAULT_FINALIZER;
 
 }

operator-framework/src/main/java/com/github/containersolutions/operator/api/ResourceController.java

@@ -10,6 +10,8 @@ public interface ResourceController<R extends CustomResource> {
      * The implementation should delete the associated component(s). Note that this is method is called when an object
      * is marked for deletion. After its executed the default finalizer is automatically removed by the framework;
      * unless the return value is false - note that this is almost never the case.
+     * Its important to have the implementation also idempotent, in the current implementation to cover all edge cases
+     * actually will be executed mostly twice.
      *
      * @param resource
      * @return true - so the finalizer is automatically removed after the call.

operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventDispatcher.java

@@ -32,44 +32,38 @@ public EventDispatcher(ResourceController controller,
 
     public void handleEvent(Watcher.Action action, CustomResource resource) {
         log.info("Handling event {} for resource {}", action, resource.getMetadata());
-        if (action == Watcher.Action.MODIFIED || action == Watcher.Action.ADDED) {
-            // Its interesting problem if we should call delete if received event after object is marked for deletion
-            // but there is not our finalizer. Since it can happen that there are multiple finalizers, also other events after
-            // we called delete and remove finalizers already. But also it can happen that we did not manage to put
-            // finalizer into the resource before marked for delete. So for now we will call delete every time, since delete
-            // operation should be idempotent too, and this way we cover the corner case.
-            if (markedForDeletion(resource)) {
-                boolean removeFinalizer = controller.deleteResource(resource);
-                if (removeFinalizer && hasDefaultFinalizer(resource)) {
-                    log.debug("Removing finalizer on {}: {}", resource.getMetadata().getName(), resource.getMetadata());
-                    removeDefaultFinalizer(resource);
-                }
-            } else {
-                Optional<CustomResource> updateResult = controller.createOrUpdateResource(resource);
-                if (updateResult.isPresent()) {
-                    log.debug("Updating resource: {} with version: {}", resource.getMetadata().getName(),
-                            resource.getMetadata().getResourceVersion());
-                    log.trace("Resource before update: {}", resource);
-                    CustomResource updatedResource = updateResult.get();
-                    addFinalizerIfNotPresent(updatedResource);
-                    replace(updatedResource);
-                    log.trace("Resource after update: {}", resource);
-                    // We always add the default finalizer if missing and not marked for deletion.
-                } else if (!hasDefaultFinalizer(resource) && !markedForDeletion(resource)) {
-                    log.debug("Adding finalizer for resource: {} version: {}", resource.getMetadata().getName(),
-                            resource.getMetadata().getResourceVersion());
-                    addFinalizerIfNotPresent(resource);
-                    replace(resource);
-                }
-            }
-        }
         if (Watcher.Action.ERROR == action) {
             log.error("Received error for resource: {}", resource.getMetadata().getName());
             return;
         }
-        if (Watcher.Action.DELETED == action) {
-            log.debug("Resource deleted: {}", resource.getMetadata().getName());
-            return;
+        // Its interesting problem if we should call delete if received event after object is marked for deletion
+        // but there is not our finalizer. Since it can happen that there are multiple finalizers, also other events after
+        // we called delete and remove finalizers already. But also it can happen that we did not manage to put
+        // finalizer into the resource before marked for delete. So for now we will call delete every time, since delete
+        // operation should be idempotent too, and this way we cover the corner case.
+        if (markedForDeletion(resource) || action == Watcher.Action.DELETED) {
+            boolean removeFinalizer = controller.deleteResource(resource);
+            if (removeFinalizer && hasDefaultFinalizer(resource)) {
+                log.debug("Removing finalizer on {}: {}", resource.getMetadata().getName(), resource.getMetadata());
+                removeDefaultFinalizer(resource);
+            }
+        } else {
+            Optional<CustomResource> updateResult = controller.createOrUpdateResource(resource);
+            if (updateResult.isPresent()) {
+                log.debug("Updating resource: {} with version: {}", resource.getMetadata().getName(),
+                        resource.getMetadata().getResourceVersion());
+                log.trace("Resource before update: {}", resource);
+                CustomResource updatedResource = updateResult.get();
+                addFinalizerIfNotPresent(updatedResource);
+                replace(updatedResource);
+                log.trace("Resource after update: {}", resource);
+                // We always add the default finalizer if missing and not marked for deletion.
+            } else if (!hasDefaultFinalizer(resource) && !markedForDeletion(resource)) {
+                log.debug("Adding finalizer for resource: {} version: {}", resource.getMetadata().getName(),
+                        resource.getMetadata().getResourceVersion());
+                addFinalizerIfNotPresent(resource);
+                replace(resource);
+            }
         }
     }
 

operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventScheduler.java

@@ -76,16 +76,12 @@ void scheduleEvent(CustomResourceEvent event) {
         log.debug("Scheduling event: {}", event);
         try {
             lock.lock();
-            if (event.getAction() == Action.DELETED) {
-                // we might want to have a state check here that only an event is scheduled for delete.
-                log.debug("Received delete action for event: {}", event);
-                return;
-            }
-            if (eventStore.processedNewerVersionBefore(event)) {
+            if (eventStore.receivedMoreRecentEventBefore(event)) {
                 log.debug("Skipping event processing since was processed event with newer version before. {}", event);
                 return;
             }
-            eventStore.updateLatestResourceVersionProcessed(event);
+            eventStore.updateLatestResourceVersionReceived(event);
+
             if (eventStore.containsOlderVersionOfNotScheduledEvent(event)) {
                 log.debug("Replacing event which is not scheduled yet, since incoming event is more recent. new Event:{}", event);
                 eventStore.addOrReplaceEventAsNotScheduledYet(event);
@@ -97,6 +93,7 @@ void scheduleEvent(CustomResourceEvent event) {
                 eventStore.addOrReplaceEventAsNotScheduledYet(event);
                 return;
             }
+
             Optional<Long> nextBackOff = event.nextBackOff();
             if (!nextBackOff.isPresent()) {
                 log.warn("Event limited max retry limit ({}), will be discarded. {}", MAX_RETRY_COUNT, event);

operator-framework/src/main/java/com/github/containersolutions/operator/processing/EventStore.java

@@ -42,7 +42,7 @@ public CustomResourceEvent removeEventUnderProcessing(String uid) {
         return eventsUnderProcessing.remove(uid);
     }
 
-    public void updateLatestResourceVersionProcessed(CustomResourceEvent event) {
+    public void updateLatestResourceVersionReceived(CustomResourceEvent event) {
         Long current = lastResourceVersion.get(event.resourceUid());
         long received = Long.parseLong(event.getResource().getMetadata().getResourceVersion());
         if (current == null || received > current) {
@@ -53,7 +53,7 @@ public void updateLatestResourceVersionProcessed(CustomResourceEvent event) {
         }
     }
 
-    public boolean processedNewerVersionBefore(CustomResourceEvent customResourceEvent) {
+    public boolean receivedMoreRecentEventBefore(CustomResourceEvent customResourceEvent) {
         Long lastVersionProcessed = lastResourceVersion.get(customResourceEvent.resourceUid());
         if (lastVersionProcessed == null) {
             return false;

operator-framework/src/test/java/com/github/containersolutions/operator/EventDispatcherTest.java

@@ -25,7 +25,7 @@
 class EventDispatcherTest {
 
     private CustomResource testCustomResource;
-    private EventDispatcher<CustomResource> eventDispatcher;
+    private EventDispatcher eventDispatcher;
     private ResourceController<CustomResource> resourceController = mock(ResourceController.class);
     private CustomResourceOperationsImpl<CustomResource, CustomResourceList<CustomResource>,
             CustomResourceDoneable<CustomResource>> resourceOperation = mock(CustomResourceOperationsImpl.class);

operator-framework/src/test/java/com/github/containersolutions/operator/EventSchedulerTest.java

@@ -25,9 +25,9 @@ class EventSchedulerTest {
 
     public static final int INVOCATION_DURATION = 80;
     @SuppressWarnings("unchecked")
-    private EventDispatcher<CustomResource> eventDispatcher = mock(EventDispatcher.class);
+    private EventDispatcher eventDispatcher = mock(EventDispatcher.class);
 
-    private EventScheduler<CustomResource> eventScheduler = new EventScheduler(eventDispatcher);
+    private EventScheduler eventScheduler = new EventScheduler(eventDispatcher);
 
     private List<EventProcessingDetail> eventProcessingList = Collections.synchronizedList(new ArrayList<>());