Skip to content
Closed
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
29 changes: 29 additions & 0 deletions util/template/expression_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ func expressionReplace(w io.Writer, expression string, env map[string]interface{
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}

if (hasVariableInExpression(unmarshalledExpression, "lastRetry.exitCode") && !hasVarInEnv(env, "lastRetry.exitCode") ||
hasVariableInExpression(unmarshalledExpression, "lastRetry.status") && !hasVarInEnv(env, "lastRetry.status") ||
hasVariableInExpression(unmarshalledExpression, "lastRetry.duration") && !hasVarInEnv(env, "lastRetry.duration") ||
hasVariableInExpression(unmarshalledExpression, "lastRetry.message") && !hasVarInEnv(env, "lastRetry.message")) &&
allowUnresolved {
// This is to make sure expressions which contains `lastRetry.*` don't get resolved to nil
// when they don't exist in the env.
log.WithError(err).Debug("LastRetry variables are present and unresolved is allowed")
return w.Write([]byte(fmt.Sprintf("{{%s%s}}", kindExpression, expression)))
}

// This is to make sure expressions which contains `workflow.status` and `work.failures` don't get resolved to nil
// when `workflow.status` and `workflow.failures` don't exist in the env.
// See https://github.com/argoproj/argo-workflows/issues/10393, https://github.com/expr-lang/expr/issues/330
Expand Down Expand Up @@ -107,6 +118,24 @@ func hasRetries(expression string) bool {
return false
}

func hasVariableInExpression(expression, variable string) bool {
if !strings.Contains(expression, variable) {
return false
}
// Even if the expression contains `<variable>`, it could be the case that it represents a string (`"<variable>"`),
// not a variable, so we need to parse it and handle filter the string case.
tokens, err := lexer.Lex(file.NewSource(expression))
if err != nil {
return false
}
for i := 0; i < len(tokens)-2; i++ {
if tokens[i].Value+tokens[i+1].Value+tokens[i+2].Value == variable {
return true
}
}
return false
}

// hasWorkflowStatus checks if expression contains `workflow.status`
func hasWorkflowStatus(expression string) bool {
if !strings.Contains(expression, "workflow.status") {
Expand Down
18 changes: 18 additions & 0 deletions workflow/controller/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -2128,6 +2128,24 @@ func (woc *wfOperationCtx) executeTemplate(ctx context.Context, nodeName string,
// Inject the retryAttempt number
localParams[common.LocalVarRetries] = strconv.Itoa(retryNum)
Copy link
Contributor

@juliev0 juliev0 May 13, 2024

Choose a reason for hiding this comment

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

Since we're already using LocalVarRetries in this scope, it does seem reasonable to me to use these other variables in the general template scope as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I haven't looked at it long enough, but I am kind of wondering if there's a way to do this and not have to be setting these same variables twice. Should processNodeRetries() possibly take a localScope and then be able to modify it and return it to the caller? You have probably looked at the code longer than I have - what do you think?


// Inject lastRetry variables
// the first node will not have "lastRetry" variables so they must have default values
// for the expression to resolve
Comment on lines +2131 to +2133
Copy link
Member

Choose a reason for hiding this comment

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

👍

lastRetryExitCode, lastRetryDuration := "0", "0"
var lastRetryStatus, lastRetryMessage string
if lastChildNode != nil {
if lastChildNode.Outputs != nil && lastChildNode.Outputs.ExitCode != nil {
lastRetryExitCode = *lastChildNode.Outputs.ExitCode
}
lastRetryStatus = string(lastChildNode.Phase)
lastRetryDuration = fmt.Sprint(lastChildNode.GetDuration().Seconds())
lastRetryMessage = lastChildNode.Message
}
localParams[common.LocalVarRetriesLastExitCode] = lastRetryExitCode
localParams[common.LocalVarRetriesLastDuration] = lastRetryDuration
localParams[common.LocalVarRetriesLastStatus] = lastRetryStatus
localParams[common.LocalVarRetriesLastMessage] = lastRetryMessage

Copy link
Contributor

@juliev0 juliev0 May 13, 2024

Choose a reason for hiding this comment

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

Since this function is already so long, can we separate this new logic into a function to build the local scope from lastChildNode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ignore this comment if this comment ends up making more sense

Copy link
Member

Choose a reason for hiding this comment

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

we could use the output from buildRetryStrategyLocalScope,
and add them in localParams

processedTmpl, err = common.SubstituteParams(processedTmpl, woc.globalParams, localParams)
if errorsutil.IsTransientErr(err) {
return node, err
Expand Down
53 changes: 53 additions & 0 deletions workflow/controller/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1379,6 +1379,59 @@ func TestRetriesVariableWithGlobalVariableInPodSpecPatch(t *testing.T) {
assert.ElementsMatch(t, actual, expected)
}

var lastretryVariableInPodSpecPatchTemplate = `
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
name: whalesay
spec:
entrypoint: whalesay
templates:
- name: whalesay
retryStrategy:
limit: 10
podSpecPatch: |
containers:
- name: main
resources:
limits:
memory: "{{= (sprig.int(lastRetry.exitCode)==1 ? (sprig.int(retries)+1) : 1)* 100}}Mi"
container:
image: python:alpine3.6
command: ["python", -c]
args: ["import sys; sys.exit(1)"]
`

func TestLastRetryVariableInPodSpecPatch(t *testing.T) {
wf := wfv1.MustUnmarshalWorkflow(lastretryVariableInPodSpecPatchTemplate)
cancel, controller := newController(wf)
defer cancel()
ctx := context.Background()
iterations := 5
var woc *wfOperationCtx
for i := 1; i <= iterations; i++ {
woc = newWorkflowOperationCtx(wf, controller)
if i != 1 {
makePodsPhase(ctx, woc, apiv1.PodFailed, withExitCode(1))
}
woc.operate(ctx)
wf = woc.wf
}

pods, err := listPods(woc)
assert.NoError(t, err)
assert.Len(t, pods.Items, iterations)
expected := []string{}
actual := []string{}
for i := 0; i < iterations; i++ {
actual = append(actual, pods.Items[i].Spec.Containers[1].Resources.Limits.Memory().String())
expected = append(expected, fmt.Sprintf("%dMi", (i+1)*100))
}
// expecting memory limit to increase after each retry: "100Mi", "200Mi", "300Mi", "400Mi", "500Mi"
// ordering not preserved
assert.ElementsMatch(t, actual, expected)
}

var stepsRetriesVariableTemplate = `
apiVersion: argoproj.io/v1alpha1
kind: Workflow
Expand Down