You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2022/10/23 16:54:33 UTC
[brooklyn-server] branch master updated: fix some error handling edge cases
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
The following commit(s) were added to refs/heads/master by this push:
new e535976362 fix some error handling edge cases
e535976362 is described below
commit e5359763623a660b3591b254adff5825cbabfff4
Author: Alex Heneveld <al...@cloudsoft.io>
AuthorDate: Sun Oct 23 17:54:20 2022 +0100
fix some error handling edge cases
---
.../brooklyn/camp/brooklyn/WorkflowYamlTest.java | 44 ++++++++++++++++++++++
.../core/workflow/WorkflowErrorHandling.java | 4 +-
.../core/workflow/WorkflowExecutionContext.java | 21 +++++++----
.../core/workflow/WorkflowReplayUtils.java | 4 +-
.../core/workflow/WorkflowStepDefinition.java | 7 +++-
.../core/workflow/WorkflowStepResolution.java | 8 +++-
.../workflow/WorkflowPersistReplayErrorsTest.java | 18 +++++++++
7 files changed, 93 insertions(+), 13 deletions(-)
diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java
index 728283f24a..a77910a9f3 100644
--- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java
+++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java
@@ -540,4 +540,48 @@ public class WorkflowYamlTest extends AbstractYamlTest {
EntityAsserts.assertAttributeEqualsEventually(child, Attributes.SERVICE_UP, false);
EntityAsserts.assertAttributeEqualsEventually(child, Attributes.SERVICE_STATE_ACTUAL, Lifecycle.STOPPED);
}
+
+ @Test
+ public void testConditionNormal() throws Exception {
+ Asserts.assertEquals(doTestCondition("regex: .*oh no.*"), "expected failure");
+ }
+ @Test
+ public void testConditionBadSerialization() throws Exception {
+ Asserts.assertFailsWith(() -> doTestCondition("- regex: .*oh no.*"),
+ e -> Asserts.expectedFailureContainsIgnoreCase(e, "unresolveable", "regex"));
+ }
+ @Test
+ public void testConditionBadExpression() throws Exception {
+ // TODO would be nice if it could silently ignore this condition
+ // TODO also handle multi-line errors (eg from freemarker)
+ Asserts.assertFailsWith(() -> doTestCondition(Strings.lines(
+ "any:",
+ "- regex: .*oh no.*",
+ "- target: ${bad_var}",
+ " when: absent")),
+ e -> Asserts.expectedFailureContainsIgnoreCase(e, "unresolveable", "bad_var"));
+ }
+
+ Object doTestCondition(String lines) throws Exception {
+ Entity app = createAndStartApplication(
+ "services:",
+ "- type: " + BasicEntity.class.getName(),
+ " brooklyn.initializers:",
+ " - type: workflow-effector",
+ " brooklyn.config:",
+ " name: myWorkflow",
+ " steps:",
+ " - step: fail message oh no",
+ " on-error:",
+ " - step: return expected failure",
+ " condition:",
+ Strings.indent(14, lines));
+ waitForApplicationTasks(app);
+ Entity entity = Iterables.getOnlyElement(app.getChildren());
+ Effector<?> effector = entity.getEntityType().getEffectorByName("myWorkflow").get();
+
+ Task<?> invocation = entity.invoke(effector, null);
+ return invocation.getUnchecked();
+ }
+
}
diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowErrorHandling.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowErrorHandling.java
index 532ab13ee4..3ab02f17fe 100644
--- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowErrorHandling.java
+++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowErrorHandling.java
@@ -134,8 +134,8 @@ public class WorkflowErrorHandling implements Callable<WorkflowErrorHandling.Wor
}
// if none apply
- log.debug("No error handler options applied at "+handlerParent.get()+"; will rethrow error");
- return null;
+ log.debug("No error handler options applied at "+handlerParent.getId()+"; will rethrow error");
+ throw Exceptions.propagate(error);
}
@JsonInclude(JsonInclude.Include.NON_NULL)
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 2d2eb1688e..849b980a90 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
@@ -841,14 +841,17 @@ public class WorkflowExecutionContext {
continueOnErrorHandledOrNextReplay = true;
continue RecoveryAndReplay;
}
- } else {
- log.debug("Handler not applicable for error in workflow around step " + workflowStepReference(currentStepIndex) + ", rethrowing: " + Exceptions.collapseText(e));
}
} catch (Exception e2) {
- log.warn("Error in workflow '" + getName() + "' around step " + workflowStepReference(currentStepIndex) + " error handler for -- " + Exceptions.collapseText(e) + " -- threw another error (rethrowing): " + Exceptions.collapseText(e2));
- log.debug("Full trace of original error was: " + e, e);
- e = e2;
+ Throwable e0 = e;
+ if (Exceptions.getCausalChain(e2).stream().anyMatch(e3 -> e3==e0)) {
+ // wraps/rethrows original, don't need to log
+ } else {
+ log.warn("Error in workflow '" + getName() + "' around step " + workflowStepReference(currentStepIndex) + " error handler for -- " + Exceptions.collapseText(e) + " -- threw another error (rethrowing): " + Exceptions.collapseText(e2));
+ log.debug("Full trace of original error was: " + e, e);
+ e = e2;
+ }
}
} else {
@@ -994,8 +997,12 @@ public class WorkflowExecutionContext {
}
} catch (Exception e2) {
- log.warn("Error in step '" + t.getDisplayName() + "' error handler for -- "+Exceptions.collapseText(e)+" -- threw another error (rethrowing): " + Exceptions.collapseText(e2));
- log.debug("Full trace of original error was: "+e, e);
+ if (Exceptions.getCausalChain(e2).stream().anyMatch(e3 -> e3==e)) {
+ // is, or wraps, original error, don't need to log
+ } else {
+ log.warn("Error in step '" + t.getDisplayName() + "' error handler for -- " + Exceptions.collapseText(e) + " -- threw another error (rethrowing): " + Exceptions.collapseText(e2));
+ log.debug("Full trace of original error was: " + e, e);
+ }
throw Exceptions.propagate(e2);
}
if (result==null) {
diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowReplayUtils.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowReplayUtils.java
index d1449cff01..f42fa00179 100644
--- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowReplayUtils.java
+++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowReplayUtils.java
@@ -37,7 +37,7 @@ public class WorkflowReplayUtils {
private static final Logger log = LoggerFactory.getLogger(WorkflowReplayUtils.class);
public enum ReplayableOption {
- OFF, ON, YES, NO
+ OFF, ON, YES, NO, TRUE, FALSE,
// where a step has nested workflow,
// if it is replaying from an explicit step
// - all previous children workflows are marked as old
@@ -180,7 +180,7 @@ public class WorkflowReplayUtils {
}
public static boolean isReplayable(ReplayableOption setting) {
- return setting==ReplayableOption.ON || setting==ReplayableOption.YES;
+ return setting==ReplayableOption.ON || setting==ReplayableOption.YES || setting==ReplayableOption.TRUE;
}
public static boolean isReplayable(WorkflowExecutionContext workflowExecutionContext, Integer stepIndex) {
diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepDefinition.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepDefinition.java
index 2026721992..3ef5f39934 100644
--- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepDefinition.java
+++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepDefinition.java
@@ -33,6 +33,7 @@ import org.apache.brooklyn.util.collections.MutableMap;
import org.apache.brooklyn.util.core.predicates.DslPredicates;
import org.apache.brooklyn.util.core.task.TaskTags;
import org.apache.brooklyn.util.core.task.Tasks;
+import org.apache.brooklyn.util.exceptions.Exceptions;
import org.apache.brooklyn.util.guava.Maybe;
import org.apache.brooklyn.util.text.Strings;
import org.apache.brooklyn.util.time.Duration;
@@ -84,7 +85,11 @@ public abstract class WorkflowStepDefinition {
}
@JsonIgnore
public DslPredicates.DslPredicate getConditionResolved(WorkflowStepInstanceExecutionContext context) {
- return context.resolveWrapped(condition, TypeToken.of(DslPredicates.DslPredicate.class));
+ try {
+ return context.resolveWrapped(condition, TypeToken.of(DslPredicates.DslPredicate.class));
+ } catch (Exception e) {
+ throw Exceptions.propagateAnnotated("Unresolveable condition ("+condition+")", e);
+ }
}
// output of steps can be overridden
diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepResolution.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepResolution.java
index b38667f315..24cdb4af1b 100644
--- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepResolution.java
+++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowStepResolution.java
@@ -38,7 +38,13 @@ public class WorkflowStepResolution {
static List<WorkflowStepDefinition> resolveSteps(ManagementContext mgmt, List<Object> steps) {
List<WorkflowStepDefinition> result = MutableList.of();
if (steps==null || steps.isEmpty()) throw new IllegalStateException("No steps defined in workflow");
- steps.forEach((def) -> result.add(resolveStep(mgmt, def)));
+ for (int i=0; i<steps.size(); i++) {
+ try {
+ result.add(resolveStep(mgmt, steps.get(i)));
+ } catch (Exception e) {
+ throw Exceptions.propagateAnnotated("Error in definition of step "+(i+1)+" ("+steps.get(i)+")", e);
+ }
+ }
WorkflowExecutionContext.validateSteps(mgmt, result, true);
return result;
}
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 3c86216a9e..24e08ccf5b 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
@@ -551,6 +551,9 @@ public class WorkflowPersistReplayErrorsTest extends RebindTestFixture<BasicAppl
m -> m.matches("Starting .*-1-error-handler-1 in task .*"),
m -> m.matches("Completed handler .*-1-error-handler-1; proceeding to default next step"),
m -> m.matches("Completed step .*-1; no further steps: Workflow completed")));
+
+ lastWorkflowContext = new WorkflowStatePersistenceViaSensors(mgmt()).getWorkflows(app).values().iterator().next();
+ Asserts.assertEquals(lastWorkflowContext.currentStepIndex, (Integer) 1);
}
}
@@ -584,6 +587,21 @@ public class WorkflowPersistReplayErrorsTest extends RebindTestFixture<BasicAppl
}
}
+ @Test
+ public void testErrorHandlerRethrows() throws IOException {
+ lastInvocation = runSteps(MutableList.of(
+ MutableMap.of("step", "fail message expected exception",
+ "output", "should have failed",
+ "on-error", MutableList.of(
+ MutableMap.of("step", "return not applicable",
+ "condition", "not matched")))),
+ null);
+ Asserts.assertFailsWith(() -> Asserts.fail("Did not fail, returned: "+lastInvocation.getUnchecked()),
+ e -> Asserts.expectedFailureContainsIgnoreCase(e, "expected exception"));
+ lastWorkflowContext = new WorkflowStatePersistenceViaSensors(mgmt()).getWorkflows(app).values().iterator().next();
+ Asserts.assertEquals(lastWorkflowContext.currentStepIndex, (Integer) 0);
+ }
+
@Test
public void testTimeoutOnStep() throws Exception {
doTestTimeout(false, true);