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.

Sign up for GitHub

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

Jump to bottom

fix(controller): handle nil processedTmpl in DAGs #13548

Merged
merged 1 commit into from
Sep 18, 2024
Merged

fix(controller): handle nil processedTmpl in DAGs #13548

merged 1 commit into from
Sep 18, 2024

Conversation

chengjoey
Copy link
Contributor

Fixes #13547

Motivation

It seems more reasonable to judge processedTmpl != nil

@Joibel Joibel self-assigned this Sep 2, 2024
@chengjoey
fix workflow controller make panic due to processedTmpl is nil
d7da631 
Signed-off-by: joey <zchengjoey@gmail.com>
Joibel
Joibel requested changes Sep 2, 2024
View reviewed changes
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.

As you have a reproducible test case, could you add it as a regression test please?

@chengjoey
Copy link
Contributor Author

chengjoey commented Sep 2, 2024 •
edited by agilgur5
Loading

@Joibel , I accidentally reproduced it in #13547, but I can't reproduce it 100% now. It seems to be a one-time thing, so I'm not sure if I can add e2e tests

@agilgur5 agilgur5 changed the title fix(controller): workflow controller recovered from panic due to processedTmpl is nil fix(controller): handle nil processedTmpl Sep 2, 2024
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Sep 2, 2024
@agilgur5 agilgur5 changed the title fix(controller): handle nil processedTmpl fix(controller): handle nil processedTmpl in DAGs Sep 2, 2024
terrytangyuan
terrytangyuan approved these changes Sep 11, 2024
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

I think this is a great fix for edge cases even though it cannot be reproduced easily. WDYT? @Joibel

@Joibel Joibel merged commit 5b8ae35 into argoproj:main Sep 18, 2024
29 checks passed
@Joibel
Copy link
Member

Joibel commented Sep 18, 2024

@chengjoey Thanks for this

Joibel pushed a commit to pipekit/argo-workflows that referenced this pull request Sep 19, 2024
@chengjoey @Joibel
fix(controller): handle nil processedTmpl in DAGs (argoproj#13548)
d1f569b 
Signed-off-by: joey <zchengjoey@gmail.com>
@Joibel Joibel mentioned this pull request Sep 20, 2024
Open
Joibel pushed a commit that referenced this pull request Sep 20, 2024
@chengjoey @Joibel
fix(controller): handle nil processedTmpl in DAGs (#13548)
dc004a7 
Signed-off-by: joey <zchengjoey@gmail.com>
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Sep 20, 2024
@chengjoey chengjoey mentioned this pull request Sep 22, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@Joibel Joibel Joibel approved these changes

Assignees

@Joibel Joibel

Labels
area/controller Controller issues, panics area/templates/dag
Projects
None yet
Milestone
v3.5.x patches
Development

Successfully merging this pull request may close these issues.

controller panic: nil pointer dereference on DAG processedTmpl
4 participants
@chengjoey @Joibel @terrytangyuan @agilgur5