Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
@JsonSubTypes.Type(name = "DYNAMIC_OUTPUT", value = DynamicOutputArtifact.class),
@JsonSubTypes.Type(name = "KUBERNETES", value = KubernetesArtifact.class),
@JsonSubTypes.Type(name = "HTTP", value = HttpArtifact.class),
@JsonSubTypes.Type(name = "RETRY", value = RetryArtifact.class),
})
@SuppressWarnings("PMD.ImplicitFunctionalInterface")
public interface Artifact {
Expand All @@ -58,7 +59,9 @@ enum Type {
/** kubernetes artifact. */
KUBERNETES(Constants.MAESTRO_PREFIX + "kubernetes"),
/** http artifact. */
HTTP(Constants.MAESTRO_PREFIX + "http");
HTTP(Constants.MAESTRO_PREFIX + "http"),
/** retry artifact. */
RETRY(Constants.MAESTRO_PREFIX + "retry");

private final String key;

Expand Down Expand Up @@ -152,4 +155,13 @@ default KubernetesArtifact asKubernetes() {
default HttpArtifact asHttp() {
throw new MaestroInternalError("Artifact type [%s] cannot be used as HTTP", getType());
}

/**
* Get Retry type artifact.
*
* @return concrete artifact object.
*/
default RetryArtifact asRetry() {
throw new MaestroInternalError("Artifact type [%s] cannot be used as RETRY", getType());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* Copyright 2024 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/
package com.netflix.maestro.models.artifact;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
import com.fasterxml.jackson.databind.PropertyNamingStrategies;
import com.fasterxml.jackson.databind.annotation.JsonNaming;
import lombok.Data;

/** Retry artifact for a step to influence whether the system retries it on failure. */
@JsonNaming(PropertyNamingStrategies.SnakeCaseStrategy.class)
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonPropertyOrder(
value = {"retryable"},
alphabetic = true)
@Data
public class RetryArtifact implements Artifact {
private boolean retryable = true; // whether the system should retry the step on failure

@JsonIgnore
@Override
public RetryArtifact asRetry() {
return this;
}

@Override
public Type getType() {
return Type.RETRY;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2024 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
* specific language governing permissions and limitations under the License.
*/
package com.netflix.maestro.models.artifact;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import com.netflix.maestro.MaestroBaseTest;
import org.junit.Test;

public class RetryArtifactTest extends MaestroBaseTest {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that other tests in this module, such as WhileArtifactTest.java and TitusArtifactTest.java, include a testRoundTripSerde that verifies the general serde contract.

Would it make sense to add a similar testRoundTripSerde here for consistency and to cover the round-trip behavior as well?

@Test
public void testRoundTripSerde() throws Exception {
RetryArtifact artifact =
loadObject("fixtures/artifact/sample-retry-artifact.json", RetryArtifact.class);
assertEquals(
artifact, MAPPER.readValue(MAPPER.writeValueAsString(artifact), RetryArtifact.class));
}

@Test
public void testDeserializeRetryable() throws Exception {
Artifact artifact =
MAPPER.readValue(
"""
{"type": "RETRY", "retryable": false}
""", Artifact.class);
assertEquals(Artifact.Type.RETRY, artifact.getType());
assertFalse(artifact.asRetry().isRetryable());
}

@Test
public void testDeserializeDefaultsToRetryable() throws Exception {
Artifact artifact =
MAPPER.readValue("""
{"type": "RETRY"}
""", Artifact.class);
assertTrue(artifact.asRetry().isRetryable());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"retryable": false,
"type": "RETRY"
}
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,26 @@ public void validateAndMergeOutputParamsAndArtifacts(StepRuntimeSummary runtimeS
}
}

/**
* Checks whether the step's output data marks a failed step as non-retryable by the system.
*
* @param runtimeSummary step runtime summary used to locate the output data
* @return true if the output data marks the step non-retryable, false otherwise
*/
public boolean isStepNonRetryable(StepRuntimeSummary runtimeSummary) {
Optional<String> externalJobId = extractExternalJobId(runtimeSummary);
if (externalJobId.isEmpty()) {
return false;
}
Optional<OutputData> outputDataOpt =
outputDataDao.getOutputDataForExternalJob(externalJobId.get(), runtimeSummary.getType());
return outputDataOpt
.map(OutputData::getArtifacts)
.map(artifacts -> artifacts.get(Artifact.Type.RETRY.key()))
.map(artifact -> !artifact.asRetry().isRetryable())
.orElse(false);
}

private Optional<String> extractExternalJobId(StepRuntimeSummary runtimeSummary) {
Map<String, Artifact> artifacts = runtimeSummary.getArtifacts();
String jobId = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ private void handleTimeoutError(
}

/** Executes the step instance. It returns true, if the task is in dummy run mode. */
@SuppressWarnings("PMD.ExhaustiveSwitchHasDefault")
@SuppressWarnings({"PMD.ExhaustiveSwitchHasDefault", "checkstyle:MethodLength"})
private boolean doExecute(
Flow flow,
Task task,
Expand Down Expand Up @@ -847,6 +847,21 @@ private boolean doExecute(
evaluateNextConditionParams(flow, stepDefinition, runtimeSummary);
doneWithExecute = true;
break;
case USER_FAILED:
case PLATFORM_FAILED:
// A retryable failure is escalated to FATALLY_FAILED when the step's output data marks
// it as non-retryable, then falls through to the FATALLY_FAILED handling.
if (!outputDataManager.isStepNonRetryable(runtimeSummary)) {
doneWithExecute = true;
break;
}
StepInstance.Status failedStatus = runtimeSummary.getRuntimeState().getStatus();
runtimeSummary.markTerminated(StepInstance.Status.FATALLY_FAILED, tracingManager);
runtimeSummary.addTimeline(
TimelineLogEvent.info(
"Step failed with [%s] and its output data classified it as non-retryable.",
failedStatus));
// fall through, to apply failure mode handling
case FATALLY_FAILED: // Failure mode only applies to FATALLY_FAILED
if (!runtimeSummary.isIgnoreFailureMode()) {
if (FailureMode.IGNORE_FAILURE == stepDefinition.getFailureMode()) {
Expand All @@ -864,8 +879,6 @@ private boolean doExecute(
}
// fall through, otherwise
case INTERNALLY_FAILED: // Ignoring failure model as the error happens within Maestro
case USER_FAILED:
case PLATFORM_FAILED:
case TIMEOUT_FAILED:
case STOPPED:
case TIMED_OUT:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.netflix.maestro.models.artifact.Artifact;
import com.netflix.maestro.models.artifact.DynamicOutputArtifact;
import com.netflix.maestro.models.artifact.KubernetesArtifact;
import com.netflix.maestro.models.artifact.RetryArtifact;
import com.netflix.maestro.models.artifact.TitusArtifact;
import com.netflix.maestro.models.definition.StepType;
import com.netflix.maestro.models.parameter.InternalParamMode;
Expand Down Expand Up @@ -110,6 +111,40 @@ public void testSaveOutputData() {
Mockito.verify(outputDataDao, times(1)).insertOrUpdateOutputData(outputData);
}

@Test
public void testIsStepNonRetryableWhenMarkedNonRetryable() {
RetryArtifact retryArtifact = new RetryArtifact();
retryArtifact.setRetryable(false);
OutputData output = new OutputData(null, Map.of(Artifact.Type.RETRY.key(), retryArtifact));
when(outputDataDao.getOutputDataForExternalJob(TASK_ID, StepType.TITUS))
.thenReturn(Optional.of(output));
runtimeSummary = runtimeSummaryBuilder().type(StepType.TITUS).artifacts(artifacts).build();
assertTrue(outputDataManager.isStepNonRetryable(runtimeSummary));
}

@Test
public void testIsStepNonRetryableWhenMarkedRetryable() {
OutputData output =
new OutputData(null, Map.of(Artifact.Type.RETRY.key(), new RetryArtifact()));
when(outputDataDao.getOutputDataForExternalJob(TASK_ID, StepType.TITUS))
.thenReturn(Optional.of(output));
runtimeSummary = runtimeSummaryBuilder().type(StepType.TITUS).artifacts(artifacts).build();
assertFalse(outputDataManager.isStepNonRetryable(runtimeSummary));
}

@Test
public void testIsStepNonRetryableWhenNoRetryArtifact() {
setupOutputDataDao();
runtimeSummary = runtimeSummaryBuilder().type(StepType.TITUS).artifacts(artifacts).build();
assertFalse(outputDataManager.isStepNonRetryable(runtimeSummary));
}

@Test
public void testIsStepNonRetryableWhenNoOutputData() {
runtimeSummary = runtimeSummaryBuilder().type(StepType.TITUS).artifacts(artifacts).build();
assertFalse(outputDataManager.isStepNonRetryable(runtimeSummary));
}

@Test
public void testMissingJobIdArtifact() {
outputDataManager.validateAndMergeOutputParamsAndArtifacts(runtimeSummary);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"properties": {
"owner": "tester",
"run_strategy": "sequential"
},
"workflow": {
"id": "sample-kubernetes-nonretryable-wf",
"name": "Test kubernetes workflow that fails and is classified non-retryable via output data",
"steps": [
{
"step": {
"id": "job1",
"type": "kubernetes",
"retry_policy": {
"error_retry_limit": 3
},
"params": {
"kubernetes": {
"value": {
"image": {
"value": "busybox",
"type": "STRING"
},
"command": {
"value": ["/bin/sh"],
"type": "STRING_ARRAY"
},
"args": {
"value": ["-c", "sleep 5 && echo $$MAESTRO_OUTPUT_START$1$$MAESTRO_OUTPUT_END && exit 1", "sh", "{\"artifacts\":{\"maestro_retry\":{\"type\":\"RETRY\",\"retryable\":false}}}"],
"type": "STRING_ARRAY"
}
},
"type": "MAP"
}
}
}
}
]
}
}
Loading