From 87b4f6cd52539fb677125e02a8c9d81d85389fab Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 10:01:48 +0100 Subject: [PATCH 01/19] feat: make it possible to control how workflows are created --- .../api/config/ConfigurationService.java | 5 ++++ .../operator/processing/Controller.java | 8 +++--- .../dependent/workflow/ManagedWorkflow.java | 18 ++----------- .../workflow/ManagedWorkflowFactory.java | 25 +++++++++++++++++++ 4 files changed, 36 insertions(+), 20 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java index 1fe9485588..036aa76bbf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java @@ -16,6 +16,7 @@ import io.javaoperatorsdk.operator.api.monitoring.Metrics; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResourceFactory; +import io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowFactory; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; @@ -193,4 +194,8 @@ default Optional getInformerStoppedHandler() { } }); } + + default ManagedWorkflowFactory getWorkflowFactory() { + return ManagedWorkflowFactory.DEFAULT; + } } 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 ca27ad4f2c..1a98b617d4 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 @@ -77,12 +77,12 @@ public Controller(Reconciler

reconciler, this.reconciler = reconciler; this.configuration = configuration; this.kubernetesClient = kubernetesClient; - this.metrics = Optional.ofNullable(ConfigurationServiceProvider.instance().getMetrics()) - .orElse(Metrics.NOOP); + final var configurationService = ConfigurationServiceProvider.instance(); + this.metrics = Optional.ofNullable(configurationService.getMetrics()).orElse(Metrics.NOOP); contextInitializer = reconciler instanceof ContextInitializer; isCleaner = reconciler instanceof Cleaner; - managedWorkflow = - ManagedWorkflow.workflowFor(kubernetesClient, configuration.getDependentResources()); + managedWorkflow = configurationService.getWorkflowFactory() + .workflowFor(kubernetesClient, configuration.getDependentResources()); eventSourceManager = new EventSourceManager<>(this); eventProcessor = new EventProcessor<>(eventSourceManager); 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 08e2c497b0..1cc5201359 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,14 +1,10 @@ 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.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import java.util.Collections; +import java.util.Map; @SuppressWarnings("rawtypes") public interface ManagedWorkflow

{ @@ -40,16 +36,6 @@ public Map getDependentResourcesByName() { } }; - @SuppressWarnings("unchecked") - static ManagedWorkflow workflowFor(KubernetesClient client, - List dependentResourceSpecs) { - if (dependentResourceSpecs == null || dependentResourceSpecs.isEmpty()) { - return noOpWorkflow; - } - return new DefaultManagedWorkflow(client, dependentResourceSpecs, - ManagedWorkflowSupport.instance()); - } - WorkflowReconcileResult reconcile(P primary, Context

context); WorkflowCleanupResult cleanup(P primary, Context

context); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java new file mode 100644 index 0000000000..22d01a125c --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java @@ -0,0 +1,25 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.List; + +import io.fabric8.kubernetes.client.KubernetesClient; +import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; + +import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflow.noOpWorkflow; + +public interface ManagedWorkflowFactory { + + ManagedWorkflowFactory DEFAULT = (client, dependentResourceSpecs) -> { + if (dependentResourceSpecs == null || dependentResourceSpecs.isEmpty()) { + return noOpWorkflow; + } + return new DefaultManagedWorkflow(client, dependentResourceSpecs, + ManagedWorkflowSupport.instance()); + }; + + + @SuppressWarnings("rawtypes") + ManagedWorkflow workflowFor(KubernetesClient client, + List dependentResourceSpecs); +} + From f671a21317b5f5bc48fe0bb86f2e218b03072150 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 10:49:19 +0100 Subject: [PATCH 02/19] refactor: rename DependentResourceNode to DefaultDependentResourceNode --- ...java => DefaultDependentResourceNode.java} | 26 ++++--- .../dependent/workflow/ManagedWorkflow.java | 5 +- .../dependent/workflow/Workflow.java | 23 +++--- .../workflow/WorkflowCleanupExecutor.java | 42 +++++------ .../workflow/WorkflowReconcileExecutor.java | 73 ++++++++++--------- .../workflow/builder/WorkflowBuilder.java | 11 +-- .../workflow/ManagedWorkflowSupportTest.java | 6 +- .../dependent/workflow/WorkflowTest.java | 4 +- 8 files changed, 102 insertions(+), 88 deletions(-) rename operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/{DependentResourceNode.java => DefaultDependentResourceNode.java} (69%) 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/DefaultDependentResourceNode.java similarity index 69% rename from operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java rename to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 32cef6c68e..3c954606d9 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/DefaultDependentResourceNode.java @@ -9,25 +9,25 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") -public class DependentResourceNode { +public class DefaultDependentResourceNode { private final DependentResource dependentResource; private Condition reconcilePrecondition; private Condition deletePostcondition; private Condition readyPostcondition; - private final List dependsOn = new LinkedList<>(); - private final List parents = new LinkedList<>(); + private final List dependsOn = new LinkedList<>(); + private final List parents = new LinkedList<>(); - public DependentResourceNode(DependentResource dependentResource) { + public DefaultDependentResourceNode(DependentResource dependentResource) { this(dependentResource, null, null); } - public DependentResourceNode(DependentResource dependentResource, + public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition) { this(dependentResource, reconcilePrecondition, null); } - public DependentResourceNode(DependentResource dependentResource, + public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition, Condition deletePostcondition) { this.dependentResource = dependentResource; this.reconcilePrecondition = reconcilePrecondition; @@ -46,12 +46,12 @@ public Optional> getDeletePostcondition() { return Optional.ofNullable(deletePostcondition); } - public List getDependsOn() { + public List getDependsOn() { return dependsOn; } @SuppressWarnings("unchecked") - public void addDependsOnRelation(DependentResourceNode node) { + public void addDependsOnRelation(DefaultDependentResourceNode node) { node.parents.add(this); dependsOn.add(node); } @@ -63,13 +63,14 @@ public String toString() { '}'; } - public DependentResourceNode setReconcilePrecondition( + public DefaultDependentResourceNode setReconcilePrecondition( Condition reconcilePrecondition) { this.reconcilePrecondition = reconcilePrecondition; return this; } - public DependentResourceNode setDeletePostcondition(Condition cleanupCondition) { + public DefaultDependentResourceNode setDeletePostcondition( + Condition cleanupCondition) { this.deletePostcondition = cleanupCondition; return this; } @@ -78,12 +79,13 @@ public Optional> getReadyPostcondition() { return Optional.ofNullable(readyPostcondition); } - public DependentResourceNode setReadyPostcondition(Condition readyPostcondition) { + public DefaultDependentResourceNode setReadyPostcondition( + Condition readyPostcondition) { this.readyPostcondition = readyPostcondition; return this; } - public List getParents() { + public List getParents() { return parents; } 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 1cc5201359..0497ee6fb0 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,10 +1,11 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.Collections; +import java.util.Map; + import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import java.util.Collections; -import java.util.Map; @SuppressWarnings("rawtypes") public interface ManagedWorkflow

