refactor: compute if workflow has cleaner based on specs

Author: metacosm

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java

@@ -1,6 +1,6 @@
 package io.javaoperatorsdk.operator.processing.dependent;
 
-import java.util.*;
+import java.util.Optional;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -9,6 +9,7 @@
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.Ignore;
 import io.javaoperatorsdk.operator.api.reconciler.ResourceDiscriminator;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult;
 import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result;
@@ -21,6 +22,7 @@ public abstract class AbstractDependentResource<R, P extends HasMetadata>
 
   private final boolean creatable = this instanceof Creator;
   private final boolean updatable = this instanceof Updater;
+  private final boolean deletable = this instanceof Deleter;
 
   protected Creator<R, P> creator;
   protected Updater<R, P> updater;
@@ -172,4 +174,9 @@ protected boolean isCreatable() {
   protected boolean isUpdatable() {
     return updatable;
   }
+
+  @Override
+  public boolean isDeletable() {
+    return deletable;
+  }
 }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupport.java

@@ -50,16 +50,18 @@ public <P extends HasMetadata> Workflow<P> createWorkflow(
       List<DependentResourceSpec> dependentResourceSpecs) {
     var orderedResourceSpecs = orderAndDetectCycles(dependentResourceSpecs);
     final var alreadyCreated = new ArrayList<DependentResourceNode>(orderedResourceSpecs.size());
+    final boolean cleanerHolder[] = {false};
     final var nodes = orderedResourceSpecs.stream()
-        .map(spec -> createFrom(spec, alreadyCreated))
+        .map(spec -> createFrom(spec, alreadyCreated, cleanerHolder))
         .collect(Collectors.toSet());
-    return new Workflow<>(nodes);
+    return new Workflow<>(nodes, cleanerHolder[0]);
   }
 
   private DependentResourceNode createFrom(DependentResourceSpec spec,
-      List<DependentResourceNode> alreadyCreated) {
+      List<DependentResourceNode> alreadyCreated, boolean[] cleanerHolder) {
     final var node = new SpecDependentResourceNode<>(spec);
     alreadyCreated.add(node);
+    cleanerHolder[0] = Workflow.isDeletable(spec.getDependentResourceClass());
     spec.getDependsOn().forEach(depend -> {
       final DependentResourceNode dependsOn = alreadyCreated.stream()
           .filter(drn -> depend.equals(drn.getName())).findFirst()

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Workflow.java

@@ -34,23 +34,40 @@ public class Workflow<P extends HasMetadata> {
   // it's "global" executor service shared between multiple reconciliations running parallel
   private final ExecutorService executorService;
   private boolean resolved;
-  private boolean hasCleaner;
+  private final boolean hasCleaner;
 
-  Workflow(Set<DependentResourceNode> dependentResourceNodes) {
+  Workflow(Set<DependentResourceNode> dependentResourceNodes, boolean hasCleaner) {
     this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(),
-        THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false, false);
+        THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false, hasCleaner);
   }
 
   Workflow(Set<DependentResourceNode> dependentResourceNodes,
       ExecutorService executorService, boolean throwExceptionAutomatically, boolean resolved,
       boolean hasCleaner) {
     this.executorService = executorService;
-    this.dependentResourceNodes = dependentResourceNodes.stream()
-        .collect(Collectors.toMap(DependentResourceNode::getName, Function.identity()));
+    this.dependentResourceNodes = toMap(dependentResourceNodes);
     this.throwExceptionAutomatically = throwExceptionAutomatically;
     this.resolved = resolved;
     this.hasCleaner = hasCleaner;
-    preprocessForReconcile();
+  }
+
+  private Map<String, DependentResourceNode> toMap(
+      Set<DependentResourceNode> dependentResourceNodes) {
+    final var nodes = new ArrayList<>(dependentResourceNodes);
+    bottomLevelResource.addAll(nodes);
+    return dependentResourceNodes.stream()
+        .peek(drn -> {
+          // add cycle detection?
+          if (drn.getDependsOn().isEmpty()) {
+            topLevelResources.add(drn);
+          } else {
+            for (DependentResourceNode dependsOn : (List<DependentResourceNode>) drn
+                .getDependsOn()) {
+              bottomLevelResource.remove(dependsOn);
+            }
+          }
+        })
+        .collect(Collectors.toMap(DependentResourceNode::getName, Function.identity()));
   }
 
   public DependentResource getDependentResourceFor(DependentResourceNode node) {
@@ -92,22 +109,6 @@ public WorkflowCleanupResult cleanup(P primary, Context<P> context) {
     return result;
   }
 
-  // add cycle detection?
-  @SuppressWarnings("unchecked")
-  private void preprocessForReconcile() {
-    final var nodes = new ArrayList<>(dependentResourceNodes.values());
-    bottomLevelResource.addAll(nodes);
-    for (DependentResourceNode<?, P> node : nodes) {
-      if (node.getDependsOn().isEmpty()) {
-        topLevelResources.add(node);
-      } else {
-        for (DependentResourceNode dependsOn : node.getDependsOn()) {
-          bottomLevelResource.remove(dependsOn);
-        }
-      }
-    }
-  }
-
   Set<DependentResourceNode> getTopLevelDependentResources() {
     return topLevelResources;
   }
@@ -127,22 +128,25 @@ Map<String, DependentResourceNode> nodes() {
   @SuppressWarnings("unchecked")
   void resolve(KubernetesClient client, List<DependentResourceSpec> dependentResources) {
     if (!resolved) {
-      final boolean[] cleanerHolder = {false};
-      dependentResourceNodes.values()
-          .forEach(drn -> {
-            drn.resolve(client, dependentResources);
-            final var dr = dependentResource(drn);
-            if (dr.isDeletable()) {
-              cleanerHolder[0] = true;
-            }
-          });
+      dependentResourceNodes.values().forEach(drn -> drn.resolve(client, dependentResources));
       resolved = true;
       hasCleaner = cleanerHolder[0];
     }
   }
 
   boolean hasCleaner() {
-    throwIfUnresolved();
     return hasCleaner;
   }
+
+  static boolean isDeletable(Class<? extends DependentResource> drClass) {
+    final var isDeleter = Deleter.class.isAssignableFrom(drClass);
+    if (!isDeleter) {
+      return false;
+    }
+
+    if (KubernetesDependentResource.class.isAssignableFrom(drClass)) {
+      return !GarbageCollected.class.isAssignableFrom(drClass);
+    }
+    return true;
+  }
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowTest.java

@@ -5,11 +5,17 @@
 
 import org.junit.jupiter.api.Test;
 
+import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
+import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
 import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.withSettings;
 
 @SuppressWarnings("rawtypes")
 class WorkflowTest {
@@ -54,4 +60,23 @@ void calculatesBottomLevelResources() {
     assertThat(bottomResources).containsExactlyInAnyOrder(dr2, independentDR);
   }
 
+
+  @Test
+  void isDeletableShouldWork() {
+    var dr = mock(DependentResource.class);
+    assertFalse(Workflow.isDeletable(dr.getClass()));
+
+    dr = mock(DependentResource.class, withSettings().extraInterfaces(Deleter.class));
+    assertTrue(Workflow.isDeletable(dr.getClass()));
+
+    dr = mock(KubernetesDependentResource.class);
+    assertFalse(Workflow.isDeletable(dr.getClass()));
+
+    dr = mock(KubernetesDependentResource.class, withSettings().extraInterfaces(Deleter.class));
+    assertTrue(Workflow.isDeletable(dr.getClass()));
+
+    dr = mock(KubernetesDependentResource.class, withSettings().extraInterfaces(Deleter.class,
+        GarbageCollected.class));
+    assertFalse(Workflow.isDeletable(dr.getClass()));
+  }
 }