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(resource): properly handle TERM signal after catch #11582

Merged
merged 1 commit into from Aug 20, 2023

Conversation

feiyudev
Copy link
Contributor

TERM signal was catched but not handled properly, which causing waiting so long to delete a pod

Fixes #10658

Motivation

pod can't be stopped via argo terminate, works after I changed the context.

Modifications

changed the parent context in argoexec resource subcommand.

Verification

  1. stop pod via kubectl delete pod, no more 30s needed.
  2. stop via argo terminate works.

…waiting so long to delete a pod

Signed-off-by: feiyudev <feiyu.dev@gmail.com>
@juliev0
Copy link
Contributor

juliev0 commented Aug 15, 2023

Thanks for fixing. Can you clarify where "argoexec resource" is invoked?

@feiyudev
Copy link
Contributor Author

Thanks for fixing. Can you clarify where "argoexec resource" is invoked?

When resource defined in one step, then the pod to run this step, generated by workflow controller, runs commands as follows:

/usr/bin/containerd-shim-runc-v2 -namespace k8s.io -id <id> -address /run/containerd/containerd.sock
 \_ /pause
 \_ /var/run/argo/argoexec emissary --loglevel info --log-format text -- argoexec resource apply --loglevel debug
     \_ /bin/argoexec resource apply --loglevel debug

@juliev0
Copy link
Contributor

juliev0 commented Aug 17, 2023

Thanks for fixing. Can you clarify where "argoexec resource" is invoked?

When resource defined in one step, then the pod to run this step, generated by workflow controller, runs commands as follows:

/usr/bin/containerd-shim-runc-v2 -namespace k8s.io -id <id> -address /run/containerd/containerd.sock
 \_ /pause
 \_ /var/run/argo/argoexec emissary --loglevel info --log-format text -- argoexec resource apply --loglevel debug
     \_ /bin/argoexec resource apply --loglevel debug

Thank you! I’m OOO right now but I’ll review next week when back if this hasn’t been merged by then.

@feiyudev
Copy link
Contributor Author

All right, thanks anyway.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

LGTM

@terrytangyuan terrytangyuan merged commit 8a52da5 into argoproj:master Aug 20, 2023
23 checks passed
@agilgur5
Copy link
Member

This seems to have effectively been a follow-up to #10523 (comment), if I'm understanding the history correctly

@agilgur5 agilgur5 changed the title fix: TERM signal was catched but not handled properly, which causing … fix(resource): properly handle TERM signal after catch Mar 18, 2024
dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
argoproj#11582)

Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pod continues running after Workflow is stopped
5 participants