fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10523

alexec · 2023-02-13T03:50:53Z

Signed-off-by: Alex Collins alex_collins@intuit.com

This MIGHT fix these:

Fixes #10518
Fixes #10337
Fixes #10033
Fixes #10490

Please do not open a pull request until you have checked ALL of these:

Create the PR as draft .
Run make pre-commit -B to fix codegen and lint problems.
Sign-off your commits (otherwise the DCO check will fail).
Use a conventional commit message (otherwise the commit message check will fail).
"Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
Github checks are green.
Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

Signed-off-by: Alex Collins <alex_collins@intuit.com>

sxllwx · 2023-02-13T04:11:51Z

FIY (😄): #10520 (comment)

alexec · 2023-02-13T04:23:18Z

Could you test this out? Start the workflow controller with --executor-image quay.io/argoproj/argoexec:dev-sigterm (I've not tested that, so LMK if you have problems/find typos).

sxllwx · 2023-02-13T04:48:23Z

Could you test this out? Start the workflow controller with --executor-image quay.io/argoproj/argoexec:dev-sigterm (I've not tested that, so LMK if you have problems/find typos).

Ok～

sxllwx · 2023-02-13T06:43:14Z

cmd/argoexec/commands/wait.go

@@ -16,7 +14,7 @@ func NewWaitCommand() *cobra.Command {
 		Use:   "wait",
 		Short: "wait for main container to finish and save artifacts",
 		Run: func(cmd *cobra.Command, args []string) {
-			ctx := context.Background()
+			ctx := cmd.Context()


I suggest putting the code related to signal handling here. The current implementation affects all subcommands of argoexec.

That’s intentional, as they’re currently not able to deal with SIGTERM. That said, each one needs to be updated to have this line in them.

data was followed up in #12544 (comment)

alexec · 2023-02-16T15:25:06Z

@juliev0 @sarabala1979 v3.4 is broken for large artifacts. This PR should fix it.

juliev0 · 2023-02-16T15:56:13Z

@juliev0 @sarabala1979 v3.4 is broken for large artifacts. This PR should fix it.

Just large artifacts? If this is related to the wait container failing with an exit code of 2, I was seeing that with my artifact GC e2e test (small artifacts). I can test your PR with that. Thanks for fixing it.

alexec · 2023-02-16T19:19:27Z

Not exactly. Artifacts that take a longer than the reconciliation loop to upload. They tend to be large.

juliev0 · 2023-02-16T20:21:19Z

Thanks for fixing this in your free time. It would be great generally to get a description of the change in the PR to help reviewers (like myself). :)

I gather from @sxllwx 's comment here that the issue was that the wait container received a SIGTERM and called cancel() and then received another SIGTERM and failed with exit code 2?

So, your change is to allow the wait container to play everything out rather than looking for SIGTERM, and instead have the main container respond to the SIGTERM, passing the context into the container so it can prematurely terminate, right?

That does seem better.

juliev0 · 2023-02-16T20:31:50Z

Not exactly. Artifacts that take a longer than the reconciliation loop to upload. They tend to be large.

Well, I was definitely seeing premature exiting with exit code 2 intermittently with the Artifact GC test.

This comment seemed to indicate it tends to affect short running Workflows, which definitely applies in my case.

juliev0 · 2023-02-16T20:35:28Z

Okay, I've now run this on the ArtifactGC e2e test 10 times and the Workflows never failed with exit code 2. :)

alexec · 2023-02-16T20:45:11Z

Does this need @sarabala1979 approval?

sarabala1979

LGTM

) Signed-off-by: Alex Collins <alex_collins@intuit.com>

yanxingponyai · 2023-04-25T09:59:30Z

Hi @alexec, Thanks for your great work.
May I ask a question that how this pr fixes #10033? I have read all the related comments, but I did not understand.

alexec and others added 4 commits February 12, 2023 13:34

fix: Correct SIGTERM handling. Fixes ##10518 #10337 #10033 #10490

3a09875

Signed-off-by: Alex Collins <alex_collins@intuit.com>

fix: Correct SIGTERM handling. Fixes ##10518 #10337 #10033 #10490

eae32e8

Signed-off-by: Alex Collins <alex_collins@intuit.com>

Update wait.go

2ff128a

fix: ok

8d66df0

Signed-off-by: Alex Collins <alex_collins@intuit.com>

alexec mentioned this pull request Feb 13, 2023

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10520

Closed

8 tasks

sxllwx reviewed Feb 13, 2023

View reviewed changes

alexec marked this pull request as ready for review February 13, 2023 15:09

sxllwx approved these changes Feb 14, 2023

View reviewed changes

This was referenced Feb 14, 2023

Wait container failing: level=info msg="stopping progress monitor (context done)" error="context canceled" #10490

Closed

fix: added logs related to executing commands in the container #10530

Merged

alexec enabled auto-merge (squash) February 16, 2023 15:24

alexec requested a review from juliev0 February 16, 2023 15:24

JPZ13 assigned juliev0, isubasinghe and rohankmr414 and unassigned juliev0, isubasinghe and rohankmr414 Feb 16, 2023

JPZ13 requested review from isubasinghe, rohankmr414 and dpadhiar February 16, 2023 18:39

juliev0 approved these changes Feb 16, 2023

View reviewed changes

JPZ13 mentioned this pull request Feb 23, 2023

stages with Error (exit code 2) #10526

Closed

3 tasks

sarabala1979 approved these changes Feb 23, 2023

View reviewed changes

alexec merged commit d75e37e into master Feb 23, 2023

alexec deleted the dev-sigterm branch February 23, 2023 23:31

caelan-io mentioned this pull request Mar 2, 2023

Sidecars not terminated in a timely manner at scale when main container completes #10612

Open

3 tasks

JPZ13 mentioned this pull request Mar 16, 2023

Pod continues running after Workflow is stopped #10658

Closed

3 tasks

caelan-io mentioned this pull request Mar 21, 2023

Sigterm if using generated step name (after upgrade to 3.4.3) #10002

Closed

3 tasks

terrytangyuan mentioned this pull request Mar 24, 2023

Release v3.4.6 cherry-pick candidates #10747

Closed

33 tasks

terrytangyuan pushed a commit that referenced this pull request Mar 29, 2023

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 (#10523

fe522b6

) Signed-off-by: Alex Collins <alex_collins@intuit.com>

juliev0 mentioned this pull request Jun 22, 2023

Workflow can't be stopped/terminated when wait container was terminated while main container is still running. #11247

Closed

3 tasks

agilgur5 added the area/executor label Sep 8, 2023

This was referenced Mar 13, 2024

fix(resource): properly handle TERM signal after catch #11582

Merged

fix: Mark resource & data template report-outputs-completed: "true" #12544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10523

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10523

alexec commented Feb 13, 2023

sxllwx commented Feb 13, 2023 •

edited

Loading

alexec commented Feb 13, 2023

sxllwx commented Feb 13, 2023

sxllwx Feb 13, 2023

alexec Feb 13, 2023

sxllwx Feb 14, 2023

agilgur5 Apr 26, 2024

alexec commented Feb 16, 2023

juliev0 commented Feb 16, 2023

alexec commented Feb 16, 2023 •

edited

Loading

juliev0 commented Feb 16, 2023

juliev0 commented Feb 16, 2023 •

edited

Loading

juliev0 commented Feb 16, 2023

alexec commented Feb 16, 2023

sarabala1979 left a comment

yanxingponyai commented Apr 25, 2023 •

edited

Loading

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10523

fix: Correct SIGTERM handling. Fixes #10518 #10337 #10033 #10490 #10523

Conversation

alexec commented Feb 13, 2023

sxllwx commented Feb 13, 2023 • edited Loading

alexec commented Feb 13, 2023

sxllwx commented Feb 13, 2023

sxllwx Feb 13, 2023

Choose a reason for hiding this comment

alexec Feb 13, 2023

Choose a reason for hiding this comment

sxllwx Feb 14, 2023

Choose a reason for hiding this comment

agilgur5 Apr 26, 2024

Choose a reason for hiding this comment

alexec commented Feb 16, 2023

juliev0 commented Feb 16, 2023

alexec commented Feb 16, 2023 • edited Loading

juliev0 commented Feb 16, 2023

juliev0 commented Feb 16, 2023 • edited Loading

juliev0 commented Feb 16, 2023

alexec commented Feb 16, 2023

sarabala1979 left a comment

Choose a reason for hiding this comment

yanxingponyai commented Apr 25, 2023 • edited Loading

sxllwx commented Feb 13, 2023 •

edited

Loading

alexec commented Feb 16, 2023 •

edited

Loading

juliev0 commented Feb 16, 2023 •

edited

Loading

yanxingponyai commented Apr 25, 2023 •

edited

Loading