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: return itself when getOutboundNodes from memoization Hit steps/DAG. Fixes: #7873 #12780

Merged

Conversation

shuangkun
Copy link
Member

@shuangkun shuangkun commented Mar 11, 2024

Fixes #7873

Motivation

The first step "hello-steps" should have one child (The SgNode of "whatever"),but when “hello-steps” hit memoize,it has no child because of getOutboundNodes from the node return nil. Prevents the next sgNode from becoming the child of the previous sgNode’s outboundNodes.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: memoized-bug-
spec:
  entrypoint: main
  templates:
  - name: main
    steps:
    - - name: hello-steps
        template: memoized
    - - name: whatever
        template: hello

  - name: memoized
    outputs:
      parameters:
      - name: msg
        valueFrom:
          parameter: "{{steps.hello-step.outputs.result}}"
    steps:
    - - name: hello-step
        template: hello
    memoize:
      key: "memoized-bug-steps-0"
      cache:
        configMap:
          name: my-config

  - name: hello
    container:
      image: alpine:latest
      command: [sh, -c]
      args: ["echo Hello"]

Modifications

return itself when getOutboundNodes from memoization Hit steps/DAG.

Verification

Local test and unit tests.

shuangkun and others added 2 commits March 11, 2024 15:35
Co-authored-by: zjgemi <liuxin_zijian@163.com>
Co-authored-by: sherwinkoo29 <sherwinkoo@163.com>
Co-authored-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Co-authored-by: zjgemi <liuxin_zijian@163.com>
Co-authored-by: sherwinkoo29 <sherwinkoo@163.com>
Co-authored-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun marked this pull request as draft March 11, 2024 09:02
@shuangkun shuangkun closed this Mar 11, 2024
@shuangkun shuangkun reopened this Mar 11, 2024
@shuangkun shuangkun marked this pull request as ready for review March 11, 2024 09:52
@shuangkun shuangkun added area/controller Controller issues, panics area/memoization labels Mar 11, 2024
@shuangkun
Copy link
Member Author

Hi,Joibel @Joibel I know you are very familiar with memoize, can you help review this, when a node hits the cache, its node id can be returned when getOutboundNodes. Thank you so much!

@Joibel Joibel self-assigned this Mar 17, 2024
@Joibel
Copy link
Member

Joibel commented Mar 17, 2024

Happy to look at it. I am at Kubecon/Argocon this week, so not sure whether I will get a chance until next week.

Can you get the test suite to ✅?

@shuangkun
Copy link
Member Author

Happy to look at it. I am at Kubecon/Argocon this week, so not sure whether I will get a chance until next week.

Can you get the test suite to ✅?

Thanks, enjoy your conference! My question is not urgent, you can take a look at my question later when you have time. The test suite is OK.

Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of things to improve test robustness

assert.Equal(t, wfv1.WorkflowSucceeded, woc.wf.Status.Phase)

for _, node := range woc.wf.Status.Nodes {
if node.DisplayName == "hello-steps" {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to test that we hit this condition once and only once (just in case we end up without this node in the Nodes list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I add it.

workflow/controller/operator_test.go Show resolved Hide resolved
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@shuangkun shuangkun force-pushed the fix/getOutBoundNodesFromMemoizeHit branch 4 times, most recently from a682acc to c2b9efe Compare March 21, 2024 12:52
@shuangkun shuangkun requested a review from Joibel March 21, 2024 13:40
Copy link
Member

@Joibel Joibel left a comment

Choose a reason for hiding this comment

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

LGTM. @agilgur5 @isubasinghe @terrytangyuan could you do the magic please

@isubasinghe isubasinghe enabled auto-merge (squash) March 22, 2024 21:15
@isubasinghe isubasinghe merged commit a719d94 into argoproj:main Mar 22, 2024
27 checks passed
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 19, 2024
agilgur5 pushed a commit that referenced this pull request Apr 19, 2024
…AG. Fixes: #7873 (#12780)

Signed-off-by: shuangkun <tsk2013uestc@163.com>
Co-authored-by: zjgemi <liuxin_zijian@163.com>
Co-authored-by: sherwinkoo29 <sherwinkoo@163.com>
Co-authored-by: Isitha Subasinghe <isitha@pipekit.io>
(cherry picked from commit a719d94)
@agilgur5
Copy link
Member

Backported cleanly to release-3.5 as 4e7d471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics area/memoization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A memoized steps hit, the following nodes not displayed in the UI
4 participants