From 1e9021986a81d822fe986150aa250d909bffaead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Tue, 6 Feb 2024 10:43:25 +0100 Subject: [PATCH 01/23] feat: JDK client is now the default (#2235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 0b6a53dd40..edc56c0816 100644 --- a/pom.xml +++ b/pom.xml @@ -58,7 +58,7 @@ ${java.version} java-operator-sdk https://sonarcloud.io - jdk + jdk 5.10.1 6.13.1 From 53372ae0fe76e3e627b9065811b8b78a4bd52fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Thu, 21 Mar 2024 09:38:00 +0100 Subject: [PATCH 02/23] improve: replace current formatting plugins with spotless plugin (#2302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- pom.xml | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pom.xml b/pom.xml index edc56c0816..523eab8856 100644 --- a/pom.xml +++ b/pom.xml @@ -58,7 +58,7 @@ ${java.version} java-operator-sdk https://sonarcloud.io - jdk + jdk 5.10.1 6.13.1 @@ -78,21 +78,21 @@ 0.9.11 2.16.1 - 2.11 - 3.12.1 - 3.3.0 - 3.7.0 - 3.3.1 - 3.3.1 + 2.11 + 3.12.1 + 3.3.0 + 3.7.0 + 3.3.1 + 3.3.1 3.4.2 3.4.0 - 3.2.4 - 1.7.0 - 3.0.0 - 3.1.2 + 3.2.4 + 1.7.0 + 3.0.0 + 3.1.2 9.0.1 - 3.4.1 - 2.43.0 + 3.4.1 + 2.43.0 From 8c10dbfd946777c2cd72acbe09f82334d9c1f7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 10 Apr 2024 16:03:03 +0200 Subject: [PATCH 03/23] fix: format after rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Attila Mészáros --- pom.xml | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pom.xml b/pom.xml index 523eab8856..0b6a53dd40 100644 --- a/pom.xml +++ b/pom.xml @@ -78,21 +78,21 @@ 0.9.11 2.16.1 - 2.11 - 3.12.1 - 3.3.0 - 3.7.0 - 3.3.1 - 3.3.1 + 2.11 + 3.12.1 + 3.3.0 + 3.7.0 + 3.3.1 + 3.3.1 3.4.2 3.4.0 - 3.2.4 - 1.7.0 - 3.0.0 - 3.1.2 + 3.2.4 + 1.7.0 + 3.0.0 + 3.1.2 9.0.1 - 3.4.1 - 2.43.0 + 3.4.1 + 2.43.0 From 8e578f2754b0942c3edf2a753b62ab36c55c5051 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 12 Apr 2024 09:49:47 +0200 Subject: [PATCH 04/23] refactor: clean-up (#2325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Chris Laprun Co-authored-by: Attila Mészáros Signed-off-by: Attila Mészáros --- .../operator/processing/event/NamedEventSource.java | 0 .../operator/config/BaseConfigurationServiceTest.java | 7 ++++++- .../CreateUpdateEventFilterTestReconciler.java | 4 +++- 3 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java new file mode 100644 index 0000000000..e69de29bb2 diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java index b69aae6fc7..72fb90f9bd 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java @@ -19,7 +19,12 @@ import io.javaoperatorsdk.operator.api.config.dependent.ConfigurationConverter; import io.javaoperatorsdk.operator.api.config.dependent.Configured; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; -import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.MaxReconciliationInterval; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.Workflow; import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java index 8714b8c31f..5f8b20ffa8 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java @@ -74,7 +74,9 @@ public List> prepareEv .withInformerConfiguration(c -> c .withLabelSelector("integrationtest = " + this.getClass().getSimpleName())) .build(); - final var informerEventSource = new InformerEventSource<>(informerConfiguration, context); + final var informerEventSource = + new InformerEventSource( + informerConfiguration, context); this.configMapDR.setEventSource(informerEventSource); return List.of(informerEventSource); From 934a1fb44cfce010c37c58008ccc0730b3b49034 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 5 Jun 2024 16:26:39 +0200 Subject: [PATCH 05/23] feat: allow return additional information for conditions Fixes #2424. Currently, only exposed for ready post conditions but is supported for all conditions if needed. Signed-off-by: Chris Laprun --- .../dependent/workflow/DefaultWorkflow.java | 5 ++ .../dependent/workflow/ResultCondition.java | 22 +++++ .../dependent/workflow/Workflow.java | 6 +- .../workflow/WorkflowReconcileExecutor.java | 57 +++++++------ .../workflow/WorkflowReconcileResult.java | 33 ++++++-- .../WorkflowReconcileExecutorTest.java | 83 ++++++++++++++++++- 6 files changed, 169 insertions(+), 37 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java index 5ed5b5cb9e..4c54011600 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java @@ -140,6 +140,11 @@ public boolean isEmpty() { return dependentResourceNodes.isEmpty(); } + @Override + public int size() { + return dependentResourceNodes.size(); + } + @Override public Map getDependentResourcesByName() { final var resources = new HashMap(dependentResourceNodes.size()); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java new file mode 100644 index 0000000000..9de816b8f2 --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java @@ -0,0 +1,22 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; + +public interface ResultCondition extends Condition { + Result detailedIsMet(DependentResource dependentResource, P primary, Context

context); + + Object NULL = new Object(); + + @Override + default boolean isMet(DependentResource dependentResource, P primary, Context

context) { + return detailedIsMet(dependentResource, primary, context).isSuccess(); + } + + interface Result { + T getResult(); + + boolean isSuccess(); + } +} 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 7f30ba6c9e..470e64ba21 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 @@ -36,7 +36,11 @@ default boolean hasCleaner() { } default boolean isEmpty() { - return true; + return size() == 0; + } + + default int size() { + return getDependentResourcesByName().size(); } @SuppressWarnings("rawtypes") 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 2da0bd7525..c1bb3b0ebb 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,5 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.HashMap; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -22,19 +23,18 @@ public class WorkflowReconcileExecutor

