From a70ff789d1fba09a5db744380b417702595f5d3b Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Mon, 14 Oct 2024 17:33:39 +0200 Subject: [PATCH] fix: managed workflow result is available even when exception is thrown Fixes #2551 Signed-off-by: Chris Laprun --- ...efaultManagedDependentResourceContext.java | 21 ++++--------------- .../operator/processing/Controller.java | 9 +------- .../dependent/workflow/DefaultWorkflow.java | 7 ++++++- .../dependent/workflow/WorkflowBuilder.java | 3 +-- .../workflow/WorkflowCleanupExecutorTest.java | 10 ++++----- .../WorkflowReconcileExecutorTest.java | 5 +++-- 6 files changed, 20 insertions(+), 35 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java index 5b1a21e5dd..61414468c8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java @@ -8,9 +8,8 @@ @SuppressWarnings("rawtypes") public class DefaultManagedDependentResourceContext implements ManagedDependentResourceContext { - - private WorkflowReconcileResult workflowReconcileResult; - private WorkflowCleanupResult workflowCleanupResult; + public static final Object RECONCILE_RESULT_KEY = new Object(); + public static final Object CLEANUP_RESULT_KEY = new Object(); private final ConcurrentHashMap attributes = new ConcurrentHashMap(); @Override @@ -37,25 +36,13 @@ public T getMandatory(Object key, Class expectedType) { + ") is missing or not of the expected type")); } - public DefaultManagedDependentResourceContext setWorkflowExecutionResult( - WorkflowReconcileResult workflowReconcileResult) { - this.workflowReconcileResult = workflowReconcileResult; - return this; - } - - public DefaultManagedDependentResourceContext setWorkflowCleanupResult( - WorkflowCleanupResult workflowCleanupResult) { - this.workflowCleanupResult = workflowCleanupResult; - return this; - } - @Override public Optional getWorkflowReconcileResult() { - return Optional.ofNullable(workflowReconcileResult); + return get(RECONCILE_RESULT_KEY, WorkflowReconcileResult.class); } @Override public Optional getWorkflowCleanupResult() { - return Optional.ofNullable(workflowCleanupResult); + return get(CLEANUP_RESULT_KEY, WorkflowCleanupResult.class); } } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java index bc2d3f9eec..93f10d3e21 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java @@ -38,7 +38,6 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceNotFoundException; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceProvider; import io.javaoperatorsdk.operator.api.reconciler.dependent.EventSourceReferencer; -import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext; import io.javaoperatorsdk.operator.health.ControllerHealthInfo; import io.javaoperatorsdk.operator.processing.dependent.workflow.Workflow; import io.javaoperatorsdk.operator.processing.dependent.workflow.WorkflowCleanupResult; @@ -145,10 +144,7 @@ public Map metadata() { public UpdateControl

