Execute `on_failure_callback` when SIGTERM is received #15172

ephraimbuddy · 2021-04-03T08:02:27Z

Currently, on_failure_callback is only called when a task finishes
executing not while executing. When a pod is deleted, a SIGTERM is
sent to the task and the task is stopped immediately. The task is
still running when it was killed and therefore on_failure_callback
is not called.

This PR makes sure that when a pod is marked for deletion and the
task is killed, if the task has on_failure_callback, the callback
is called

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

github-actions · 2021-04-04T21:17:55Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

kaxil

I don't think this is a correct fix check my comments in #14422 (comment)

ephraimbuddy · 2021-04-08T18:47:24Z

I don't think this is a correct fix check my comments in #14422 (comment)

I have updated it with a test

github-actions · 2021-04-08T22:39:46Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

kaxil

Sent you an invite to talk about this on Monday

ephraimbuddy · 2021-04-10T05:28:13Z

Sent you an invite to talk about this on Monday

Accepted. Thanks

kaxil · 2021-04-12T16:38:36Z

@houqp Can you take a look at this please since you worked on #10917 coz this can probably lead to the same race condition as the one you fixed

kaxil · 2021-04-14T13:39:11Z

@houqp Did you get a chance to take a look

houqp · 2021-04-16T01:21:56Z

@kaxil @ephraimbuddy sorry I was in vacation. I commented my analysis and recommended fix in #14422 (comment). @ephraimbuddy could you give that a try?

This fix will not cause race condition as far as I can tell, because it is still only executing the callback in a one process (success in local task job and failure in run_raw_task). but it will cause a regression for #11086.

houqp · 2021-04-16T01:27:29Z

@ephraimbuddy to simulate the scenario in #11086, we could send a sigkill (9) instead of sigterm (15) in the unit test to force kill the task_runner subprocess.

houqp · 2021-04-16T01:27:54Z

tests/jobs/test_local_task_job.py

+                break
+            time.sleep(0.2)
+        assert ti.state == State.RUNNING
+        os.kill(ti.pid, 15)


good unit test, thanks for adding it 👍

github-actions · 2021-04-20T09:31:40Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-04-20T19:17:06Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

houqp

LGTM assuming suggestions from @kaxil are incorporated.

github-actions · 2021-04-21T06:44:16Z

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

tests/jobs/test_local_task_job.py

Currently, on_failure_callback is only called when a task finishes executing not while executing. When a pod is deleted, a SIGTERM is sent to the task and the task is stopped immediately. The task is still running when it was killed and therefore on_failure_callback is not called. This PR makes sure that when a pod is marked for deletion and the task is killed, if the task has on_failure_callback, the callback is called

…letion

… for deletion

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>

kaxil · 2021-04-23T22:46:12Z

Failing Helm chart test is already fixed in Master by 17c38be

Currently, on_failure_callback is only called when a task finishes executing not while executing. When a pod is deleted, a SIGTERM is sent to the task and the task is stopped immediately. The task is still running when it was killed and therefore on_failure_callback is not called. This PR makes sure that when a pod is marked for deletion and the task is killed, if the task has on_failure_callback, the callback is called. Closes: #14422 (cherry picked from commit def1e7c)

i know it’s not “exact” but setting tasks to FAILED when there are more reties is soooo “upsetting” to a lot of tasks and dags that it’s not worth it.

ephraimbuddy requested review from ashb, kaxil, turbaszek and XD-DENG as code owners April 3, 2021 08:02

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from 0b8eef8 to d1658e7 Compare April 3, 2021 08:06

ephraimbuddy marked this pull request as draft April 3, 2021 10:00

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from d1658e7 to ec2f0c7 Compare April 4, 2021 16:57

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from f7121d9 to 850e583 Compare April 5, 2021 09:22

kaxil requested changes Apr 8, 2021

View reviewed changes

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from 850e583 to 02953ed Compare April 8, 2021 18:44

ephraimbuddy marked this pull request as ready for review April 8, 2021 18:46

ephraimbuddy requested a review from kaxil April 8, 2021 18:48

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from 02953ed to a6c3e34 Compare April 8, 2021 22:11

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from d472173 to ceb390b Compare April 9, 2021 11:26

ephraimbuddy closed this Apr 9, 2021

ephraimbuddy reopened this Apr 9, 2021