extends AbstractWo private static final String RECONCILE = "reconcile"; private static final String DELETE = "delete"; - - private final Set notReady = ConcurrentHashMap.newKeySet(); - - 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 Map reconcileResults = - new ConcurrentHashMap<>(); + private final Map notReady; + private final Set markedForDelete; + private final Set deletePostConditionNotMet; + private final Map reconcileResults; public WorkflowReconcileExecutor(Workflow

workflow, P primary, Context

context) { super(workflow, primary, context); + final var size = workflow.size(); + reconcileResults = new HashMap<>(size); + notReady = new HashMap<>(size); + markedForDelete = ConcurrentHashMap.newKeySet(size); + deletePostConditionNotMet = ConcurrentHashMap.newKeySet(size); } public synchronized WorkflowReconcileResult reconcile() { @@ -96,16 +96,16 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo private boolean allDependentsDeletedAlready(DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); - return dependents.stream().allMatch(d -> alreadyVisited(d) && !notReady.contains(d) + return dependents.stream().allMatch(d -> alreadyVisited(d) && !notReady.containsKey(d) && !isInError(d) && !deletePostConditionNotMet.contains(d)); } // needs to be in one step private synchronized void setAlreadyReconciledButNotReady( - DependentResourceNode dependentResourceNode) { + DependentResourceNode dependentResourceNode, Object result) { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); markAsVisited(dependentResourceNode); - notReady.add(dependentResourceNode); + notReady.put(dependentResourceNode, result == null ? ResultCondition.NULL : result); } private class NodeReconcileExecutor extends NodeExecutor { @@ -122,17 +122,25 @@ protected void doRun(DependentResourceNode dependentResourceNode, "Reconciling for primary: {} node: {} ", primaryID, dependentResourceNode); ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); reconcileResults.put(dependentResource, reconcileResult); - reconciled.add(dependentResourceNode); - boolean ready = isConditionMet(dependentResourceNode.getReadyPostcondition(), - dependentResource); - if (ready) { + final Object[] result = new Object[1]; + final var shouldProceed = dependentResourceNode.getReadyPostcondition().map(rpCondition -> { + if (rpCondition instanceof ResultCondition resultCondition) { + final var detailed = resultCondition.detailedIsMet(dependentResource, primary, context); + result[0] = detailed.getResult(); + return detailed.isSuccess(); + } else { + return rpCondition.isMet(dependentResource, primary, context); + } + }).orElse(true); + + if (shouldProceed) { log.debug("Setting already reconciled for: {} primaryID: {}", dependentResourceNode, primaryID); markAsVisited(dependentResourceNode); handleDependentsReconcile(dependentResourceNode); } else { - setAlreadyReconciledButNotReady(dependentResourceNode); + setAlreadyReconciledButNotReady(dependentResourceNode, result[0]); } } } @@ -191,7 +199,6 @@ private synchronized void handleDependentsReconcile( }); } - private void handleReconcileOrActivationConditionNotMet( DependentResourceNode dependentResourceNode, boolean activationConditionMet) { @@ -226,7 +233,7 @@ 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.containsKey(d)); } private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { @@ -237,14 +244,10 @@ private boolean hasErroredParent(DependentResourceNode dependentResourceNo private WorkflowReconcileResult createReconcileResult() { return new WorkflowReconcileResult( - reconciled.stream() - .map(DependentResourceNode::getDependentResource) - .collect(Collectors.toList()), - notReady.stream() - .map(DependentResourceNode::getDependentResource) - .collect(Collectors.toList()), + notReady.entrySet().stream() + .collect(Collectors.toMap(entry -> entry.getKey().getDependentResource(), + Map.Entry::getValue)), getErroredDependents(), reconcileResults); } - } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index 6fbd06486c..908c06ff65 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -2,35 +2,54 @@ import java.util.List; import java.util.Map; +import java.util.Optional; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { - - private final List reconciledDependents; - private final List notReadyDependents; + private final Map notReadyDependents; private final Map reconcileResults; - public WorkflowReconcileResult(List reconciledDependents, - List notReadyDependents, + public WorkflowReconcileResult( + Map notReadyDependents, Map erroredDependents, Map reconcileResults) { super(erroredDependents); - this.reconciledDependents = reconciledDependents; this.notReadyDependents = notReadyDependents; this.reconcileResults = reconcileResults; } public List getReconciledDependents() { - return reconciledDependents; + return reconcileResults.keySet().stream().toList(); } public List getNotReadyDependents() { + return notReadyDependents.keySet().stream().toList(); + } + + @SuppressWarnings("unused") + public Map getNotReadyDependentsWithDetails() { return notReadyDependents; } + public T getNotReadyDependentResult(DependentResource dependentResource, + Class expectedResultType) { + final var result = new Object[1]; + try { + return Optional.ofNullable(notReadyDependents.get(dependentResource)) + .filter(cr -> !ResultCondition.NULL.equals(cr)) + .map(r -> result[0] = r) + .map(expectedResultType::cast) + .orElse(null); + } catch (Exception e) { + throw new IllegalArgumentException("Condition result " + result[0] + + " for Dependent " + dependentResource.name() + " doesn't match expected type " + + expectedResultType.getSimpleName(), e); + } + } + @SuppressWarnings("unused") public Map getReconcileResults() { return reconcileResults; 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 c66fcb02d6..cf7a1604b9 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 @@ -7,16 +7,17 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.*; -@SuppressWarnings("rawtypes") class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest { @SuppressWarnings("unchecked") @@ -27,6 +28,7 @@ class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest { TestDependent dr4 = new TestDependent("DR_4"); @BeforeEach + @SuppressWarnings("unchecked") void setup() { when(mockContext.getWorkflowExecutorService()).thenReturn(executorService); when(mockContext.eventSourceRetriever()).thenReturn(mock(EventSourceRetriever.class)); @@ -521,6 +523,7 @@ void readyConditionNotCheckedOnNonActiveDependent() { } @Test + @SuppressWarnings("unchecked") void reconcilePreconditionNotCheckedOnNonActiveDependent() { var precondition = mock(Condition.class); @@ -557,6 +560,7 @@ void deletesDependentsOfNonActiveDependentButNotTheNonActive() { } @Test + @SuppressWarnings("unchecked") void activationConditionOnlyCalledOnceOnDeleteDependents() { TestDeleterDependent drDeleter2 = new TestDeleterDependent("DR_DELETER_2"); var condition = mock(Condition.class); @@ -573,4 +577,79 @@ void activationConditionOnlyCalledOnceOnDeleteDependents() { verify(condition, times(1)).isMet(any(), any(), any()); } + + @Test + void resultFromReadyConditionShouldBeAvailableIfExisting() { + final var result = Integer.valueOf(42); + final var resultCondition = new ResultCondition<>() { + @Override + public Result detailedIsMet(DependentResource dependentResource, + HasMetadata primary, Context context) { + return new Result<>() { + @Override + public Object getResult() { + return result; + } + + @Override + public boolean isSuccess() { + return false; // force not ready + } + }; + } + }; + var workflow = new WorkflowBuilder() + .addDependentResource(dr1) + .withReadyPostcondition(resultCondition) + .build(); + + final var reconcileResult = workflow.reconcile(new TestCustomResource(), mockContext); + assertEquals(result, reconcileResult.getNotReadyDependentResult(dr1, Integer.class)); + } + + @Test + void shouldThrowIllegalArgumentExceptionIfTypesDoNotMatch() { + final var result = "FOO"; + final var resultCondition = new ResultCondition<>() { + @Override + public Result detailedIsMet(DependentResource dependentResource, + HasMetadata primary, Context context) { + return new Result<>() { + @Override + public Object getResult() { + return result; + } + + @Override + public boolean isSuccess() { + return false; // force not ready + } + }; + } + }; + var workflow = new WorkflowBuilder() + .addDependentResource(dr1) + .withReadyPostcondition(resultCondition) + .build(); + + final var reconcileResult = workflow.reconcile(new TestCustomResource(), mockContext); + final var expectedResultType = Integer.class; + final var e = assertThrows(IllegalArgumentException.class, + () -> reconcileResult.getNotReadyDependentResult(dr1, expectedResultType)); + final var message = e.getMessage(); + assertTrue(message.contains(dr1.name())); + assertTrue(message.contains(expectedResultType.getSimpleName())); + assertTrue(message.contains(result)); + } + + @Test + void shouldReturnNullIfNoConditionResultExists() { + var workflow = new WorkflowBuilder() + .addDependentResource(dr1) + .withReadyPostcondition(notMetCondition) + .build(); + + final var reconcileResult = workflow.reconcile(new TestCustomResource(), mockContext); + assertNull(reconcileResult.getNotReadyDependentResult(dr1, Integer.class)); + } } From ed4ccc78bf3b703161c0776e16f682d65d44844e Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 14 Jun 2024 21:55:05 +0200 Subject: [PATCH 06/23] wip: record results for all conditions Signed-off-by: Chris Laprun --- .../workflow/AbstractWorkflowExecutor.java | 60 +++++++--- .../dependent/workflow/Condition.java | 4 + .../dependent/workflow/DefaultWorkflow.java | 2 - .../workflow/DependentResourceNode.java | 69 ++++++++--- .../dependent/workflow/NodeExecutor.java | 8 +- .../dependent/workflow/ResultCondition.java | 28 +++++ .../dependent/workflow/Workflow.java | 11 -- .../dependent/workflow/WorkflowBuilder.java | 4 + .../workflow/WorkflowCleanupExecutor.java | 56 +++------ .../workflow/WorkflowCleanupResult.java | 17 +-- .../workflow/WorkflowReconcileExecutor.java | 74 +++--------- .../workflow/WorkflowReconcileResult.java | 32 ++---- .../dependent/workflow/WorkflowResult.java | 108 ++++++++++++++++-- .../workflow/WorkflowResultTest.java | 10 +- .../dependent/workflow/WorkflowTest.java | 6 +- 15 files changed, 282 insertions(+), 207 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 319b1a9e56..a6938e1697 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 @@ -1,9 +1,7 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; 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.ExecutorService; import java.util.concurrent.Future; @@ -20,25 +18,24 @@ @SuppressWarnings("rawtypes") abstract class AbstractWorkflowExecutor

{ - protected final Workflow

workflow; + protected final DefaultWorkflow

workflow; protected final P primary; protected final ResourceID primaryID; protected final Context

context; + protected final Map, WorkflowResult.DetailBuilder> results; /** * Covers both deleted and reconciled */ - private final Set alreadyVisited = ConcurrentHashMap.newKeySet(); private final Map> actualExecutions = new ConcurrentHashMap<>(); - private final Map exceptionsDuringExecution = - new ConcurrentHashMap<>(); private final ExecutorService executorService; - public AbstractWorkflowExecutor(Workflow

workflow, P primary, Context

context) { + protected AbstractWorkflowExecutor(DefaultWorkflow

workflow, P primary, Context

context) { this.workflow = workflow; this.primary = primary; this.context = context; this.primaryID = ResourceID.fromResource(primary); executorService = context.getWorkflowExecutorService(); + results = new ConcurrentHashMap<>(workflow.getDependentResourcesByName().size()); } protected abstract Logger logger(); @@ -75,11 +72,22 @@ protected boolean noMoreExecutionsScheduled() { } protected boolean alreadyVisited(DependentResourceNode dependentResourceNode) { - return alreadyVisited.contains(dependentResourceNode); + return results.containsKey(dependentResourceNode); } protected void markAsVisited(DependentResourceNode dependentResourceNode) { - alreadyVisited.add(dependentResourceNode); + createOrGetResultFor(dependentResourceNode); + } + + protected boolean postDeleteConditionNotMet(DependentResourceNode dependentResourceNode) { + final var builder = results.get(dependentResourceNode); + return builder != null && builder.hasPostDeleteConditionNotMet(); + } + + protected WorkflowResult.DetailBuilder createOrGetResultFor( + DependentResourceNode dependentResourceNode) { + return results.computeIfAbsent(dependentResourceNode, + unused -> new WorkflowResult.DetailBuilder()); } protected boolean isExecutingNow(DependentResourceNode dependentResourceNode) { @@ -94,17 +102,17 @@ protected void markAsExecuting(DependentResourceNode dependentResourceNode protected synchronized void handleExceptionInExecutor( DependentResourceNode dependentResourceNode, RuntimeException e) { - exceptionsDuringExecution.put(dependentResourceNode, e); + createOrGetResultFor(dependentResourceNode).withError(e); } - protected boolean isInError(DependentResourceNode dependentResourceNode) { - return exceptionsDuringExecution.containsKey(dependentResourceNode); + protected boolean isNotReady(DependentResourceNode dependentResourceNode) { + final var builder = results.get(dependentResourceNode); + return builder != null && builder.isNotReady(); } - protected Map getErroredDependents() { - return exceptionsDuringExecution.entrySet().stream() - .collect( - Collectors.toMap(e -> e.getKey().getDependentResource(), Entry::getValue)); + protected boolean isInError(DependentResourceNode dependentResourceNode) { + final var builder = results.get(dependentResourceNode); + return builder != null && builder.hasError(); } protected synchronized void handleNodeExecutionFinish( @@ -116,9 +124,17 @@ protected synchronized void handleNodeExecutionFinish( } } - protected boolean isConditionMet(Optional> condition, - DependentResource dependentResource) { - return condition.map(c -> c.isMet(dependentResource, primary, context)).orElse(true); + @SuppressWarnings("unchecked") + protected boolean isConditionMet( + Optional> condition, + DependentResourceNode dependentResource) { + final var dr = dependentResource.getDependentResource(); + return condition.map(c -> { + final ResultCondition.Result r = c.detailedIsMet(dr, primary, context); + results.computeIfAbsent(dependentResource, unused -> new WorkflowResult.DetailBuilder()) + .withResultForCondition(c, r); + return r; + }).orElse(ResultCondition.Result.metWithoutResult).isSuccess(); } protected void submit(DependentResourceNode dependentResourceNode, @@ -145,4 +161,10 @@ protected void registerOrDeregisterEventSourceBasedOnActivation( } } } + + protected Map asDetails() { + return results.entrySet().stream() + .collect( + Collectors.toMap(e -> e.getKey().getDependentResource(), e -> e.getValue().build())); + } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java index 87690ed69b..de96c99ca5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/Condition.java @@ -6,6 +6,10 @@ public interface Condition { + enum Type { + ACTIVATION, DELETE, READY, RECONCILE + } + /** * Checks whether a condition holds true for a given * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} based on the diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java index 4c54011600..e3f43f9e12 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java @@ -108,12 +108,10 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { return result; } - @Override public Set getTopLevelDependentResources() { return topLevelResources; } - @Override public Set getBottomLevelResource() { return bottomLevelResource; } 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 3e8a762c5e..afbf624e52 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java @@ -5,6 +5,7 @@ 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") @@ -13,12 +14,38 @@ class DependentResourceNode { private final List dependsOn = new LinkedList<>(); private final List parents = new LinkedList<>(); - private Condition reconcilePrecondition; - private Condition deletePostcondition; - private Condition readyPostcondition; - private Condition activationCondition; + private ConditionWithType reconcilePrecondition; + private ConditionWithType deletePostcondition; + private ConditionWithType readyPostcondition; + private ConditionWithType activationCondition; private final DependentResource dependentResource; + static class ConditionWithType implements ResultCondition { + private final Condition condition; + private final Type type; + + ConditionWithType(Condition condition, Type type) { + this.condition = condition; + this.type = type; + } + + public Type type() { + return type; + } + + @SuppressWarnings("unchecked") + @Override + public Result detailedIsMet(DependentResource dependentResource, P primary, + Context

context) { + if (condition instanceof ResultCondition resultCondition) { + return resultCondition.detailedIsMet(dependentResource, primary, context); + } else { + return ResultCondition.Result + .withoutResult(condition.isMet(dependentResource, primary, context)); + } + } + } + DependentResourceNode(DependentResource dependentResource) { this(null, null, null, null, dependentResource); } @@ -26,10 +53,10 @@ class DependentResourceNode { public DependentResourceNode(Condition reconcilePrecondition, Condition deletePostcondition, Condition readyPostcondition, Condition activationCondition, DependentResource dependentResource) { - this.reconcilePrecondition = reconcilePrecondition; - this.deletePostcondition = deletePostcondition; - this.readyPostcondition = readyPostcondition; - this.activationCondition = activationCondition; + setReconcilePrecondition(reconcilePrecondition); + setDeletePostcondition(deletePostcondition); + setReadyPostcondition(readyPostcondition); + setActivationCondition(activationCondition); this.dependentResource = dependentResource; } @@ -50,36 +77,40 @@ public List getParents() { return parents; } - public Optional> getReconcilePrecondition() { + public Optional> getReconcilePrecondition() { return Optional.ofNullable(reconcilePrecondition); } - public Optional> getDeletePostcondition() { + public Optional> getDeletePostcondition() { return Optional.ofNullable(deletePostcondition); } - public Optional> getActivationCondition() { + public Optional> getActivationCondition() { return Optional.ofNullable(activationCondition); } + public Optional> getReadyPostcondition() { + return Optional.ofNullable(readyPostcondition); + } + void setReconcilePrecondition(Condition reconcilePrecondition) { - this.reconcilePrecondition = reconcilePrecondition; + this.reconcilePrecondition = reconcilePrecondition == null ? null + : new ConditionWithType<>(reconcilePrecondition, Condition.Type.RECONCILE); } void setDeletePostcondition(Condition cleanupCondition) { - this.deletePostcondition = cleanupCondition; + this.deletePostcondition = deletePostcondition == null ? null + : new ConditionWithType<>(deletePostcondition, Condition.Type.DELETE); } void setActivationCondition(Condition activationCondition) { - this.activationCondition = activationCondition; - } - - public Optional> getReadyPostcondition() { - return Optional.ofNullable(readyPostcondition); + this.activationCondition = activationCondition == null ? null + : new ConditionWithType<>(activationCondition, Condition.Type.ACTIVATION); } void setReadyPostcondition(Condition readyPostcondition) { - this.readyPostcondition = readyPostcondition; + this.readyPostcondition = readyPostcondition == null ? null + : new ConditionWithType<>(readyPostcondition, Condition.Type.READY); } public DependentResource getDependentResource() { 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 index 0067c55321..2006486dc1 100644 --- 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 @@ -1,7 +1,6 @@ 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 { @@ -17,9 +16,7 @@ protected NodeExecutor(DependentResourceNode dependentResourceNode, @Override public void run() { try { - var dependentResource = dependentResourceNode.getDependentResource(); - - doRun(dependentResourceNode, dependentResource); + doRun(dependentResourceNode); } catch (RuntimeException e) { workflowExecutor.handleExceptionInExecutor(dependentResourceNode, e); @@ -28,6 +25,5 @@ public void run() { } } - protected abstract void doRun(DependentResourceNode dependentResourceNode, - DependentResource dependentResource); + protected abstract void doRun(DependentResourceNode dependentResourceNode); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java index 9de816b8f2..2db2274d05 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java @@ -15,6 +15,34 @@ default boolean isMet(DependentResource dependentResource, P primary, Cont } interface Result { + Result metWithoutResult = new Result() { + @Override + public Object getResult() { + return NULL; + } + + @Override + public boolean isSuccess() { + return true; + } + }; + + Result unmetWithoutResult = new Result() { + @Override + public Object getResult() { + return NULL; + } + + @Override + public boolean isSuccess() { + return false; + } + }; + + static Result withoutResult(boolean success) { + return success ? metWithoutResult : unmetWithoutResult; + } + T getResult(); boolean isSuccess(); 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 470e64ba21..02f7d9c89f 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 @@ -3,7 +3,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Set; import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -21,16 +20,6 @@ default WorkflowCleanupResult cleanup(P primary, Context

context) { throw new UnsupportedOperationException("Implement this"); } - @SuppressWarnings("rawtypes") - default Set getTopLevelDependentResources() { - return Collections.emptySet(); - } - - @SuppressWarnings("rawtypes") - default Set getBottomLevelResource() { - return Collections.emptySet(); - } - default boolean hasCleaner() { return false; } 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 90ece70d26..8e231e802a 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 @@ -82,6 +82,10 @@ public WorkflowBuilder

withThrowExceptionFurther(boolean throwExceptionFurthe } public Workflow

build() { + return buildAsDefaultWorkflow(); + } + + DefaultWorkflow

buildAsDefaultWorkflow() { return new DefaultWorkflow(new HashSet<>(dependentResourceNodes.values()), throwExceptionAutomatically, isCleaner); } 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 dda4db6729..b88a8afc05 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,9 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; import java.util.List; -import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -11,19 +8,14 @@ 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; @SuppressWarnings("rawtypes") -public class WorkflowCleanupExecutor

extends AbstractWorkflowExecutor

{ +class WorkflowCleanupExecutor

extends AbstractWorkflowExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowCleanupExecutor.class); private static final String CLEANUP = "cleanup"; - private final Set postDeleteConditionNotMet = - ConcurrentHashMap.newKeySet(); - private final Set deleteCalled = ConcurrentHashMap.newKeySet(); - - public WorkflowCleanupExecutor(Workflow

workflow, P primary, Context

context) { + WorkflowCleanupExecutor(DefaultWorkflow

workflow, P primary, Context

context) { super(workflow, primary, context); } @@ -64,33 +56,26 @@ private CleanupExecutor(DependentResourceNode drn) { @Override @SuppressWarnings("unchecked") - protected void doRun(DependentResourceNode dependentResourceNode, - DependentResource dependentResource) { - var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - - var active = - isConditionMet(dependentResourceNode.getActivationCondition(), dependentResource); + protected void doRun(DependentResourceNode dependentResourceNode) { + final var active = + isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode); registerOrDeregisterEventSourceBasedOnActivation(active, dependentResourceNode); - if (dependentResource.isDeletable() && active) { - ((Deleter

) dependentResource).delete(primary, context); - deleteCalled.add(dependentResourceNode); - } - - boolean deletePostConditionMet; + boolean deletePostConditionMet = true; if (active) { - deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); - } else { - deletePostConditionMet = true; + final var dependentResource = dependentResourceNode.getDependentResource(); + if (dependentResource.isDeletable()) { + ((Deleter

) dependentResource).delete(primary, context); + createOrGetResultFor(dependentResourceNode).withDeleted(true); + } + + deletePostConditionMet = + isConditionMet(dependentResourceNode.getDeletePostcondition(), dependentResourceNode); } + 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); } } } @@ -112,7 +97,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(d)); } @SuppressWarnings("unchecked") @@ -122,13 +107,6 @@ private boolean hasErroredDependent(DependentResourceNode dependentResourceNode) } private WorkflowCleanupResult createCleanupResult() { - final var erroredDependents = getErroredDependents(); - final var postConditionNotMet = postDeleteConditionNotMet.stream() - .map(DependentResourceNode::getDependentResource) - .collect(Collectors.toList()); - final var deleteCalled = this.deleteCalled.stream() - .map(DependentResourceNode::getDependentResource) - .collect(Collectors.toList()); - return new WorkflowCleanupResult(erroredDependents, postConditionNotMet, deleteCalled); + return new WorkflowCleanupResult(asDetails()); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java index 860a553c39..942c7eccf5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java @@ -7,26 +7,19 @@ @SuppressWarnings("rawtypes") public class WorkflowCleanupResult extends WorkflowResult { - - private final List deleteCalledOnDependents; - private final List postConditionNotMetDependents; - - WorkflowCleanupResult(Map erroredDependents, - List postConditionNotMet, List deleteCalled) { - super(erroredDependents); - this.deleteCalledOnDependents = deleteCalled; - this.postConditionNotMetDependents = postConditionNotMet; + WorkflowCleanupResult(Map results) { + super(results); } public List getDeleteCalledOnDependents() { - return deleteCalledOnDependents; + return listFilteredBy(Detail::deleted); } public List getPostConditionNotMetDependents() { - return postConditionNotMetDependents; + return listFilteredBy(detail -> !Detail.isConditionMet(detail.deletePostconditionResult())); } public boolean allPostConditionsMet() { - return postConditionNotMetDependents.isEmpty(); + return getPostConditionNotMetDependents().isEmpty(); } } 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 c1bb3b0ebb..685ed6f1d7 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,11 +1,8 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.HashMap; import java.util.HashSet; -import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; -import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -13,28 +10,21 @@ 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.ReconcileResult; @SuppressWarnings({"rawtypes", "unchecked"}) -public class WorkflowReconcileExecutor

extends AbstractWorkflowExecutor

{ +class WorkflowReconcileExecutor

extends AbstractWorkflowExecutor

{ private static final Logger log = LoggerFactory.getLogger(WorkflowReconcileExecutor.class); private static final String RECONCILE = "reconcile"; private static final String DELETE = "delete"; - private final Map notReady; private final Set markedForDelete; - private final Set deletePostConditionNotMet; - private final Map reconcileResults; - public WorkflowReconcileExecutor(Workflow

workflow, P primary, Context

context) { + public WorkflowReconcileExecutor(DefaultWorkflow

workflow, P primary, Context

context) { super(workflow, primary, context); final var size = workflow.size(); - reconcileResults = new HashMap<>(size); - notReady = new HashMap<>(size); markedForDelete = ConcurrentHashMap.newKeySet(size); - deletePostConditionNotMet = ConcurrentHashMap.newKeySet(size); } public synchronized WorkflowReconcileResult reconcile() { @@ -63,13 +53,13 @@ private synchronized void handleReconcile(DependentResourceNode depend } boolean activationConditionMet = isConditionMet(dependentResourceNode.getActivationCondition(), - dependentResourceNode.getDependentResource()); + dependentResourceNode); registerOrDeregisterEventSourceBasedOnActivation(activationConditionMet, dependentResourceNode); boolean reconcileConditionMet = true; if (activationConditionMet) { reconcileConditionMet = isConditionMet(dependentResourceNode.getReconcilePrecondition(), - dependentResourceNode.getDependentResource()); + dependentResourceNode); } if (!reconcileConditionMet || !activationConditionMet) { handleReconcileOrActivationConditionNotMet(dependentResourceNode, activationConditionMet); @@ -96,16 +86,8 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo private boolean allDependentsDeletedAlready(DependentResourceNode dependentResourceNode) { var dependents = dependentResourceNode.getParents(); - return dependents.stream().allMatch(d -> alreadyVisited(d) && !notReady.containsKey(d) - && !isInError(d) && !deletePostConditionNotMet.contains(d)); - } - - // needs to be in one step - private synchronized void setAlreadyReconciledButNotReady( - DependentResourceNode dependentResourceNode, Object result) { - log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); - markAsVisited(dependentResourceNode); - notReady.put(dependentResourceNode, result == null ? ResultCondition.NULL : result); + return dependents.stream().allMatch(d -> alreadyVisited(d) && !isNotReady(d) + && !isInError(d) && !postDeleteConditionNotMet(d)); } private class NodeReconcileExecutor extends NodeExecutor { @@ -115,32 +97,20 @@ private NodeReconcileExecutor(DependentResourceNode dependentResourceNode) } @Override - protected void doRun(DependentResourceNode dependentResourceNode, - DependentResource dependentResource) { - + protected void doRun(DependentResourceNode dependentResourceNode) { + final var dependentResource = dependentResourceNode.getDependentResource(); log.debug( "Reconciling for primary: {} node: {} ", primaryID, dependentResourceNode); ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); - reconcileResults.put(dependentResource, reconcileResult); - - final Object[] result = new Object[1]; - final var shouldProceed = dependentResourceNode.getReadyPostcondition().map(rpCondition -> { - if (rpCondition instanceof ResultCondition resultCondition) { - final var detailed = resultCondition.detailedIsMet(dependentResource, primary, context); - result[0] = detailed.getResult(); - return detailed.isSuccess(); - } else { - return rpCondition.isMet(dependentResource, primary, context); - } - }).orElse(true); + createOrGetResultFor(dependentResourceNode).withReconcileResult(reconcileResult); - if (shouldProceed) { + if (isConditionMet(dependentResourceNode.getReconcilePrecondition(), dependentResourceNode)) { log.debug("Setting already reconciled for: {} primaryID: {}", dependentResourceNode, primaryID); markAsVisited(dependentResourceNode); handleDependentsReconcile(dependentResourceNode); } else { - setAlreadyReconciledButNotReady(dependentResourceNode, result[0]); + log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); } } } @@ -153,29 +123,24 @@ private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { @Override @SuppressWarnings("unchecked") - protected void doRun(DependentResourceNode dependentResourceNode, - DependentResource dependentResource) { + protected void doRun(DependentResourceNode dependentResourceNode) { var deletePostCondition = dependentResourceNode.getDeletePostcondition(); + final var dependentResource = dependentResourceNode.getDependentResource(); boolean deletePostConditionMet = true; - if (isConditionMet(dependentResourceNode.getActivationCondition(), dependentResource)) { + if (isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode)) { // GarbageCollected status is irrelevant here, as this method is only called when a // precondition does not hold, // a deleter should be deleted even if it is otherwise garbage collected if (dependentResource instanceof Deleter) { ((Deleter

) dependentResource).delete(primary, context); } - deletePostConditionMet = isConditionMet(deletePostCondition, dependentResource); + deletePostConditionMet = isConditionMet(deletePostCondition, dependentResourceNode); } 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); } } } @@ -233,7 +198,7 @@ private void markDependentsForDelete(DependentResourceNode dependentResour private boolean allParentsReconciledAndReady(DependentResourceNode dependentResourceNode) { return dependentResourceNode.getDependsOn().isEmpty() || dependentResourceNode.getDependsOn().stream() - .allMatch(d -> alreadyVisited(d) && !notReady.containsKey(d)); + .allMatch(d -> alreadyVisited(d) && !isNotReady(d)); } private boolean hasErroredParent(DependentResourceNode dependentResourceNode) { @@ -243,11 +208,6 @@ private boolean hasErroredParent(DependentResourceNode dependentResourceNo } private WorkflowReconcileResult createReconcileResult() { - return new WorkflowReconcileResult( - notReady.entrySet().stream() - .collect(Collectors.toMap(entry -> entry.getKey().getDependentResource(), - Map.Entry::getValue)), - getErroredDependents(), - reconcileResults); + return new WorkflowReconcileResult(asDetails()); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index 908c06ff65..070bdc915a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -5,40 +5,27 @@ import java.util.Optional; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { - private final Map notReadyDependents; - private final Map reconcileResults; - - public WorkflowReconcileResult( - Map notReadyDependents, - Map erroredDependents, - Map reconcileResults) { - super(erroredDependents); - this.notReadyDependents = notReadyDependents; - this.reconcileResults = reconcileResults; + + public WorkflowReconcileResult(Map results) { + super(results); } public List getReconciledDependents() { - return reconcileResults.keySet().stream().toList(); + return listFilteredBy(detail -> detail.reconcileResult() != null); } public List getNotReadyDependents() { - return notReadyDependents.keySet().stream().toList(); - } - - @SuppressWarnings("unused") - public Map getNotReadyDependentsWithDetails() { - return notReadyDependents; + return listFilteredBy(detail -> !Detail.isConditionMet(detail.reconcilePostconditionResult())); } public T getNotReadyDependentResult(DependentResource dependentResource, Class expectedResultType) { final var result = new Object[1]; try { - return Optional.ofNullable(notReadyDependents.get(dependentResource)) + return Optional.ofNullable(results().get(dependentResource)) .filter(cr -> !ResultCondition.NULL.equals(cr)) .map(r -> result[0] = r) .map(expectedResultType::cast) @@ -50,12 +37,7 @@ public T getNotReadyDependentResult(DependentResource dependentResource, } } - @SuppressWarnings("unused") - public Map getReconcileResults() { - return reconcileResults; - } - public boolean allDependentResourcesReady() { - return notReadyDependents.isEmpty(); + return getNotReadyDependents().isEmpty(); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 7014ccd5d4..0953da55cd 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -1,36 +1,124 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.function.Function; import java.util.stream.Collectors; +import java.util.stream.Stream; import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.api.reconciler.dependent.ReconcileResult; @SuppressWarnings("rawtypes") class WorkflowResult { + private final Map results; + private Boolean hasErroredDependents; - private final Map erroredDependents; - - WorkflowResult(Map erroredDependents) { - this.erroredDependents = erroredDependents != null ? erroredDependents : Collections.emptyMap(); + WorkflowResult(Map results) { + this.results = results; } public Map getErroredDependents() { - return erroredDependents; + return getErroredDependentsStream() + .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().error)); + } + + private Stream> getErroredDependentsStream() { + return results.entrySet().stream().filter(entry -> entry.getValue().error != null); + } + + protected Map results() { + return results; + } + + protected List listFilteredBy( + Function filter) { + return results.entrySet().stream() + .filter(e -> filter.apply(e.getValue())) + .map(Map.Entry::getKey) + .toList(); } - @SuppressWarnings("unused") public boolean erroredDependentsExist() { - return !erroredDependents.isEmpty(); + if (hasErroredDependents == null) { + hasErroredDependents = !getErroredDependents().isEmpty(); + } + return hasErroredDependents; } public void throwAggregateExceptionIfErrorsPresent() { if (erroredDependentsExist()) { throw new AggregatedOperatorException("Exception(s) during workflow execution.", - erroredDependents.entrySet().stream() - .collect(Collectors.toMap(e -> e.getKey().name(), Entry::getValue))); + getErroredDependentsStream() + .collect(Collectors.toMap(e -> e.getKey().name(), e -> e.getValue().error))); + } + } + + static class DetailBuilder { + private Exception error; + private ReconcileResult reconcileResult; + private ResultCondition.Result activationConditionResult; + private ResultCondition.Result deletePostconditionResult; + private ResultCondition.Result readyPostconditionResult; + private ResultCondition.Result reconcilePostconditionResult; + private boolean deleted; + + Detail build() { + return new Detail<>(error, reconcileResult, activationConditionResult, + deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted); + } + + DetailBuilder withResultForCondition(DependentResourceNode.ConditionWithType conditionWithType, ResultCondition.Result conditionResult) { + switch (conditionWithType.type()) { + case ACTIVATION -> activationConditionResult = conditionResult; + case DELETE -> deletePostconditionResult = conditionResult; + case READY -> readyPostconditionResult = conditionResult; + case RECONCILE -> reconcilePostconditionResult = conditionResult; + default -> throw new IllegalStateException("Unexpected condition type: " + conditionWithType); + } + return this; + } + + DetailBuilder withError(Exception error) { + this.error = error; + return this; + } + + DetailBuilder withReconcileResult(ReconcileResult reconcileResult) { + this.reconcileResult = reconcileResult; + return this; + } + + DetailBuilder withDeleted(boolean deleted) { + this.deleted = deleted; + return this; + } + + public boolean hasError() { + return error != null; + } + + public boolean hasPostDeleteConditionNotMet() { + return deletePostconditionResult != null && !readyPostconditionResult.isSuccess(); + } + + public boolean isNotReady() { + return readyPostconditionResult != null && !readyPostconditionResult.isSuccess(); + } + } + + + record Detail(Exception error, ReconcileResult reconcileResult, + ResultCondition.Result activationConditionResult, + ResultCondition.Result deletePostconditionResult, + ResultCondition.Result readyPostconditionResult, + ResultCondition.Result reconcilePostconditionResult, + boolean deleted) { + + static boolean isConditionMet(ResultCondition.Result conditionResult) { + return conditionResult != null && conditionResult.isSuccess(); } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java index c89ca53f07..c3898e9bb5 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -13,11 +13,13 @@ import static org.assertj.core.api.Assertions.assertThat; class WorkflowResultTest { + private final static WorkflowResult.Detail detail = + new WorkflowResult.Detail<>(new RuntimeException(), null, null, null, null, null, false); @Test void throwsExceptionWithoutNumberingIfAllDifferentClass() { - var res = new WorkflowResult(Map.of(new DependentA(), new RuntimeException(), - new DependentB(), new RuntimeException())); + var res = new WorkflowResult(Map.of(new DependentA(), detail, + new DependentB(), detail)); try { res.throwAggregateExceptionIfErrorsPresent(); } catch (AggregatedOperatorException e) { @@ -28,8 +30,8 @@ void throwsExceptionWithoutNumberingIfAllDifferentClass() { @Test void numbersDependentClassNamesIfMoreOfSameType() { - var res = new WorkflowResult(Map.of(new DependentA("name1"), new RuntimeException(), - new DependentA("name2"), new RuntimeException())); + var res = new WorkflowResult(Map.of(new DependentA("name1"), detail, + new DependentA("name2"), detail)); try { res.throwAggregateExceptionIfErrorsPresent(); } catch (AggregatedOperatorException e) { 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 ef48fccd6e..d6e14a1645 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 @@ -50,7 +50,7 @@ void calculatesTopLevelResources() { .addDependentResource(independentDR) .addDependentResource(dr1) .addDependentResource(dr2).dependsOn(dr1) - .build(); + .buildAsDefaultWorkflow(); Set topResources = workflow.getTopLevelDependentResources().stream() @@ -66,11 +66,11 @@ void calculatesBottomLevelResources() { var dr2 = mockDependent("dr2"); var independentDR = mockDependent("independentDR"); - Workflow workflow = new WorkflowBuilder() + final var workflow = new WorkflowBuilder() .addDependentResource(independentDR) .addDependentResource(dr1) .addDependentResource(dr2).dependsOn(dr1) - .build(); + .buildAsDefaultWorkflow(); Set bottomResources = workflow.getBottomLevelResource().stream() From ed502163f1eb88a71a9c2e0fa90a2c1909c68caa Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 15 Jun 2024 18:37:07 +0200 Subject: [PATCH 07/23] fix: properly output target dependent Signed-off-by: Chris Laprun --- .../dependent/workflow/ExecutionAssert.java | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ExecutionAssert.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ExecutionAssert.java index 095b090208..d857c0e9dd 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ExecutionAssert.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ExecutionAssert.java @@ -21,12 +21,13 @@ public static ExecutionAssert assertThat(List actual) { public ExecutionAssert reconciled(DependentResource... dependentResources) { for (int i = 0; i < dependentResources.length; i++) { - var rr = getReconcileRecordFor(dependentResources[i]); + final DependentResource dr = dependentResources[i]; + var rr = getReconcileRecordFor(dr); if (rr.isEmpty()) { - failWithMessage("Resource not reconciled: %s with index %d", dependentResources, i); + failWithMessage("Resource not reconciled: %s with index %d", dr, i); } else { if (rr.get().isDeleted()) { - failWithMessage("Resource deleted: %s with index %d", dependentResources, i); + failWithMessage("Resource deleted: %s with index %d", dr, i); } } } @@ -35,12 +36,13 @@ public ExecutionAssert reconciled(DependentResource... dependentResources) public ExecutionAssert deleted(DependentResource... dependentResources) { for (int i = 0; i < dependentResources.length; i++) { - var rr = getReconcileRecordFor(dependentResources[i]); + final DependentResource dr = dependentResources[i]; + var rr = getReconcileRecordFor(dr); if (rr.isEmpty()) { - failWithMessage("Resource not reconciled: %s with index %d", dependentResources, i); + failWithMessage("Resource not reconciled: %s with index %d", dr, i); } else { if (!rr.get().isDeleted()) { - failWithMessage("Resource not deleted: %s with index %d", dependentResources, i); + failWithMessage("Resource not deleted: %s with index %d", dr, i); } } } @@ -75,17 +77,18 @@ public ExecutionAssert reconciledInOrder(DependentResource... dependentRes public ExecutionAssert notReconciled(DependentResource... dependentResources) { for (int i = 0; i < dependentResources.length; i++) { - if (getActualDependentResources().contains(dependentResources[i])) { - failWithMessage("Resource was reconciled: %s with index %d", dependentResources, i); + final DependentResource dr = dependentResources[i]; + if (getActualDependentResources().contains(dr)) { + failWithMessage("Resource was reconciled: %s with index %d", dr, i); } } return this; } private void checkIfReconciled(int i, DependentResource[] dependentResources) { - if (!getActualDependentResources().contains(dependentResources[i])) { - failWithMessage("Dependent resource: %s, not reconciled on place %d", dependentResources[i], - i); + final DependentResource dr = dependentResources[i]; + if (!getActualDependentResources().contains(dr)) { + failWithMessage("Dependent resource: %s, not reconciled on place %d", dr, i); } } } From c8c2b5d18b3a3a704a8614231d3cf48d8e719ac8 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 15 Jun 2024 18:37:52 +0200 Subject: [PATCH 08/23] feat: add causes when skipping reconcile Signed-off-by: Chris Laprun --- .../workflow/WorkflowReconcileExecutor.java | 62 +++++++++++++++---- 1 file changed, 50 insertions(+), 12 deletions(-) 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 685ed6f1d7..b89c2a270d 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 @@ -43,12 +43,33 @@ protected Logger logger() { private synchronized void handleReconcile(DependentResourceNode dependentResourceNode) { log.debug("Submitting for reconcile: {} primaryID: {}", dependentResourceNode, primaryID); - if (alreadyVisited(dependentResourceNode) - || isExecutingNow(dependentResourceNode) - || !allParentsReconciledAndReady(dependentResourceNode) - || markedForDelete.contains(dependentResourceNode) - || hasErroredParent(dependentResourceNode)) { - log.debug("Skipping submit of: {}, primaryID: {}", dependentResourceNode, primaryID); + final var alreadyVisited = alreadyVisited(dependentResourceNode); + final var executingNow = isExecutingNow(dependentResourceNode); + final var isWaitingOnParents = !allParentsReconciledAndReady(dependentResourceNode); + final var isMarkedForDelete = markedForDelete.contains(dependentResourceNode); + final var hasErroredParent = hasErroredParent(dependentResourceNode); + if (alreadyVisited || executingNow || isMarkedForDelete || isWaitingOnParents + || hasErroredParent) { + if (log.isDebugEnabled()) { + final var causes = new ArrayList(); + if (alreadyVisited) { + causes.add("already visited"); + } + if (executingNow) { + causes.add("executing now"); + } + if (isMarkedForDelete) { + causes.add("marked for delete"); + } + if (isWaitingOnParents) { + causes.add("waiting on parents"); + } + if (hasErroredParent) { + causes.add("errored parent"); + } + log.debug("Skipping submit of: {} primaryID: {} causes: {}", dependentResourceNode, + primaryID, String.join(", ", causes)); + } return; } @@ -71,12 +92,29 @@ private synchronized void handleReconcile(DependentResourceNode depend private synchronized void handleDelete(DependentResourceNode dependentResourceNode) { log.debug("Submitting for delete: {}", dependentResourceNode); - if (alreadyVisited(dependentResourceNode) - || isExecutingNow(dependentResourceNode) - || !markedForDelete.contains(dependentResourceNode) - || !allDependentsDeletedAlready(dependentResourceNode)) { - log.debug("Skipping submit for delete of: {} primaryID: {} ", dependentResourceNode, - primaryID); + final var alreadyVisited = alreadyVisited(dependentResourceNode); + final var executingNow = isExecutingNow(dependentResourceNode); + final var isNotMarkedForDelete = !markedForDelete.contains(dependentResourceNode); + final var isWaitingOnDependents = !allDependentsDeletedAlready(dependentResourceNode); + if (alreadyVisited || executingNow || isNotMarkedForDelete || isWaitingOnDependents) { + if (log.isDebugEnabled()) { + final var causes = new ArrayList(); + if (alreadyVisited) { + causes.add("already visited"); + } + if (executingNow) { + causes.add("executing now"); + } + if (isNotMarkedForDelete) { + causes.add("not marked for delete"); + } + if (isWaitingOnDependents) { + causes.add("waiting on dependents"); + } + log.debug("Skipping submit for delete of: {} primaryID: {} causes: {}", + dependentResourceNode, + primaryID, String.join(", ", causes)); + } return; } From b959688b455d4929dfdb6dd7fcb74c72a0e02293 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 15 Jun 2024 18:39:56 +0200 Subject: [PATCH 09/23] fix: explicitly mark as visited Signed-off-by: Chris Laprun --- .../workflow/AbstractWorkflowExecutor.java | 9 ++--- .../dependent/workflow/ResultCondition.java | 20 ++++++++--- .../workflow/WorkflowCleanupExecutor.java | 2 +- .../workflow/WorkflowCleanupResult.java | 4 +-- .../workflow/WorkflowReconcileExecutor.java | 15 ++++---- .../workflow/WorkflowReconcileResult.java | 4 +-- .../dependent/workflow/WorkflowResult.java | 36 ++++++++++++++----- .../workflow/WorkflowResultTest.java | 3 +- 8 files changed, 62 insertions(+), 31 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 a6938e1697..b8732d38ce 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 @@ -72,11 +72,8 @@ protected boolean noMoreExecutionsScheduled() { } protected boolean alreadyVisited(DependentResourceNode dependentResourceNode) { - return results.containsKey(dependentResourceNode); - } - - protected void markAsVisited(DependentResourceNode dependentResourceNode) { - createOrGetResultFor(dependentResourceNode); + final WorkflowResult.DetailBuilder builder = results.get(dependentResourceNode); + return builder != null && builder.isVisited(); } protected boolean postDeleteConditionNotMet(DependentResourceNode dependentResourceNode) { @@ -162,7 +159,7 @@ protected void registerOrDeregisterEventSourceBasedOnActivation( } } - protected Map asDetails() { + protected Map> asDetails() { return results.entrySet().stream() .collect( Collectors.toMap(e -> e.getKey().getDependentResource(), e -> e.getValue().build())); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java index 2db2274d05..00c0fe4956 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java @@ -7,8 +7,6 @@ public interface ResultCondition extends Condition { Result detailedIsMet(DependentResource dependentResource, P primary, Context

context); - Object NULL = new Object(); - @Override default boolean isMet(DependentResource dependentResource, P primary, Context

context) { return detailedIsMet(dependentResource, primary, context).isSuccess(); @@ -18,31 +16,45 @@ interface Result { Result metWithoutResult = new Result() { @Override public Object getResult() { - return NULL; + return null; } @Override public boolean isSuccess() { return true; } + + @Override + public String toString() { + return asString(); + } }; Result unmetWithoutResult = new Result() { @Override public Object getResult() { - return NULL; + return null; } @Override public boolean isSuccess() { return false; } + + @Override + public String toString() { + return asString(); + } }; static Result withoutResult(boolean success) { return success ? metWithoutResult : unmetWithoutResult; } + default String asString() { + return "Result: " + getResult() + " met: " + isSuccess(); + } + T getResult(); boolean isSuccess(); 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 b88a8afc05..a1e504079f 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 @@ -74,7 +74,7 @@ protected void doRun(DependentResourceNode dependentResourceNode) { } if (deletePostConditionMet) { - markAsVisited(dependentResourceNode); + createOrGetResultFor(dependentResourceNode).markAsVisited(); handleDependentCleaned(dependentResourceNode); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java index 942c7eccf5..420073af3c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java @@ -7,7 +7,7 @@ @SuppressWarnings("rawtypes") public class WorkflowCleanupResult extends WorkflowResult { - WorkflowCleanupResult(Map results) { + WorkflowCleanupResult(Map> results) { super(results); } @@ -16,7 +16,7 @@ public List getDeleteCalledOnDependents() { } public List getPostConditionNotMetDependents() { - return listFilteredBy(detail -> !Detail.isConditionMet(detail.deletePostconditionResult())); + return listFilteredBy(detail -> !detail.isConditionWithTypeMet(Condition.Type.DELETE)); } public boolean allPostConditionsMet() { 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 b89c2a270d..8e668416dc 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,5 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.ArrayList; import java.util.HashSet; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -140,12 +141,13 @@ protected void doRun(DependentResourceNode dependentResourceNode) { log.debug( "Reconciling for primary: {} node: {} ", primaryID, dependentResourceNode); ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); - createOrGetResultFor(dependentResourceNode).withReconcileResult(reconcileResult); + final var detailBuilder = createOrGetResultFor(dependentResourceNode); + detailBuilder.withReconcileResult(reconcileResult); if (isConditionMet(dependentResourceNode.getReconcilePrecondition(), dependentResourceNode)) { log.debug("Setting already reconciled for: {} primaryID: {}", dependentResourceNode, primaryID); - markAsVisited(dependentResourceNode); + detailBuilder.markAsVisited(); handleDependentsReconcile(dependentResourceNode); } else { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); @@ -162,8 +164,6 @@ private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { @Override @SuppressWarnings("unchecked") protected void doRun(DependentResourceNode dependentResourceNode) { - var deletePostCondition = dependentResourceNode.getDeletePostcondition(); - final var dependentResource = dependentResourceNode.getDependentResource(); boolean deletePostConditionMet = true; if (isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode)) { @@ -173,11 +173,12 @@ protected void doRun(DependentResourceNode dependentResourceNode) { if (dependentResource instanceof Deleter) { ((Deleter

) dependentResource).delete(primary, context); } - deletePostConditionMet = isConditionMet(deletePostCondition, dependentResourceNode); + deletePostConditionMet = + isConditionMet(dependentResourceNode.getDeletePostcondition(), dependentResourceNode); } if (deletePostConditionMet) { - markAsVisited(dependentResourceNode); + createOrGetResultFor(dependentResourceNode).markAsVisited(); handleDependentDeleted(dependentResourceNode); } } @@ -224,7 +225,7 @@ private void markDependentsForDelete(DependentResourceNode dependentResour } } else { // this is for an edge case when there is only one resource but that is not active - markAsVisited(dependentResourceNode); + createOrGetResultFor(dependentResourceNode).markAsVisited(); if (dependents.isEmpty()) { handleNodeExecutionFinish(dependentResourceNode); } else { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index 070bdc915a..ea95f2644d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -9,7 +9,7 @@ @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { - public WorkflowReconcileResult(Map results) { + public WorkflowReconcileResult(Map> results) { super(results); } @@ -18,7 +18,7 @@ public List getReconciledDependents() { } public List getNotReadyDependents() { - return listFilteredBy(detail -> !Detail.isConditionMet(detail.reconcilePostconditionResult())); + return listFilteredBy(detail -> !detail.isConditionWithTypeMet(Condition.Type.READY)); } public T getNotReadyDependentResult(DependentResource dependentResource, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 0953da55cd..d5ad7bbe44 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -3,6 +3,7 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Optional; import java.util.function.Function; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -13,10 +14,10 @@ @SuppressWarnings("rawtypes") class WorkflowResult { - private final Map results; + private final Map> results; private Boolean hasErroredDependents; - WorkflowResult(Map results) { + WorkflowResult(Map> results) { this.results = results; } @@ -25,11 +26,11 @@ public Map getErroredDependents() { .collect(Collectors.toMap(Entry::getKey, entry -> entry.getValue().error)); } - private Stream> getErroredDependentsStream() { + private Stream>> getErroredDependentsStream() { return results.entrySet().stream().filter(entry -> entry.getValue().error != null); } - protected Map results() { + protected Map> results() { return results; } @@ -64,10 +65,11 @@ static class DetailBuilder { private ResultCondition.Result readyPostconditionResult; private ResultCondition.Result reconcilePostconditionResult; private boolean deleted; + private boolean visited; Detail build() { return new Detail<>(error, reconcileResult, activationConditionResult, - deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted); + deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted, visited); } DetailBuilder withResultForCondition(DependentResourceNode.ConditionWithType conditionWithType, ResultCondition.Result conditionResult) { @@ -107,6 +109,15 @@ public boolean hasPostDeleteConditionNotMet() { public boolean isNotReady() { return readyPostconditionResult != null && !readyPostconditionResult.isSuccess(); } + + DetailBuilder markAsVisited() { + visited = true; + return this; + } + + public boolean isVisited() { + return visited; + } } @@ -115,10 +126,19 @@ record Detail(Exception error, ReconcileResult reconcileResult, ResultCondition.Result deletePostconditionResult, ResultCondition.Result readyPostconditionResult, ResultCondition.Result reconcilePostconditionResult, - boolean deleted) { + boolean deleted, boolean visited) { + + boolean isConditionWithTypeMet(Condition.Type conditionType) { + return getResultForConditionWithType(conditionType).map(ResultCondition.Result::isSuccess).orElse(true); + } - static boolean isConditionMet(ResultCondition.Result conditionResult) { - return conditionResult != null && conditionResult.isSuccess(); + Optional> getResultForConditionWithType(Condition.Type conditionType) { + return switch (conditionType) { + case ACTIVATION -> Optional.ofNullable(activationConditionResult); + case DELETE -> Optional.ofNullable(deletePostconditionResult); + case READY -> Optional.ofNullable(readyPostconditionResult); + case RECONCILE -> Optional.ofNullable(reconcilePostconditionResult); + }; } } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java index c3898e9bb5..1a91cd1f16 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -14,7 +14,8 @@ class WorkflowResultTest { private final static WorkflowResult.Detail detail = - new WorkflowResult.Detail<>(new RuntimeException(), null, null, null, null, null, false); + new WorkflowResult.Detail<>(new RuntimeException(), null, null, null, null, null, false, + false); @Test void throwsExceptionWithoutNumberingIfAllDifferentClass() { From 0cfc3d6752b0ae5d4d268a59f9a29b409f2d8cb9 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 15 Jun 2024 18:40:24 +0200 Subject: [PATCH 10/23] fix: properly extract result Signed-off-by: Chris Laprun --- .../dependent/workflow/WorkflowReconcileResult.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index ea95f2644d..51fcc6d3fc 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -26,8 +26,8 @@ public T getNotReadyDependentResult(DependentResource dependentResource, final var result = new Object[1]; try { return Optional.ofNullable(results().get(dependentResource)) - .filter(cr -> !ResultCondition.NULL.equals(cr)) - .map(r -> result[0] = r) + .flatMap(detail -> detail.getResultForConditionWithType(Condition.Type.READY)) + .map(r -> result[0] = r.getResult()) .map(expectedResultType::cast) .orElse(null); } catch (Exception e) { From 55259927e2ef002755aa44354693d01f261a14fc Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Sat, 15 Jun 2024 18:40:36 +0200 Subject: [PATCH 11/23] fix: wrong condition Signed-off-by: Chris Laprun --- .../dependent/workflow/WorkflowReconcileExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8e668416dc..fe5bf0f93b 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 @@ -144,7 +144,7 @@ protected void doRun(DependentResourceNode dependentResourceNode) { final var detailBuilder = createOrGetResultFor(dependentResourceNode); detailBuilder.withReconcileResult(reconcileResult); - if (isConditionMet(dependentResourceNode.getReconcilePrecondition(), dependentResourceNode)) { + if (isConditionMet(dependentResourceNode.getReadyPostcondition(), dependentResourceNode)) { log.debug("Setting already reconciled for: {} primaryID: {}", dependentResourceNode, primaryID); detailBuilder.markAsVisited(); From 4e48ccbfe8f10b989faa6039fb967c6795efff20 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 20 Jun 2024 18:50:30 +0200 Subject: [PATCH 12/23] fix: wrong provided or checked conditions Signed-off-by: Chris Laprun --- .../dependent/workflow/DependentResourceNode.java | 2 +- .../workflow/WorkflowReconcileExecutor.java | 15 +++++++-------- .../dependent/workflow/WorkflowResult.java | 3 ++- 3 files changed, 10 insertions(+), 10 deletions(-) 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 afbf624e52..901278ab12 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 @@ -98,7 +98,7 @@ void setReconcilePrecondition(Condition reconcilePrecondition) { : new ConditionWithType<>(reconcilePrecondition, Condition.Type.RECONCILE); } - void setDeletePostcondition(Condition cleanupCondition) { + void setDeletePostcondition(Condition deletePostcondition) { this.deletePostcondition = deletePostcondition == null ? null : new ConditionWithType<>(deletePostcondition, Condition.Type.DELETE); } 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 fe5bf0f93b..b9a11cf0c4 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 @@ -42,14 +42,14 @@ protected Logger logger() { } private synchronized void handleReconcile(DependentResourceNode dependentResourceNode) { - log.debug("Submitting for reconcile: {} primaryID: {}", dependentResourceNode, primaryID); + log.debug("Considering for reconcile: {} primaryID: {}", dependentResourceNode, primaryID); final var alreadyVisited = alreadyVisited(dependentResourceNode); final var executingNow = isExecutingNow(dependentResourceNode); final var isWaitingOnParents = !allParentsReconciledAndReady(dependentResourceNode); final var isMarkedForDelete = markedForDelete.contains(dependentResourceNode); final var hasErroredParent = hasErroredParent(dependentResourceNode); - if (alreadyVisited || executingNow || isMarkedForDelete || isWaitingOnParents + if (isWaitingOnParents || alreadyVisited || executingNow || isMarkedForDelete || hasErroredParent) { if (log.isDebugEnabled()) { final var causes = new ArrayList(); @@ -68,7 +68,7 @@ private synchronized void handleReconcile(DependentResourceNode depend if (hasErroredParent) { causes.add("errored parent"); } - log.debug("Skipping submit of: {} primaryID: {} causes: {}", dependentResourceNode, + log.debug("Skipping: {} primaryID: {} causes: {}", dependentResourceNode, primaryID, String.join(", ", causes)); } return; @@ -97,7 +97,7 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo final var executingNow = isExecutingNow(dependentResourceNode); final var isNotMarkedForDelete = !markedForDelete.contains(dependentResourceNode); final var isWaitingOnDependents = !allDependentsDeletedAlready(dependentResourceNode); - if (alreadyVisited || executingNow || isNotMarkedForDelete || isWaitingOnDependents) { + if (isNotMarkedForDelete || alreadyVisited || executingNow || isWaitingOnDependents) { if (log.isDebugEnabled()) { final var causes = new ArrayList(); if (alreadyVisited) { @@ -142,12 +142,11 @@ protected void doRun(DependentResourceNode dependentResourceNode) { "Reconciling for primary: {} node: {} ", primaryID, dependentResourceNode); ReconcileResult reconcileResult = dependentResource.reconcile(primary, context); final var detailBuilder = createOrGetResultFor(dependentResourceNode); - detailBuilder.withReconcileResult(reconcileResult); + detailBuilder.withReconcileResult(reconcileResult).markAsVisited(); if (isConditionMet(dependentResourceNode.getReadyPostcondition(), dependentResourceNode)) { log.debug("Setting already reconciled for: {} primaryID: {}", dependentResourceNode, primaryID); - detailBuilder.markAsVisited(); handleDependentsReconcile(dependentResourceNode); } else { log.debug("Setting already reconciled but not ready for: {}", dependentResourceNode); @@ -164,12 +163,12 @@ private NodeDeleteExecutor(DependentResourceNode dependentResourceNode) { @Override @SuppressWarnings("unchecked") protected void doRun(DependentResourceNode dependentResourceNode) { - final var dependentResource = dependentResourceNode.getDependentResource(); boolean deletePostConditionMet = true; if (isConditionMet(dependentResourceNode.getActivationCondition(), dependentResourceNode)) { // GarbageCollected status is irrelevant here, as this method is only called when a // precondition does not hold, // a deleter should be deleted even if it is otherwise garbage collected + final var dependentResource = dependentResourceNode.getDependentResource(); if (dependentResource instanceof Deleter) { ((Deleter

) dependentResource).delete(primary, context); } @@ -177,8 +176,8 @@ protected void doRun(DependentResourceNode dependentResourceNode) { isConditionMet(dependentResourceNode.getDeletePostcondition(), dependentResourceNode); } + createOrGetResultFor(dependentResourceNode).markAsVisited(); if (deletePostConditionMet) { - createOrGetResultFor(dependentResourceNode).markAsVisited(); handleDependentDeleted(dependentResourceNode); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index d5ad7bbe44..3f6fc7921f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -57,6 +57,7 @@ public void throwAggregateExceptionIfErrorsPresent() { } } + @SuppressWarnings("UnusedReturnValue") static class DetailBuilder { private Exception error; private ReconcileResult reconcileResult; @@ -103,7 +104,7 @@ public boolean hasError() { } public boolean hasPostDeleteConditionNotMet() { - return deletePostconditionResult != null && !readyPostconditionResult.isSuccess(); + return deletePostconditionResult != null && !deletePostconditionResult.isSuccess(); } public boolean isNotReady() { From 530b1743d1613091f399afcb91218029005c77b1 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 20 Jun 2024 21:15:17 +0200 Subject: [PATCH 13/23] fix: inverted condition, display causes when skipping a node Signed-off-by: Chris Laprun --- .../workflow/WorkflowCleanupExecutor.java | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 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 a1e504079f..2a89950b22 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,5 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; +import java.util.ArrayList; import java.util.List; import org.slf4j.Logger; @@ -34,13 +35,30 @@ protected Logger logger() { @SuppressWarnings({"rawtypes", "unchecked"}) private synchronized void handleCleanup(DependentResourceNode dependentResourceNode) { - log.debug("Submitting for cleanup: {} primaryID: {}", dependentResourceNode, primaryID); - - if (alreadyVisited(dependentResourceNode) - || isExecutingNow(dependentResourceNode) - || !allDependentsCleaned(dependentResourceNode) - || hasErroredDependent(dependentResourceNode)) { - log.debug("Skipping submit of: {} primaryID: {}", dependentResourceNode, primaryID); + log.debug("Considering for cleanup: {} primaryID: {}", dependentResourceNode, primaryID); + + final var alreadyVisited = alreadyVisited(dependentResourceNode); + final var executingNow = isExecutingNow(dependentResourceNode); + final var waitingOnDependents = !allDependentsCleaned(dependentResourceNode); + final var hasErroredDependent = hasErroredDependent(dependentResourceNode); + if (waitingOnDependents || alreadyVisited || executingNow || hasErroredDependent) { + if (log.isDebugEnabled()) { + final var causes = new ArrayList(); + if (alreadyVisited) { + causes.add("already visited"); + } + if (executingNow) { + causes.add("executing now"); + } + if (waitingOnDependents) { + causes.add("waiting on dependents"); + } + if (hasErroredDependent) { + causes.add("errored dependent"); + } + log.debug("Skipping: {} primaryID: {} causes: {}", dependentResourceNode, + primaryID, String.join(", ", causes)); + } return; } @@ -73,8 +91,9 @@ protected void doRun(DependentResourceNode dependentResourceNode) { isConditionMet(dependentResourceNode.getDeletePostcondition(), dependentResourceNode); } + createOrGetResultFor(dependentResourceNode).markAsVisited(); + if (deletePostConditionMet) { - createOrGetResultFor(dependentResourceNode).markAsVisited(); handleDependentCleaned(dependentResourceNode); } } @@ -97,7 +116,7 @@ private boolean allDependentsCleaned(DependentResourceNode dependentResourceNode List parents = dependentResourceNode.getParents(); return parents.isEmpty() || parents.stream() - .allMatch(d -> alreadyVisited(d) && postDeleteConditionNotMet(d)); + .allMatch(d -> alreadyVisited(d) && !postDeleteConditionNotMet(d)); } @SuppressWarnings("unchecked") From e50e1416ba7918ce8b19529ef11b6d281b42bdd6 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 21 Jun 2024 10:13:48 +0200 Subject: [PATCH 14/23] refactor: remove markedForDelete field Signed-off-by: Chris Laprun --- .../workflow/AbstractWorkflowExecutor.java | 29 +++++++++++++------ .../workflow/WorkflowCleanupExecutor.java | 2 +- .../workflow/WorkflowReconcileExecutor.java | 11 ++----- .../workflow/WorkflowReconcileResult.java | 2 +- .../dependent/workflow/WorkflowResult.java | 18 +++++++++--- .../workflow/WorkflowResultTest.java | 4 +-- 6 files changed, 41 insertions(+), 25 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 b8732d38ce..c49a6c420a 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 @@ -5,6 +5,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ExecutorService; import java.util.concurrent.Future; +import java.util.function.Function; import java.util.stream.Collectors; import org.slf4j.Logger; @@ -72,13 +73,15 @@ protected boolean noMoreExecutionsScheduled() { } protected boolean alreadyVisited(DependentResourceNode dependentResourceNode) { - final WorkflowResult.DetailBuilder builder = results.get(dependentResourceNode); - return builder != null && builder.isVisited(); + return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::isVisited); } - protected boolean postDeleteConditionNotMet(DependentResourceNode dependentResourceNode) { - final var builder = results.get(dependentResourceNode); - return builder != null && builder.hasPostDeleteConditionNotMet(); + protected boolean postDeleteConditionNotMet(DependentResourceNode drn) { + return getResultFlagFor(drn, WorkflowResult.DetailBuilder::hasPostDeleteConditionNotMet); + } + + protected boolean isMarkedForDelete(DependentResourceNode drn) { + return getResultFlagFor(drn, WorkflowResult.DetailBuilder::isMarkedForDelete); } protected WorkflowResult.DetailBuilder createOrGetResultFor( @@ -87,6 +90,16 @@ protected WorkflowResult.DetailBuilder createOrGetResultFor( unused -> new WorkflowResult.DetailBuilder()); } + protected Optional> getResultFor( + DependentResourceNode dependentResourceNode) { + return Optional.ofNullable(results.get(dependentResourceNode)); + } + + protected boolean getResultFlagFor(DependentResourceNode dependentResourceNode, + Function, Boolean> flag) { + return getResultFor(dependentResourceNode).map(flag).orElse(false); + } + protected boolean isExecutingNow(DependentResourceNode dependentResourceNode) { return actualExecutions.containsKey(dependentResourceNode); } @@ -103,13 +116,11 @@ protected synchronized void handleExceptionInExecutor( } protected boolean isNotReady(DependentResourceNode dependentResourceNode) { - final var builder = results.get(dependentResourceNode); - return builder != null && builder.isNotReady(); + return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::isNotReady); } protected boolean isInError(DependentResourceNode dependentResourceNode) { - final var builder = results.get(dependentResourceNode); - return builder != null && builder.hasError(); + return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::hasError); } protected synchronized void handleNodeExecutionFinish( 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 2a89950b22..da518be6ce 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 @@ -84,7 +84,7 @@ protected void doRun(DependentResourceNode dependentResourceNode) { final var dependentResource = dependentResourceNode.getDependentResource(); if (dependentResource.isDeletable()) { ((Deleter

) dependentResource).delete(primary, context); - createOrGetResultFor(dependentResourceNode).withDeleted(true); + createOrGetResultFor(dependentResourceNode).markAsDeleted(); } deletePostConditionMet = 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 b9a11cf0c4..b130e8fd5f 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 @@ -3,7 +3,6 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -20,12 +19,8 @@ class WorkflowReconcileExecutor

extends AbstractWorkflowE private static final String RECONCILE = "reconcile"; private static final String DELETE = "delete"; - private final Set markedForDelete; - public WorkflowReconcileExecutor(DefaultWorkflow

workflow, P primary, Context

context) { super(workflow, primary, context); - final var size = workflow.size(); - markedForDelete = ConcurrentHashMap.newKeySet(size); } public synchronized WorkflowReconcileResult reconcile() { @@ -47,7 +42,7 @@ private synchronized void handleReconcile(DependentResourceNode depend final var alreadyVisited = alreadyVisited(dependentResourceNode); final var executingNow = isExecutingNow(dependentResourceNode); final var isWaitingOnParents = !allParentsReconciledAndReady(dependentResourceNode); - final var isMarkedForDelete = markedForDelete.contains(dependentResourceNode); + final var isMarkedForDelete = isMarkedForDelete(dependentResourceNode); final var hasErroredParent = hasErroredParent(dependentResourceNode); if (isWaitingOnParents || alreadyVisited || executingNow || isMarkedForDelete || hasErroredParent) { @@ -95,7 +90,7 @@ private synchronized void handleDelete(DependentResourceNode dependentResourceNo final var alreadyVisited = alreadyVisited(dependentResourceNode); final var executingNow = isExecutingNow(dependentResourceNode); - final var isNotMarkedForDelete = !markedForDelete.contains(dependentResourceNode); + final var isNotMarkedForDelete = !isMarkedForDelete(dependentResourceNode); final var isWaitingOnDependents = !allDependentsDeletedAlready(dependentResourceNode); if (isNotMarkedForDelete || alreadyVisited || executingNow || isWaitingOnDependents) { if (log.isDebugEnabled()) { @@ -216,7 +211,7 @@ private void markDependentsForDelete(DependentResourceNode dependentResour // so if the activation condition was false, this node is not meant to be deleted. var dependents = dependentResourceNode.getParents(); if (activationConditionMet) { - markedForDelete.add(dependentResourceNode); + createOrGetResultFor(dependentResourceNode).markForDelete(); if (dependents.isEmpty()) { bottomNodes.add(dependentResourceNode); } else { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index 51fcc6d3fc..abc9bfac10 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -9,7 +9,7 @@ @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { - public WorkflowReconcileResult(Map> results) { + WorkflowReconcileResult(Map> results) { super(results); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 3f6fc7921f..f88bfdbe34 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -67,10 +67,11 @@ static class DetailBuilder { private ResultCondition.Result reconcilePostconditionResult; private boolean deleted; private boolean visited; + private boolean markedForDelete; Detail build() { return new Detail<>(error, reconcileResult, activationConditionResult, - deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted, visited); + deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted, visited, markedForDelete); } DetailBuilder withResultForCondition(DependentResourceNode.ConditionWithType conditionWithType, ResultCondition.Result conditionResult) { @@ -94,8 +95,8 @@ DetailBuilder withReconcileResult(ReconcileResult reconcileResult) { return this; } - DetailBuilder withDeleted(boolean deleted) { - this.deleted = deleted; + DetailBuilder markAsDeleted() { + this.deleted = true; return this; } @@ -119,6 +120,15 @@ DetailBuilder markAsVisited() { public boolean isVisited() { return visited; } + + public boolean isMarkedForDelete() { + return markedForDelete; + } + + DetailBuilder markForDelete() { + markedForDelete = true; + return this; + } } @@ -127,7 +137,7 @@ record Detail(Exception error, ReconcileResult reconcileResult, ResultCondition.Result deletePostconditionResult, ResultCondition.Result readyPostconditionResult, ResultCondition.Result reconcilePostconditionResult, - boolean deleted, boolean visited) { + boolean deleted, boolean visited, boolean markedForDelete) { boolean isConditionWithTypeMet(Condition.Type conditionType) { return getResultForConditionWithType(conditionType).map(ResultCondition.Result::isSuccess).orElse(true); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java index 1a91cd1f16..ca0b883e99 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResultTest.java @@ -13,9 +13,9 @@ import static org.assertj.core.api.Assertions.assertThat; class WorkflowResultTest { - private final static WorkflowResult.Detail detail = + private final static WorkflowResult.Detail detail = new WorkflowResult.Detail<>(new RuntimeException(), null, null, null, null, null, false, - false); + false, false); @Test void throwsExceptionWithoutNumberingIfAllDifferentClass() { From f819c69d239c65d95e975703d18a234c01cd4142 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 21 Jun 2024 10:54:17 +0200 Subject: [PATCH 15/23] feat: add getDependentConditionResult method and document it Signed-off-by: Chris Laprun --- .../workflow/AbstractWorkflowExecutor.java | 2 +- .../workflow/WorkflowCleanupResult.java | 7 ++- .../workflow/WorkflowReconcileResult.java | 21 +++------ .../dependent/workflow/WorkflowResult.java | 46 ++++++++++++++++--- .../WorkflowReconcileExecutorTest.java | 7 +-- 5 files changed, 58 insertions(+), 25 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 c49a6c420a..5772a0ef1a 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 @@ -120,7 +120,7 @@ protected boolean isNotReady(DependentResourceNode dependentResourceNode) } protected boolean isInError(DependentResourceNode dependentResourceNode) { - return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::hasError); + return getResultFlagFor(dependentResourceNode, WorkflowResult.DetailBuilder::hasError); } protected synchronized void handleNodeExecutionFinish( diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java index 420073af3c..b93741ef56 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupResult.java @@ -7,6 +7,8 @@ @SuppressWarnings("rawtypes") public class WorkflowCleanupResult extends WorkflowResult { + private Boolean allPostConditionsMet; + WorkflowCleanupResult(Map> results) { super(results); } @@ -20,6 +22,9 @@ public List getPostConditionNotMetDependents() { } public boolean allPostConditionsMet() { - return getPostConditionNotMetDependents().isEmpty(); + if (allPostConditionsMet == null) { + allPostConditionsMet = getPostConditionNotMetDependents().isEmpty(); + } + return allPostConditionsMet; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index abc9bfac10..99e4b9e8a1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -8,6 +8,7 @@ @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { + private Boolean allDependentsReady; WorkflowReconcileResult(Map> results) { super(results); @@ -21,23 +22,15 @@ public List getNotReadyDependents() { return listFilteredBy(detail -> !detail.isConditionWithTypeMet(Condition.Type.READY)); } - public T getNotReadyDependentResult(DependentResource dependentResource, + public Optional getNotReadyDependentResult(DependentResource dependentResource, Class expectedResultType) { - final var result = new Object[1]; - try { - return Optional.ofNullable(results().get(dependentResource)) - .flatMap(detail -> detail.getResultForConditionWithType(Condition.Type.READY)) - .map(r -> result[0] = r.getResult()) - .map(expectedResultType::cast) - .orElse(null); - } catch (Exception e) { - throw new IllegalArgumentException("Condition result " + result[0] + - " for Dependent " + dependentResource.name() + " doesn't match expected type " - + expectedResultType.getSimpleName(), e); - } + return getDependentConditionResult(dependentResource, Condition.Type.READY, expectedResultType); } public boolean allDependentResourcesReady() { - return getNotReadyDependents().isEmpty(); + if (allDependentsReady == null) { + allDependentsReady = getNotReadyDependents().isEmpty(); + } + return allDependentsReady; } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index f88bfdbe34..3f64bbaf23 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -34,6 +34,34 @@ protected Map> results() { return results; } + /** + * Retrieves the optional result of the condition with the specified type for the specified + * dependent resource. + * + * @param the expected result type of the condition + * @param dependentResource the dependent resource for which we want to retrieve a condition + * result + * @param conditionType the condition type which result we're interested in + * @param expectedResultType the expected result type of the condition + * @return the dependent condition result if it exists or {@link Optional#empty()} otherwise + * @throws IllegalArgumentException if a result exists but is not of the expected type + */ + public Optional getDependentConditionResult(DependentResource dependentResource, + Condition.Type conditionType, Class expectedResultType) { + final var result = new Object[1]; + try { + return Optional.ofNullable(results().get(dependentResource)) + .flatMap(detail -> detail.getResultForConditionWithType(conditionType)) + .map(r -> result[0] = r.getResult()) + .map(expectedResultType::cast); + } catch (Exception e) { + throw new IllegalArgumentException("Condition " + + "result " + result[0] + + " for Dependent " + dependentResource.name() + " doesn't match expected type " + + expectedResultType.getSimpleName(), e); + } + } + protected List listFilteredBy( Function filter) { return results.entrySet().stream() @@ -71,16 +99,20 @@ static class DetailBuilder { Detail build() { return new Detail<>(error, reconcileResult, activationConditionResult, - deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, deleted, visited, markedForDelete); + deletePostconditionResult, readyPostconditionResult, reconcilePostconditionResult, + deleted, visited, markedForDelete); } - DetailBuilder withResultForCondition(DependentResourceNode.ConditionWithType conditionWithType, ResultCondition.Result conditionResult) { + DetailBuilder withResultForCondition( + DependentResourceNode.ConditionWithType conditionWithType, + ResultCondition.Result conditionResult) { switch (conditionWithType.type()) { case ACTIVATION -> activationConditionResult = conditionResult; case DELETE -> deletePostconditionResult = conditionResult; case READY -> readyPostconditionResult = conditionResult; case RECONCILE -> reconcilePostconditionResult = conditionResult; - default -> throw new IllegalStateException("Unexpected condition type: " + conditionWithType); + default -> + throw new IllegalStateException("Unexpected condition type: " + conditionWithType); } return this; } @@ -137,13 +169,15 @@ record Detail(Exception error, ReconcileResult reconcileResult, ResultCondition.Result deletePostconditionResult, ResultCondition.Result readyPostconditionResult, ResultCondition.Result reconcilePostconditionResult, - boolean deleted, boolean visited, boolean markedForDelete) { + boolean deleted, boolean visited, boolean markedForDelete) { boolean isConditionWithTypeMet(Condition.Type conditionType) { - return getResultForConditionWithType(conditionType).map(ResultCondition.Result::isSuccess).orElse(true); + return getResultForConditionWithType(conditionType).map(ResultCondition.Result::isSuccess) + .orElse(true); } - Optional> getResultForConditionWithType(Condition.Type conditionType) { + Optional> getResultForConditionWithType( + Condition.Type conditionType) { return switch (conditionType) { case ACTIVATION -> Optional.ofNullable(activationConditionResult); case DELETE -> Optional.ofNullable(deletePostconditionResult); 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 cf7a1604b9..bd1a2a1d2c 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 @@ -604,7 +604,8 @@ public boolean isSuccess() { .build(); final var reconcileResult = workflow.reconcile(new TestCustomResource(), mockContext); - assertEquals(result, reconcileResult.getNotReadyDependentResult(dr1, Integer.class)); + assertEquals(result, + reconcileResult.getNotReadyDependentResult(dr1, Integer.class).orElseThrow()); } @Test @@ -643,13 +644,13 @@ public boolean isSuccess() { } @Test - void shouldReturnNullIfNoConditionResultExists() { + void shouldReturnEmptyIfNoConditionResultExists() { var workflow = new WorkflowBuilder() .addDependentResource(dr1) .withReadyPostcondition(notMetCondition) .build(); final var reconcileResult = workflow.reconcile(new TestCustomResource(), mockContext); - assertNull(reconcileResult.getNotReadyDependentResult(dr1, Integer.class)); + assertTrue(reconcileResult.getNotReadyDependentResult(dr1, Integer.class).isEmpty()); } } From 79a70d1c2818a14b6e51a8adc5ff4e366687bd38 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 4 Jul 2024 14:26:32 +0200 Subject: [PATCH 16/23] refactor: move ConditionWithType to top level Signed-off-by: Chris Laprun --- .../workflow/AbstractWorkflowExecutor.java | 2 +- .../dependent/workflow/ConditionWithType.java | 31 +++++++++++++++++++ .../workflow/DependentResourceNode.java | 27 ---------------- .../dependent/workflow/WorkflowResult.java | 2 +- 4 files changed, 33 insertions(+), 29 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.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 5772a0ef1a..0e89c62df8 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 @@ -134,7 +134,7 @@ protected synchronized void handleNodeExecutionFinish( @SuppressWarnings("unchecked") protected boolean isConditionMet( - Optional> condition, + Optional> condition, DependentResourceNode dependentResource) { final var dr = dependentResource.getDependentResource(); return condition.map(c -> { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java new file mode 100644 index 0000000000..513697ddcc --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java @@ -0,0 +1,31 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; + +class ConditionWithType implements ResultCondition { + private final Condition condition; + private final Type type; + + ConditionWithType(Condition condition, Type type) { + this.condition = condition; + this.type = type; + } + + public Type type() { + return type; + } + + @SuppressWarnings("unchecked") + @Override + public Result detailedIsMet(DependentResource dependentResource, P primary, + Context

context) { + if (condition instanceof ResultCondition resultCondition) { + return resultCondition.detailedIsMet(dependentResource, primary, context); + } else { + return Result + .withoutResult(condition.isMet(dependentResource, primary, context)); + } + } +} 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 901278ab12..aa8bb31c66 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DependentResourceNode.java @@ -5,7 +5,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") @@ -20,32 +19,6 @@ class DependentResourceNode { private ConditionWithType activationCondition; private final DependentResource dependentResource; - static class ConditionWithType implements ResultCondition { - private final Condition condition; - private final Type type; - - ConditionWithType(Condition condition, Type type) { - this.condition = condition; - this.type = type; - } - - public Type type() { - return type; - } - - @SuppressWarnings("unchecked") - @Override - public Result detailedIsMet(DependentResource dependentResource, P primary, - Context

context) { - if (condition instanceof ResultCondition resultCondition) { - return resultCondition.detailedIsMet(dependentResource, primary, context); - } else { - return ResultCondition.Result - .withoutResult(condition.isMet(dependentResource, primary, context)); - } - } - } - DependentResourceNode(DependentResource dependentResource) { this(null, null, null, null, dependentResource); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 3f64bbaf23..53c91cc668 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -104,7 +104,7 @@ Detail build() { } DetailBuilder withResultForCondition( - DependentResourceNode.ConditionWithType conditionWithType, + ConditionWithType conditionWithType, ResultCondition.Result conditionResult) { switch (conditionWithType.type()) { case ACTIVATION -> activationConditionResult = conditionResult; From b53629000d6ebeaa13ff5e1d120a61363ee7fc45 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 4 Jul 2024 14:54:06 +0200 Subject: [PATCH 17/23] refactor: remove caching Signed-off-by: Chris Laprun --- .../dependent/workflow/WorkflowReconcileResult.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java index 99e4b9e8a1..055fca3bfe 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java @@ -8,7 +8,6 @@ @SuppressWarnings("rawtypes") public class WorkflowReconcileResult extends WorkflowResult { - private Boolean allDependentsReady; WorkflowReconcileResult(Map> results) { super(results); @@ -28,9 +27,6 @@ public Optional getNotReadyDependentResult(DependentResource dependentRes } public boolean allDependentResourcesReady() { - if (allDependentsReady == null) { - allDependentsReady = getNotReadyDependents().isEmpty(); - } - return allDependentsReady; + return getNotReadyDependents().isEmpty(); } } From 86232ab8190e8ecf735714a98c20e83b5742dbfb Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Jul 2024 14:54:59 +0200 Subject: [PATCH 18/23] refactor: clean-up ResultCondition Signed-off-by: Chris Laprun --- .../dependent/workflow/DefaultResult.java | 26 ++++++++++++ .../dependent/workflow/ResultCondition.java | 41 ++++--------------- 2 files changed, 34 insertions(+), 33 deletions(-) create mode 100644 operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java new file mode 100644 index 0000000000..b5ba99f82b --- /dev/null +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java @@ -0,0 +1,26 @@ +package io.javaoperatorsdk.operator.processing.dependent.workflow; + +public class DefaultResult implements ResultCondition.Result { + private final T result; + private final boolean success; + + public DefaultResult(boolean success, T result) { + this.result = result; + this.success = success; + } + + @Override + public T getResult() { + return result; + } + + @Override + public boolean isSuccess() { + return success; + } + + @Override + public String toString() { + return asString(); + } +} diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java index 00c0fe4956..ebc23735a1 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java @@ -12,45 +12,20 @@ default boolean isMet(DependentResource dependentResource, P primary, Cont return detailedIsMet(dependentResource, primary, context).isSuccess(); } + @SuppressWarnings({"rawtypes", "unchecked"}) interface Result { - Result metWithoutResult = new Result() { - @Override - public Object getResult() { - return null; - } - - @Override - public boolean isSuccess() { - return true; - } - - @Override - public String toString() { - return asString(); - } - }; - - Result unmetWithoutResult = new Result() { - @Override - public Object getResult() { - return null; - } - - @Override - public boolean isSuccess() { - return false; - } - - @Override - public String toString() { - return asString(); - } - }; + ResultCondition.Result metWithoutResult = new DefaultResult(true, null); + + ResultCondition.Result unmetWithoutResult = new DefaultResult(false, null); static Result withoutResult(boolean success) { return success ? metWithoutResult : unmetWithoutResult; } + static Result withResult(boolean success, T result) { + return new DefaultResult<>(success, result); + } + default String asString() { return "Result: " + getResult() + " met: " + isSuccess(); } From d68441a9fbd4b1de304b1ee320a40c83420cf84e Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Jul 2024 14:55:59 +0200 Subject: [PATCH 19/23] feat: add retrieval of condition result and DR by name Signed-off-by: Chris Laprun --- .../dependent/workflow/WorkflowResult.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 53c91cc668..461307febe 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -34,6 +34,40 @@ protected Map> results() { return results; } + /** + * Retrieves the {@link DependentResource} associated with the specified name if it exists, + * {@link Optional#empty()} otherwise. + * + * @param name the name of the {@link DependentResource} to retrieve + * @return the {@link DependentResource} associated with the specified name if it exists, + * {@link Optional#empty()} otherwise + */ + public Optional getDependentResourceByName(String name) { + if (name == null || name.isEmpty()) { + return Optional.empty(); + } + return results.keySet().stream().filter(dr -> dr.name().equals(name)).findFirst(); + } + + /** + * Retrieves the optional result of the condition with the specified type for the specified + * dependent resource. + * + * @param the expected result type of the condition + * @param dependentResourceName the dependent resource for which we want to retrieve a condition + * result + * @param conditionType the condition type which result we're interested in + * @param expectedResultType the expected result type of the condition + * @return the dependent condition result if it exists or {@link Optional#empty()} otherwise + * @throws IllegalArgumentException if a result exists but is not of the expected type + */ + public Optional getDependentConditionResult(String dependentResourceName, + Condition.Type conditionType, Class expectedResultType) { + return getDependentConditionResult( + getDependentResourceByName(dependentResourceName).orElse(null), conditionType, + expectedResultType); + } + /** * Retrieves the optional result of the condition with the specified type for the specified * dependent resource. @@ -48,6 +82,10 @@ protected Map> results() { */ public Optional getDependentConditionResult(DependentResource dependentResource, Condition.Type conditionType, Class expectedResultType) { + if (dependentResource == null) { + return Optional.empty(); + } + final var result = new Object[1]; try { return Optional.ofNullable(results().get(dependentResource)) From 0fcdd7fc87b693487a8132ef4c974202a6339b47 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 5 Jul 2024 14:56:40 +0200 Subject: [PATCH 20/23] chore(tests): add condition with result tests Signed-off-by: Chris Laprun --- .../WorkflowReconcileExecutorTest.java | 15 ++++++++++- .../operator/WorkflowAllFeatureIT.java | 26 ++++++++++++------- .../ConfigMapReconcileCondition.java | 17 +++++++----- .../WorkflowAllFeatureReconciler.java | 22 ++++++++++++---- .../WorkflowAllFeatureStatus.java | 13 +++++++++- 5 files changed, 70 insertions(+), 23 deletions(-) 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 bd1a2a1d2c..a1bf5b6ef4 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 @@ -7,6 +7,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; @@ -184,8 +185,18 @@ void onlyOneDependsOnErroredResourceNotReconciled() { @Test void simpleReconcileCondition() { + final var result = "Some error message"; + final var unmetWithResult = new ResultCondition() { + @Override + public Result detailedIsMet( + DependentResource dependentResource, + TestCustomResource primary, Context context) { + return Result.withResult(false, result); + } + }; + var workflow = new WorkflowBuilder() - .addDependentResource(dr1).withReconcilePrecondition(notMetCondition) + .addDependentResource(dr1).withReconcilePrecondition(unmetWithResult) .addDependentResource(dr2).withReconcilePrecondition(metCondition) .addDependentResource(drDeleter).withReconcilePrecondition(notMetCondition) .build(); @@ -196,6 +207,8 @@ void simpleReconcileCondition() { Assertions.assertThat(res.getErroredDependents()).isEmpty(); Assertions.assertThat(res.getReconciledDependents()).containsExactlyInAnyOrder(dr2); Assertions.assertThat(res.getNotReadyDependents()).isEmpty(); + res.getDependentConditionResult(dr1, Condition.Type.RECONCILE, String.class) + .ifPresentOrElse(s -> assertEquals(result, s), org.junit.jupiter.api.Assertions::fail); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/WorkflowAllFeatureIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/WorkflowAllFeatureIT.java index f60f2b6fde..0473c50ddb 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/WorkflowAllFeatureIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/WorkflowAllFeatureIT.java @@ -10,9 +10,11 @@ import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; import io.fabric8.kubernetes.api.model.apps.Deployment; import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension; +import io.javaoperatorsdk.operator.sample.workflowallfeature.ConfigMapReconcileCondition; import io.javaoperatorsdk.operator.sample.workflowallfeature.WorkflowAllFeatureCustomResource; import io.javaoperatorsdk.operator.sample.workflowallfeature.WorkflowAllFeatureReconciler; import io.javaoperatorsdk.operator.sample.workflowallfeature.WorkflowAllFeatureSpec; +import io.javaoperatorsdk.operator.sample.workflowallfeature.WorkflowAllFeatureStatus; import static io.javaoperatorsdk.operator.sample.workflowallfeature.ConfigMapDependentResource.READY_TO_DELETE_ANNOTATION; import static org.assertj.core.api.Assertions.assertThat; @@ -39,6 +41,8 @@ void configMapNotReconciledUntilDeploymentReady() { .isPositive(); assertThat(operator.get(Deployment.class, RESOURCE_NAME)).isNotNull(); assertThat(operator.get(ConfigMap.class, RESOURCE_NAME)).isNull(); + assertThat(getPrimaryStatus().getMsgFromCondition()) + .isEqualTo(ConfigMapReconcileCondition.NOT_RECONCILED_YET); }); await().atMost(ONE_MINUTE).untilAsserted(() -> { @@ -47,13 +51,20 @@ void configMapNotReconciledUntilDeploymentReady() { .getNumberOfReconciliationExecution()) .isGreaterThan(1); assertThat(operator.get(ConfigMap.class, RESOURCE_NAME)).isNotNull(); - assertThat(operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME) - .getStatus().getReady()).isTrue(); + final var primaryStatus = getPrimaryStatus(); + assertThat(primaryStatus.getReady()).isTrue(); + assertThat(primaryStatus.getMsgFromCondition()) + .isEqualTo(ConfigMapReconcileCondition.CREATE_SET); }); markConfigMapForDelete(); } + private WorkflowAllFeatureStatus getPrimaryStatus() { + return operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME) + .getStatus(); + } + @Test void configMapNotReconciledIfReconcileConditionNotMet() { @@ -61,8 +72,7 @@ void configMapNotReconciledIfReconcileConditionNotMet() { await().atMost(ONE_MINUTE).untilAsserted(() -> { assertThat(operator.get(ConfigMap.class, RESOURCE_NAME)).isNull(); - assertThat(operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME) - .getStatus().getReady()).isTrue(); + assertThat(getPrimaryStatus().getReady()).isTrue(); }); resource.getSpec().setCreateConfigMap(true); @@ -70,8 +80,7 @@ void configMapNotReconciledIfReconcileConditionNotMet() { await().untilAsserted(() -> { assertThat(operator.get(ConfigMap.class, RESOURCE_NAME)).isNotNull(); - assertThat(operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME) - .getStatus().getReady()).isTrue(); + assertThat(getPrimaryStatus().getReady()).isTrue(); }); } @@ -81,10 +90,9 @@ void configMapNotDeletedUntilNotMarked() { var resource = operator.create(customResource(true)); await().atMost(ONE_MINUTE).untilAsserted(() -> { - assertThat(operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME).getStatus()) + assertThat(getPrimaryStatus()) .isNotNull(); - assertThat(operator.get(WorkflowAllFeatureCustomResource.class, RESOURCE_NAME) - .getStatus().getReady()).isTrue(); + assertThat(getPrimaryStatus().getReady()).isTrue(); assertThat(operator.get(ConfigMap.class, RESOURCE_NAME)).isNotNull(); }); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java index b3d9d7a541..2222072f6c 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java @@ -3,17 +3,20 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; +import io.javaoperatorsdk.operator.processing.dependent.workflow.ResultCondition; public class ConfigMapReconcileCondition - implements Condition { + implements ResultCondition { + + public static final String CREATE_SET = "create set"; + public static final String CREATE_NOT_SET = "create not set"; + public static final String NOT_RECONCILED_YET = "Not reconciled yet"; @Override - public boolean isMet( + public Result detailedIsMet( DependentResource dependentResource, - WorkflowAllFeatureCustomResource primary, - Context context) { - - return primary.getSpec().isCreateConfigMap(); + WorkflowAllFeatureCustomResource primary, Context context) { + final var createConfigMap = primary.getSpec().isCreateConfigMap(); + return Result.withResult(createConfigMap, createConfigMap ? CREATE_SET : CREATE_NOT_SET); } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java index f5c14d6f96..61b42e0c64 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java @@ -2,8 +2,16 @@ import java.util.concurrent.atomic.AtomicInteger; -import io.javaoperatorsdk.operator.api.reconciler.*; +import io.javaoperatorsdk.operator.api.reconciler.Cleaner; +import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.DeleteControl; +import io.javaoperatorsdk.operator.api.reconciler.Reconciler; +import io.javaoperatorsdk.operator.api.reconciler.UpdateControl; +import io.javaoperatorsdk.operator.api.reconciler.Workflow; import io.javaoperatorsdk.operator.api.reconciler.dependent.Dependent; +import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.processing.dependent.workflow.Condition; import static io.javaoperatorsdk.operator.sample.workflowallfeature.WorkflowAllFeatureReconciler.DEPLOYMENT_NAME; @@ -33,11 +41,15 @@ public UpdateControl reconcile( if (resource.getStatus() == null) { resource.setStatus(new WorkflowAllFeatureStatus()); } + final var reconcileResult = context.managedWorkflowAndDependentResourceContext() + .getWorkflowReconcileResult(); + final var msgFromCondition = reconcileResult.getDependentConditionResult( + DependentResource.defaultNameFor(ConfigMapDependentResource.class), + Condition.Type.RECONCILE, String.class) + .orElse(ConfigMapReconcileCondition.NOT_RECONCILED_YET); resource.getStatus() - .setReady( - context.managedWorkflowAndDependentResourceContext() - .getWorkflowReconcileResult() - .allDependentResourcesReady()); + .withReady(reconcileResult.allDependentResourcesReady()) + .withMsgFromCondition(msgFromCondition); return UpdateControl.patchStatus(resource); } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureStatus.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureStatus.java index 11d0798fca..c53931b206 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureStatus.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureStatus.java @@ -3,13 +3,24 @@ public class WorkflowAllFeatureStatus { private Boolean ready; + private String msgFromCondition; public Boolean getReady() { return ready; } - public WorkflowAllFeatureStatus setReady(Boolean ready) { + public String getMsgFromCondition() { + return msgFromCondition; + } + + public WorkflowAllFeatureStatus withReady(Boolean ready) { this.ready = ready; return this; } + + @SuppressWarnings("UnusedReturnValue") + public WorkflowAllFeatureStatus withMsgFromCondition(String msgFromCondition) { + this.msgFromCondition = msgFromCondition; + return this; + } } From ed392be21c9953f37110390fc1ce36343796bdbc Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 8 Jul 2024 17:23:25 +0200 Subject: [PATCH 21/23] chore(docs): document ResultCondition and how to use it Signed-off-by: Chris Laprun --- docs/content/en/docs/workflows/_index.md | 17 +++++- .../dependent/workflow/DefaultResult.java | 2 +- .../dependent/workflow/ResultCondition.java | 61 +++++++++++++++++-- .../dependent/workflow/WorkflowResult.java | 2 +- .../WorkflowReconcileExecutorTest.java | 4 +- 5 files changed, 77 insertions(+), 9 deletions(-) diff --git a/docs/content/en/docs/workflows/_index.md b/docs/content/en/docs/workflows/_index.md index cbb39840b0..1e266e63af 100644 --- a/docs/content/en/docs/workflows/_index.md +++ b/docs/content/en/docs/workflows/_index.md @@ -49,11 +49,26 @@ reconciliation process. See related [integration test](https://github.com/operator-framework/java-operator-sdk/blob/ba5e33527bf9e3ea0bd33025ccb35e677f9d44b4/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDPresentActivationConditionIT.java). To have multiple resources of same type with an activation condition is a bit tricky, since you - don't want to have multiple `InformerEvetnSource` for the same type, you have to explicitly + don't want to have multiple `InformerEventSource` for the same type, you have to explicitly name the informer for the Dependent Resource (`@KubernetesDependent(informerConfig = @InformerConfig(name = "configMapInformer"))`) for all resource of same type with activation condition. This will make sure that only one is registered. See details at [low level api](https://github.com/operator-framework/java-operator-sdk/blob/main/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceRetriever.java#L20-L52). +### Result conditions + +While simple conditions are usually enough, it might happen you want to convey extra information as a result of the +evaluation of the conditions (e.g., to report error messages or because the result of the condition evaluation might be +interesting for other purposes). In this situation, you should implement `ResultCondition` instead of `Condition` and +provide an implementation of the `detailedIsMet` method, which allows you to return a more detailed `Result` object via +which you can provide extra information. The `ResultCondition.Result` interface provides factory method for your +convenience but you can also provide your own implementation if required. + +You can access the results for conditions from the `WorkflowResult` instance that is returned whenever a workflow is +evaluated. You can access that result from the `ManagedWorkflowAndDependentResourceContext` accessible from the +reconciliation `Context`. You can then access individual condition results using the ` +getDependentConditionResult` methods. You can see an example of this +in [this integration test](https://github.com/operator-framework/java-operator-sdk/blob/fd0e92c0de55c47d5df50658cf4e147ee5e6102d/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/WorkflowAllFeatureReconciler.java#L44-L49). + ## Defining Workflows Similarly to dependent resources, there are two ways to define workflows, in managed and standalone diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java index b5ba99f82b..f6c6df6291 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java @@ -10,7 +10,7 @@ public DefaultResult(boolean success, T result) { } @Override - public T getResult() { + public T getDetail() { return result; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java index ebc23735a1..a907cf9f5f 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java @@ -4,7 +4,27 @@ import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +/** + * A condition that can return extra information in addition of whether it is met or not. + * + * @param the resource type this condition applies to + * @param

the primary resource type associated with the dependent workflow this condition is + * part of + * @param the type of the extra information returned by the condition + */ public interface ResultCondition extends Condition { + + /** + * Checks whether a condition holds true for the specified {@link DependentResource}, returning + * additional information as needed. + * + * @param dependentResource the {@link DependentResource} for which we want to check the condition + * @param primary the primary resource being considered + * @param context the current reconciliation {@link Context} + * @return a {@link Result} instance containing the result of the evaluation of the condition as + * well as additional detail + * @see Condition#isMet(DependentResource, HasMetadata, Context) + */ Result detailedIsMet(DependentResource dependentResource, P primary, Context

context); @Override @@ -12,26 +32,59 @@ default boolean isMet(DependentResource dependentResource, P primary, Cont return detailedIsMet(dependentResource, primary, context).isSuccess(); } + /** + * Holds a more detailed {@link Condition} result. + * + * @param the type of the extra information provided in condition evaluation + */ @SuppressWarnings({"rawtypes", "unchecked"}) interface Result { + /** + * A result expressing a condition has been met without extra information + */ ResultCondition.Result metWithoutResult = new DefaultResult(true, null); + /** + * A result expressing a condition has not been met without extra information + */ ResultCondition.Result unmetWithoutResult = new DefaultResult(false, null); + /** + * Creates a {@link Result} without extra information + * + * @param success whether or not the condition has been met + * @return a {@link Result} without extra information + */ static Result withoutResult(boolean success) { return success ? metWithoutResult : unmetWithoutResult; } - static Result withResult(boolean success, T result) { - return new DefaultResult<>(success, result); + /** + * Creates a {@link Result} with the specified condition evaluation result and extra information + * + * @param success whether or not the condition has been met + * @param detail the extra information that the condition provided during its evaluation + * @return a {@link Result} with the specified condition evaluation result and extra information + * @param the type of the extra information provided by the condition + */ + static Result withResult(boolean success, T detail) { + return new DefaultResult<>(success, detail); } default String asString() { - return "Result: " + getResult() + " met: " + isSuccess(); + return "Detail: " + getDetail() + " met: " + isSuccess(); } - T getResult(); + /** + * The extra information provided by the associated {@link ResultCondition} during its evaluation + * @return extra information provided by the associated {@link ResultCondition} during its evaluation or {@code null} if none was provided + */ + T getDetail(); + /** + * Whether the associated condition held true + * @return {@code true} if the associated condition was met, {@code false} otherwise + */ boolean isSuccess(); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 461307febe..12b54285de 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -90,7 +90,7 @@ public Optional getDependentConditionResult(DependentResource dependentRe try { return Optional.ofNullable(results().get(dependentResource)) .flatMap(detail -> detail.getResultForConditionWithType(conditionType)) - .map(r -> result[0] = r.getResult()) + .map(r -> result[0] = r.getDetail()) .map(expectedResultType::cast); } catch (Exception e) { throw new IllegalArgumentException("Condition " + 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 a1bf5b6ef4..1c0b16b90a 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 @@ -600,7 +600,7 @@ public Result detailedIsMet(DependentResource depen HasMetadata primary, Context context) { return new Result<>() { @Override - public Object getResult() { + public Object getDetail() { return result; } @@ -630,7 +630,7 @@ public Result detailedIsMet(DependentResource depen HasMetadata primary, Context context) { return new Result<>() { @Override - public Object getResult() { + public Object getDetail() { return result; } From 7d8181258823c31f1368eff98117b0c5c745adc3 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 8 Jul 2024 17:40:57 +0200 Subject: [PATCH 22/23] refactor: rename ResultCondition to DetailedCondition Signed-off-by: Chris Laprun --- docs/content/en/docs/workflows/_index.md | 4 ++-- .../workflow/AbstractWorkflowExecutor.java | 4 ++-- .../dependent/workflow/ConditionWithType.java | 6 ++--- .../dependent/workflow/DefaultResult.java | 2 +- ...tCondition.java => DetailedCondition.java} | 14 +++++++----- .../dependent/workflow/WorkflowResult.java | 22 +++++++++---------- .../WorkflowReconcileExecutorTest.java | 6 ++--- .../ConfigMapReconcileCondition.java | 4 ++-- 8 files changed, 33 insertions(+), 29 deletions(-) rename operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/{ResultCondition.java => DetailedCondition.java} (87%) diff --git a/docs/content/en/docs/workflows/_index.md b/docs/content/en/docs/workflows/_index.md index 1e266e63af..4b61c6f1b0 100644 --- a/docs/content/en/docs/workflows/_index.md +++ b/docs/content/en/docs/workflows/_index.md @@ -58,9 +58,9 @@ reconciliation process. While simple conditions are usually enough, it might happen you want to convey extra information as a result of the evaluation of the conditions (e.g., to report error messages or because the result of the condition evaluation might be -interesting for other purposes). In this situation, you should implement `ResultCondition` instead of `Condition` and +interesting for other purposes). In this situation, you should implement `DetailedCondition` instead of `Condition` and provide an implementation of the `detailedIsMet` method, which allows you to return a more detailed `Result` object via -which you can provide extra information. The `ResultCondition.Result` interface provides factory method for your +which you can provide extra information. The `DetailedCondition.Result` interface provides factory method for your convenience but you can also provide your own implementation if required. You can access the results for conditions from the `WorkflowResult` instance that is returned whenever a workflow is 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 0e89c62df8..32c0bddbc0 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 @@ -138,11 +138,11 @@ protected boolean isConditionMet( DependentResourceNode dependentResource) { final var dr = dependentResource.getDependentResource(); return condition.map(c -> { - final ResultCondition.Result r = c.detailedIsMet(dr, primary, context); + final DetailedCondition.Result r = c.detailedIsMet(dr, primary, context); results.computeIfAbsent(dependentResource, unused -> new WorkflowResult.DetailBuilder()) .withResultForCondition(c, r); return r; - }).orElse(ResultCondition.Result.metWithoutResult).isSuccess(); + }).orElse(DetailedCondition.Result.metWithoutResult).isSuccess(); } protected void submit(DependentResourceNode dependentResourceNode, diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java index 513697ddcc..de95830a92 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ConditionWithType.java @@ -4,7 +4,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -class ConditionWithType implements ResultCondition { +class ConditionWithType implements DetailedCondition { private final Condition condition; private final Type type; @@ -21,8 +21,8 @@ public Type type() { @Override public Result detailedIsMet(DependentResource dependentResource, P primary, Context

context) { - if (condition instanceof ResultCondition resultCondition) { - return resultCondition.detailedIsMet(dependentResource, primary, context); + if (condition instanceof DetailedCondition detailedCondition) { + return detailedCondition.detailedIsMet(dependentResource, primary, context); } else { return Result .withoutResult(condition.isMet(dependentResource, primary, context)); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java index f6c6df6291..cc63f637cf 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultResult.java @@ -1,6 +1,6 @@ package io.javaoperatorsdk.operator.processing.dependent.workflow; -public class DefaultResult implements ResultCondition.Result { +public class DefaultResult implements DetailedCondition.Result { private final T result; private final boolean success; diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DetailedCondition.java similarity index 87% rename from operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java rename to operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DetailedCondition.java index a907cf9f5f..200743cc0e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/ResultCondition.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DetailedCondition.java @@ -12,7 +12,7 @@ * part of * @param the type of the extra information returned by the condition */ -public interface ResultCondition extends Condition { +public interface DetailedCondition extends Condition { /** * Checks whether a condition holds true for the specified {@link DependentResource}, returning @@ -42,12 +42,12 @@ interface Result { /** * A result expressing a condition has been met without extra information */ - ResultCondition.Result metWithoutResult = new DefaultResult(true, null); + Result metWithoutResult = new DefaultResult(true, null); /** * A result expressing a condition has not been met without extra information */ - ResultCondition.Result unmetWithoutResult = new DefaultResult(false, null); + Result unmetWithoutResult = new DefaultResult(false, null); /** * Creates a {@link Result} without extra information @@ -76,13 +76,17 @@ default String asString() { } /** - * The extra information provided by the associated {@link ResultCondition} during its evaluation - * @return extra information provided by the associated {@link ResultCondition} during its evaluation or {@code null} if none was provided + * The extra information provided by the associated {@link DetailedCondition} during its + * evaluation + * + * @return extra information provided by the associated {@link DetailedCondition} during its + * evaluation or {@code null} if none was provided */ T getDetail(); /** * Whether the associated condition held true + * * @return {@code true} if the associated condition was met, {@code false} otherwise */ boolean isSuccess(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java index 12b54285de..900022444d 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowResult.java @@ -127,10 +127,10 @@ public void throwAggregateExceptionIfErrorsPresent() { static class DetailBuilder { private Exception error; private ReconcileResult reconcileResult; - private ResultCondition.Result activationConditionResult; - private ResultCondition.Result deletePostconditionResult; - private ResultCondition.Result readyPostconditionResult; - private ResultCondition.Result reconcilePostconditionResult; + private DetailedCondition.Result activationConditionResult; + private DetailedCondition.Result deletePostconditionResult; + private DetailedCondition.Result readyPostconditionResult; + private DetailedCondition.Result reconcilePostconditionResult; private boolean deleted; private boolean visited; private boolean markedForDelete; @@ -143,7 +143,7 @@ Detail build() { DetailBuilder withResultForCondition( ConditionWithType conditionWithType, - ResultCondition.Result conditionResult) { + DetailedCondition.Result conditionResult) { switch (conditionWithType.type()) { case ACTIVATION -> activationConditionResult = conditionResult; case DELETE -> deletePostconditionResult = conditionResult; @@ -203,18 +203,18 @@ DetailBuilder markForDelete() { record Detail(Exception error, ReconcileResult reconcileResult, - ResultCondition.Result activationConditionResult, - ResultCondition.Result deletePostconditionResult, - ResultCondition.Result readyPostconditionResult, - ResultCondition.Result reconcilePostconditionResult, + DetailedCondition.Result activationConditionResult, + DetailedCondition.Result deletePostconditionResult, + DetailedCondition.Result readyPostconditionResult, + DetailedCondition.Result reconcilePostconditionResult, boolean deleted, boolean visited, boolean markedForDelete) { boolean isConditionWithTypeMet(Condition.Type conditionType) { - return getResultForConditionWithType(conditionType).map(ResultCondition.Result::isSuccess) + return getResultForConditionWithType(conditionType).map(DetailedCondition.Result::isSuccess) .orElse(true); } - Optional> getResultForConditionWithType( + Optional> getResultForConditionWithType( Condition.Type conditionType) { return switch (conditionType) { case ACTIVATION -> Optional.ofNullable(activationConditionResult); 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 1c0b16b90a..dce39f2050 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 @@ -186,7 +186,7 @@ void onlyOneDependsOnErroredResourceNotReconciled() { @Test void simpleReconcileCondition() { final var result = "Some error message"; - final var unmetWithResult = new ResultCondition() { + final var unmetWithResult = new DetailedCondition() { @Override public Result detailedIsMet( DependentResource dependentResource, @@ -594,7 +594,7 @@ void activationConditionOnlyCalledOnceOnDeleteDependents() { @Test void resultFromReadyConditionShouldBeAvailableIfExisting() { final var result = Integer.valueOf(42); - final var resultCondition = new ResultCondition<>() { + final var resultCondition = new DetailedCondition<>() { @Override public Result detailedIsMet(DependentResource dependentResource, HasMetadata primary, Context context) { @@ -624,7 +624,7 @@ public boolean isSuccess() { @Test void shouldThrowIllegalArgumentExceptionIfTypesDoNotMatch() { final var result = "FOO"; - final var resultCondition = new ResultCondition<>() { + final var resultCondition = new DetailedCondition<>() { @Override public Result detailedIsMet(DependentResource dependentResource, HasMetadata primary, Context context) { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java index 2222072f6c..dfdc0ad8a4 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/workflowallfeature/ConfigMapReconcileCondition.java @@ -3,10 +3,10 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; -import io.javaoperatorsdk.operator.processing.dependent.workflow.ResultCondition; +import io.javaoperatorsdk.operator.processing.dependent.workflow.DetailedCondition; public class ConfigMapReconcileCondition - implements ResultCondition { + implements DetailedCondition { public static final String CREATE_SET = "create set"; public static final String CREATE_NOT_SET = "create not set"; From 047c66e8db63261e8debc5e8ebb52cba2e6704bc Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 8 Jul 2024 18:15:43 +0200 Subject: [PATCH 23/23] fix: adjust after rebase Signed-off-by: Chris Laprun --- .../operator/config/BaseConfigurationServiceTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java index 72fb90f9bd..127282daf2 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/BaseConfigurationServiceTest.java @@ -19,6 +19,7 @@ import io.javaoperatorsdk.operator.api.config.dependent.ConfigurationConverter; import io.javaoperatorsdk.operator.api.config.dependent.Configured; import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec; +import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.MaxReconciliationInterval;