This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit 46267ef0d35c9be4dc78c69013201af65a97a29f Author: Alex Heneveld <[email protected]> AuthorDate: Sat Jun 17 15:32:53 2023 +0100 fix recording of errors in step in subworkflow in some cases --- .../core/workflow/WorkflowExecutionContext.java | 38 ++++++----- .../WorkflowStepInstanceExecutionContext.java | 3 +- .../workflow/WorkflowPersistReplayErrorsTest.java | 74 ++++++++++++++++++++++ .../brooklyn/core/workflow/WorkflowRetryTest.java | 1 + 4 files changed, 100 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java index 26fc66df27..ec05fcb380 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java @@ -83,7 +83,6 @@ import static org.apache.brooklyn.core.workflow.WorkflowReplayUtils.ReplayResume @JsonInclude(JsonInclude.Include.NON_NULL) @JsonDeserialize(converter = WorkflowExecutionContext.Converter.class) public class WorkflowExecutionContext { - private static final Logger log = LoggerFactory.getLogger(WorkflowExecutionContext.class); public static final String LABEL_FOR_ERROR_HANDLER = "error-handler"; @@ -213,7 +212,7 @@ public class WorkflowExecutionContext { public static class OldStepRecord { /** count of runs started */ int countStarted = 0; - /** count of runs completed */ + /** count of runs completed (without error) */ int countCompleted = 0; /** context for last _completed_ instance of step */ @@ -1102,6 +1101,9 @@ public class WorkflowExecutionContext { if (unhandledError != null) { return () -> endWithError(unhandledError.getLeft(), unhandledError.getRight()); } + if (!continueOnErrorHandledOrNextReplay) { + updateOnSuccessfulCompletion(); + } } catch (Throwable e2) { // do not propagateIfFatal, we need to handle most throwables log.debug("Uncaught error in workflow exception handler: "+ e2, e2); @@ -1500,20 +1502,26 @@ public class WorkflowExecutionContext { onFinish.accept(step.output, null); } catch (Exception e) { - handleErrorAtStep(step, t, onFinish, e); - } - - oldStepInfo.compute(currentStepIndex, (index, old) -> { - if (old==null) { - log.warn("Lost old step info for "+this+", step "+index); - old = new OldStepRecord(); + try { + handleErrorAtStep(step, t, onFinish, e); + } catch (Exception e2) { + currentStepInstance.error = e2; + throw e2; } - old.countCompleted++; - // okay if this gets picked up by accident because we will check the stepIndex it records against the currentStepIndex, - // and ignore it if different - old.context = currentStepInstance; - return old; - }); + } finally { + // do this whether or not error + oldStepInfo.compute(currentStepIndex, (index, old) -> { + if (old == null) { + log.warn("Lost old step info for " + this + ", step " + index); + old = new OldStepRecord(); + } + if (currentStepInstance.error==null) old.countCompleted++; + // okay if this gets picked up by accident because we will check the stepIndex it records against the currentStepIndex, + // and ignore it if different + old.context = currentStepInstance; + return old; + }); + } previousStepTaskId = currentStepInstance.taskId; previousStepIndex = currentStepIndex; diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepInstanceExecutionContext.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepInstanceExecutionContext.java index 4f80f5da71..94c2da19ee 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepInstanceExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepInstanceExecutionContext.java @@ -68,7 +68,8 @@ public class WorkflowStepInstanceExecutionContext { // replay instructions or a string explicit next step identifier public Object next; - /** set if the step is in an error handler context, containing the error being handled */ + /** set if the step is in an error handler context, containing the error being handled, + * or if the step had an error that was not handled */ Throwable error; /** set if there was an error handled locally */ diff --git a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowPersistReplayErrorsTest.java b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowPersistReplayErrorsTest.java index d95b053402..8afc16f79d 100644 --- a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowPersistReplayErrorsTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowPersistReplayErrorsTest.java @@ -19,6 +19,7 @@ package org.apache.brooklyn.core.workflow; import com.google.common.base.Stopwatch; +import com.google.common.collect.Iterables; import org.apache.brooklyn.api.entity.EntityLocal; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.mgmt.ManagementContext; @@ -56,6 +57,7 @@ import org.testng.annotations.Test; import java.io.IOException; import java.io.Serializable; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Objects; @@ -1041,4 +1043,76 @@ public class WorkflowPersistReplayErrorsTest extends RebindTestFixture<BasicAppl // and worst case they can be manually deleted) } + @Test + public void testErrorInSubWorkflowCaughtUpdatesContextAndStep() throws Exception { + app = mgmt().getEntityManager().createEntity(EntitySpec.create(BasicApplication.class)); + WorkflowExecutionContext run = WorkflowBasicTest.runWorkflow(app, Strings.lines( + "steps:", + "- log 1", + "- type: workflow", + " steps:", + " - log 2-1", + " - step: fail message 2-2", + " on-error:", + " - log 2-2-error", + " - fail message 2-2-done", + " - log 2-3", + " on-error: ", + " - return 2-done", + "- log 3" + ), "test-error-in-subworkflow"); + Asserts.assertEquals(run.getTask(true).get().getUnchecked(), "2-done"); + + WorkflowExecutionContext.OldStepRecord step2 = run.oldStepInfo.get(1); + Asserts.assertNotNull(step2); + Asserts.assertNotNull(step2.context); + Asserts.assertNull(step2.context.error); // should be null because handled + Asserts.assertNull(step2.context.errorHandlerTaskId); // should be null because not treated as a step handler, but handler for the workflow - step2sub.errorHandlerTaskId + + BrooklynTaskTags.WorkflowTaskTag step2subTag = Iterables.getOnlyElement(step2.context.getSubWorkflows()); + + WorkflowExecutionContext step2sub = new WorkflowStatePersistenceViaSensors(mgmt()).getWorkflows(app).get(step2subTag.getWorkflowId()); + Asserts.assertEquals(step2sub.getStatus(), WorkflowExecutionContext.WorkflowStatus.SUCCESS); + Asserts.assertNotNull(step2sub.errorHandlerTaskId); + + WorkflowExecutionContext.OldStepRecord step22 = step2sub.oldStepInfo.get(1); + Asserts.assertNotNull(step22); + + Asserts.assertNotNull(step22); + Asserts.assertNotNull(step22.context); + Asserts.assertNotNull(step22.context.error); // not null because not handled here + Asserts.assertNotNull(step22.context.errorHandlerTaskId); + } + + @Test + public void testErrorInSubWorkflowUncaughtUpdatesContextAndStep() throws Exception { + app = mgmt().getEntityManager().createEntity(EntitySpec.create(BasicApplication.class)); + WorkflowExecutionContext run = WorkflowBasicTest.runWorkflow(app, Strings.lines( + "steps:", + "- log 1", + "- type: workflow", + " steps:", + " - log 2-1", + " - step: fail message 2-2", + " on-error:", + " - log 2-2-error", + " - fail message 2-2-done", + " - log 2-3", + "- log 3" + ), "test-error-in-subworkflow"); + run.getTask(true).get().blockUntilEnded(); + Asserts.assertEquals(run.getStatus(), WorkflowExecutionContext.WorkflowStatus.ERROR); + + WorkflowExecutionContext.OldStepRecord step2 = run.oldStepInfo.get(1); + BrooklynTaskTags.WorkflowTaskTag step2subTag = Iterables.getOnlyElement(step2.context.getSubWorkflows()); + + WorkflowExecutionContext step2sub = new WorkflowStatePersistenceViaSensors(mgmt()).getWorkflows(app).get(step2subTag.getWorkflowId()); + Asserts.assertEquals(step2sub.getStatus(), WorkflowExecutionContext.WorkflowStatus.ERROR); + + WorkflowExecutionContext.OldStepRecord step22 = step2sub.oldStepInfo.get(1); + Asserts.assertNotNull(step22); + Asserts.assertNotNull(step22.context); + Asserts.assertNotNull(step22.context.error); + Asserts.assertNotNull(step22.context.errorHandlerTaskId); + } } diff --git a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowRetryTest.java b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowRetryTest.java index ef3b500f36..03686deb82 100644 --- a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowRetryTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowRetryTest.java @@ -337,6 +337,7 @@ public class WorkflowRetryTest extends RebindTestFixture<BasicApplication> { while (lastInvocation==null) Time.sleep(Duration.millis(10)); EntityAsserts.assertAttributeEventually(app, Sensors.newIntegerSensor("count"), v -> v!=null && v > 1); Asserts.assertFalse(lastInvocation.isDone()); + log.info("setting sensor so workflow can proceed"); app.sensors().set(Sensors.newIntegerSensor("no_count"), -1); lastInvocation.getUnchecked(Duration.ONE_SECOND); EntityAsserts.assertAttributeEquals(app, Sensors.newIntegerSensor("no_count"), 0);
