fix: managed workflow result is available even when exception is thro…

…wn (#2552) Fixes #2551 Signed-off-by: Chris Laprun <[email protected]>
operator-framework · Oct 28, 2024 · 73e3dfc · 73e3dfc
1 parent 57c4482
commit 73e3dfc
Show file tree

Hide file tree

Showing 6 changed files with 20 additions and 35 deletions.
diff --git a/...sdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java b/...sdk/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> T getMandatory(Object key, Class<T> 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<WorkflowReconcileResult> getWorkflowReconcileResult() {
-    return Optional.ofNullable(workflowReconcileResult);
+    return get(RECONCILE_RESULT_KEY, WorkflowReconcileResult.class);
   }
 
   @Override
   public Optional<WorkflowCleanupResult> 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
@@ -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<String, Object> metadata() {
           public UpdateControl<P> 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<P>) reconciler).cleanup(resource, context);

diff --git a/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/DefaultWorkflow.java b/.../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<String, DependentResourceNode> toMap(Set<DependentResourceNode> 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<P> context) {
     WorkflowReconcileExecutor<P> 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<P> context) {
     WorkflowCleanupExecutor<P> 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/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java b/.../main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowBuilder.java
@@ -14,8 +14,7 @@
 @SuppressWarnings({"rawtypes", "unchecked"})
 public class WorkflowBuilder<P extends HasMetadata> {
 
-  private final Map<String, DependentResourceNode<?, P>> dependentResourceNodes =
-      new HashMap<>();
+  private final Map<String, DependentResourceNode<?, P>> dependentResourceNodes = new HashMap<>();
   private boolean throwExceptionAutomatically = THROW_EXCEPTION_AUTOMATICALLY_DEFAULT;
   private DependentResourceNode currentNode;
   private boolean isCleaner = false;

diff --git a/...o/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutorTest.java b/...o/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<TestCustomResource> mockContext = mock(Context.class);
+  Context<TestCustomResource> 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/...javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java b/...javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutorTest.java
@@ -9,25 +9,26 @@
 
 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;
 
 import static io.javaoperatorsdk.operator.processing.dependent.workflow.ExecutionAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.*;
 
-@SuppressWarnings("rawtypes")
 class WorkflowReconcileExecutorTest extends AbstractWorkflowExecutorTest {
 
   @SuppressWarnings("unchecked")
-  Context<TestCustomResource> mockContext = mock(Context.class);
+  Context<TestCustomResource> mockContext = spy(Context.class);
   ExecutorService executorService = Executors.newCachedThreadPool();
 
   TestDependent dr3 = new TestDependent("DR_3");
   TestDependent dr4 = new TestDependent("DR_4");
 
   @BeforeEach
   void setup() {
+    when(mockContext.managedDependentResourceContext()).thenReturn(mock(ManagedDependentResourceContext.class));
     when(mockContext.getWorkflowExecutorService()).thenReturn(executorService);
     when(mockContext.eventSourceRetriever()).thenReturn(mock(EventSourceRetriever.class));
   }