feat: compute cleaner status based on specs, resolve from config (#1648)

metacosm · web-flow · commit 47ef0760a5e1 · 2022-12-09T14:22:42.000+01:00
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java
@@ -84,7 +84,7 @@ public Controller(Reconciler<P> reconciler,
     contextInitializer = reconciler instanceof ContextInitializer;
     isCleaner = reconciler instanceof Cleaner;
     managedWorkflow = configurationService.getWorkflowFactory().workflowFor(configuration);
-    managedWorkflow.resolve(kubernetesClient, configuration.getDependentResources());
+    managedWorkflow.resolve(kubernetesClient, configuration);
 
     eventSourceManager = new EventSourceManager<>(this);
     eventProcessor = new EventProcessor<>(eventSourceManager);
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java b/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;
+  }
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultManagedWorkflow.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultManagedWorkflow.java
@@ -6,6 +6,7 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
@@ -49,9 +50,10 @@ public Map<String, DependentResource> getDependentResourcesByName() {
   }
 
   @Override
-  public ManagedWorkflow<P> resolve(KubernetesClient client, List<DependentResourceSpec> specs) {
+  public ManagedWorkflow<P> resolve(KubernetesClient client,
+      ControllerConfiguration<P> configuration) {
     if (!resolved) {
-      workflow.resolve(client, specs);
+      workflow.resolve(client, configuration);
       resolved = true;
     }
     return this;
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java
@@ -5,7 +5,7 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
-import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 
 @SuppressWarnings("rawtypes")
 public interface DependentResourceNode<R, P extends HasMetadata> {
@@ -26,5 +26,5 @@ public interface DependentResourceNode<R, P extends HasMetadata> {
 
   String getName();
 
-  default void resolve(KubernetesClient client, List<DependentResourceSpec> dependentResources) {}
+  default void resolve(KubernetesClient client, ControllerConfiguration<P> configuration) {}
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflow.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflow.java
@@ -1,12 +1,11 @@
 package io.javaoperatorsdk.operator.processing.dependent.workflow;
 
 import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
-import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 
@@ -40,7 +39,7 @@ public Map<String, DependentResource> getDependentResourcesByName() {
     }
 
     @Override
-    public ManagedWorkflow resolve(KubernetesClient client, List dependentResources) {
+    public ManagedWorkflow resolve(KubernetesClient client, ControllerConfiguration configuration) {
       return this;
     }
   };
@@ -55,6 +54,5 @@ public ManagedWorkflow resolve(KubernetesClient client, List dependentResources)
 
   Map<String, DependentResource> getDependentResourcesByName();
 
-  ManagedWorkflow<P> resolve(KubernetesClient client,
-      List<DependentResourceSpec> dependentResources);
+  ManagedWorkflow<P> resolve(KubernetesClient client, ControllerConfiguration<P> configuration);
 }
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupport.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupport.java
@@ -50,16 +50,19 @@ 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);
+    // if any previously checked dependent was a cleaner, no need to check further
+    cleanerHolder[0] = cleanerHolder[0] || Workflow.isDeletable(spec.getDependentResourceClass());
     spec.getDependsOn().forEach(depend -> {
       final DependentResourceNode dependsOn = alreadyCreated.stream()
           .filter(drn -> depend.equals(drn.getName())).findFirst()
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/SpecDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/SpecDependentResourceNode.java
@@ -1,9 +1,8 @@
 package io.javaoperatorsdk.operator.processing.dependent.workflow;
 
-import java.util.List;
-
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer;
@@ -21,8 +20,8 @@ public SpecDependentResourceNode(DependentResourceSpec<R, P, ?> spec) {
 
   @Override
   @SuppressWarnings({"rawtypes", "unchecked"})
-  public void resolve(KubernetesClient client, List<DependentResourceSpec> dependentResources) {
-    final var spec = dependentResources.stream()
+  public void resolve(KubernetesClient client, ControllerConfiguration<P> configuration) {
+    final var spec = configuration.getDependentResources().stream()
         .filter(drs -> drs.getName().equals(getName()))
         .findFirst().orElseThrow();
 
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Workflow.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Workflow.java
@@ -11,10 +11,13 @@
 
 import io.fabric8.kubernetes.api.model.HasMetadata;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager;
-import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 import io.javaoperatorsdk.operator.api.reconciler.Context;
+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;
 
 /**
  * Dependents definition: so if B depends on A, the B is dependent of A.
@@ -34,23 +37,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 +112,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;
   }
@@ -125,24 +129,26 @@ Map<String, DependentResourceNode> nodes() {
   }
 
   @SuppressWarnings("unchecked")
-  void resolve(KubernetesClient client, List<DependentResourceSpec> dependentResources) {
+  void resolve(KubernetesClient client, ControllerConfiguration<P> configuration) {
     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, configuration));
       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;
+  }
 }
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupportTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowSupportTest.java
@@ -8,13 +8,11 @@
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
-import io.fabric8.kubernetes.client.KubernetesClient;
 import io.javaoperatorsdk.operator.OperatorException;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
 
 import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowTestUtils.createDRS;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.mock;
 
 class ManagedWorkflowSupportTest {
 
@@ -140,9 +138,7 @@ void createsWorkflow() {
         createDRS(NAME_3, NAME_1),
         createDRS(NAME_4, NAME_3, NAME_2));
 
-    final var client = mock(KubernetesClient.class);
     var workflow = managedWorkflowSupport.createWorkflow(specs);
-    workflow.resolve(client, specs);
 
     assertThat(workflow.nodes().values()).map(DependentResourceNode::getName)
         .containsExactlyInAnyOrder(NAME_1, NAME_2, NAME_3, NAME_4);
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowTest.java
@@ -4,7 +4,6 @@
 
 import org.junit.jupiter.api.Test;
 
-import io.fabric8.kubernetes.client.KubernetesClient;
 import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
 import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
@@ -17,7 +16,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-@SuppressWarnings({"rawtypes", "unchecked"})
+@SuppressWarnings({"rawtypes"})
 class ManagedWorkflowTest {
 
   public static final String NAME = "name";
@@ -39,6 +38,19 @@ void isNotCleanerIfGarbageCollected() {
         .isCleaner()).isFalse();
   }
 
+  @Test
+  void isCleanerShouldWork() {
+    assertThat(managedWorkflow(
+        createDRSWithTraits(NAME, GarbageCollected.class),
+        createDRSWithTraits("foo", Deleter.class))
+        .isCleaner()).isTrue();
+
+    assertThat(managedWorkflow(
+        createDRSWithTraits("foo", Deleter.class),
+        createDRSWithTraits(NAME, GarbageCollected.class))
+        .isCleaner()).isTrue();
+  }
+
   @Test
   void isCleanerIfHasDeleter() {
     var spec = createDRSWithTraits(NAME, Deleter.class);
@@ -49,12 +61,9 @@ ManagedWorkflow managedWorkflow(DependentResourceSpec... specs) {
     final var configuration = mock(ControllerConfiguration.class);
     final var specList = List.of(specs);
 
-    KubernetesClient kubernetesClientMock = mock(KubernetesClient.class);
-
     when(configuration.getDependentResources()).thenReturn(specList);
     return ConfigurationServiceProvider.instance().getWorkflowFactory()
-        .workflowFor(configuration)
-        .resolve(kubernetesClientMock, specList);
+        .workflowFor(configuration);
   }
 
 }
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowTestUtils.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowTestUtils.java
@@ -9,6 +9,7 @@
 import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource;
 import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected;
 import io.javaoperatorsdk.operator.processing.dependent.EmptyTestDependentResource;
+import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -31,13 +32,12 @@ public static DependentResourceSpec createDRSWithTraits(String name,
     Class<? extends DependentResource> toMock = DependentResource.class;
     final var garbageCollected = dependentResourceTraits != null &&
         Arrays.asList(dependentResourceTraits).contains(GarbageCollected.class);
+    if (garbageCollected) {
+      toMock = KubernetesDependentResource.class;
+    }
 
     final var dr = mock(toMock, withSettings().extraInterfaces(dependentResourceTraits));
-    // it would be better to call the real method here but it doesn't work because
-    // KubernetesDependentResource checks for GarbageCollected trait when instantiated which doesn't
-    // happen when using mocks
-    when(dr.isDeletable()).thenReturn(!garbageCollected);
-    when(spy.getDependentResource()).thenReturn(dr);
+    when(spy.getDependentResourceClass()).thenReturn(dr.getClass());
     return spy;
   }
 
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowTest.java