execute() throws Exception { initContextIfNeeded(resource, context); if (!managedWorkflow.isEmpty()) { - var res = managedWorkflow.reconcile(resource, context); - ((DefaultManagedDependentResourceContext) context.managedDependentResourceContext()) - .setWorkflowExecutionResult(res); - res.throwAggregateExceptionIfErrorsPresent(); + managedWorkflow.reconcile(resource, context); } return reconciler.reconcile(resource, context); } @@ -191,9 +187,6 @@ public DeleteControl execute() { WorkflowCleanupResult workflowCleanupResult = null; if (managedWorkflow.hasCleaner()) { workflowCleanupResult = managedWorkflow.cleanup(resource, context); - ((DefaultManagedDependentResourceContext) context.managedDependentResourceContext()) - .setWorkflowCleanupResult(workflowCleanupResult); - workflowCleanupResult.throwAggregateExceptionIfErrorsPresent(); } if (isCleaner) { var cleanupResult = ((Cleaner

) reconciler).cleanup(resource, context); 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 d31f42419d..056b5906a0 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 @@ -12,6 +12,7 @@ import io.javaoperatorsdk.operator.api.reconciler.dependent.Deleter; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; +import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.DefaultManagedDependentResourceContext; import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResource; /** @@ -79,7 +80,7 @@ private Map toMap(Set node } map.put(node.getName(), node); } - if (topLevelResources.size() == 0) { + if (topLevelResources.isEmpty()) { throw new IllegalStateException( "No top-level dependent resources found. This might indicate a cyclic Set of DependentResourceNode has been provided."); } @@ -91,6 +92,8 @@ public WorkflowReconcileResult reconcile(P primary, Context

context) { WorkflowReconcileExecutor

workflowReconcileExecutor = new WorkflowReconcileExecutor<>(this, primary, context); var result = workflowReconcileExecutor.reconcile(); + context.managedDependentResourceContext() + .put(DefaultManagedDependentResourceContext.RECONCILE_RESULT_KEY, result); if (throwExceptionAutomatically) { result.throwAggregateExceptionIfErrorsPresent(); } @@ -102,6 +105,8 @@ public WorkflowCleanupResult cleanup(P primary, Context

context) { WorkflowCleanupExecutor

workflowCleanupExecutor = new WorkflowCleanupExecutor<>(this, primary, context); var result = workflowCleanupExecutor.cleanup(); + context.managedDependentResourceContext() + .put(DefaultManagedDependentResourceContext.CLEANUP_RESULT_KEY, result); if (throwExceptionAutomatically) { result.throwAggregateExceptionIfErrorsPresent(); } 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 d65e21659c..e416de0260 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 @@ -14,8 +14,7 @@ @SuppressWarnings({"rawtypes", "unchecked"}) public class WorkflowBuilder

{ - private final Map> dependentResourceNodes = - new HashMap<>(); + private final Map> dependentResourceNodes = new HashMap<>(); private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT; private DependentResourceNode currentNode; private boolean isCleaner = false; diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java index ed6ba5f80c..461a043862 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java @@ -14,13 +14,13 @@ import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext; +import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedDependentResourceContext; 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.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest { @@ -29,8 +29,7 @@ class WorkflowCleanupExecutorTest extends AbstractWorkflowExecutorTest { protected TestDeleterDependent dd3 = new TestDeleterDependent("DR_DELETER_3"); protected TestDeleterDependent dd4 = new TestDeleterDependent("DR_DELETER_4"); @SuppressWarnings("unchecked") - - Context mockContext = mock(Context.class); + Context mockContext = spy(Context.class); ExecutorService executorService = Executors.newCachedThreadPool(); @BeforeEach @@ -45,7 +44,8 @@ void setup() { when(eventSourceContextMock.getControllerConfiguration()).thenReturn(mockControllerConfig); when(mockControllerConfig.getConfigurationService()) .thenReturn(mock(ConfigurationService.class)); - + when(mockContext.managedDependentResourceContext()) + .thenReturn(mock(ManagedDependentResourceContext.class)); when(mockContext.getWorkflowExecutorService()).thenReturn(executorService); when(mockContext.eventSourceRetriever()).thenReturn(eventSourceRetrieverMock); } 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 099d5fad2b..d90e8d6d97 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 @@ -9,6 +9,7 @@ import io.javaoperatorsdk.operator.AggregatedOperatorException; import io.javaoperatorsdk.operator.api.reconciler.Context; +import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ManagedDependentResourceContext; import io.javaoperatorsdk.operator.processing.event.EventSourceRetriever; import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; @@ -16,11 +17,10 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.*; -@SuppressWarnings("rawtypes") class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest { @SuppressWarnings("unchecked") - Context mockContext = mock(Context.class); + Context mockContext = spy(Context.class); ExecutorService executorService = Executors.newCachedThreadPool(); TestDependent dr3 = new TestDependent("DR_3"); @@ -28,6 +28,7 @@ class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest { @BeforeEach void setup() { + when(mockContext.managedDependentResourceContext()).thenReturn(mock(ManagedDependentResourceContext.class)); when(mockContext.getWorkflowExecutorService()).thenReturn(executorService); when(mockContext.eventSourceRetriever()).thenReturn(mock(EventSourceRetriever.class)); }