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 index 2d9e6b4007..8cdf98837f 100644 --- 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 @@ -82,7 +82,7 @@ public Controller(Reconciler

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 index dbfc34fe42..7a32938403 100644 --- 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 private final boolean creatable = this instanceof Creator; private final boolean updatable = this instanceof Updater; + private final boolean deletable = this instanceof Deleter; protected Creator creator; protected Updater 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 index 3096f1d4fb..b9bc29a32f 100644 --- 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 getDependentResourcesByName() { } @Override - public ManagedWorkflow

resolve(KubernetesClient client, List specs) { + public ManagedWorkflow

resolve(KubernetesClient client, + ControllerConfiguration

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 index 2c069bf999..11700c8d25 100644 --- 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 { @@ -26,5 +26,5 @@ public interface DependentResourceNode { String getName(); - default void resolve(KubernetesClient client, List dependentResources) {} + default void resolve(KubernetesClient client, ControllerConfiguration

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 index de2b634b5a..518ab69289 100644 --- 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 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 getDependentResourcesByName(); - ManagedWorkflow

resolve(KubernetesClient client, - List dependentResources); + ManagedWorkflow

resolve(KubernetesClient client, ControllerConfiguration

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 index 9b11784443..24525357cf 100644 --- 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

Workflow

createWorkflow( List dependentResourceSpecs) { var orderedResourceSpecs = orderAndDetectCycles(dependentResourceSpecs); final var alreadyCreated = new ArrayList(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 alreadyCreated) { + List 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 index ae8ed282c6..fc6e77db6d 100644 --- 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 spec) { @Override @SuppressWarnings({"rawtypes", "unchecked"}) - public void resolve(KubernetesClient client, List dependentResources) { - final var spec = dependentResources.stream() + public void resolve(KubernetesClient client, ControllerConfiguration

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 index 07163ab8c5..99e2fa94e1 100644 --- 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

{ // 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 dependentResourceNodes) { + Workflow(Set dependentResourceNodes, boolean hasCleaner) { this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), - THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false, false); + THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false, hasCleaner); } Workflow(Set 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 toMap( + Set 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) 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

context) { return result; } - // add cycle detection? - @SuppressWarnings("unchecked") - private void preprocessForReconcile() { - final var nodes = new ArrayList<>(dependentResourceNodes.values()); - bottomLevelResource.addAll(nodes); - for (DependentResourceNode node : nodes) { - if (node.getDependsOn().isEmpty()) { - topLevelResources.add(node); - } else { - for (DependentResourceNode dependsOn : node.getDependsOn()) { - bottomLevelResource.remove(dependsOn); - } - } - } - } - Set getTopLevelDependentResources() { return topLevelResources; } @@ -125,24 +129,26 @@ Map nodes() { } @SuppressWarnings("unchecked") - void resolve(KubernetesClient client, List dependentResources) { + void resolve(KubernetesClient client, ControllerConfiguration

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 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 index f4ef13d122..3d5bde29db 100644 --- 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 index 96a55aa85f..d8771af5e5 100644 --- 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 index 52c6840b1c..9b97963366 100644 --- 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 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 index 7da98f66b2..382022782e 100644 --- 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 @@ -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())); + } }