kaxil reviewed Apr 9, 2021

View reviewed changes

kaxil requested a review from houqp April 12, 2021 16:38

houqp reviewed Apr 16, 2021

View reviewed changes

ephraimbuddy mentioned this pull request Apr 19, 2021

on_failure_callback does not seem to fire on pod deletion/eviction #14422

Closed

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from ceb390b to 3def8f9 Compare April 20, 2021 09:10

houqp approved these changes Apr 21, 2021

View reviewed changes

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from 1ca85c1 to 67224d8 Compare April 21, 2021 06:39

ephraimbuddy closed this Apr 21, 2021

ephraimbuddy reopened this Apr 21, 2021

houqp reviewed Apr 21, 2021

View reviewed changes

tests/jobs/test_local_task_job.py Show resolved Hide resolved

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from 0a37e45 to c4e218f Compare April 23, 2021 15:59

ephraimbuddy and others added 11 commits April 23, 2021 18:13

fixup! Execute on failure callback when a pod is marked for deletion

79286cb

fixup! fixup! Execute on failure callback when a pod is marked for de…

3f9bb93

…letion

fixup! fixup! fixup! Execute on failure callback when a pod is marked…

4111c8f

… for deletion

add test for the fix

d4793ab

Revert back to the old solution

e8a6f8a

fixup! Revert back to the old solution

47dca8a

Apply suggestions from code review

6d352fe

Use sequential executor in test

7ab3e2e

Update tests/jobs/test_local_task_job.py

b9ccd0c

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>

fixup! Update tests/jobs/test_local_task_job.py

ed39194

ephraimbuddy force-pushed the call_on_failure_on_signal_receipt branch from c4e218f to ed39194 Compare April 23, 2021 17:13

kaxil merged commit def1e7c into apache:master Apr 23, 2021

kaxil deleted the call_on_failure_on_signal_receipt branch April 23, 2021 22:47

ashb modified the milestones: Airflow 2.0.3, Airflow 2.1 May 18, 2021

eejbyfeldt mentioned this pull request Jul 9, 2021

Task marked as UP_FOR_RETRY but was not retried #16847

Closed

gh4n mentioned this pull request Jul 14, 2021

on_failure_callback on DAG level is not executed #16983

Closed

hliu47 mentioned this pull request Jul 26, 2022

on_failure_callback is not called when task is terminated externally #25297

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute `on_failure_callback` when SIGTERM is received #15172

Execute `on_failure_callback` when SIGTERM is received #15172

ephraimbuddy commented Apr 3, 2021 •

edited

github-actions bot commented Apr 4, 2021

kaxil left a comment

ephraimbuddy commented Apr 8, 2021

github-actions bot commented Apr 8, 2021

kaxil left a comment

ephraimbuddy commented Apr 10, 2021

kaxil commented Apr 12, 2021 •

edited

kaxil commented Apr 14, 2021

houqp commented Apr 16, 2021

houqp commented Apr 16, 2021

houqp Apr 16, 2021

github-actions bot commented Apr 20, 2021

github-actions bot commented Apr 20, 2021

houqp left a comment

github-actions bot commented Apr 21, 2021

kaxil commented Apr 23, 2021

Execute on_failure_callback when SIGTERM is received #15172

Execute on_failure_callback when SIGTERM is received #15172

Conversation

ephraimbuddy commented Apr 3, 2021 • edited

github-actions bot commented Apr 4, 2021

kaxil left a comment

Choose a reason for hiding this comment

ephraimbuddy commented Apr 8, 2021

github-actions bot commented Apr 8, 2021

kaxil left a comment

Choose a reason for hiding this comment

ephraimbuddy commented Apr 10, 2021

kaxil commented Apr 12, 2021 • edited

kaxil commented Apr 14, 2021

houqp commented Apr 16, 2021

houqp commented Apr 16, 2021

houqp Apr 16, 2021

Choose a reason for hiding this comment

github-actions bot commented Apr 20, 2021

github-actions bot commented Apr 20, 2021

houqp left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 21, 2021

kaxil commented Apr 23, 2021

Execute `on_failure_callback` when SIGTERM is received #15172

Execute `on_failure_callback` when SIGTERM is received #15172

ephraimbuddy commented Apr 3, 2021 •

edited

kaxil commented Apr 12, 2021 •

edited