-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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: change fatal to panic. #12931
fix: change fatal to panic. #12931
Conversation
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
@agilgur5 could you help have a look for this, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small changes -- we can use log.Panic
actually
I also renamed this to a fix
since Fatal
/ os.Exit
is normally not recoverable; so these cases probably should use panic
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up for a discussion,
This util FormulateResubmitWorkflow
function
is only run by the argo servers,
I'm thinking maybe we should remove and replace all panic/fatal in this function with error/warn
so the server keeps alive and returns the appropriate status code,err
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Yes, that's why I suggested the Regarding the status code, these would generally all be 500s; they're generally indicative of a bug. I would think a |
Signed-off-by: shuangkun <tsk2013uestc@163.com>
Ok, changed fatal to error in |
Signed-off-by: shuangkun <tsk2013uestc@163.com>
right, found recover https://github.com/argoproj/argo-workflows/blob/main/util/grpc/interceptor.go#L21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for fixing these cases
@tczhao for reference all these node not found errors were added in #11451 as there were some actual bugs where nodes didn't exist but silently returned fine (because of how in the cases where we know for sure these errors will never happen, we should ignore the errors and not handle them at all, similar to what I did in #12917 . the problem is that we don't know all the cases 😕 cc @isubasinghe for reference who did the most work on this (and I may have the second most context on it nowadays?) |
@agilgur5 Got it, thank you for the context |
Backported cleanly to |
Co-authored-by: agilgur5 <agilgur5@gmail.com> Signed-off-by: shuangkun <tsk2013uestc@163.com> (cherry picked from commit ec5b5d5)
Co-authored-by: agilgur5 <agilgur5@gmail.com> Signed-off-by: shuangkun <tsk2013uestc@163.com>
Co-authored-by: agilgur5 <agilgur5@gmail.com> Signed-off-by: shuangkun <tsk2013uestc@163.com>
comments here: #12817 (comment)
Motivation
Modifications
Verification