{ 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 f955b83f89..5abce16a16 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 @@ -21,22 +21,22 @@ public class Workflow

{ public static final boolean THROW_EXCEPTION_AUTOMATICALLY_DEFAULT = true; - private final Set dependentResourceNodes; - private final Set topLevelResources = new HashSet<>(); - private final Set bottomLevelResource = new HashSet<>(); + private final Set dependentResourceNodes; + private final Set topLevelResources = new HashSet<>(); + private final Set bottomLevelResource = new HashSet<>(); private final boolean throwExceptionAutomatically; // it's "global" executor service shared between multiple reconciliations running parallel private ExecutorService executorService; - public Workflow(Set dependentResourceNodes) { + public Workflow(Set dependentResourceNodes) { this.executorService = ExecutorServiceManager.instance().workflowExecutorService(); this.dependentResourceNodes = dependentResourceNodes; this.throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; preprocessForReconcile(); } - public Workflow(Set dependentResourceNodes, + public Workflow(Set dependentResourceNodes, ExecutorService executorService, boolean throwExceptionAutomatically) { this.executorService = executorService; this.dependentResourceNodes = dependentResourceNodes; @@ -44,7 +44,7 @@ public Workflow(Set dependentResourceNodes, preprocessForReconcile(); } - public Workflow(Set dependentResourceNodes, int globalParallelism) { + public Workflow(Set dependentResourceNodes, int globalParallelism) { this(dependentResourceNodes, Executors.newFixedThreadPool(globalParallelism), true); } @@ -69,13 +69,14 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { } // add cycle detection? + @SuppressWarnings("unchecked") private void preprocessForReconcile() { bottomLevelResource.addAll(dependentResourceNodes); - for (DependentResourceNode node : dependentResourceNodes) { + for (DefaultDependentResourceNode node : dependentResourceNodes) { if (node.getDependsOn().isEmpty()) { topLevelResources.add(node); } else { - for (DependentResourceNode dependsOn : node.getDependsOn()) { + for (DefaultDependentResourceNode dependsOn : node.getDependsOn()) { bottomLevelResource.remove(dependsOn); } } @@ -90,11 +91,11 @@ public void setExecutorService(ExecutorService executorService) { this.executorService = executorService; } - Set getTopLevelDependentResources() { + Set getTopLevelDependentResources() { return topLevelResources; } - Set getBottomLevelResource() { + Set getBottomLevelResource() { return bottomLevelResource; } @@ -103,7 +104,7 @@ ExecutorService getExecutorService() { } public Set getDependentResources() { - return dependentResourceNodes.stream().map(DependentResourceNode::getDependentResource) + return dependentResourceNodes.stream().map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toSet()); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index b1dcfb938c..4a06fe0ee5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -22,14 +22,14 @@ public class WorkflowCleanupExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowCleanupExecutor.class); - private final Map> actualExecutions = + private final Map> actualExecutions = new ConcurrentHashMap<>(); - private final Map exceptionsDuringExecution = + private final Map exceptionsDuringExecution = new ConcurrentHashMap<>(); - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); - private final Set postDeleteConditionNotMet = + private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); + private final Set postDeleteConditionNotMet = ConcurrentHashMap.newKeySet(); - private final Set deleteCalled = ConcurrentHashMap.newKeySet(); + private final Set deleteCalled = ConcurrentHashMap.newKeySet(); private final Workflow

workflow; private final P primary; @@ -42,7 +42,7 @@ public WorkflowCleanupExecutor(Workflow

workflow, P primary, Context

conte } public synchronized WorkflowCleanupResult cleanup() { - for (DependentResourceNode dependentResourceNode : workflow + for (DefaultDependentResourceNode dependentResourceNode : workflow .getBottomLevelResource()) { handleCleanup(dependentResourceNode); } @@ -67,7 +67,7 @@ private synchronized boolean noMoreExecutionsScheduled() { } @SuppressWarnings({"rawtypes", "unchecked"}) - private synchronized void handleCleanup(DependentResourceNode dependentResourceNode) { + private synchronized void handleCleanup(DefaultDependentResourceNode dependentResourceNode) { log.debug("Submitting for cleanup: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -86,9 +86,9 @@ private synchronized void handleCleanup(DependentResourceNode dependentResourceN private class NodeExecutor implements Runnable { - private final DependentResourceNode dependentResourceNode; + private final DefaultDependentResourceNode dependentResourceNode; - private NodeExecutor(DependentResourceNode dependentResourceNode) { + private NodeExecutor(DefaultDependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -128,7 +128,7 @@ public void run() { private synchronized void handleDependentCleaned( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { var dependOns = dependentResourceNode.getDependsOn(); if (dependOns != null) { dependOns.forEach(d -> { @@ -139,13 +139,13 @@ private synchronized void handleDependentCleaned( } private synchronized void handleExceptionInExecutor( - DependentResourceNode dependentResourceNode, + DefaultDependentResourceNode dependentResourceNode, RuntimeException e) { exceptionsDuringExecution.put(dependentResourceNode, e); } private synchronized void handleNodeExecutionFinish( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { log.debug("Finished execution for: {}", dependentResourceNode); actualExecutions.remove(dependentResourceNode); if (actualExecutions.isEmpty()) { @@ -153,25 +153,25 @@ private synchronized void handleNodeExecutionFinish( } } - private boolean isCleaningNow(DependentResourceNode dependentResourceNode) { + private boolean isCleaningNow(DefaultDependentResourceNode dependentResourceNode) { return actualExecutions.containsKey(dependentResourceNode); } - private boolean alreadyVisited(DependentResourceNode dependentResourceNode) { + private boolean alreadyVisited(DefaultDependentResourceNode dependentResourceNode) { return alreadyVisited.contains(dependentResourceNode); } @SuppressWarnings("unchecked") - private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode) { - List parents = dependentResourceNode.getParents(); + private boolean allDependentsCleaned(DefaultDependentResourceNode dependentResourceNode) { + List parents = dependentResourceNode.getParents(); return parents.isEmpty() || parents.stream() - .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet.contains(d)); + .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet.contains(d)); } @SuppressWarnings("unchecked") - private boolean hasErroredDependent(DependentResourceNode dependentResourceNode) { - List parents = dependentResourceNode.getParents(); + private boolean hasErroredDependent(DefaultDependentResourceNode dependentResourceNode) { + List parents = dependentResourceNode.getParents(); return !parents.isEmpty() && parents.stream().anyMatch(exceptionsDuringExecution::containsKey); } @@ -180,10 +180,10 @@ private WorkflowCleanupResult createCleanupResult() { final var erroredDependents = exceptionsDuringExecution.entrySet().stream() .collect(Collectors.toMap(e -> e.getKey().getDependentResource(), Entry::getValue)); final var postConditionNotMet = postDeleteConditionNotMet.stream() - .map(DependentResourceNode::getDependentResource) + .map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toList()); final var deleteCalled = - this.deleteCalled.stream().map(DependentResourceNode::getDependentResource) + this.deleteCalled.stream().map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toList()); return new WorkflowCleanupResult(erroredDependents, postConditionNotMet, deleteCalled); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 1168d523a9..1f3a58dac9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -26,19 +26,21 @@ public class WorkflowReconcileExecutor

{ private final Workflow

workflow; - /** Covers both deleted and reconciled */ - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); - private final Set notReady = ConcurrentHashMap.newKeySet(); - private final Map> actualExecutions = + /** + * Covers both deleted and reconciled + */ + private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); + private final Set notReady = ConcurrentHashMap.newKeySet(); + private final Map> actualExecutions = new HashMap<>(); - private final Map exceptionsDuringExecution = + private final Map exceptionsDuringExecution = new ConcurrentHashMap<>(); - private final Set markedForDelete = ConcurrentHashMap.newKeySet(); - private final Set deletePostConditionNotMet = + private final Set markedForDelete = ConcurrentHashMap.newKeySet(); + private final Set deletePostConditionNotMet = ConcurrentHashMap.newKeySet(); // used to remember reconciled (not deleted or errored) dependents - private final Set reconciled = ConcurrentHashMap.newKeySet(); + private final Set reconciled = ConcurrentHashMap.newKeySet(); private final Map reconcileResults = new ConcurrentHashMap<>(); @@ -52,7 +54,7 @@ public WorkflowReconcileExecutor(Workflow

workflow, P primary, Context

con } public synchronized WorkflowReconcileResult reconcile() { - for (DependentResourceNode dependentResourceNode : workflow + for (DefaultDependentResourceNode dependentResourceNode : workflow .getTopLevelDependentResources()) { handleReconcile(dependentResourceNode); } @@ -72,7 +74,8 @@ public synchronized WorkflowReconcileResult reconcile() { return createReconcileResult(); } - private synchronized void handleReconcile(DependentResourceNode dependentResourceNode) { + private synchronized void handleReconcile( + DefaultDependentResourceNode dependentResourceNode) { log.debug("Submitting for reconcile: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -99,7 +102,7 @@ private synchronized void handleReconcile(DependentResourceNode depend } } - private synchronized void handleDelete(DependentResourceNode dependentResourceNode) { + private synchronized void handleDelete(DefaultDependentResourceNode dependentResourceNode) { log.debug("Submitting for delete: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -116,19 +119,22 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo log.debug("Submitted to delete: {}", dependentResourceNode); } - private boolean allDependentsDeletedAlready(DependentResourceNode dependentResourceNode) { + private boolean allDependentsDeletedAlready( + DefaultDependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); return dependents.stream().allMatch(d -> alreadyVisited.contains(d) && !notReady.contains(d) && !exceptionsDuringExecution.containsKey(d) && !deletePostConditionNotMet.contains(d)); } - private synchronized void handleExceptionInExecutor(DependentResourceNode dependentResourceNode, + private synchronized void handleExceptionInExecutor( + DefaultDependentResourceNode dependentResourceNode, RuntimeException e) { exceptionsDuringExecution.put(dependentResourceNode, e); } - private synchronized void handleNodeExecutionFinish(DependentResourceNode dependentResourceNode) { + private synchronized void handleNodeExecutionFinish( + DefaultDependentResourceNode dependentResourceNode) { log.debug("Finished execution for: {}", dependentResourceNode); actualExecutions.remove(dependentResourceNode); if (actualExecutions.isEmpty()) { @@ -138,7 +144,7 @@ private synchronized void handleNodeExecutionFinish(DependentResourceNode depend // needs to be in one step private synchronized void setAlreadyReconciledButNotReady( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); alreadyVisited.add(dependentResourceNode); notReady.add(dependentResourceNode); @@ -146,9 +152,9 @@ private synchronized void setAlreadyReconciledButNotReady( private class NodeReconcileExecutor implements Runnable { - private final DependentResourceNode dependentResourceNode; + private final DefaultDependentResourceNode dependentResourceNode; - private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) { + private NodeReconcileExecutor(DefaultDependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -191,9 +197,9 @@ public void run() { private class NodeDeleteExecutor implements Runnable { - private final DependentResourceNode dependentResourceNode; + private final DefaultDependentResourceNode dependentResourceNode; - private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { + private NodeDeleteExecutor(DefaultDependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -231,19 +237,19 @@ public void run() { } private synchronized void handleDependentDeleted( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { dependentResourceNode.getDependsOn().forEach(dr -> { log.debug("Handle deleted for: {} with dependent: {}", dr, dependentResourceNode); handleDelete(dr); }); } - private boolean isReconcilingNow(DependentResourceNode dependentResourceNode) { + private boolean isReconcilingNow(DefaultDependentResourceNode dependentResourceNode) { return actualExecutions.containsKey(dependentResourceNode); } private synchronized void handleDependentsReconcile( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); dependents.forEach(d -> { log.debug("Handle reconcile for dependent: {} of parent:{}", d, dependentResourceNode); @@ -256,19 +262,20 @@ private boolean noMoreExecutionsScheduled() { } private boolean alreadyVisited( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { return alreadyVisited.contains(dependentResourceNode); } - private void handleReconcileConditionNotMet(DependentResourceNode dependentResourceNode) { - Set bottomNodes = new HashSet<>(); + private void handleReconcileConditionNotMet( + DefaultDependentResourceNode dependentResourceNode) { + Set bottomNodes = new HashSet<>(); markDependentsForDelete(dependentResourceNode, bottomNodes); bottomNodes.forEach(this::handleDelete); } - private void markDependentsForDelete(DependentResourceNode dependentResourceNode, - Set bottomNodes) { + private void markDependentsForDelete(DefaultDependentResourceNode dependentResourceNode, + Set bottomNodes) { markedForDelete.add(dependentResourceNode); var dependents = dependentResourceNode.getParents(); if (dependents.isEmpty()) { @@ -279,25 +286,25 @@ private void markDependentsForDelete(DependentResourceNode dependentResour } private boolean allParentsReconciledAndReady( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { return dependentResourceNode.getDependsOn().isEmpty() || dependentResourceNode.getDependsOn().stream() - .allMatch(d -> alreadyVisited(d) && !notReady.contains(d)); + .allMatch(d -> alreadyVisited(d) && !notReady.contains(d)); } private boolean hasErroredParent( - DependentResourceNode dependentResourceNode) { + DefaultDependentResourceNode dependentResourceNode) { return !dependentResourceNode.getDependsOn().isEmpty() && dependentResourceNode.getDependsOn().stream() - .anyMatch(exceptionsDuringExecution::containsKey); + .anyMatch(exceptionsDuringExecution::containsKey); } private WorkflowReconcileResult createReconcileResult() { return new WorkflowReconcileResult( reconciled.stream() - .map(DependentResourceNode::getDependentResource).collect(Collectors.toList()), + .map(DefaultDependentResourceNode::getDependentResource).collect(Collectors.toList()), notReady.stream() - .map(DependentResourceNode::getDependentResource) + .map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toList()), exceptionsDuringExecution .entrySet().stream() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java index a9dd3f121d..01aedb7193 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java @@ -9,7 +9,7 @@ import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; -import io.javaoperatorsdk.operator.processing.dependent.workflow.DependentResourceNode; +import io.javaoperatorsdk.operator.processing.dependent.workflow.DefaultDependentResourceNode; import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow; import static io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; @@ -17,13 +17,13 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public class WorkflowBuilder

{ - private final Set> dependentResourceNodes = new HashSet<>(); + private final Set> dependentResourceNodes = new HashSet<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; - private DependentResourceNode currentNode; + private DefaultDependentResourceNode currentNode; public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { - currentNode = new DependentResourceNode<>(dependentResource); + currentNode = new DefaultDependentResourceNode<>(dependentResource); dependentResourceNodes.add(currentNode); return this; } @@ -58,7 +58,8 @@ public WorkflowBuilder

withDeletePostcondition(Condition deletePostcondition) return this; } - DependentResourceNode getNodeByDependentResource(DependentResource dependentResource) { + DefaultDependentResourceNode getNodeByDependentResource( + DependentResource dependentResource) { return dependentResourceNodes.stream() .filter(dr -> dr.getDependentResource() == dependentResource) .findFirst() 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 85f04f220f..63976504c0 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 @@ -151,8 +151,10 @@ void createsWorkflow() { assertThat(workflow.getDependentResources()).containsExactlyInAnyOrder(drByName.values() .toArray(new DependentResource[0])); assertThat(workflow.getTopLevelDependentResources()) - .map(DependentResourceNode::getDependentResource).containsExactly(drByName.get(NAME_1)); - assertThat(workflow.getBottomLevelResource()).map(DependentResourceNode::getDependentResource) + .map(DefaultDependentResourceNode::getDependentResource) + .containsExactly(drByName.get(NAME_1)); + assertThat(workflow.getBottomLevelResource()) + .map(DefaultDependentResourceNode::getDependentResource) .containsExactly(drByName.get(NAME_4)); } 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 df12c8af54..aa4198ab82 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 @@ -29,7 +29,7 @@ void calculatesTopLevelResources() { Set topResources = workflow.getTopLevelDependentResources().stream() - .map(DependentResourceNode::getDependentResource) + .map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toSet()); assertThat(topResources).containsExactlyInAnyOrder(dr1, independentDR); @@ -49,7 +49,7 @@ void calculatesBottomLevelResources() { Set bottomResources = workflow.getBottomLevelResource().stream() - .map(DependentResourceNode::getDependentResource) + .map(DefaultDependentResourceNode::getDependentResource) .collect(Collectors.toSet()); assertThat(bottomResources).containsExactlyInAnyOrder(dr2, independentDR); From e02b03857cece4227da81805fc09c972ef64fb64 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 10:51:05 +0100 Subject: [PATCH 03/19] refactor: remove unneeded methods on Workflow --- .../processing/dependent/workflow/Workflow.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) 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 5abce16a16..41a5ab0021 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 @@ -27,7 +27,7 @@ public class Workflow

{ private final boolean throwExceptionAutomatically; // it's "global" executor service shared between multiple reconciliations running parallel - private ExecutorService executorService; + private final ExecutorService executorService; public Workflow(Set dependentResourceNodes) { this.executorService = ExecutorServiceManager.instance().workflowExecutorService(); @@ -83,14 +83,6 @@ private void preprocessForReconcile() { } } - public boolean isThrowExceptionAutomatically() { - return throwExceptionAutomatically; - } - - public void setExecutorService(ExecutorService executorService) { - this.executorService = executorService; - } - Set getTopLevelDependentResources() { return topLevelResources; } From aa508e88fb6a1729f3fddb848ed27dc4b55519c9 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 11:18:17 +0100 Subject: [PATCH 04/19] refactor: extract DependentResourceNode interface and use it --- .../DefaultDependentResourceNode.java | 47 +++++++------ .../workflow/DependentResourceNode.java | 30 +++++++++ .../dependent/workflow/Workflow.java | 22 +++--- .../workflow/WorkflowCleanupExecutor.java | 40 +++++------ .../workflow/WorkflowReconcileExecutor.java | 67 +++++++++---------- .../workflow/builder/WorkflowBuilder.java | 6 +- .../workflow/ManagedWorkflowSupportTest.java | 4 +- .../dependent/workflow/WorkflowTest.java | 4 +- 8 files changed, 125 insertions(+), 95 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 3c954606d9..1e30b321f3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -9,14 +9,15 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") -public class DefaultDependentResourceNode { +public class DefaultDependentResourceNode implements + DependentResourceNode { private final DependentResource dependentResource; private Condition reconcilePrecondition; private Condition deletePostcondition; private Condition readyPostcondition; - private final List dependsOn = new LinkedList<>(); - private final List parents = new LinkedList<>(); + private final List dependsOn = new LinkedList<>(); + private final List parents = new LinkedList<>(); public DefaultDependentResourceNode(DependentResource dependentResource) { this(dependentResource, null, null); @@ -34,62 +35,66 @@ public DefaultDependentResourceNode(DependentResource dependentResource, this.deletePostcondition = deletePostcondition; } + @Override public DependentResource getDependentResource() { return dependentResource; } + @Override public Optional> getReconcilePrecondition() { return Optional.ofNullable(reconcilePrecondition); } + @Override public Optional> getDeletePostcondition() { return Optional.ofNullable(deletePostcondition); } - public List getDependsOn() { + @Override + public List getDependsOn() { return dependsOn; } - @SuppressWarnings("unchecked") - public void addDependsOnRelation(DefaultDependentResourceNode node) { - node.parents.add(this); + @Override + public void addParent(DependentResourceNode parent) { + parents.add(parent); + } + + @Override + public void addDependsOnRelation(DependentResourceNode node) { + node.addParent(this); dependsOn.add(node); } @Override public String toString() { - return "DependentResourceNode{" + - "dependentResource=" + dependentResource + - '}'; + return "DependentResourceNode{" + dependentResource + '}'; } - public DefaultDependentResourceNode setReconcilePrecondition( - Condition reconcilePrecondition) { + public void setReconcilePrecondition(Condition reconcilePrecondition) { this.reconcilePrecondition = reconcilePrecondition; - return this; } - public DefaultDependentResourceNode setDeletePostcondition( - Condition cleanupCondition) { + public void setDeletePostcondition(Condition cleanupCondition) { this.deletePostcondition = cleanupCondition; - return this; } + @Override public Optional> getReadyPostcondition() { return Optional.ofNullable(readyPostcondition); } - public DefaultDependentResourceNode setReadyPostcondition( - Condition readyPostcondition) { + public void setReadyPostcondition(Condition readyPostcondition) { this.readyPostcondition = readyPostcondition; - return this; } - public List getParents() { + @Override + public List getParents() { return parents; } - protected R getSecondaryResource(P primary, Context

context) { + @Override + public R getSecondaryResource(P primary, Context

context) { return getDependentResource().getSecondaryResource(primary, context).orElse(null); } } 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 new file mode 100644 index 0000000000..d1c1a27ce6 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java @@ -0,0 +1,30 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.List; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; + +@SuppressWarnings("rawtypes") +public interface DependentResourceNode { + + DependentResource getDependentResource(); + + Optional> getReconcilePrecondition(); + + Optional> getDeletePostcondition(); + + List getDependsOn(); + + void addDependsOnRelation(DependentResourceNode node); + + Optional> getReadyPostcondition(); + + List getParents(); + + R getSecondaryResource(P primary, Context

context); + + void addParent(DependentResourceNode parent); +} 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 41a5ab0021..c3db77eec1 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 @@ -21,22 +21,22 @@ public class Workflow

{ public static final boolean THROW_EXCEPTION_AUTOMATICALLY_DEFAULT = true; - private final Set dependentResourceNodes; - private final Set topLevelResources = new HashSet<>(); - private final Set bottomLevelResource = new HashSet<>(); + private final Set dependentResourceNodes; + private final Set topLevelResources = new HashSet<>(); + private final Set bottomLevelResource = new HashSet<>(); private final boolean throwExceptionAutomatically; // it's "global" executor service shared between multiple reconciliations running parallel private final ExecutorService executorService; - public Workflow(Set dependentResourceNodes) { + public Workflow(Set dependentResourceNodes) { this.executorService = ExecutorServiceManager.instance().workflowExecutorService(); this.dependentResourceNodes = dependentResourceNodes; this.throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; preprocessForReconcile(); } - public Workflow(Set dependentResourceNodes, + public Workflow(Set dependentResourceNodes, ExecutorService executorService, boolean throwExceptionAutomatically) { this.executorService = executorService; this.dependentResourceNodes = dependentResourceNodes; @@ -44,7 +44,7 @@ public Workflow(Set dependentResourceNodes, preprocessForReconcile(); } - public Workflow(Set dependentResourceNodes, int globalParallelism) { + public Workflow(Set dependentResourceNodes, int globalParallelism) { this(dependentResourceNodes, Executors.newFixedThreadPool(globalParallelism), true); } @@ -72,22 +72,22 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { @SuppressWarnings("unchecked") private void preprocessForReconcile() { bottomLevelResource.addAll(dependentResourceNodes); - for (DefaultDependentResourceNode node : dependentResourceNodes) { + for (DependentResourceNode node : dependentResourceNodes) { if (node.getDependsOn().isEmpty()) { topLevelResources.add(node); } else { - for (DefaultDependentResourceNode dependsOn : node.getDependsOn()) { + for (DependentResourceNode dependsOn : node.getDependsOn()) { bottomLevelResource.remove(dependsOn); } } } } - Set getTopLevelDependentResources() { + Set getTopLevelDependentResources() { return topLevelResources; } - Set getBottomLevelResource() { + Set getBottomLevelResource() { return bottomLevelResource; } @@ -96,7 +96,7 @@ ExecutorService getExecutorService() { } public Set getDependentResources() { - return dependentResourceNodes.stream().map(DefaultDependentResourceNode::getDependentResource) + return dependentResourceNodes.stream().map(DependentResourceNode::getDependentResource) .collect(Collectors.toSet()); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 4a06fe0ee5..7a96fe7a40 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -22,14 +22,14 @@ public class WorkflowCleanupExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowCleanupExecutor.class); - private final Map> actualExecutions = + private final Map> actualExecutions = new ConcurrentHashMap<>(); - private final Map exceptionsDuringExecution = + private final Map exceptionsDuringExecution = new ConcurrentHashMap<>(); - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); - private final Set postDeleteConditionNotMet = + private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); + private final Set postDeleteConditionNotMet = ConcurrentHashMap.newKeySet(); - private final Set deleteCalled = ConcurrentHashMap.newKeySet(); + private final Set deleteCalled = ConcurrentHashMap.newKeySet(); private final Workflow

workflow; private final P primary; @@ -42,7 +42,7 @@ public WorkflowCleanupExecutor(Workflow

workflow, P primary, Context

conte } public synchronized WorkflowCleanupResult cleanup() { - for (DefaultDependentResourceNode dependentResourceNode : workflow + for (DependentResourceNode dependentResourceNode : workflow .getBottomLevelResource()) { handleCleanup(dependentResourceNode); } @@ -67,7 +67,7 @@ private synchronized boolean noMoreExecutionsScheduled() { } @SuppressWarnings({"rawtypes", "unchecked"}) - private synchronized void handleCleanup(DefaultDependentResourceNode dependentResourceNode) { + private synchronized void handleCleanup(DependentResourceNode dependentResourceNode) { log.debug("Submitting for cleanup: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -86,9 +86,9 @@ private synchronized void handleCleanup(DefaultDependentResourceNode dependentRe private class NodeExecutor implements Runnable { - private final DefaultDependentResourceNode dependentResourceNode; + private final DependentResourceNode dependentResourceNode; - private NodeExecutor(DefaultDependentResourceNode dependentResourceNode) { + private NodeExecutor(DependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -128,7 +128,7 @@ public void run() { private synchronized void handleDependentCleaned( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { var dependOns = dependentResourceNode.getDependsOn(); if (dependOns != null) { dependOns.forEach(d -> { @@ -139,13 +139,13 @@ private synchronized void handleDependentCleaned( } private synchronized void handleExceptionInExecutor( - DefaultDependentResourceNode dependentResourceNode, + DependentResourceNode dependentResourceNode, RuntimeException e) { exceptionsDuringExecution.put(dependentResourceNode, e); } private synchronized void handleNodeExecutionFinish( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { log.debug("Finished execution for: {}", dependentResourceNode); actualExecutions.remove(dependentResourceNode); if (actualExecutions.isEmpty()) { @@ -153,25 +153,25 @@ private synchronized void handleNodeExecutionFinish( } } - private boolean isCleaningNow(DefaultDependentResourceNode dependentResourceNode) { + private boolean isCleaningNow(DependentResourceNode dependentResourceNode) { return actualExecutions.containsKey(dependentResourceNode); } - private boolean alreadyVisited(DefaultDependentResourceNode dependentResourceNode) { + private boolean alreadyVisited(DependentResourceNode dependentResourceNode) { return alreadyVisited.contains(dependentResourceNode); } @SuppressWarnings("unchecked") - private boolean allDependentsCleaned(DefaultDependentResourceNode dependentResourceNode) { - List parents = dependentResourceNode.getParents(); + private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode) { + List parents = dependentResourceNode.getParents(); return parents.isEmpty() || parents.stream() .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet.contains(d)); } @SuppressWarnings("unchecked") - private boolean hasErroredDependent(DefaultDependentResourceNode dependentResourceNode) { - List parents = dependentResourceNode.getParents(); + private boolean hasErroredDependent(DependentResourceNode dependentResourceNode) { + List parents = dependentResourceNode.getParents(); return !parents.isEmpty() && parents.stream().anyMatch(exceptionsDuringExecution::containsKey); } @@ -180,10 +180,10 @@ private WorkflowCleanupResult createCleanupResult() { final var erroredDependents = exceptionsDuringExecution.entrySet().stream() .collect(Collectors.toMap(e -> e.getKey().getDependentResource(), Entry::getValue)); final var postConditionNotMet = postDeleteConditionNotMet.stream() - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .collect(Collectors.toList()); final var deleteCalled = - this.deleteCalled.stream().map(DefaultDependentResourceNode::getDependentResource) + this.deleteCalled.stream().map(DependentResourceNode::getDependentResource) .collect(Collectors.toList()); return new WorkflowCleanupResult(erroredDependents, postConditionNotMet, deleteCalled); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 1f3a58dac9..6ebe513a6c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -29,18 +29,17 @@ public class WorkflowReconcileExecutor

{ /** * Covers both deleted and reconciled */ - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); - private final Set notReady = ConcurrentHashMap.newKeySet(); - private final Map> actualExecutions = - new HashMap<>(); - private final Map exceptionsDuringExecution = + private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); + private final Set notReady = ConcurrentHashMap.newKeySet(); + private final Map> actualExecutions = new HashMap<>(); + private final Map exceptionsDuringExecution = new ConcurrentHashMap<>(); - private final Set markedForDelete = ConcurrentHashMap.newKeySet(); - private final Set deletePostConditionNotMet = + private final Set markedForDelete = ConcurrentHashMap.newKeySet(); + private final Set deletePostConditionNotMet = ConcurrentHashMap.newKeySet(); // used to remember reconciled (not deleted or errored) dependents - private final Set reconciled = ConcurrentHashMap.newKeySet(); + private final Set reconciled = ConcurrentHashMap.newKeySet(); private final Map reconcileResults = new ConcurrentHashMap<>(); @@ -54,7 +53,7 @@ public WorkflowReconcileExecutor(Workflow

workflow, P primary, Context

con } public synchronized WorkflowReconcileResult reconcile() { - for (DefaultDependentResourceNode dependentResourceNode : workflow + for (DependentResourceNode dependentResourceNode : workflow .getTopLevelDependentResources()) { handleReconcile(dependentResourceNode); } @@ -74,8 +73,7 @@ public synchronized WorkflowReconcileResult reconcile() { return createReconcileResult(); } - private synchronized void handleReconcile( - DefaultDependentResourceNode dependentResourceNode) { + private synchronized void handleReconcile(DependentResourceNode dependentResourceNode) { log.debug("Submitting for reconcile: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -102,7 +100,7 @@ private synchronized void handleReconcile( } } - private synchronized void handleDelete(DefaultDependentResourceNode dependentResourceNode) { + private synchronized void handleDelete(DependentResourceNode dependentResourceNode) { log.debug("Submitting for delete: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) @@ -120,7 +118,7 @@ private synchronized void handleDelete(DefaultDependentResourceNode dependentRes } private boolean allDependentsDeletedAlready( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); return dependents.stream().allMatch(d -> alreadyVisited.contains(d) && !notReady.contains(d) && !exceptionsDuringExecution.containsKey(d) && !deletePostConditionNotMet.contains(d)); @@ -128,13 +126,13 @@ private boolean allDependentsDeletedAlready( private synchronized void handleExceptionInExecutor( - DefaultDependentResourceNode dependentResourceNode, + DependentResourceNode dependentResourceNode, RuntimeException e) { exceptionsDuringExecution.put(dependentResourceNode, e); } private synchronized void handleNodeExecutionFinish( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { log.debug("Finished execution for: {}", dependentResourceNode); actualExecutions.remove(dependentResourceNode); if (actualExecutions.isEmpty()) { @@ -144,7 +142,7 @@ private synchronized void handleNodeExecutionFinish( // needs to be in one step private synchronized void setAlreadyReconciledButNotReady( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); alreadyVisited.add(dependentResourceNode); notReady.add(dependentResourceNode); @@ -152,9 +150,9 @@ private synchronized void setAlreadyReconciledButNotReady( private class NodeReconcileExecutor implements Runnable { - private final DefaultDependentResourceNode dependentResourceNode; + private final DependentResourceNode dependentResourceNode; - private NodeReconcileExecutor(DefaultDependentResourceNode dependentResourceNode) { + private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -197,9 +195,9 @@ public void run() { private class NodeDeleteExecutor implements Runnable { - private final DefaultDependentResourceNode dependentResourceNode; + private final DependentResourceNode dependentResourceNode; - private NodeDeleteExecutor(DefaultDependentResourceNode dependentResourceNode) { + private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { this.dependentResourceNode = dependentResourceNode; } @@ -237,19 +235,19 @@ public void run() { } private synchronized void handleDependentDeleted( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { dependentResourceNode.getDependsOn().forEach(dr -> { log.debug("Handle deleted for: {} with dependent: {}", dr, dependentResourceNode); handleDelete(dr); }); } - private boolean isReconcilingNow(DefaultDependentResourceNode dependentResourceNode) { + private boolean isReconcilingNow(DependentResourceNode dependentResourceNode) { return actualExecutions.containsKey(dependentResourceNode); } private synchronized void handleDependentsReconcile( - DefaultDependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); dependents.forEach(d -> { log.debug("Handle reconcile for dependent: {} of parent:{}", d, dependentResourceNode); @@ -261,21 +259,19 @@ private boolean noMoreExecutionsScheduled() { return actualExecutions.isEmpty(); } - private boolean alreadyVisited( - DefaultDependentResourceNode dependentResourceNode) { + private boolean alreadyVisited(DependentResourceNode dependentResourceNode) { return alreadyVisited.contains(dependentResourceNode); } - private void handleReconcileConditionNotMet( - DefaultDependentResourceNode dependentResourceNode) { - Set bottomNodes = new HashSet<>(); + private void handleReconcileConditionNotMet(DependentResourceNode dependentResourceNode) { + Set bottomNodes = new HashSet<>(); markDependentsForDelete(dependentResourceNode, bottomNodes); bottomNodes.forEach(this::handleDelete); } - private void markDependentsForDelete(DefaultDependentResourceNode dependentResourceNode, - Set bottomNodes) { + private void markDependentsForDelete(DependentResourceNode dependentResourceNode, + Set bottomNodes) { markedForDelete.add(dependentResourceNode); var dependents = dependentResourceNode.getParents(); if (dependents.isEmpty()) { @@ -285,15 +281,13 @@ private void markDependentsForDelete(DefaultDependentResourceNode dependen } } - private boolean allParentsReconciledAndReady( - DefaultDependentResourceNode dependentResourceNode) { + private boolean allParentsReconciledAndReady(DependentResourceNode dependentResourceNode) { return dependentResourceNode.getDependsOn().isEmpty() || dependentResourceNode.getDependsOn().stream() .allMatch(d -> alreadyVisited(d) && !notReady.contains(d)); } - private boolean hasErroredParent( - DefaultDependentResourceNode dependentResourceNode) { + private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { return !dependentResourceNode.getDependsOn().isEmpty() && dependentResourceNode.getDependsOn().stream() .anyMatch(exceptionsDuringExecution::containsKey); @@ -302,9 +296,10 @@ private boolean hasErroredParent( private WorkflowReconcileResult createReconcileResult() { return new WorkflowReconcileResult( reconciled.stream() - .map(DefaultDependentResourceNode::getDependentResource).collect(Collectors.toList()), + .map(DependentResourceNode::getDependentResource) + .collect(Collectors.toList()), notReady.stream() - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .collect(Collectors.toList()), exceptionsDuringExecution .entrySet().stream() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java index 01aedb7193..48669b35b8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java @@ -10,6 +10,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; import io.javaoperatorsdk.operator.processing.dependent.workflow.DefaultDependentResourceNode; +import io.javaoperatorsdk.operator.processing.dependent.workflow.DependentResourceNode; import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow; import static io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; @@ -17,7 +18,7 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public class WorkflowBuilder

{ - private final Set> dependentResourceNodes = new HashSet<>(); + private final Set> dependentResourceNodes = new HashSet<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; private DefaultDependentResourceNode currentNode; @@ -58,8 +59,7 @@ public WorkflowBuilder

withDeletePostcondition(Condition deletePostcondition) return this; } - DefaultDependentResourceNode getNodeByDependentResource( - DependentResource dependentResource) { + DependentResourceNode getNodeByDependentResource(DependentResource dependentResource) { return dependentResourceNodes.stream() .filter(dr -> dr.getDependentResource() == dependentResource) .findFirst() 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 63976504c0..b43478ff6d 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 @@ -151,10 +151,10 @@ void createsWorkflow() { assertThat(workflow.getDependentResources()).containsExactlyInAnyOrder(drByName.values() .toArray(new DependentResource[0])); assertThat(workflow.getTopLevelDependentResources()) - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .containsExactly(drByName.get(NAME_1)); assertThat(workflow.getBottomLevelResource()) - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .containsExactly(drByName.get(NAME_4)); } 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 aa4198ab82..df12c8af54 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 @@ -29,7 +29,7 @@ void calculatesTopLevelResources() { Set topResources = workflow.getTopLevelDependentResources().stream() - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .collect(Collectors.toSet()); assertThat(topResources).containsExactlyInAnyOrder(dr1, independentDR); @@ -49,7 +49,7 @@ void calculatesBottomLevelResources() { Set bottomResources = workflow.getBottomLevelResource().stream() - .map(DefaultDependentResourceNode::getDependentResource) + .map(DependentResourceNode::getDependentResource) .collect(Collectors.toSet()); assertThat(bottomResources).containsExactlyInAnyOrder(dr2, independentDR); From 5e12ceb56f184c3b64dbd0bc2f92fa1b59f69204 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 19:40:45 +0100 Subject: [PATCH 05/19] feat: workflows can be created without associated dependent resource This allows for the workflow graph to be resolved at build time from a factory, the dependent resources being injected as needed at runtime via the resolve mechanism. --- .../operator/processing/Controller.java | 4 +- .../DefaultDependentResourceNode.java | 7 +- .../workflow/DefaultManagedWorkflow.java | 56 ++++++++++------ .../workflow/DependentResourceNode.java | 8 +-- .../dependent/workflow/ManagedWorkflow.java | 11 ++++ .../workflow/ManagedWorkflowFactory.java | 20 +++--- .../workflow/ManagedWorkflowSupport.java | 40 +++++------ .../workflow/SpecDependentResourceNode.java | 63 ++++++++++++++++++ .../dependent/workflow/Workflow.java | 66 +++++++++++++++---- .../workflow/WorkflowCleanupExecutor.java | 24 +++---- .../workflow/WorkflowReconcileExecutor.java | 25 +++---- .../workflow/builder/WorkflowBuilder.java | 9 ++- .../workflow/ManagedWorkflowSupportTest.java | 22 +++---- .../workflow/ManagedWorkflowTest.java | 57 ++++++++++++---- .../dependent/workflow/WorkflowTest.java | 4 +- 15 files changed, 287 insertions(+), 129 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/SpecDependentResourceNode.java 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 1a98b617d4..2d9e6b4007 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 @@ -81,8 +81,8 @@ public Controller(Reconciler

reconciler, this.metrics = Optional.ofNullable(configurationService.getMetrics()).orElse(Metrics.NOOP); contextInitializer = reconciler instanceof ContextInitializer; isCleaner = reconciler instanceof Cleaner; - managedWorkflow = configurationService.getWorkflowFactory() - .workflowFor(kubernetesClient, configuration.getDependentResources()); + managedWorkflow = configurationService.getWorkflowFactory().workflowFor(configuration); + managedWorkflow.resolve(kubernetesClient, configuration.getDependentResources()); eventSourceManager = new EventSourceManager<>(this); eventProcessor = new EventProcessor<>(eventSourceManager); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 1e30b321f3..5c165316c6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -5,7 +5,6 @@ import java.util.Optional; import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") @@ -35,7 +34,6 @@ public DefaultDependentResourceNode(DependentResource dependentResource, this.deletePostcondition = deletePostcondition; } - @Override public DependentResource getDependentResource() { return dependentResource; } @@ -94,7 +92,8 @@ public List getParents() { } @Override - public R getSecondaryResource(P primary, Context

context) { - return getDependentResource().getSecondaryResource(primary, context).orElse(null); + public String getName() { + return DependentResource.defaultNameFor(dependentResource.getClass()) + "#" + + dependentResource.hashCode(); } } 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 e1162ebcca..261f7010b3 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 @@ -1,8 +1,8 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import io.fabric8.kubernetes.api.model.HasMetadata; import io.fabric8.kubernetes.client.KubernetesClient; @@ -16,42 +16,32 @@ public class DefaultManagedWorkflow

implements ManagedWorkflow

{ private final Workflow

workflow; - private final boolean isCleaner; + private boolean isCleaner; private final boolean isEmptyWorkflow; private final Map dependentResourcesByName; + private boolean resolved; + private final ManagedWorkflowSupport managedWorkflowSupport; - DefaultManagedWorkflow(KubernetesClient client, - List dependentResourceSpecs, + DefaultManagedWorkflow(List dependentResourceSpecs, Workflow

workflow, ManagedWorkflowSupport managedWorkflowSupport) { - managedWorkflowSupport.checkForNameDuplication(dependentResourceSpecs); - dependentResourcesByName = dependentResourceSpecs - .stream().collect(Collectors.toMap(DependentResourceSpec::getName, - spec -> managedWorkflowSupport.createAndConfigureFrom(spec, client))); - + dependentResourcesByName = new HashMap<>(dependentResourceSpecs.size()); isEmptyWorkflow = dependentResourceSpecs.isEmpty(); - workflow = - managedWorkflowSupport.createWorkflow(dependentResourceSpecs, dependentResourcesByName); - isCleaner = checkIfCleaner(); + this.workflow = workflow; + this.managedWorkflowSupport = managedWorkflowSupport; } public WorkflowReconcileResult reconcile(P primary, Context

context) { + checkIfResolved(); return workflow.reconcile(primary, context); } public WorkflowCleanupResult cleanup(P primary, Context

context) { + checkIfResolved(); return workflow.cleanup(primary, context); } - private boolean checkIfCleaner() { - for (var dr : workflow.getDependentResources()) { - if (dr instanceof Deleter && !(dr instanceof GarbageCollected)) { - return true; - } - } - return false; - } - public boolean isCleaner() { + checkIfResolved(); return isCleaner; } @@ -60,6 +50,30 @@ public boolean isEmptyWorkflow() { } public Map getDependentResourcesByName() { + checkIfResolved(); return dependentResourcesByName; } + + @Override + public ManagedWorkflow

resolve(KubernetesClient client, List specs) { + final boolean[] cleanerHolder = {false}; + specs.forEach(spec -> { + final var dr = managedWorkflowSupport.createAndConfigureFrom(spec, client); + dependentResourcesByName.put(spec.getName(), dr); + if (dr instanceof Deleter && !(dr instanceof GarbageCollected)) { + cleanerHolder[0] = true; + } + }); + + workflow.resolve(client, specs); + isCleaner = cleanerHolder[0]; + resolved = true; + return this; + } + + private void checkIfResolved() { + if (!resolved) { + throw new IllegalStateException("resolve should be called before"); + } + } } 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 d1c1a27ce6..43dd89e05b 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 @@ -4,14 +4,10 @@ import java.util.Optional; import io.fabric8.kubernetes.api.model.HasMetadata; -import io.javaoperatorsdk.operator.api.reconciler.Context; -import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") public interface DependentResourceNode { - DependentResource getDependentResource(); - Optional> getReconcilePrecondition(); Optional> getDeletePostcondition(); @@ -24,7 +20,7 @@ public interface DependentResourceNode { List getParents(); - R getSecondaryResource(P primary, Context

context); - void addParent(DependentResourceNode parent); + + String getName(); } 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 0497ee6fb0..de2b634b5a 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,9 +1,12 @@ 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.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @@ -35,6 +38,11 @@ public boolean isEmptyWorkflow() { public Map getDependentResourcesByName() { return Collections.emptyMap(); } + + @Override + public ManagedWorkflow resolve(KubernetesClient client, List dependentResources) { + return this; + } }; WorkflowReconcileResult reconcile(P primary, Context

context); @@ -46,4 +54,7 @@ public Map getDependentResourcesByName() { boolean isEmptyWorkflow(); Map getDependentResourcesByName(); + + ManagedWorkflow

resolve(KubernetesClient client, + List dependentResources); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java index 22d01a125c..3fe37a8a27 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java @@ -1,25 +1,27 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.List; - -import io.fabric8.kubernetes.client.KubernetesClient; -import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflow.noOpWorkflow; public interface ManagedWorkflowFactory { - ManagedWorkflowFactory DEFAULT = (client, dependentResourceSpecs) -> { + ManagedWorkflowFactory DEFAULT = (configuration, managedWorkflowSupport) -> { + final var dependentResourceSpecs = configuration.getDependentResources(); if (dependentResourceSpecs == null || dependentResourceSpecs.isEmpty()) { return noOpWorkflow; } - return new DefaultManagedWorkflow(client, dependentResourceSpecs, - ManagedWorkflowSupport.instance()); + return new DefaultManagedWorkflow(dependentResourceSpecs, + managedWorkflowSupport.createWorkflow(dependentResourceSpecs), managedWorkflowSupport); }; + @SuppressWarnings("rawtypes") + default ManagedWorkflow workflowFor(ControllerConfiguration configuration) { + return workflowFor(configuration, ManagedWorkflowSupport.instance()); + } @SuppressWarnings("rawtypes") - ManagedWorkflow workflowFor(KubernetesClient client, - List dependentResourceSpecs); + ManagedWorkflow workflowFor(ControllerConfiguration configuration, + ManagedWorkflowSupport managedWorkflowSupport); } 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 7a5a7521ba..c14e1f10ca 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 @@ -16,7 +16,6 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.KubernetesClientAware; -import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder; @SuppressWarnings({"rawtypes", "unchecked"}) class ManagedWorkflowSupport { @@ -51,24 +50,27 @@ public void checkForNameDuplication(List dependentResourc } } - @SuppressWarnings("unchecked") public

Workflow

createWorkflow( - List dependentResourceSpecs, - Map dependentResourceByName) { + List dependentResourceSpecs) { var orderedResourceSpecs = orderAndDetectCycles(dependentResourceSpecs); - var workflowBuilder = new WorkflowBuilder

().withThrowExceptionFurther(false); - orderedResourceSpecs.forEach(spec -> { - final var dependentResource = dependentResourceByName.get(spec.getName()); - final var dependsOn = (Set) spec.getDependsOn() - .stream().map(dependentResourceByName::get).collect(Collectors.toSet()); - workflowBuilder - .addDependentResource(dependentResource) - .dependsOn(dependsOn) - .withDeletePostcondition(spec.getDeletePostCondition()) - .withReconcilePrecondition(spec.getReconcileCondition()) - .withReadyPostcondition(spec.getReadyCondition()); + final var alreadyCreated = new ArrayList(orderedResourceSpecs.size()); + final var nodes = orderedResourceSpecs.stream() + .map(spec -> createFrom(spec, alreadyCreated)) + .collect(Collectors.toSet()); + return new Workflow<>(nodes); + } + + private DependentResourceNode createFrom(DependentResourceSpec spec, + List alreadyCreated) { + final var node = new SpecDependentResourceNode<>(spec); + alreadyCreated.add(node); + spec.getDependsOn().forEach(depend -> { + final DependentResourceNode dependsOn = alreadyCreated.stream() + .filter(drn -> depend.equals(drn.getName())).findFirst() + .orElseThrow(); + node.addDependsOnRelation(dependsOn); }); - return workflowBuilder.build(); + return node; } @SuppressWarnings({"rawtypes"}) @@ -96,11 +98,9 @@ public DependentResource createAndConfigureFrom(DependentResourceSpec spec, } /** - * * @param dependentResourceSpecs list of specs * @return top-bottom ordered resources that can be added safely to workflow * @throws OperatorException if there is a cycle in the dependencies - * */ public List orderAndDetectCycles( List dependentResourceSpecs) { @@ -137,6 +137,7 @@ public List orderAndDetectCycles( } private static class DRInfo { + private final DependentResourceSpec spec; private final List waitingForCompletion; @@ -157,8 +158,9 @@ String name() { private boolean isReadyForVisit(DependentResourceSpec dr, Set alreadyVisited, String alreadyPresentName) { for (var name : dr.getDependsOn()) { - if (name.equals(alreadyPresentName)) + if (name.equals(alreadyPresentName)) { continue; + } if (!alreadyVisited.contains(name)) { return false; } 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 new file mode 100644 index 0000000000..37df5bfcbb --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/SpecDependentResourceNode.java @@ -0,0 +1,63 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.LinkedList; +import java.util.List; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; + +@SuppressWarnings("rawtypes") +public class SpecDependentResourceNode + implements DependentResourceNode { + + private final DependentResourceSpec spec; + private final List dependsOn = new LinkedList<>(); + private final List parents = new LinkedList<>(); + + public SpecDependentResourceNode(DependentResourceSpec spec) { + this.spec = spec; + } + + @Override + public Optional> getReconcilePrecondition() { + return Optional.ofNullable(spec.getReconcileCondition()); + } + + @Override + public Optional> getDeletePostcondition() { + return Optional.ofNullable(spec.getDeletePostCondition()); + } + + @Override + public List getDependsOn() { + return dependsOn; + } + + @Override + public void addDependsOnRelation(DependentResourceNode node) { + node.addParent(this); + dependsOn.add(node); + } + + @Override + public Optional> getReadyPostcondition() { + return Optional.ofNullable(spec.getReadyCondition()); + } + + @Override + public List getParents() { + return parents; + } + + + @Override + public void addParent(DependentResourceNode parent) { + parents.add(parent); + } + + @Override + public String getName() { + return spec.getName(); + } +} 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 c3db77eec1..bc769c9631 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 @@ -1,13 +1,16 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.HashSet; +import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; import java.util.stream.Collectors; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.KubernetesClient; 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.DependentResource; @@ -21,7 +24,7 @@ public class Workflow

{ public static final boolean THROW_EXCEPTION_AUTOMATICALLY_DEFAULT = true; - private final Set dependentResourceNodes; + private final Map dependentResourceNodes; private final Set topLevelResources = new HashSet<>(); private final Set bottomLevelResource = new HashSet<>(); @@ -30,22 +33,21 @@ public class Workflow

{ private final ExecutorService executorService; public Workflow(Set dependentResourceNodes) { - this.executorService = ExecutorServiceManager.instance().workflowExecutorService(); - this.dependentResourceNodes = dependentResourceNodes; - this.throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; - preprocessForReconcile(); + this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), + THROW_EXCEPTION_AUTOMATICALLY_DEFAULT); } public Workflow(Set dependentResourceNodes, ExecutorService executorService, boolean throwExceptionAutomatically) { this.executorService = executorService; - this.dependentResourceNodes = dependentResourceNodes; + this.dependentResourceNodes = dependentResourceNodes.stream() + .collect(Collectors.toMap(DependentResourceNode::getName, ResolvedNode::new)); this.throwExceptionAutomatically = throwExceptionAutomatically; preprocessForReconcile(); } - public Workflow(Set dependentResourceNodes, int globalParallelism) { - this(dependentResourceNodes, Executors.newFixedThreadPool(globalParallelism), true); + public DependentResource getDependentResourceFor(DependentResourceNode node) { + return dependentResourceNodes.get(node.getName()).dependentResource(); } public WorkflowReconcileResult reconcile(P primary, Context

context) { @@ -71,8 +73,10 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { // add cycle detection? @SuppressWarnings("unchecked") private void preprocessForReconcile() { - bottomLevelResource.addAll(dependentResourceNodes); - for (DependentResourceNode node : dependentResourceNodes) { + final var nodes = dependentResourceNodes.values().stream() + .map(ResolvedNode::node).collect(Collectors.toList()); + bottomLevelResource.addAll(nodes); + for (DependentResourceNode node : nodes) { if (node.getDependsOn().isEmpty()) { topLevelResources.add(node); } else { @@ -95,8 +99,44 @@ ExecutorService getExecutorService() { return executorService; } - public Set getDependentResources() { - return dependentResourceNodes.stream().map(DependentResourceNode::getDependentResource) + Set nodes() { + return dependentResourceNodes.values().stream().map(ResolvedNode::node) .collect(Collectors.toSet()); } + + public void resolve(KubernetesClient client, List dependentResources) { + dependentResourceNodes.values().forEach(drn -> drn.resolve(client, dependentResources)); + } + + private static class ResolvedNode { + + private final DependentResourceNode node; + private DependentResource dependentResource; + + private ResolvedNode(DependentResourceNode node) { + this.node = node; + if (node instanceof DefaultDependentResourceNode) { + this.dependentResource = ((DefaultDependentResourceNode) node).getDependentResource(); + } + } + + public void setDependentResource(DependentResource dependentResource) { + this.dependentResource = dependentResource; + } + + public DependentResource dependentResource() { + return dependentResource; + } + + public DependentResourceNode node() { + return node; + } + + public void resolve(KubernetesClient client, List dependentResources) { + final var spec = dependentResources.stream() + .filter(drs -> drs.getName().equals(node.getName())) + .findFirst().orElseThrow(); + dependentResource = ManagedWorkflowSupport.instance().createAndConfigureFrom(spec, client); + } + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 7a96fe7a40..05ed43d742 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -3,7 +3,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; -import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Future; @@ -15,6 +14,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; 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; @SuppressWarnings("rawtypes") @@ -96,17 +96,18 @@ private NodeExecutor(DependentResourceNode dependentResourceNode) { @SuppressWarnings("unchecked") public void run() { try { - var dependentResource = dependentResourceNode.getDependentResource(); - Optional> deletePostCondition = - dependentResourceNode.getDeletePostcondition(); + var dependentResource = (DependentResource) workflow.getDependentResourceFor( + dependentResourceNode); + var deletePostCondition = dependentResourceNode.getDeletePostcondition(); if (dependentResource instanceof Deleter && !(dependentResource instanceof GarbageCollected)) { - ((Deleter

) dependentResourceNode.getDependentResource()).delete(primary, context); + ((Deleter

) dependentResource).delete(primary, context); deleteCalled.add(dependentResourceNode); } boolean deletePostConditionMet = deletePostCondition - .map(c -> c.isMet(primary, dependentResourceNode.getSecondaryResource(primary, context), + .map(c -> c.isMet(primary, + dependentResource.getSecondaryResource(primary, context).orElse(null), context)) .orElse(true); if (deletePostConditionMet) { @@ -178,13 +179,14 @@ private boolean hasErroredDependent(DependentResourceNode dependentResourceNode) private WorkflowCleanupResult createCleanupResult() { final var erroredDependents = exceptionsDuringExecution.entrySet().stream() - .collect(Collectors.toMap(e -> e.getKey().getDependentResource(), Entry::getValue)); + .collect( + Collectors.toMap(e -> workflow.getDependentResourceFor(e.getKey()), Entry::getValue)); final var postConditionNotMet = postDeleteConditionNotMet.stream() - .map(DependentResourceNode::getDependentResource) + .map(workflow::getDependentResourceFor) + .collect(Collectors.toList()); + final var deleteCalled = this.deleteCalled.stream() + .map(workflow::getDependentResourceFor) .collect(Collectors.toList()); - final var deleteCalled = - this.deleteCalled.stream().map(DependentResourceNode::getDependentResource) - .collect(Collectors.toList()); return new WorkflowCleanupResult(erroredDependents, postConditionNotMet, deleteCalled); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 6ebe513a6c..d4b06fa8b0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -85,8 +85,10 @@ private synchronized void handleReconcile(DependentResourceNode depend return; } + final var dr = + (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); boolean reconcileConditionMet = dependentResourceNode.getReconcilePrecondition() - .map(rc -> rc.isMet(primary, dependentResourceNode.getSecondaryResource(primary, context), + .map(rc -> rc.isMet(primary, dr.getSecondaryResource(primary, context).orElse(null), context)) .orElse(true); @@ -160,7 +162,8 @@ private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) { @SuppressWarnings("unchecked") public void run() { try { - DependentResource dependentResource = dependentResourceNode.getDependentResource(); + final var dependentResource = + (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); if (log.isDebugEnabled()) { log.debug( "Reconciling {} for primary: {}", @@ -173,8 +176,7 @@ public void run() { boolean ready = dependentResourceNode.getReadyPostcondition() .map(rc -> rc.isMet(primary, - dependentResourceNode.getDependentResource().getSecondaryResource(primary, context) - .orElse(null), + dependentResource.getSecondaryResource(primary, context).orElse(null), context)) .orElse(true); @@ -205,17 +207,17 @@ private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { @SuppressWarnings("unchecked") public void run() { try { - DependentResource dependentResource = dependentResourceNode.getDependentResource(); + final var dependentResource = + (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); var deletePostCondition = dependentResourceNode.getDeletePostcondition(); if (dependentResource instanceof Deleter && !(dependentResource instanceof GarbageCollected)) { - ((Deleter

) dependentResourceNode.getDependentResource()).delete(primary, context); + ((Deleter

) dependentResource).delete(primary, context); } boolean deletePostConditionMet = deletePostCondition.map(c -> c.isMet(primary, - dependentResourceNode.getDependentResource().getSecondaryResource(primary, context) - .orElse(null), + dependentResource.getSecondaryResource(primary, context).orElse(null), context)).orElse(true); if (deletePostConditionMet) { alreadyVisited.add(dependentResourceNode); @@ -296,14 +298,15 @@ private boolean hasErroredParent(DependentResourceNode dependentResourceNo private WorkflowReconcileResult createReconcileResult() { return new WorkflowReconcileResult( reconciled.stream() - .map(DependentResourceNode::getDependentResource) + .map(workflow::getDependentResourceFor) .collect(Collectors.toList()), notReady.stream() - .map(DependentResourceNode::getDependentResource) + .map(workflow::getDependentResourceFor) .collect(Collectors.toList()), exceptionsDuringExecution .entrySet().stream() - .collect(Collectors.toMap(e -> e.getKey().getDependentResource(), Map.Entry::getValue)), + .collect(Collectors.toMap(e -> workflow.getDependentResourceFor(e.getKey()), + Map.Entry::getValue)), reconcileResults); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java index 48669b35b8..f86c95dee0 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java @@ -4,6 +4,7 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; @@ -18,7 +19,7 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public class WorkflowBuilder

{ - private final Set> dependentResourceNodes = new HashSet<>(); + private final Set> dependentResourceNodes = new HashSet<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; private DefaultDependentResourceNode currentNode; @@ -76,13 +77,11 @@ public WorkflowBuilder

withThrowExceptionFurther(boolean throwExceptionFurthe } public Workflow

build() { - return new Workflow( - dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), - throwExceptionAutomatically); + return build(ExecutorServiceManager.instance().workflowExecutorService()); } public Workflow

build(int parallelism) { - return new Workflow(dependentResourceNodes, parallelism); + return build(Executors.newFixedThreadPool(parallelism)); } public Workflow

build(ExecutorService executorService) { 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 b43478ff6d..7db824d7ad 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 @@ -11,7 +11,6 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; -import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowTestUtils.createDRS; import static org.assertj.core.api.Assertions.assertThat; @@ -141,21 +140,18 @@ void createsWorkflow() { createDRS(NAME_3, NAME_1), createDRS(NAME_4, NAME_3, NAME_2)); - var drByName = specs - .stream().collect(Collectors.toMap(DependentResourceSpec::getName, - spec -> managedWorkflowSupport.createAndConfigureFrom(spec, - mock(KubernetesClient.class)))); + final var client = mock(KubernetesClient.class); + var workflow = managedWorkflowSupport.createWorkflow(specs); + workflow.resolve(client, specs); - var workflow = managedWorkflowSupport.createWorkflow(specs, drByName); - - assertThat(workflow.getDependentResources()).containsExactlyInAnyOrder(drByName.values() - .toArray(new DependentResource[0])); + assertThat(workflow.nodes()).map(DependentResourceNode::getName) + .containsExactlyInAnyOrder(NAME_1, NAME_2, NAME_3, NAME_4); assertThat(workflow.getTopLevelDependentResources()) - .map(DependentResourceNode::getDependentResource) - .containsExactly(drByName.get(NAME_1)); + .map(DependentResourceNode::getName) + .containsExactly(NAME_1); assertThat(workflow.getBottomLevelResource()) - .map(DependentResourceNode::getDependentResource) - .containsExactly(drByName.get(NAME_4)); + .map(DependentResourceNode::getName) + .containsExactly(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 df556885b1..b867f3adfd 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 @@ -1,11 +1,12 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.List; -import java.util.Set; 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; import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @@ -14,6 +15,7 @@ import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowTestUtils.createDRS; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static org.mockito.Mockito.withSettings; @@ -29,37 +31,66 @@ class ManagedWorkflowTest { @Test void checksIfWorkflowEmpty() { var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any(), any())).thenReturn(mockWorkflow); + when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) .thenReturn(mock(DependentResource.class)); assertThat(managedWorkflow().isEmptyWorkflow()).isTrue(); - when(mockWorkflow.getDependentResources()).thenReturn(Set.of(mock(DependentResource.class))); + // when(mockWorkflow.getDependentResources()).thenReturn(Set.of(mock(DependentResource.class))); assertThat(managedWorkflow(createDRS(NAME)).isEmptyWorkflow()).isFalse(); } @Test - void isCleanerIfAtLeastOneDRIsDeleterAndNoGC() { + void isNotCleanerIfNoDeleter() { var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any(), any())).thenReturn(mockWorkflow); + when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) .thenReturn(mock(DependentResource.class)); - when(mockWorkflow.getDependentResources()).thenReturn(Set.of(mock(DependentResource.class))); assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); + } + + @Test + void isNotCleanerIfNoGarbageCollected() { + var mockWorkflow = mock(Workflow.class); + when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); + when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) + .thenReturn( + mock(DependentResource.class, withSettings().extraInterfaces(GarbageCollected.class))); + + assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); + } - when(mockWorkflow.getDependentResources()).thenReturn( - Set.of(mock(DependentResource.class, withSettings().extraInterfaces(Deleter.class)))); - assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isTrue(); + @Test + void isCleanerIfHasDeleter() { + var mockWorkflow = mock(Workflow.class); + when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); + + var spec = createDRS(NAME); + when(managedWorkflowSupportMock.createAndConfigureFrom(eq(spec), any())) + .thenReturn(mock(DependentResource.class, withSettings().extraInterfaces(Deleter.class))); + assertThat(managedWorkflow(spec).isCleaner()).isTrue(); + } + + @Test + void isNotCleanerIfDeleterIsGarbageCollected() { + var mockWorkflow = mock(Workflow.class); + when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - when(mockWorkflow.getDependentResources()).thenReturn(Set.of(mock(DependentResource.class, - withSettings().extraInterfaces(Deleter.class, GarbageCollected.class)))); + var spec = createDRS(NAME); + when(managedWorkflowSupportMock.createAndConfigureFrom(eq(spec), any())) + .thenReturn(mock(DependentResource.class, + withSettings().extraInterfaces(Deleter.class, GarbageCollected.class))); assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); } ManagedWorkflow managedWorkflow(DependentResourceSpec... specs) { - return new DefaultManagedWorkflow(kubernetesClientMock, List.of(specs), - managedWorkflowSupportMock); + final var configuration = mock(ControllerConfiguration.class); + final var specList = List.of(specs); + when(configuration.getDependentResources()).thenReturn(specList); + return ConfigurationServiceProvider.instance().getWorkflowFactory() + .workflowFor(configuration, managedWorkflowSupportMock) + .resolve(kubernetesClientMock, specList); } } 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 df12c8af54..8e4f3fe4e5 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 @@ -29,7 +29,7 @@ void calculatesTopLevelResources() { Set topResources = workflow.getTopLevelDependentResources().stream() - .map(DependentResourceNode::getDependentResource) + .map(workflow::getDependentResourceFor) .collect(Collectors.toSet()); assertThat(topResources).containsExactlyInAnyOrder(dr1, independentDR); @@ -49,7 +49,7 @@ void calculatesBottomLevelResources() { Set bottomResources = workflow.getBottomLevelResource().stream() - .map(DependentResourceNode::getDependentResource) + .map(workflow::getDependentResourceFor) .collect(Collectors.toSet()); assertThat(bottomResources).containsExactlyInAnyOrder(dr2, independentDR); From 8df1b329a63b0f74227b7214cb9ff412417f243f Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 25 Nov 2022 21:06:26 +0100 Subject: [PATCH 06/19] fix: format --- .../dependent/workflow/WorkflowCleanupExecutor.java | 2 +- .../dependent/workflow/WorkflowReconcileExecutor.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 05ed43d742..c83df3b7c3 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -167,7 +167,7 @@ private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode List parents = dependentResourceNode.getParents(); return parents.isEmpty() || parents.stream() - .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet.contains(d)); + .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet.contains(d)); } @SuppressWarnings("unchecked") diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index d4b06fa8b0..2cc48374d7 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -286,13 +286,13 @@ private void markDependentsForDelete(DependentResourceNode dependentResour private boolean allParentsReconciledAndReady(DependentResourceNode dependentResourceNode) { return dependentResourceNode.getDependsOn().isEmpty() || dependentResourceNode.getDependsOn().stream() - .allMatch(d -> alreadyVisited(d) && !notReady.contains(d)); + .allMatch(d -> alreadyVisited(d) && !notReady.contains(d)); } private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { return !dependentResourceNode.getDependsOn().isEmpty() && dependentResourceNode.getDependsOn().stream() - .anyMatch(exceptionsDuringExecution::containsKey); + .anyMatch(exceptionsDuringExecution::containsKey); } private WorkflowReconcileResult createReconcileResult() { From 27145f8c6097027916dd84806bcbd28cea453c7b Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 26 Nov 2022 16:59:30 +0100 Subject: [PATCH 07/19] refactor: extract common behavior from workflow reconcile and cleanup --- .../workflow/AbstractWorkflowExecutor.java | 100 ++++++++++++++++++ .../workflow/WorkflowCleanupExecutor.java | 78 +++----------- .../workflow/WorkflowReconcileExecutor.java | 98 ++++------------- .../workflow/ManagedWorkflowTest.java | 3 +- 4 files changed, 136 insertions(+), 143 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java new file mode 100644 index 0000000000..8342fa3e24 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java @@ -0,0 +1,100 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.HashMap; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Future; +import java.util.stream.Collectors; + +import org.slf4j.Logger; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; + +@SuppressWarnings("rawtypes") +public abstract class AbstractWorkflowExecutor

{ + + protected final Workflow

workflow; + protected final P primary; + protected final Context

context; + /** + * Covers both deleted and reconciled + */ + private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); + private final Map> actualExecutions = new HashMap<>(); + private final Map exceptionsDuringExecution = + new ConcurrentHashMap<>(); + + public AbstractWorkflowExecutor(Workflow

workflow, P primary, Context

context) { + this.workflow = workflow; + this.primary = primary; + this.context = context; + } + + protected abstract Logger logger(); + + protected void waitForScheduledExecutionsToRun() { + while (true) { + try { + this.wait(); + if (noMoreExecutionsScheduled()) { + break; + } else { + logger().warn("Notified but still resources under execution. This should not happen."); + } + } catch (InterruptedException e) { + logger().warn("Thread interrupted", e); + Thread.currentThread().interrupt(); + } + } + } + + protected boolean noMoreExecutionsScheduled() { + return actualExecutions.isEmpty(); + } + + protected boolean alreadyVisited(DependentResourceNode dependentResourceNode) { + return alreadyVisited.contains(dependentResourceNode); + } + + protected void markAsVisited(DependentResourceNode dependentResourceNode) { + alreadyVisited.add(dependentResourceNode); + } + + protected boolean isExecutingNow(DependentResourceNode dependentResourceNode) { + return actualExecutions.containsKey(dependentResourceNode); + } + + protected void markAsExecuting(DependentResourceNode dependentResourceNode, + Future future) { + actualExecutions.put(dependentResourceNode, future); + } + + protected synchronized void handleExceptionInExecutor( + DependentResourceNode dependentResourceNode, + RuntimeException e) { + exceptionsDuringExecution.put(dependentResourceNode, e); + } + + protected boolean isInError(DependentResourceNode dependentResourceNode) { + return exceptionsDuringExecution.containsKey(dependentResourceNode); + } + + protected Map getErroredDependents() { + return exceptionsDuringExecution.entrySet().stream() + .collect( + Collectors.toMap(e -> workflow.getDependentResourceFor(e.getKey()), Entry::getValue)); + } + + protected synchronized void handleNodeExecutionFinish( + DependentResourceNode dependentResourceNode) { + logger().debug("Finished execution for: {}", dependentResourceNode); + actualExecutions.remove(dependentResourceNode); + if (noMoreExecutionsScheduled()) { + this.notifyAll(); + } + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index c83df3b7c3..b017a207f8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -1,8 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.List; -import java.util.Map; -import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Future; @@ -18,52 +16,29 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; @SuppressWarnings("rawtypes") -public class WorkflowCleanupExecutor

{ +public class WorkflowCleanupExecutor

extends AbstractWorkflowExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowCleanupExecutor.class); - private final Map> actualExecutions = - new ConcurrentHashMap<>(); - private final Map exceptionsDuringExecution = - new ConcurrentHashMap<>(); - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); private final Set postDeleteConditionNotMet = ConcurrentHashMap.newKeySet(); private final Set deleteCalled = ConcurrentHashMap.newKeySet(); - private final Workflow

workflow; - private final P primary; - private final Context

context; - public WorkflowCleanupExecutor(Workflow

workflow, P primary, Context

context) { - this.workflow = workflow; - this.primary = primary; - this.context = context; + super(workflow, primary, context); } public synchronized WorkflowCleanupResult cleanup() { - for (DependentResourceNode dependentResourceNode : workflow - .getBottomLevelResource()) { + for (DependentResourceNode dependentResourceNode : workflow.getBottomLevelResource()) { handleCleanup(dependentResourceNode); } - while (true) { - try { - this.wait(); - if (noMoreExecutionsScheduled()) { - break; - } else { - log.warn("Notified but still resources under execution. This should not happen."); - } - } catch (InterruptedException e) { - log.warn("Thread interrupted", e); - Thread.currentThread().interrupt(); - } - } + waitForScheduledExecutionsToRun(); return createCleanupResult(); } - private synchronized boolean noMoreExecutionsScheduled() { - return actualExecutions.isEmpty(); + @Override + protected Logger logger() { + return log; } @SuppressWarnings({"rawtypes", "unchecked"}) @@ -71,7 +46,7 @@ private synchronized void handleCleanup(DependentResourceNode dependentResourceN log.debug("Submitting for cleanup: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) - || isCleaningNow(dependentResourceNode) + || isExecutingNow(dependentResourceNode) || !allDependentsCleaned(dependentResourceNode) || hasErroredDependent(dependentResourceNode)) { log.debug("Skipping submit of: {}, ", dependentResourceNode); @@ -80,7 +55,7 @@ private synchronized void handleCleanup(DependentResourceNode dependentResourceN Future nodeFuture = workflow.getExecutorService().submit(new NodeExecutor(dependentResourceNode)); - actualExecutions.put(dependentResourceNode, nodeFuture); + markAsExecuting(dependentResourceNode, nodeFuture); log.debug("Submitted for cleanup: {}", dependentResourceNode); } @@ -111,13 +86,13 @@ public void run() { context)) .orElse(true); if (deletePostConditionMet) { - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); handleDependentCleaned(dependentResourceNode); } else { // updating alreadyVisited needs to be the last operation otherwise could lead to a race // condition in handleCleanup condition checks postDeleteConditionNotMet.add(dependentResourceNode); - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); } } catch (RuntimeException e) { handleExceptionInExecutor(dependentResourceNode, e); @@ -127,7 +102,6 @@ public void run() { } } - private synchronized void handleDependentCleaned( DependentResourceNode dependentResourceNode) { var dependOns = dependentResourceNode.getDependsOn(); @@ -139,29 +113,6 @@ private synchronized void handleDependentCleaned( } } - private synchronized void handleExceptionInExecutor( - DependentResourceNode dependentResourceNode, - RuntimeException e) { - exceptionsDuringExecution.put(dependentResourceNode, e); - } - - private synchronized void handleNodeExecutionFinish( - DependentResourceNode dependentResourceNode) { - log.debug("Finished execution for: {}", dependentResourceNode); - actualExecutions.remove(dependentResourceNode); - if (actualExecutions.isEmpty()) { - this.notifyAll(); - } - } - - private boolean isCleaningNow(DependentResourceNode dependentResourceNode) { - return actualExecutions.containsKey(dependentResourceNode); - } - - private boolean alreadyVisited(DependentResourceNode dependentResourceNode) { - return alreadyVisited.contains(dependentResourceNode); - } - @SuppressWarnings("unchecked") private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode) { List parents = dependentResourceNode.getParents(); @@ -173,14 +124,11 @@ private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode @SuppressWarnings("unchecked") private boolean hasErroredDependent(DependentResourceNode dependentResourceNode) { List parents = dependentResourceNode.getParents(); - return !parents.isEmpty() - && parents.stream().anyMatch(exceptionsDuringExecution::containsKey); + return !parents.isEmpty() && parents.stream().anyMatch(this::isInError); } private WorkflowCleanupResult createCleanupResult() { - final var erroredDependents = exceptionsDuringExecution.entrySet().stream() - .collect( - Collectors.toMap(e -> workflow.getDependentResourceFor(e.getKey()), Entry::getValue)); + final var erroredDependents = getErroredDependents(); final var postConditionNotMet = postDeleteConditionNotMet.stream() .map(workflow::getDependentResourceFor) .collect(Collectors.toList()); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 2cc48374d7..8f9f824afd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -1,6 +1,5 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -20,20 +19,12 @@ import io.javaoperatorsdk.operator.processing.event.ResourceID; @SuppressWarnings({"rawtypes", "unchecked"}) -public class WorkflowReconcileExecutor

{ +public class WorkflowReconcileExecutor

extends AbstractWorkflowExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowReconcileExecutor.class); - private final Workflow

workflow; - /** - * Covers both deleted and reconciled - */ - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); private final Set notReady = ConcurrentHashMap.newKeySet(); - private final Map> actualExecutions = new HashMap<>(); - private final Map exceptionsDuringExecution = - new ConcurrentHashMap<>(); private final Set markedForDelete = ConcurrentHashMap.newKeySet(); private final Set deletePostConditionNotMet = @@ -43,41 +34,28 @@ public class WorkflowReconcileExecutor

{ private final Map reconcileResults = new ConcurrentHashMap<>(); - private final P primary; - private final Context

context; - public WorkflowReconcileExecutor(Workflow

workflow, P primary, Context

context) { - this.primary = primary; - this.context = context; - this.workflow = workflow; + super(workflow, primary, context); } public synchronized WorkflowReconcileResult reconcile() { - for (DependentResourceNode dependentResourceNode : workflow - .getTopLevelDependentResources()) { + for (DependentResourceNode dependentResourceNode : workflow.getTopLevelDependentResources()) { handleReconcile(dependentResourceNode); } - while (true) { - try { - this.wait(); - if (noMoreExecutionsScheduled()) { - break; - } else { - log.warn("Notified but still resources under execution. This should not happen."); - } - } catch (InterruptedException e) { - log.warn("Thread interrupted", e); - Thread.currentThread().interrupt(); - } - } + waitForScheduledExecutionsToRun(); return createReconcileResult(); } + @Override + protected Logger logger() { + return log; + } + private synchronized void handleReconcile(DependentResourceNode dependentResourceNode) { log.debug("Submitting for reconcile: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) - || isReconcilingNow(dependentResourceNode) + || isExecutingNow(dependentResourceNode) || !allParentsReconciledAndReady(dependentResourceNode) || markedForDelete.contains(dependentResourceNode) || hasErroredParent(dependentResourceNode)) { @@ -97,7 +75,7 @@ private synchronized void handleReconcile(DependentResourceNode depend } else { Future nodeFuture = workflow.getExecutorService() .submit(new NodeReconcileExecutor(dependentResourceNode)); - actualExecutions.put(dependentResourceNode, nodeFuture); + markAsExecuting(dependentResourceNode, nodeFuture); log.debug("Submitted to reconcile: {}", dependentResourceNode); } } @@ -106,7 +84,7 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo log.debug("Submitting for delete: {}", dependentResourceNode); if (alreadyVisited(dependentResourceNode) - || isReconcilingNow(dependentResourceNode) + || isExecutingNow(dependentResourceNode) || !markedForDelete.contains(dependentResourceNode) || !allDependentsDeletedAlready(dependentResourceNode)) { log.debug("Skipping submit for delete of: {}, ", dependentResourceNode); @@ -115,38 +93,21 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo Future nodeFuture = workflow.getExecutorService() .submit(new NodeDeleteExecutor(dependentResourceNode)); - actualExecutions.put(dependentResourceNode, nodeFuture); + markAsExecuting(dependentResourceNode, nodeFuture); log.debug("Submitted to delete: {}", dependentResourceNode); } - private boolean allDependentsDeletedAlready( - DependentResourceNode dependentResourceNode) { + private boolean allDependentsDeletedAlready(DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); - return dependents.stream().allMatch(d -> alreadyVisited.contains(d) && !notReady.contains(d) - && !exceptionsDuringExecution.containsKey(d) && !deletePostConditionNotMet.contains(d)); - } - - - private synchronized void handleExceptionInExecutor( - DependentResourceNode dependentResourceNode, - RuntimeException e) { - exceptionsDuringExecution.put(dependentResourceNode, e); - } - - private synchronized void handleNodeExecutionFinish( - DependentResourceNode dependentResourceNode) { - log.debug("Finished execution for: {}", dependentResourceNode); - actualExecutions.remove(dependentResourceNode); - if (actualExecutions.isEmpty()) { - this.notifyAll(); - } + return dependents.stream().allMatch(d -> alreadyVisited(d) && !notReady.contains(d) + && !isInError(d) && !deletePostConditionNotMet.contains(d)); } // needs to be in one step private synchronized void setAlreadyReconciledButNotReady( DependentResourceNode dependentResourceNode) { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); notReady.add(dependentResourceNode); } @@ -182,7 +143,7 @@ public void run() { if (ready) { log.debug("Setting already reconciled for: {}", dependentResourceNode); - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); handleDependentsReconcile(dependentResourceNode); } else { setAlreadyReconciledButNotReady(dependentResourceNode); @@ -220,13 +181,13 @@ public void run() { dependentResource.getSecondaryResource(primary, context).orElse(null), context)).orElse(true); if (deletePostConditionMet) { - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); handleDependentDeleted(dependentResourceNode); } else { // updating alreadyVisited needs to be the last operation otherwise could lead to a race // condition in handleDelete condition checks deletePostConditionNotMet.add(dependentResourceNode); - alreadyVisited.add(dependentResourceNode); + markAsVisited(dependentResourceNode); } } catch (RuntimeException e) { handleExceptionInExecutor(dependentResourceNode, e); @@ -244,10 +205,6 @@ private synchronized void handleDependentDeleted( }); } - private boolean isReconcilingNow(DependentResourceNode dependentResourceNode) { - return actualExecutions.containsKey(dependentResourceNode); - } - private synchronized void handleDependentsReconcile( DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); @@ -257,14 +214,6 @@ private synchronized void handleDependentsReconcile( }); } - private boolean noMoreExecutionsScheduled() { - return actualExecutions.isEmpty(); - } - - private boolean alreadyVisited(DependentResourceNode dependentResourceNode) { - return alreadyVisited.contains(dependentResourceNode); - } - private void handleReconcileConditionNotMet(DependentResourceNode dependentResourceNode) { Set bottomNodes = new HashSet<>(); @@ -292,7 +241,7 @@ private boolean allParentsReconciledAndReady(DependentResourceNode depende private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { return !dependentResourceNode.getDependsOn().isEmpty() && dependentResourceNode.getDependsOn().stream() - .anyMatch(exceptionsDuringExecution::containsKey); + .anyMatch(this::isInError); } private WorkflowReconcileResult createReconcileResult() { @@ -303,10 +252,7 @@ private WorkflowReconcileResult createReconcileResult() { notReady.stream() .map(workflow::getDependentResourceFor) .collect(Collectors.toList()), - exceptionsDuringExecution - .entrySet().stream() - .collect(Collectors.toMap(e -> workflow.getDependentResourceFor(e.getKey()), - Map.Entry::getValue)), + getErroredDependents(), reconcileResults); } 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 b867f3adfd..2febdefab7 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 @@ -34,9 +34,8 @@ void checksIfWorkflowEmpty() { when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) .thenReturn(mock(DependentResource.class)); - assertThat(managedWorkflow().isEmptyWorkflow()).isTrue(); - // when(mockWorkflow.getDependentResources()).thenReturn(Set.of(mock(DependentResource.class))); + assertThat(managedWorkflow().isEmptyWorkflow()).isTrue(); assertThat(managedWorkflow(createDRS(NAME)).isEmptyWorkflow()).isFalse(); } From d31eb4b8467a65a11d4f74aa31e520b8a05001e2 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 26 Nov 2022 17:26:37 +0100 Subject: [PATCH 08/19] refactor: extract common behavior from node executors --- .../workflow/AbstractWorkflowExecutor.java | 14 +++ .../dependent/workflow/NodeExecutor.java | 33 +++++ .../workflow/WorkflowCleanupExecutor.java | 58 ++++----- .../workflow/WorkflowReconcileExecutor.java | 119 +++++++----------- 4 files changed, 114 insertions(+), 110 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/NodeExecutor.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java index 8342fa3e24..9f4b6d64c2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java @@ -3,6 +3,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Future; @@ -97,4 +98,17 @@ protected synchronized void handleNodeExecutionFinish( this.notifyAll(); } } + + @SuppressWarnings("unchecked") + protected DependentResource getDependentResourceFor(DependentResourceNode drn) { + return (DependentResource) workflow.getDependentResourceFor(drn); + } + + protected boolean isConditionMet(Optional> condition, + DependentResource dependentResource) { + return condition.map(c -> c.isMet(primary, + dependentResource.getSecondaryResource(primary, context).orElse(null), + context)) + .orElse(true); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/NodeExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/NodeExecutor.java new file mode 100644 index 0000000000..393e3d2e2e --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/NodeExecutor.java @@ -0,0 +1,33 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; + +abstract class NodeExecutor implements Runnable { + + private final DependentResourceNode dependentResourceNode; + private final AbstractWorkflowExecutor

workflowExecutor; + + protected NodeExecutor(DependentResourceNode dependentResourceNode, + AbstractWorkflowExecutor

workflowExecutor) { + this.dependentResourceNode = dependentResourceNode; + this.workflowExecutor = workflowExecutor; + } + + @Override + public void run() { + try { + var dependentResource = workflowExecutor.getDependentResourceFor(dependentResourceNode); + + doRun(dependentResourceNode, dependentResource); + + } catch (RuntimeException e) { + workflowExecutor.handleExceptionInExecutor(dependentResourceNode, e); + } finally { + workflowExecutor.handleNodeExecutionFinish(dependentResourceNode); + } + } + + protected abstract void doRun(DependentResourceNode dependentResourceNode, + DependentResource dependentResource); +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index b017a207f8..060c2d2431 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -53,51 +53,39 @@ private synchronized void handleCleanup(DependentResourceNode dependentResourceN return; } - Future nodeFuture = - workflow.getExecutorService().submit(new NodeExecutor(dependentResourceNode)); + Future nodeFuture = workflow.getExecutorService() + .submit(new CleanupExecutor<>(dependentResourceNode)); markAsExecuting(dependentResourceNode, nodeFuture); log.debug("Submitted for cleanup: {}", dependentResourceNode); } - private class NodeExecutor implements Runnable { - private final DependentResourceNode dependentResourceNode; + private class CleanupExecutor extends NodeExecutor { - private NodeExecutor(DependentResourceNode dependentResourceNode) { - this.dependentResourceNode = dependentResourceNode; + private CleanupExecutor(DependentResourceNode drn) { + super(drn, WorkflowCleanupExecutor.this); } @Override @SuppressWarnings("unchecked") - public void run() { - try { - var dependentResource = (DependentResource) workflow.getDependentResourceFor( - dependentResourceNode); - var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - - if (dependentResource instanceof Deleter - && !(dependentResource instanceof GarbageCollected)) { - ((Deleter

) dependentResource).delete(primary, context); - deleteCalled.add(dependentResourceNode); - } - boolean deletePostConditionMet = deletePostCondition - .map(c -> c.isMet(primary, - dependentResource.getSecondaryResource(primary, context).orElse(null), - context)) - .orElse(true); - if (deletePostConditionMet) { - markAsVisited(dependentResourceNode); - handleDependentCleaned(dependentResourceNode); - } else { - // updating alreadyVisited needs to be the last operation otherwise could lead to a race - // condition in handleCleanup condition checks - postDeleteConditionNotMet.add(dependentResourceNode); - markAsVisited(dependentResourceNode); - } - } catch (RuntimeException e) { - handleExceptionInExecutor(dependentResourceNode, e); - } finally { - handleNodeExecutionFinish(dependentResourceNode); + protected void doRun(DependentResourceNode dependentResourceNode, + DependentResource dependentResource) { + var deletePostCondition = dependentResourceNode.getDeletePostcondition(); + + if (dependentResource instanceof Deleter + && !(dependentResource instanceof GarbageCollected)) { + ((Deleter

) dependentResource).delete(primary, context); + deleteCalled.add(dependentResourceNode); + } + boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); + if (deletePostConditionMet) { + markAsVisited(dependentResourceNode); + handleDependentCleaned(dependentResourceNode); + } else { + // updating alreadyVisited needs to be the last operation otherwise could lead to a race + // condition in handleCleanup condition checks + postDeleteConditionNotMet.add(dependentResourceNode); + markAsVisited(dependentResourceNode); } } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 8f9f824afd..0941011316 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -63,13 +63,8 @@ private synchronized void handleReconcile(DependentResourceNode depend return; } - final var dr = - (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); - boolean reconcileConditionMet = dependentResourceNode.getReconcilePrecondition() - .map(rc -> rc.isMet(primary, dr.getSecondaryResource(primary, context).orElse(null), - context)) - .orElse(true); - + boolean reconcileConditionMet = isConditionMet(dependentResourceNode.getReconcilePrecondition(), + getDependentResourceFor(dependentResourceNode)); if (!reconcileConditionMet) { handleReconcileConditionNotMet(dependentResourceNode); } else { @@ -111,88 +106,62 @@ private synchronized void setAlreadyReconciledButNotReady( notReady.add(dependentResourceNode); } - private class NodeReconcileExecutor implements Runnable { - - private final DependentResourceNode dependentResourceNode; + private class NodeReconcileExecutor extends NodeExecutor { - private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) { - this.dependentResourceNode = dependentResourceNode; + private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) { + super(dependentResourceNode, WorkflowReconcileExecutor.this); } @Override - @SuppressWarnings("unchecked") - public void run() { - try { - final var dependentResource = - (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); - if (log.isDebugEnabled()) { - log.debug( - "Reconciling {} for primary: {}", - dependentResourceNode, - ResourceID.fromResource(primary)); - } - ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); - reconcileResults.put(dependentResource, reconcileResult); - reconciled.add(dependentResourceNode); - - boolean ready = dependentResourceNode.getReadyPostcondition() - .map(rc -> rc.isMet(primary, - dependentResource.getSecondaryResource(primary, context).orElse(null), - context)) - .orElse(true); - - if (ready) { - log.debug("Setting already reconciled for: {}", dependentResourceNode); - markAsVisited(dependentResourceNode); - handleDependentsReconcile(dependentResourceNode); - } else { - setAlreadyReconciledButNotReady(dependentResourceNode); - } - } catch (RuntimeException e) { - handleExceptionInExecutor(dependentResourceNode, e); - } finally { - handleNodeExecutionFinish(dependentResourceNode); + protected void doRun(DependentResourceNode dependentResourceNode, + DependentResource dependentResource) { + if (log.isDebugEnabled()) { + log.debug( + "Reconciling {} for primary: {}", + dependentResourceNode, + ResourceID.fromResource(primary)); + } + ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); + reconcileResults.put(dependentResource, reconcileResult); + reconciled.add(dependentResourceNode); + + boolean ready = isConditionMet(dependentResourceNode.getReadyPostcondition(), + dependentResource); + if (ready) { + log.debug("Setting already reconciled for: {}", dependentResourceNode); + markAsVisited(dependentResourceNode); + handleDependentsReconcile(dependentResourceNode); + } else { + setAlreadyReconciledButNotReady(dependentResourceNode); } } } - private class NodeDeleteExecutor implements Runnable { - - private final DependentResourceNode dependentResourceNode; + private class NodeDeleteExecutor extends NodeExecutor { private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { - this.dependentResourceNode = dependentResourceNode; + super(dependentResourceNode, WorkflowReconcileExecutor.this); } @Override @SuppressWarnings("unchecked") - public void run() { - try { - final var dependentResource = - (DependentResource) workflow.getDependentResourceFor(dependentResourceNode); - var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - - if (dependentResource instanceof Deleter - && !(dependentResource instanceof GarbageCollected)) { - ((Deleter

) dependentResource).delete(primary, context); - } - boolean deletePostConditionMet = - deletePostCondition.map(c -> c.isMet(primary, - dependentResource.getSecondaryResource(primary, context).orElse(null), - context)).orElse(true); - if (deletePostConditionMet) { - markAsVisited(dependentResourceNode); - handleDependentDeleted(dependentResourceNode); - } else { - // updating alreadyVisited needs to be the last operation otherwise could lead to a race - // condition in handleDelete condition checks - deletePostConditionNotMet.add(dependentResourceNode); - markAsVisited(dependentResourceNode); - } - } catch (RuntimeException e) { - handleExceptionInExecutor(dependentResourceNode, e); - } finally { - handleNodeExecutionFinish(dependentResourceNode); + protected void doRun(DependentResourceNode dependentResourceNode, + DependentResource dependentResource) { + var deletePostCondition = dependentResourceNode.getDeletePostcondition(); + + if (dependentResource instanceof Deleter + && !(dependentResource instanceof GarbageCollected)) { + ((Deleter

) dependentResource).delete(primary, context); + } + boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); + if (deletePostConditionMet) { + markAsVisited(dependentResourceNode); + handleDependentDeleted(dependentResourceNode); + } else { + // updating alreadyVisited needs to be the last operation otherwise could lead to a race + // condition in handleDelete condition checks + deletePostConditionNotMet.add(dependentResourceNode); + markAsVisited(dependentResourceNode); } } } From b6621180afbc7182052c25d4ee172abd00a69be5 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 26 Nov 2022 17:37:03 +0100 Subject: [PATCH 09/19] refactor: extract canDeleteIfAble method --- .../api/reconciler/dependent/DependentResource.java | 4 ++++ .../dependent/workflow/DefaultManagedWorkflow.java | 4 +--- .../dependent/workflow/WorkflowCleanupExecutor.java | 4 +--- .../dependent/workflow/WorkflowReconcileExecutor.java | 6 ++---- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java index ea7759b8c4..7368dbd130 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java @@ -64,4 +64,8 @@ default Optional getSecondaryResource(P primary, Context

context) { static String defaultNameFor(Class dependentResourceClass) { return dependentResourceClass.getName(); } + + static boolean canDeleteIfAble(DependentResource dependentResource) { + return dependentResource instanceof Deleter && !(dependentResource instanceof GarbageCollected); + } } 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 261f7010b3..47f2bf6646 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 @@ -8,9 +8,7 @@ import io.fabric8.kubernetes.client.KubernetesClient; 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; @SuppressWarnings("rawtypes") public class DefaultManagedWorkflow

implements ManagedWorkflow

{ @@ -60,7 +58,7 @@ public ManagedWorkflow

resolve(KubernetesClient client, List { final var dr = managedWorkflowSupport.createAndConfigureFrom(spec, client); dependentResourcesByName.put(spec.getName(), dr); - if (dr instanceof Deleter && !(dr instanceof GarbageCollected)) { + if (DependentResource.canDeleteIfAble(dr)) { cleanerHolder[0] = true; } }); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 060c2d2431..79bec9aab4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -13,7 +13,6 @@ 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; @SuppressWarnings("rawtypes") public class WorkflowCleanupExecutor

extends AbstractWorkflowExecutor

{ @@ -72,8 +71,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (dependentResource instanceof Deleter - && !(dependentResource instanceof GarbageCollected)) { + if (DependentResource.canDeleteIfAble(dependentResource)) { ((Deleter

) dependentResource).delete(primary, context); deleteCalled.add(dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 0941011316..b9b476cf4a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -14,7 +14,6 @@ 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.api.reconciler.dependent.ReconcileResult; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -149,8 +148,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (dependentResource instanceof Deleter - && !(dependentResource instanceof GarbageCollected)) { + if (DependentResource.canDeleteIfAble(dependentResource)) { ((Deleter

) dependentResource).delete(primary, context); } boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); @@ -210,7 +208,7 @@ private boolean allParentsReconciledAndReady(DependentResourceNode depende private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { return !dependentResourceNode.getDependsOn().isEmpty() && dependentResourceNode.getDependsOn().stream() - .anyMatch(this::isInError); + .anyMatch(this::isInError); } private WorkflowReconcileResult createReconcileResult() { From 52a3686a10750267848fc65ec0392e0764da180e Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 26 Nov 2022 17:42:32 +0100 Subject: [PATCH 10/19] fix: wait should be called from synchronized block --- .../processing/dependent/workflow/AbstractWorkflowExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java index 9f4b6d64c2..489eab7552 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java @@ -37,7 +37,7 @@ public AbstractWorkflowExecutor(Workflow

workflow, P primary, Context

cont protected abstract Logger logger(); - protected void waitForScheduledExecutionsToRun() { + protected synchronized void waitForScheduledExecutionsToRun() { while (true) { try { this.wait(); From ccaa52455cbe2872c64b76107a914ae66f61c836 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 26 Nov 2022 20:50:05 +0100 Subject: [PATCH 11/19] fix: format --- .../dependent/workflow/AbstractWorkflowExecutor.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java index 489eab7552..354975ebad 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutor.java @@ -107,8 +107,8 @@ protected DependentResource getDependentResourceFor(DependentResourceN protected boolean isConditionMet(Optional> condition, DependentResource dependentResource) { return condition.map(c -> c.isMet(primary, - dependentResource.getSecondaryResource(primary, context).orElse(null), - context)) + dependentResource.getSecondaryResource(primary, context).orElse(null), + context)) .orElse(true); } } From 3813febf965787450d9157b28d529ec309a78772 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sun, 27 Nov 2022 12:49:30 +0100 Subject: [PATCH 12/19] refactor: share code between node implementations --- .../AbstractDependentResourceNode.java | 93 +++++++++++++++++++ .../DefaultDependentResourceNode.java | 74 ++------------- .../workflow/SpecDependentResourceNode.java | 60 ++---------- 3 files changed, 105 insertions(+), 122 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java new file mode 100644 index 0000000000..d2a8de925b --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java @@ -0,0 +1,93 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import java.util.LinkedList; +import java.util.List; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.HasMetadata; + +@SuppressWarnings("rawtypes") +public abstract class AbstractDependentResourceNode + implements DependentResourceNode { + + private final List dependsOn = new LinkedList<>(); + private final List parents = new LinkedList<>(); + private final String name; + private Condition reconcilePrecondition; + private Condition deletePostcondition; + private Condition readyPostcondition; + + protected AbstractDependentResourceNode(String name) { + this.name = name; + } + + @Override + public List getDependsOn() { + return dependsOn; + } + + @Override + public void addParent(DependentResourceNode parent) { + parents.add(parent); + } + + @Override + public void addDependsOnRelation(DependentResourceNode node) { + node.addParent(this); + dependsOn.add(node); + } + + @Override + public List getParents() { + return parents; + } + + @Override + public String getName() { + return name; + } + + @Override + public Optional> getReconcilePrecondition() { + return Optional.ofNullable(reconcilePrecondition); + } + + @Override + public Optional> getDeletePostcondition() { + return Optional.ofNullable(deletePostcondition); + } + + public void setReconcilePrecondition(Condition reconcilePrecondition) { + this.reconcilePrecondition = reconcilePrecondition; + } + + public void setDeletePostcondition(Condition cleanupCondition) { + this.deletePostcondition = cleanupCondition; + } + + @Override + public Optional> getReadyPostcondition() { + return Optional.ofNullable(readyPostcondition); + } + + public void setReadyPostcondition(Condition readyPostcondition) { + this.readyPostcondition = readyPostcondition; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + AbstractDependentResourceNode that = (AbstractDependentResourceNode) o; + return name.equals(that.name); + } + + @Override + public int hashCode() { + return name.hashCode(); + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 5c165316c6..58686aae52 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -1,22 +1,12 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.LinkedList; -import java.util.List; -import java.util.Optional; - import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -@SuppressWarnings("rawtypes") -public class DefaultDependentResourceNode implements - DependentResourceNode { +public class DefaultDependentResourceNode + extends AbstractDependentResourceNode { private final DependentResource dependentResource; - private Condition reconcilePrecondition; - private Condition deletePostcondition; - private Condition readyPostcondition; - private final List dependsOn = new LinkedList<>(); - private final List parents = new LinkedList<>(); public DefaultDependentResourceNode(DependentResource dependentResource) { this(dependentResource, null, null); @@ -29,71 +19,19 @@ public DefaultDependentResourceNode(DependentResource dependentResource, public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition, Condition deletePostcondition) { + super(DependentResource.defaultNameFor(dependentResource.getClass()) + "#" + + dependentResource.hashCode()); this.dependentResource = dependentResource; - this.reconcilePrecondition = reconcilePrecondition; - this.deletePostcondition = deletePostcondition; + setReconcilePrecondition(reconcilePrecondition); + setDeletePostcondition(deletePostcondition); } public DependentResource getDependentResource() { return dependentResource; } - @Override - public Optional> getReconcilePrecondition() { - return Optional.ofNullable(reconcilePrecondition); - } - - @Override - public Optional> getDeletePostcondition() { - return Optional.ofNullable(deletePostcondition); - } - - @Override - public List getDependsOn() { - return dependsOn; - } - - @Override - public void addParent(DependentResourceNode parent) { - parents.add(parent); - } - - @Override - public void addDependsOnRelation(DependentResourceNode node) { - node.addParent(this); - dependsOn.add(node); - } - @Override public String toString() { return "DependentResourceNode{" + dependentResource + '}'; } - - public void setReconcilePrecondition(Condition reconcilePrecondition) { - this.reconcilePrecondition = reconcilePrecondition; - } - - public void setDeletePostcondition(Condition cleanupCondition) { - this.deletePostcondition = cleanupCondition; - } - - @Override - public Optional> getReadyPostcondition() { - return Optional.ofNullable(readyPostcondition); - } - - public void setReadyPostcondition(Condition readyPostcondition) { - this.readyPostcondition = readyPostcondition; - } - - @Override - public List getParents() { - return parents; - } - - @Override - public String getName() { - return DependentResource.defaultNameFor(dependentResource.getClass()) + "#" - + dependentResource.hashCode(); - } } 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 37df5bfcbb..3c1626255e 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,63 +1,15 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.LinkedList; -import java.util.List; -import java.util.Optional; - import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; -@SuppressWarnings("rawtypes") public class SpecDependentResourceNode - implements DependentResourceNode { - - private final DependentResourceSpec spec; - private final List dependsOn = new LinkedList<>(); - private final List parents = new LinkedList<>(); - + extends AbstractDependentResourceNode { + @SuppressWarnings("unchecked") public SpecDependentResourceNode(DependentResourceSpec spec) { - this.spec = spec; - } - - @Override - public Optional> getReconcilePrecondition() { - return Optional.ofNullable(spec.getReconcileCondition()); - } - - @Override - public Optional> getDeletePostcondition() { - return Optional.ofNullable(spec.getDeletePostCondition()); - } - - @Override - public List getDependsOn() { - return dependsOn; - } - - @Override - public void addDependsOnRelation(DependentResourceNode node) { - node.addParent(this); - dependsOn.add(node); - } - - @Override - public Optional> getReadyPostcondition() { - return Optional.ofNullable(spec.getReadyCondition()); - } - - @Override - public List getParents() { - return parents; - } - - - @Override - public void addParent(DependentResourceNode parent) { - parents.add(parent); - } - - @Override - public String getName() { - return spec.getName(); + super(spec.getName()); + setReadyPostcondition(spec.getReadyCondition()); + setDeletePostcondition(spec.getDeletePostCondition()); + setReconcilePrecondition(spec.getReconcileCondition()); } } From da9ca925e41da729ec1a0d5891b7cd515d8ac6a6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sun, 27 Nov 2022 14:01:34 +0100 Subject: [PATCH 13/19] refactor: simplify dependent resource resolution --- .../AbstractDependentResourceNode.java | 10 +++ .../DefaultDependentResourceNode.java | 10 +-- .../workflow/DependentResourceNode.java | 4 ++ .../workflow/SpecDependentResourceNode.java | 12 ++++ .../dependent/workflow/Workflow.java | 67 +++++++------------ .../workflow/builder/WorkflowBuilder.java | 3 +- 6 files changed, 56 insertions(+), 50 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java index d2a8de925b..9ca20b84bd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java @@ -5,6 +5,7 @@ import java.util.Optional; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") public abstract class AbstractDependentResourceNode @@ -16,6 +17,7 @@ public abstract class AbstractDependentResourceNode private Condition reconcilePrecondition; private Condition deletePostcondition; private Condition readyPostcondition; + private DependentResource dependentResource; protected AbstractDependentResourceNode(String name) { this.name = name; @@ -74,6 +76,14 @@ public void setReadyPostcondition(Condition readyPostcondition) { this.readyPostcondition = readyPostcondition; } + public DependentResource getDependentResource() { + return dependentResource; + } + + public void setDependentResource(DependentResource dependentResource) { + this.dependentResource = dependentResource; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 58686aae52..1baa3966ec 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -6,8 +6,6 @@ public class DefaultDependentResourceNode extends AbstractDependentResourceNode { - private final DependentResource dependentResource; - public DefaultDependentResourceNode(DependentResource dependentResource) { this(dependentResource, null, null); } @@ -21,17 +19,13 @@ public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition, Condition deletePostcondition) { super(DependentResource.defaultNameFor(dependentResource.getClass()) + "#" + dependentResource.hashCode()); - this.dependentResource = dependentResource; + setDependentResource(dependentResource); setReconcilePrecondition(reconcilePrecondition); setDeletePostcondition(deletePostcondition); } - public DependentResource getDependentResource() { - return dependentResource; - } - @Override public String toString() { - return "DependentResourceNode{" + dependentResource + '}'; + return "DependentResourceNode{" + getDependentResource() + '}'; } } 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 43dd89e05b..2c069bf999 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 @@ -4,6 +4,8 @@ import java.util.Optional; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; @SuppressWarnings("rawtypes") public interface DependentResourceNode { @@ -23,4 +25,6 @@ public interface DependentResourceNode { void addParent(DependentResourceNode parent); String getName(); + + default void resolve(KubernetesClient client, List dependentResources) {} } 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 3c1626255e..144f5c53ea 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,6 +1,9 @@ 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.dependent.DependentResourceSpec; public class SpecDependentResourceNode @@ -12,4 +15,13 @@ public SpecDependentResourceNode(DependentResourceSpec spec) { setDeletePostcondition(spec.getDeletePostCondition()); setReconcilePrecondition(spec.getReconcileCondition()); } + + @Override + @SuppressWarnings({"rawtypes", "unchecked"}) + public void resolve(KubernetesClient client, List dependentResources) { + final var spec = dependentResources.stream() + .filter(drs -> drs.getName().equals(getName())) + .findFirst().orElseThrow(); + setDependentResource(ManagedWorkflowSupport.instance().createAndConfigureFrom(spec, client)); + } } 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 bc769c9631..2dd1606ae3 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 @@ -1,10 +1,12 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutorService; +import java.util.function.Function; import java.util.stream.Collectors; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -24,33 +26,45 @@ public class Workflow

{ public static final boolean THROW_EXCEPTION_AUTOMATICALLY_DEFAULT = true; - private final Map dependentResourceNodes; + private final Map dependentResourceNodes; private final Set topLevelResources = new HashSet<>(); private final Set bottomLevelResource = new HashSet<>(); private final boolean throwExceptionAutomatically; // it's "global" executor service shared between multiple reconciliations running parallel private final ExecutorService executorService; + private boolean resolved; public Workflow(Set dependentResourceNodes) { this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), - THROW_EXCEPTION_AUTOMATICALLY_DEFAULT); + THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false); } public Workflow(Set dependentResourceNodes, - ExecutorService executorService, boolean throwExceptionAutomatically) { + ExecutorService executorService, boolean throwExceptionAutomatically, boolean resolved) { this.executorService = executorService; this.dependentResourceNodes = dependentResourceNodes.stream() - .collect(Collectors.toMap(DependentResourceNode::getName, ResolvedNode::new)); + .collect(Collectors.toMap(DependentResourceNode::getName, Function.identity())); this.throwExceptionAutomatically = throwExceptionAutomatically; + this.resolved = resolved; preprocessForReconcile(); } public DependentResource getDependentResourceFor(DependentResourceNode node) { - return dependentResourceNodes.get(node.getName()).dependentResource(); + throwIfUnresolved(); + return ((AbstractDependentResourceNode) dependentResourceNodes.get(node.getName())) + .getDependentResource(); + } + + private void throwIfUnresolved() { + if (!resolved) { + throw new IllegalStateException( + "Should call resolved before trying to access DependentResources"); + } } public WorkflowReconcileResult reconcile(P primary, Context

context) { + throwIfUnresolved(); WorkflowReconcileExecutor

workflowReconcileExecutor = new WorkflowReconcileExecutor<>(this, primary, context); var result = workflowReconcileExecutor.reconcile(); @@ -61,6 +75,7 @@ public WorkflowReconcileResult reconcile(P primary, Context

context) { } public WorkflowCleanupResult cleanup(P primary, Context

context) { + throwIfUnresolved(); WorkflowCleanupExecutor

workflowCleanupExecutor = new WorkflowCleanupExecutor<>(this, primary, context); var result = workflowCleanupExecutor.cleanup(); @@ -73,8 +88,7 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { // add cycle detection? @SuppressWarnings("unchecked") private void preprocessForReconcile() { - final var nodes = dependentResourceNodes.values().stream() - .map(ResolvedNode::node).collect(Collectors.toList()); + final var nodes = new ArrayList<>(dependentResourceNodes.values()); bottomLevelResource.addAll(nodes); for (DependentResourceNode node : nodes) { if (node.getDependsOn().isEmpty()) { @@ -100,43 +114,14 @@ ExecutorService getExecutorService() { } Set nodes() { - return dependentResourceNodes.values().stream().map(ResolvedNode::node) - .collect(Collectors.toSet()); + return new HashSet<>(dependentResourceNodes.values()); } + @SuppressWarnings("unchecked") public void resolve(KubernetesClient client, List dependentResources) { - dependentResourceNodes.values().forEach(drn -> drn.resolve(client, dependentResources)); - } - - private static class ResolvedNode { - - private final DependentResourceNode node; - private DependentResource dependentResource; - - private ResolvedNode(DependentResourceNode node) { - this.node = node; - if (node instanceof DefaultDependentResourceNode) { - this.dependentResource = ((DefaultDependentResourceNode) node).getDependentResource(); - } - } - - public void setDependentResource(DependentResource dependentResource) { - this.dependentResource = dependentResource; - } - - public DependentResource dependentResource() { - return dependentResource; - } - - public DependentResourceNode node() { - return node; - } - - public void resolve(KubernetesClient client, List dependentResources) { - final var spec = dependentResources.stream() - .filter(drs -> drs.getName().equals(node.getName())) - .findFirst().orElseThrow(); - dependentResource = ManagedWorkflowSupport.instance().createAndConfigureFrom(spec, client); + if (!resolved) { + dependentResourceNodes.values().forEach(drn -> drn.resolve(client, dependentResources)); + resolved = true; } } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java index f86c95dee0..54033e57e5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java @@ -85,6 +85,7 @@ public Workflow

build(int parallelism) { } public Workflow

build(ExecutorService executorService) { - return new Workflow(dependentResourceNodes, executorService, throwExceptionAutomatically); + // workflow has been built from dependent resources so it is already resolved + return new Workflow(dependentResourceNodes, executorService, throwExceptionAutomatically, true); } } From 46e8cdfe718933c2c63ee100bac6eb0bcf31d20d Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sun, 27 Nov 2022 14:08:20 +0100 Subject: [PATCH 14/19] refactor: move WorkflowBuilder to same directory, protect constructors --- .../dependent/workflow/AbstractDependentResourceNode.java | 2 +- .../dependent/workflow/DefaultDependentResourceNode.java | 7 +------ .../dependent/workflow/SpecDependentResourceNode.java | 2 +- .../operator/processing/dependent/workflow/Workflow.java | 4 ++-- .../dependent/workflow/{builder => }/WorkflowBuilder.java | 6 +----- .../dependent/workflow/WorkflowCleanupExecutorTest.java | 2 +- .../dependent/workflow/WorkflowReconcileExecutorTest.java | 2 +- .../processing/dependent/workflow/WorkflowTest.java | 1 - .../sample/WebPageDependentsWorkflowReconciler.java | 5 +++-- 9 files changed, 11 insertions(+), 20 deletions(-) rename operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/{builder => }/WorkflowBuilder.java (90%) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java index 9ca20b84bd..22e368249a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractDependentResourceNode.java @@ -8,7 +8,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; @SuppressWarnings("rawtypes") -public abstract class AbstractDependentResourceNode +abstract class AbstractDependentResourceNode implements DependentResourceNode { private final List dependsOn = new LinkedList<>(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index 1baa3966ec..b08301fa25 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -3,18 +3,13 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -public class DefaultDependentResourceNode +class DefaultDependentResourceNode extends AbstractDependentResourceNode { public DefaultDependentResourceNode(DependentResource dependentResource) { this(dependentResource, null, null); } - public DefaultDependentResourceNode(DependentResource dependentResource, - Condition reconcilePrecondition) { - this(dependentResource, reconcilePrecondition, null); - } - public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition, Condition deletePostcondition) { super(DependentResource.defaultNameFor(dependentResource.getClass()) + "#" 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 144f5c53ea..2ca2bc476b 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 @@ -6,7 +6,7 @@ import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; -public class SpecDependentResourceNode +class SpecDependentResourceNode extends AbstractDependentResourceNode { @SuppressWarnings("unchecked") public SpecDependentResourceNode(DependentResourceSpec spec) { 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 2dd1606ae3..b3e1d45647 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 @@ -35,12 +35,12 @@ public class Workflow

{ private final ExecutorService executorService; private boolean resolved; - public Workflow(Set dependentResourceNodes) { + Workflow(Set dependentResourceNodes) { this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false); } - public Workflow(Set dependentResourceNodes, + Workflow(Set dependentResourceNodes, ExecutorService executorService, boolean throwExceptionAutomatically, boolean resolved) { this.executorService = executorService; this.dependentResourceNodes = dependentResourceNodes.stream() diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java similarity index 90% rename from operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java rename to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index 54033e57e5..d2ffb410a5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -1,4 +1,4 @@ -package io.javaoperatorsdk.operator.processing.dependent.workflow.builder; +package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.Arrays; import java.util.HashSet; @@ -9,10 +9,6 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.config.ExecutorServiceManager; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; -import io.javaoperatorsdk.operator.processing.dependent.workflow.DefaultDependentResourceNode; -import io.javaoperatorsdk.operator.processing.dependent.workflow.DependentResourceNode; -import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow; import static io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow.THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java index 56bd876687..9aa2fb07e9 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java @@ -5,7 +5,6 @@ import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; -import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat; @@ -17,6 +16,7 @@ class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest { protected TestDeleterDependent dd1 = new TestDeleterDependent("DR_DELETER_1"); protected TestDeleterDependent dd2 = new TestDeleterDependent("DR_DELETER_2"); protected TestDeleterDependent dd3 = new TestDeleterDependent("DR_DELETER_3"); + @SuppressWarnings("unchecked") Context mockContext = mock(Context.class); @Test diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java index 873cf66cb6..a445dc783f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java @@ -5,7 +5,6 @@ import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; -import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat; @@ -23,6 +22,7 @@ class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest { private final Condition notMetReadyCondition = (primary, secondary, context) -> false; + @SuppressWarnings("unchecked") Context mockContext = mock(Context.class); @Test 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 8e4f3fe4e5..7da98f66b2 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 @@ -6,7 +6,6 @@ import org.junit.jupiter.api.Test; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static org.assertj.core.api.Assertions.assertThat; diff --git a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java index 0eeba42083..287d27a5c3 100644 --- a/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java +++ b/sample-operators/webpage/src/main/java/io/javaoperatorsdk/operator/sample/WebPageDependentsWorkflowReconciler.java @@ -19,7 +19,7 @@ import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig; import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow; -import io.javaoperatorsdk.operator.processing.dependent.workflow.builder.WorkflowBuilder; +import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowBuilder; import io.javaoperatorsdk.operator.processing.event.source.EventSource; import static io.javaoperatorsdk.operator.sample.Utils.createStatus; @@ -31,6 +31,7 @@ */ @ControllerConfiguration( labelSelector = WebPageDependentsWorkflowReconciler.DEPENDENT_RESOURCE_LABEL_SELECTOR) +@SuppressWarnings("unused") public class WebPageDependentsWorkflowReconciler implements Reconciler, ErrorStatusHandler, EventSourceInitializer { @@ -79,7 +80,7 @@ public ErrorStatusUpdateControl updateErrorStatus( return handleError(resource, e); } - @SuppressWarnings("rawtypes") + @SuppressWarnings({"rawtypes", "unchecked"}) private void initDependentResources(KubernetesClient client) { this.configMapDR = new ConfigMapDependentResource(); this.deploymentDR = new DeploymentDependentResource(); From 747aefe59a837028cef52bc2bc1e30099580afdd Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 28 Nov 2022 17:04:04 +0100 Subject: [PATCH 15/19] refactor: reduce need to expose ManagedWorkflowSupport --- .../workflow/DefaultManagedWorkflow.java | 32 ++++-------- .../workflow/ManagedWorkflowFactory.java | 13 ++--- .../workflow/ManagedWorkflowSupport.java | 28 ----------- .../workflow/SpecDependentResourceNode.java | 25 +++++++++- .../dependent/workflow/Workflow.java | 33 ++++++++++--- .../dependent/workflow/WorkflowBuilder.java | 6 ++- .../workflow/ManagedWorkflowSupportTest.java | 2 +- .../workflow/ManagedWorkflowTest.java | 49 ++++--------------- .../workflow/ManagedWorkflowTestUtils.java | 17 +++++++ 9 files changed, 97 insertions(+), 108 deletions(-) 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 47f2bf6646..3096f1d4fb 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 @@ -14,18 +14,12 @@ public class DefaultManagedWorkflow

implements ManagedWorkflow

{ private final Workflow

workflow; - private boolean isCleaner; private final boolean isEmptyWorkflow; - private final Map dependentResourcesByName; private boolean resolved; - private final ManagedWorkflowSupport managedWorkflowSupport; - DefaultManagedWorkflow(List dependentResourceSpecs, Workflow

workflow, - ManagedWorkflowSupport managedWorkflowSupport) { - dependentResourcesByName = new HashMap<>(dependentResourceSpecs.size()); + DefaultManagedWorkflow(List dependentResourceSpecs, Workflow

workflow) { isEmptyWorkflow = dependentResourceSpecs.isEmpty(); this.workflow = workflow; - this.managedWorkflowSupport = managedWorkflowSupport; } public WorkflowReconcileResult reconcile(P primary, Context

context) { @@ -39,8 +33,7 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { } public boolean isCleaner() { - checkIfResolved(); - return isCleaner; + return workflow.hasCleaner(); } public boolean isEmptyWorkflow() { @@ -49,23 +42,18 @@ public boolean isEmptyWorkflow() { public Map getDependentResourcesByName() { checkIfResolved(); - return dependentResourcesByName; + final var nodes = workflow.nodes(); + final var result = new HashMap(nodes.size()); + nodes.forEach((key, drn) -> result.put(key, workflow.getDependentResourceFor(drn))); + return result; } @Override public ManagedWorkflow

resolve(KubernetesClient client, List specs) { - final boolean[] cleanerHolder = {false}; - specs.forEach(spec -> { - final var dr = managedWorkflowSupport.createAndConfigureFrom(spec, client); - dependentResourcesByName.put(spec.getName(), dr); - if (DependentResource.canDeleteIfAble(dr)) { - cleanerHolder[0] = true; - } - }); - - workflow.resolve(client, specs); - isCleaner = cleanerHolder[0]; - resolved = true; + if (!resolved) { + workflow.resolve(client, specs); + resolved = true; + } return this; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java index 3fe37a8a27..5b5135e95a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ManagedWorkflowFactory.java @@ -6,22 +6,17 @@ public interface ManagedWorkflowFactory { - ManagedWorkflowFactory DEFAULT = (configuration, managedWorkflowSupport) -> { + @SuppressWarnings({"rawtypes", "unchecked"}) + ManagedWorkflowFactory DEFAULT = (configuration) -> { final var dependentResourceSpecs = configuration.getDependentResources(); if (dependentResourceSpecs == null || dependentResourceSpecs.isEmpty()) { return noOpWorkflow; } return new DefaultManagedWorkflow(dependentResourceSpecs, - managedWorkflowSupport.createWorkflow(dependentResourceSpecs), managedWorkflowSupport); + ManagedWorkflowSupport.instance().createWorkflow(dependentResourceSpecs)); }; @SuppressWarnings("rawtypes") - default ManagedWorkflow workflowFor(ControllerConfiguration configuration) { - return workflowFor(configuration, ManagedWorkflowSupport.instance()); - } - - @SuppressWarnings("rawtypes") - ManagedWorkflow workflowFor(ControllerConfiguration configuration, - ManagedWorkflowSupport managedWorkflowSupport); + ManagedWorkflow workflowFor(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 c14e1f10ca..9b11784443 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 @@ -10,12 +10,8 @@ import java.util.stream.Collectors; import io.fabric8.kubernetes.api.model.HasMetadata; -import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; -import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer; -import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.KubernetesClientAware; @SuppressWarnings({"rawtypes", "unchecked"}) class ManagedWorkflowSupport { @@ -73,30 +69,6 @@ private DependentResourceNode createFrom(DependentResourceSpec spec, return node; } - @SuppressWarnings({"rawtypes"}) - public DependentResource createAndConfigureFrom(DependentResourceSpec spec, - KubernetesClient client) { - final var dependentResource = spec.getDependentResource(); - - if (dependentResource instanceof KubernetesClientAware) { - ((KubernetesClientAware) dependentResource).setKubernetesClient(client); - } - - spec.getUseEventSourceWithName() - .ifPresent(esName -> { - final var name = (String) esName; - if (dependentResource instanceof EventSourceReferencer) { - ((EventSourceReferencer) dependentResource).useEventSourceWithName(name); - } else { - throw new IllegalStateException( - "DependentResource " + spec + " wants to use EventSource named " + name - + " but doesn't implement support for this feature by implementing " - + EventSourceReferencer.class.getSimpleName()); - } - }); - return dependentResource; - } - /** * @param dependentResourceSpecs list of specs * @return top-bottom ordered resources that can be added safely to workflow 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 2ca2bc476b..ae8ed282c6 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 @@ -5,6 +5,9 @@ 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.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer; +import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.KubernetesClientAware; class SpecDependentResourceNode extends AbstractDependentResourceNode { @@ -22,6 +25,26 @@ public void resolve(KubernetesClient client, List depende final var spec = dependentResources.stream() .filter(drs -> drs.getName().equals(getName())) .findFirst().orElseThrow(); - setDependentResource(ManagedWorkflowSupport.instance().createAndConfigureFrom(spec, client)); + + final DependentResource dependentResource = spec.getDependentResource(); + + if (dependentResource instanceof KubernetesClientAware) { + ((KubernetesClientAware) dependentResource).setKubernetesClient(client); + } + + spec.getUseEventSourceWithName() + .ifPresent(esName -> { + final var name = (String) esName; + if (dependentResource instanceof EventSourceReferencer) { + ((EventSourceReferencer) dependentResource).useEventSourceWithName(name); + } else { + throw new IllegalStateException( + "DependentResource " + spec + " wants to use EventSource named " + name + + " but doesn't implement support for this feature by implementing " + + EventSourceReferencer.class.getSimpleName()); + } + }); + + setDependentResource(dependentResource); } } 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 b3e1d45647..db89a4916e 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 @@ -34,24 +34,31 @@ public class Workflow

{ // it's "global" executor service shared between multiple reconciliations running parallel private final ExecutorService executorService; private boolean resolved; + private boolean hasCleaner; Workflow(Set dependentResourceNodes) { this(dependentResourceNodes, ExecutorServiceManager.instance().workflowExecutorService(), - THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false); + THROW_EXCEPTION_AUTOMATICALLY_DEFAULT, false, false); } Workflow(Set dependentResourceNodes, - ExecutorService executorService, boolean throwExceptionAutomatically, boolean resolved) { + ExecutorService executorService, boolean throwExceptionAutomatically, boolean resolved, + boolean hasCleaner) { this.executorService = executorService; this.dependentResourceNodes = dependentResourceNodes.stream() .collect(Collectors.toMap(DependentResourceNode::getName, Function.identity())); this.throwExceptionAutomatically = throwExceptionAutomatically; this.resolved = resolved; + this.hasCleaner = hasCleaner; preprocessForReconcile(); } public DependentResource getDependentResourceFor(DependentResourceNode node) { throwIfUnresolved(); + return dependentResource(node); + } + + private DependentResource dependentResource(DependentResourceNode node) { return ((AbstractDependentResourceNode) dependentResourceNodes.get(node.getName())) .getDependentResource(); } @@ -113,15 +120,29 @@ ExecutorService getExecutorService() { return executorService; } - Set nodes() { - return new HashSet<>(dependentResourceNodes.values()); + Map nodes() { + return dependentResourceNodes; } @SuppressWarnings("unchecked") - public void resolve(KubernetesClient client, List dependentResources) { + void resolve(KubernetesClient client, List dependentResources) { if (!resolved) { - dependentResourceNodes.values().forEach(drn -> drn.resolve(client, dependentResources)); + final boolean[] cleanerHolder = {false}; + dependentResourceNodes.values() + .forEach(drn -> { + drn.resolve(client, dependentResources); + final var dr = dependentResource(drn); + if (DependentResource.canDeleteIfAble(dr)) { + cleanerHolder[0] = true; + } + }); resolved = true; + hasCleaner = cleanerHolder[0]; } } + + boolean hasCleaner() { + throwIfUnresolved(); + return hasCleaner; + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index d2ffb410a5..e7733cff8a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -17,11 +17,12 @@ public class WorkflowBuilder

{ private final Set> dependentResourceNodes = new HashSet<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; - private DefaultDependentResourceNode currentNode; + private boolean isCleaner = false; public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DefaultDependentResourceNode<>(dependentResource); + isCleaner = DependentResource.canDeleteIfAble(dependentResource); dependentResourceNodes.add(currentNode); return this; } @@ -82,6 +83,7 @@ public Workflow

build(int parallelism) { public Workflow

build(ExecutorService executorService) { // workflow has been built from dependent resources so it is already resolved - return new Workflow(dependentResourceNodes, executorService, throwExceptionAutomatically, true); + return new Workflow(dependentResourceNodes, executorService, + throwExceptionAutomatically, true, isCleaner); } } 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 7db824d7ad..f4ef13d122 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 @@ -144,7 +144,7 @@ void createsWorkflow() { var workflow = managedWorkflowSupport.createWorkflow(specs); workflow.resolve(client, specs); - assertThat(workflow.nodes()).map(DependentResourceNode::getName) + assertThat(workflow.nodes().values()).map(DependentResourceNode::getName) .containsExactlyInAnyOrder(NAME_1, NAME_2, NAME_3, NAME_4); assertThat(workflow.getTopLevelDependentResources()) .map(DependentResourceNode::getName) 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 2febdefab7..88e7f4e19c 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 @@ -9,86 +9,57 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; 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 static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowTestUtils.createDRS; +import static io.javaoperatorsdk.operator.processing.dependent.workflow.ManagedWorkflowTestUtils.createDRSWithTraits; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; -import static org.mockito.Mockito.withSettings; @SuppressWarnings({"rawtypes", "unchecked"}) class ManagedWorkflowTest { public static final String NAME = "name"; - ManagedWorkflowSupport managedWorkflowSupportMock = mock(ManagedWorkflowSupport.class); - KubernetesClient kubernetesClientMock = mock(KubernetesClient.class); - @Test void checksIfWorkflowEmpty() { - var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) - .thenReturn(mock(DependentResource.class)); - assertThat(managedWorkflow().isEmptyWorkflow()).isTrue(); assertThat(managedWorkflow(createDRS(NAME)).isEmptyWorkflow()).isFalse(); } @Test void isNotCleanerIfNoDeleter() { - var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) - .thenReturn(mock(DependentResource.class)); - assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); } @Test void isNotCleanerIfNoGarbageCollected() { - var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - when(managedWorkflowSupportMock.createAndConfigureFrom(any(), any())) - .thenReturn( - mock(DependentResource.class, withSettings().extraInterfaces(GarbageCollected.class))); - - assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); + assertThat(managedWorkflow(createDRSWithTraits(NAME, GarbageCollected.class)) + .isCleaner()).isFalse(); } @Test void isCleanerIfHasDeleter() { - var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - - var spec = createDRS(NAME); - when(managedWorkflowSupportMock.createAndConfigureFrom(eq(spec), any())) - .thenReturn(mock(DependentResource.class, withSettings().extraInterfaces(Deleter.class))); + var spec = createDRSWithTraits(NAME, Deleter.class); assertThat(managedWorkflow(spec).isCleaner()).isTrue(); } @Test void isNotCleanerIfDeleterIsGarbageCollected() { - var mockWorkflow = mock(Workflow.class); - when(managedWorkflowSupportMock.createWorkflow(any())).thenReturn(mockWorkflow); - - var spec = createDRS(NAME); - when(managedWorkflowSupportMock.createAndConfigureFrom(eq(spec), any())) - .thenReturn(mock(DependentResource.class, - withSettings().extraInterfaces(Deleter.class, GarbageCollected.class))); - assertThat(managedWorkflow(createDRS(NAME)).isCleaner()).isFalse(); + var spec = createDRSWithTraits(NAME, Deleter.class, GarbageCollected.class); + assertThat(managedWorkflow(spec).isCleaner()).isFalse(); } 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, managedWorkflowSupportMock) + .workflowFor(configuration) .resolve(kubernetesClientMock, specList); } 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 85e4af05eb..51d6b533c7 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 @@ -2,9 +2,16 @@ import java.util.Set; +import org.mockito.Mockito; + import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.processing.dependent.EmptyTestDependentResource; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; + @SuppressWarnings("rawtypes") public class ManagedWorkflowTestUtils { @@ -14,4 +21,14 @@ public static DependentResourceSpec createDRS(String name, String... dependOns) null, null, null, null); } + public static DependentResourceSpec createDRSWithTraits(String name, + Class... dependentResourceTraits) { + final var drs = createDRS(name); + final var spy = Mockito.spy(drs); + when(spy.getDependentResource()) + .thenReturn( + mock(DependentResource.class, withSettings().extraInterfaces(dependentResourceTraits))); + return spy; + } + } From 96967483d29a2716a23bb5a7ab2c0ac4f500bd5c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 28 Nov 2022 19:20:49 +0100 Subject: [PATCH 16/19] refactor: associate nodes with their name --- .../DefaultDependentResourceNode.java | 9 +++++-- .../dependent/workflow/WorkflowBuilder.java | 25 +++++++++++++------ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java index b08301fa25..d0d844ea9c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultDependentResourceNode.java @@ -12,13 +12,18 @@ public DefaultDependentResourceNode(DependentResource dependentResource) { public DefaultDependentResourceNode(DependentResource dependentResource, Condition reconcilePrecondition, Condition deletePostcondition) { - super(DependentResource.defaultNameFor(dependentResource.getClass()) + "#" - + dependentResource.hashCode()); + super(getNameFor(dependentResource)); setDependentResource(dependentResource); setReconcilePrecondition(reconcilePrecondition); setDeletePostcondition(deletePostcondition); } + @SuppressWarnings("rawtypes") + static String getNameFor(DependentResource dependentResource) { + return DependentResource.defaultNameFor(dependentResource.getClass()) + "#" + + dependentResource.hashCode(); + } + @Override public String toString() { return "DependentResourceNode{" + getDependentResource() + '}'; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index e7733cff8a..6983189908 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -1,7 +1,9 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.Arrays; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -15,7 +17,8 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public class WorkflowBuilder

{ - private final Set> dependentResourceNodes = new HashSet<>(); + private final Map> dependentResourceNodes = + new HashMap<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; private DefaultDependentResourceNode currentNode; private boolean isCleaner = false; @@ -23,7 +26,8 @@ public class WorkflowBuilder

{ public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DefaultDependentResourceNode<>(dependentResource); isCleaner = DependentResource.canDeleteIfAble(dependentResource); - dependentResourceNodes.add(currentNode); + final var name = currentNode.getName(); + dependentResourceNodes.put(name, currentNode); return this; } @@ -58,10 +62,17 @@ public WorkflowBuilder

withDeletePostcondition(Condition deletePostcondition) } DependentResourceNode getNodeByDependentResource(DependentResource dependentResource) { - return dependentResourceNodes.stream() - .filter(dr -> dr.getDependentResource() == dependentResource) - .findFirst() - .orElseThrow(); + // first check by name + final var node = + dependentResourceNodes.get(DefaultDependentResourceNode.getNameFor(dependentResource)); + if (node != null) { + return node; + } else { + return dependentResourceNodes.values().stream() + .filter(dr -> dr.getDependentResource() == dependentResource) + .findFirst() + .orElseThrow(); + } } public boolean isThrowExceptionAutomatically() { @@ -83,7 +94,7 @@ public Workflow

build(int parallelism) { public Workflow

build(ExecutorService executorService) { // workflow has been built from dependent resources so it is already resolved - return new Workflow(dependentResourceNodes, executorService, + return new Workflow(new HashSet<>(dependentResourceNodes.values()), executorService, throwExceptionAutomatically, true, isCleaner); } } From 6d597ba84fa75d448934a184fa34fe26b765d21f Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 2 Dec 2022 10:19:29 +0100 Subject: [PATCH 17/19] refactor: rename canDeleteIfAble to isDeletable --- .../operator/api/reconciler/dependent/DependentResource.java | 2 +- .../operator/processing/dependent/workflow/Workflow.java | 2 +- .../operator/processing/dependent/workflow/WorkflowBuilder.java | 2 +- .../processing/dependent/workflow/WorkflowCleanupExecutor.java | 2 +- .../dependent/workflow/WorkflowReconcileExecutor.java | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java index 7368dbd130..8372a94f0c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java @@ -65,7 +65,7 @@ static String defaultNameFor(Class dependentResourc return dependentResourceClass.getName(); } - static boolean canDeleteIfAble(DependentResource dependentResource) { + static boolean isDeletable(DependentResource dependentResource) { return dependentResource instanceof Deleter && !(dependentResource instanceof GarbageCollected); } } 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 db89a4916e..553177b79b 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 @@ -132,7 +132,7 @@ void resolve(KubernetesClient client, List dependentResou .forEach(drn -> { drn.resolve(client, dependentResources); final var dr = dependentResource(drn); - if (DependentResource.canDeleteIfAble(dr)) { + if (DependentResource.isDeletable(dr)) { cleanerHolder[0] = true; } }); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index 6983189908..ad6a0fa34f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -25,7 +25,7 @@ public class WorkflowBuilder

{ public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DefaultDependentResourceNode<>(dependentResource); - isCleaner = DependentResource.canDeleteIfAble(dependentResource); + isCleaner = DependentResource.isDeletable(dependentResource); final var name = currentNode.getName(); dependentResourceNodes.put(name, currentNode); return this; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 79bec9aab4..4d82d3c0b6 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -71,7 +71,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (DependentResource.canDeleteIfAble(dependentResource)) { + if (DependentResource.isDeletable(dependentResource)) { ((Deleter

) dependentResource).delete(primary, context); deleteCalled.add(dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index b9b476cf4a..40532d559d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -148,7 +148,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (DependentResource.canDeleteIfAble(dependentResource)) { + if (DependentResource.isDeletable(dependentResource)) { ((Deleter

) dependentResource).delete(primary, context); } boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); From 7b4a50f03e8ded921586a9caaca340d41b501fff Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 2 Dec 2022 10:28:54 +0100 Subject: [PATCH 18/19] refactor: isDeletable is now an instance method, overridable in impls --- .../operator/api/reconciler/dependent/DependentResource.java | 4 ++-- .../dependent/kubernetes/KubernetesDependentResource.java | 5 +++++ .../operator/processing/dependent/workflow/Workflow.java | 2 +- .../processing/dependent/workflow/WorkflowBuilder.java | 2 +- .../dependent/workflow/WorkflowCleanupExecutor.java | 2 +- .../dependent/workflow/WorkflowReconcileExecutor.java | 2 +- 6 files changed, 11 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java index 8372a94f0c..d522a30151 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java @@ -65,7 +65,7 @@ static String defaultNameFor(Class dependentResourc return dependentResourceClass.getName(); } - static boolean isDeletable(DependentResource dependentResource) { - return dependentResource instanceof Deleter && !(dependentResource instanceof GarbageCollected); + default boolean isDeletable() { + return this instanceof Deleter; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index b9d26a0c14..5631536da5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -294,4 +294,9 @@ public KubernetesDependentResourceConfig configFrom(KubernetesDependent kubeDepe public Optional> configuration() { return Optional.ofNullable(kubernetesDependentResourceConfig); } + + @Override + public boolean isDeletable() { + return super.isDeletable() && !garbageCollected; + } } 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 553177b79b..07163ab8c5 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 @@ -132,7 +132,7 @@ void resolve(KubernetesClient client, List dependentResou .forEach(drn -> { drn.resolve(client, dependentResources); final var dr = dependentResource(drn); - if (DependentResource.isDeletable(dr)) { + if (dr.isDeletable()) { cleanerHolder[0] = true; } }); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java index ad6a0fa34f..9f24ff3bd2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java @@ -25,7 +25,7 @@ public class WorkflowBuilder

{ public WorkflowBuilder

addDependentResource(DependentResource dependentResource) { currentNode = new DefaultDependentResourceNode<>(dependentResource); - isCleaner = DependentResource.isDeletable(dependentResource); + isCleaner = dependentResource.isDeletable(); final var name = currentNode.getName(); dependentResourceNodes.put(name, currentNode); return this; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java index 4d82d3c0b6..12656c2a8a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java @@ -71,7 +71,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (DependentResource.isDeletable(dependentResource)) { + if (dependentResource.isDeletable()) { ((Deleter

) dependentResource).delete(primary, context); deleteCalled.add(dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java index 40532d559d..98abe41977 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java @@ -148,7 +148,7 @@ protected void doRun(DependentResourceNode dependentResourceNode, DependentResource dependentResource) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - if (DependentResource.isDeletable(dependentResource)) { + if (dependentResource.isDeletable()) { ((Deleter

) dependentResource).delete(primary, context); } boolean deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); From 0a1a381d48dc5d3a426d7a8ad7c5b468be3a2061 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 2 Dec 2022 10:56:36 +0100 Subject: [PATCH 19/19] fix: tests --- .../AbstractWorkflowExecutorTest.java | 16 +++++++-------- .../workflow/ManagedWorkflowTest.java | 8 +------- .../workflow/ManagedWorkflowTestUtils.java | 20 ++++++++++++++----- 3 files changed, 24 insertions(+), 20 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java index 9c10c06cc0..467820940d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java @@ -4,11 +4,14 @@ import java.util.Collections; import java.util.List; +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; 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.api.reconciler.dependent.ReconcileResult; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; public class AbstractWorkflowExecutorTest { @@ -28,24 +31,21 @@ public class AbstractWorkflowExecutorTest { protected List executionHistory = Collections.synchronizedList(new ArrayList<>()); - public class TestDependent implements DependentResource { + public class TestDependent extends KubernetesDependentResource { private final String name; public TestDependent(String name) { + super(ConfigMap.class); this.name = name; } @Override - public ReconcileResult reconcile(TestCustomResource primary, + public ReconcileResult reconcile(TestCustomResource primary, Context context) { executionHistory.add(new ReconcileRecord(this)); - return ReconcileResult.resourceCreated(VALUE); - } - - @Override - public Class resourceType() { - return String.class; + return ReconcileResult + .resourceCreated(new ConfigMapBuilder().addToBinaryData("key", VALUE).build()); } @Override 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 88e7f4e19c..96a55aa85f 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 @@ -34,7 +34,7 @@ void isNotCleanerIfNoDeleter() { } @Test - void isNotCleanerIfNoGarbageCollected() { + void isNotCleanerIfGarbageCollected() { assertThat(managedWorkflow(createDRSWithTraits(NAME, GarbageCollected.class)) .isCleaner()).isFalse(); } @@ -45,12 +45,6 @@ void isCleanerIfHasDeleter() { assertThat(managedWorkflow(spec).isCleaner()).isTrue(); } - @Test - void isNotCleanerIfDeleterIsGarbageCollected() { - var spec = createDRSWithTraits(NAME, Deleter.class, GarbageCollected.class); - assertThat(managedWorkflow(spec).isCleaner()).isFalse(); - } - ManagedWorkflow managedWorkflow(DependentResourceSpec... specs) { final var configuration = mock(ControllerConfiguration.class); final var specList = List.of(specs); 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 51d6b533c7..52c6840b1c 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 @@ -1,11 +1,13 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.Arrays; import java.util.Set; import org.mockito.Mockito; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; import io.javaoperatorsdk.operator.processing.dependent.EmptyTestDependentResource; import static org.mockito.Mockito.mock; @@ -23,11 +25,19 @@ public static DependentResourceSpec createDRS(String name, String... dependOns) public static DependentResourceSpec createDRSWithTraits(String name, Class... dependentResourceTraits) { - final var drs = createDRS(name); - final var spy = Mockito.spy(drs); - when(spy.getDependentResource()) - .thenReturn( - mock(DependentResource.class, withSettings().extraInterfaces(dependentResourceTraits))); + final var spy = Mockito.mock(DependentResourceSpec.class); + when(spy.getName()).thenReturn(name); + + Class toMock = DependentResource.class; + final var garbageCollected = dependentResourceTraits != null && + Arrays.asList(dependentResourceTraits).contains(GarbageCollected.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); return spy; }