Skip to content

feat: allow lastRetry variables in expressions#12722

Closed
eduardodbr wants to merge 4 commits intoargoproj:mainfrom
eduardodbr:allow-lastRetry-variables-expression
Closed

feat: allow lastRetry variables in expressions#12722
eduardodbr wants to merge 4 commits intoargoproj:mainfrom
eduardodbr:allow-lastRetry-variables-expression

Conversation

@eduardodbr
Copy link
Member

Fixes #10364

Motivation

Modifications

Verification

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: pod-spec-patch
spec:
  entrypoint: whalesay
  templates:
  - name: whalesay
    retryStrategy:
      limit: 5
      retryPolicy: Always
    podSpecPatch: '{"containers":[{"name":"main", "resources":{"requests":{"memory": "{{=(sprig.int(lastRetry.exitCode)==1 ? int(retries) : 1)* 10}}Mi" }}}]}'
    container:
      image: python:alpine3.6
      command: ["python", -c]
      args: ["import sys; sys.exit(1)"]

uses the exit code to know when to increase memory. This should use 10MB,20MB,30MB,40MB,50MB in each iteration

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
@eduardodbr eduardodbr added the area/templating Templating with `{{...}}` label Mar 1, 2024
@eduardodbr eduardodbr marked this pull request as ready for review March 2, 2024 12:53
@eduardodbr eduardodbr changed the title feat: allow lastRetry variables in templates feat: allow lastRetry variables in expressions Mar 2, 2024
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
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

@@ -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?

localParams[common.LocalVarRetriesLastDuration] = lastRetryDuration
localParams[common.LocalVarRetriesLastStatus] = lastRetryStatus
localParams[common.LocalVarRetriesLastMessage] = lastRetryMessage

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

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

Choose a reason for hiding this comment

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

👍

@gdhagger
Copy link

This PR seems to have stalled - would love to be able to use these vars, what needs to happen to get it moving again?

@eduardodbr
Copy link
Member Author

this was done more than a year ago. I would need to review everything which will take time. If anyone wants to jump in and contribute the requested changes I'm ok with that.

@isubasinghe
Copy link
Member

resurrected the PR here: #14450

@caelan-io
Copy link
Member

Thank you for contributing this, @eduardodbr ! Isitha picked it up and rounded it out to merge the feature in #14450 and it's released in v3.7, so we'll close this PR out. cc @gdhagger and @EladProject for visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/retryStrategy Template-level retryStrategy area/templating Templating with `{{...}}`

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow use of lastRetry in podSpecPatch resources request

7 participants