Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make WF global parameters available in retries #12698

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

eduardodbr
Copy link
Member

@eduardodbr eduardodbr commented Feb 25, 2024

Fixes #10362
This bug was found on this issue although its title complains about another bug that doesn't exist.

Motivation

It is not possible to use the retries variable in an expression that also uses global variables like workflow.parameters.<param>. When the global variables are substituted the retry number isn't available and when the retry number is injected, the global params are not being used for substitution.

Modifications

Add the global params to SubstituteParams when retrying a node

Verification

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: whalesay
spec:
  entrypoint: whalesay
  arguments:
    parameters:
      - name: memreqnum
        value: 100
  templates:
  - name: whalesay
    retryStrategy:
      limit: 10
    podSpecPatch: |
      containers:
        - name: main
          resources:
            limits:
              memory: "{{= (sprig.int(retries)+1)* sprig.int(workflow.parameters.memreqnum)}}Mi"
    container:
      image: docker/whalesay:latest
      command: [sh, -c]
      args: ["cowsay hello"]

The memory limit should be defined by the value of the parameter memreqnum multiplied by the retry number.

…ables in a retry

Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
@eduardodbr eduardodbr marked this pull request as ready for review February 25, 2024 13:12
@agilgur5 agilgur5 added area/templating Templating with `{{...}}` area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries labels Feb 25, 2024
@agilgur5 agilgur5 changed the title fix: make WF global parameters available when substituting variables in a retry fix: make WF global parameters available in retries Feb 25, 2024
Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for root causing this, fixing it, and adding tests!

@agilgur5 agilgur5 merged commit 9bec114 into argoproj:main Feb 25, 2024
30 checks passed
@agilgur5
Copy link

I was looking at some other substitution code for #12693 just yesterday; you might be interested in taking a look at that one too, although it seems potentially more complex since it regressed twice already

@eduardodbr
Copy link
Member Author

I was looking at some other substitution code for #12693 just yesterday; you might be interested in taking a look at that one too, although it seems potentially more complex since it regressed twice already

yeah sure, I'll assign it to me :)

agilgur5 pushed a commit that referenced this pull request Feb 29, 2024
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
(cherry picked from commit 9bec114)
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 3, 2024
@agilgur5 agilgur5 added area/retryStrategy Template-level retryStrategy and removed area/retry-manual Manual workflow "Retry" Action (API/CLI/UI). See retryStrategy for template-level retries labels Apr 25, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 6, 2024
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request May 7, 2024
Signed-off-by: eduardodbr <eduardodbr@hotmail.com>
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.

Can't use workflow parameters inside podSpecPatch resources request with retryStrategy
2 participants