Visitar URL original
Fix task retries when they receive sigkill and have retries. Properly handle sigterm too by ephraimbuddy · Pull Request #16301 · apache/airflow · GitHub
Skip to content

Fix task retries when they receive sigkill and have retries. Properly handle sigterm too - #16301

Merged
ephraimbuddy merged 1 commit into
apache:mainfrom
astronomer:fix-task-retries-on-sigkill
Jul 28, 2021
Merged

Fix task retries when they receive sigkill and have retries. Properly handle sigterm too#16301
ephraimbuddy merged 1 commit into
apache:mainfrom
astronomer:fix-task-retries-on-sigkill

Conversation

@ephraimbuddy

@ephraimbuddy ephraimbuddy commented Jun 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes: #16285

Currently, tasks are not retried when they receive sigkill even if the task has retries. Sigkill is uncatchable on task runner. However, sigterm on task runner fails tasks without retry. This change fixes it
and added test for both sigterm and sigkill so we don't experience regression

This change also removes quarantined marker on tests that had to do with changing tasks states

^ 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.

@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch 2 times, most recently from 034516f to 0f9ac16 Compare June 8, 2021 08:31
@ephraimbuddy ephraimbuddy changed the title Fix task retries when they receive sigkill and have retries Fix task retries when they receive sigkill and have retries. Properly handle sigterm too Jun 8, 2021
@ephraimbuddy ephraimbuddy reopened this Jun 8, 2021
uranusjr
uranusjr reviewed Jun 8, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch from 0f9ac16 to 059a1ba Compare June 8, 2021 13:14
ephraimbuddy
ephraimbuddy commented Jun 8, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
@kaxil kaxil closed this Jun 9, 2021
@kaxil kaxil reopened this Jun 9, 2021
@ephraimbuddy ephraimbuddy reopened this Jun 9, 2021
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch 4 times, most recently from aba146b to 6eff8e3 Compare June 10, 2021 07:13
@ephraimbuddy ephraimbuddy reopened this Jun 10, 2021
ashb
ashb reviewed Jun 10, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
ashb
ashb reviewed Jun 10, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
ashb
ashb reviewed Jun 10, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
ashb
ashb reviewed Jun 10, 2021
View reviewed changes
Comment thread airflow/jobs/local_task_job.py Resolved
ashb
ashb requested changes Jun 10, 2021
View reviewed changes
Comment thread tests/jobs/test_local_task_job.py Resolved
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch 3 times, most recently from 8753159 to 459dcab Compare June 10, 2021 21:53
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch 2 times, most recently from ff43dbe to 2a91d4f Compare July 27, 2021 08:00
ashb
ashb reviewed Jul 27, 2021
View reviewed changes
Comment thread tests/jobs/test_local_task_job.py Resolved
ashb
ashb reviewed Jul 27, 2021
View reviewed changes
Comment thread tests/jobs/test_local_task_job.py Resolved
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch from f316c5f to d3b176c Compare July 27, 2021 10:06
ephraimbuddy
ephraimbuddy commented Jul 27, 2021
View reviewed changes
Comment thread tests/models/test_taskinstance.py Resolved
@kaxil

kaxil commented Jul 27, 2021

Copy link
Copy Markdown
Member

All the MSSQL tests are failing, not sure if they are related or not. The static check failure is unrelated and caused by #17237 - @ashb is fixing it

@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch from 0e3fd32 to 1f57248 Compare July 27, 2021 15:45
@ephraimbuddy ephraimbuddy reopened this Jul 27, 2021
@ephraimbuddy
Fix task retries when they receive sigkill and have retries and prope…
089a3c2 
…rly handle sigterm

Currently tasks are not retried when they receive sigkill or sigterm even if the task has retries. This change fixes it
and added test for both sigterm and sigkill so we don't experience regression

Before, sigterm sets the task as failed and raises AirflowException which heartbeat sometimes see as externally set to fail
and not call failure_callbacks. This commit also fixes this by calling handle_task_exit when task gets sigterm

add comment about exit code 143

fix tests

Apply suggestions from code review

fixup! Apply suggestions from code review

fixup! fixup! Apply suggestions from code review

fixup! fixup! fixup! Apply suggestions from code review

Properly test sigkill and sigterm fort 'ti'

fixup! Properly test sigkill and sigterm fort 'ti'

Move sigterm test for tasks to taskinstance.py

fix sigkill

fixup! fix sigkill

Fix tests and move retry callback out of finished callback

Apply suggestions from code review

Fix conflict

fixup! Apply suggestions from code review

fixup! fixup! Apply suggestions from code review

fixup! Fix task retries when they receive sigkill and have retries and properly handle sigterm

fixup! fixup! Fix task retries when they receive sigkill and have retries and properly handle sigterm

Fix and remove other tests marked quarantined that changes task state

Properly document each tests and set error to None when there's sigkill/sigterm

remove task instantiation as it's not needed

Take back retry callback to finished callback since finished callback is always run

fixup! Fix task retries when they receive sigkill and have retries and properly handle sigterm

fixup! Fix task retries when they receive sigkill and have retries and properly handle sigterm

fixup! fixup! Fix task retries when they receive sigkill and have retries and properly handle sigterm

Apply suggestions from code review

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>

fixup! Apply suggestions from code review
@ephraimbuddy
ephraimbuddy force-pushed the fix-task-retries-on-sigkill branch from 1f57248 to 089a3c2 Compare July 27, 2021 20:46
@uranusjr uranusjr closed this Jul 28, 2021
@uranusjr uranusjr reopened this Jul 28, 2021
@ephraimbuddy
ephraimbuddy merged commit 4e2a94c into apache:main Jul 28, 2021
@ephraimbuddy
ephraimbuddy deleted the fix-task-retries-on-sigkill branch July 28, 2021 14:57
@ephraimbuddy ephraimbuddy mentioned this pull request Jul 28, 2021
Closed
@jedcunningham jedcunningham mentioned this pull request Jul 29, 2021
Closed
This was referenced Aug 3, 2021
Closed
Closed
@ephraimbuddy ephraimbuddy mentioned this pull request Aug 9, 2021
Closed
jhtimmins pushed a commit that referenced this pull request Aug 12, 2021
@ephraimbuddy @ashb @jhtimmins
Fix task retries when they receive sigkill and have retries and prope…
1be2003 
…rly handle sigterm (#16301)

Currently, tasks are not retried when they receive SIGKILL or SIGTERM even if the task has retry. This change fixes it
and added test for both SIGTERM and SIGKILL so we don't experience regression

Also, SIGTERM sets the task as failed and raises AirflowException which heartbeat sometimes see as externally set to fail
and not call failure_callbacks. This commit also fixes this by calling handle_task_exit when a task gets SIGTERM

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 4e2a94c)
kaxil pushed a commit that referenced this pull request Aug 13, 2021
@ephraimbuddy @ashb @kaxil
Fix task retries when they receive sigkill and have retries and prope…
96092eb 
…rly handle sigterm (#16301)

Currently, tasks are not retried when they receive SIGKILL or SIGTERM even if the task has retry. This change fixes it
and added test for both SIGTERM and SIGKILL so we don't experience regression

Also, SIGTERM sets the task as failed and raises AirflowException which heartbeat sometimes see as externally set to fail
and not call failure_callbacks. This commit also fixes this by calling handle_task_exit when a task gets SIGTERM

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 4e2a94c)
kaxil pushed a commit that referenced this pull request Aug 13, 2021
@ephraimbuddy @ashb @kaxil
Fix task retries when they receive sigkill and have retries and prope…
5e097c7 
…rly handle sigterm (#16301)

Currently, tasks are not retried when they receive SIGKILL or SIGTERM even if the task has retry. This change fixes it
and added test for both SIGTERM and SIGKILL so we don't experience regression

Also, SIGTERM sets the task as failed and raises AirflowException which heartbeat sometimes see as externally set to fail
and not call failure_callbacks. This commit also fixes this by calling handle_task_exit when a task gets SIGTERM

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 4e2a94c)
@WattsInABox WattsInABox mentioned this pull request Sep 17, 2021
Closed
1 task done
@ephraimbuddy ephraimbuddy mentioned this pull request Oct 4, 2021
Closed
@hliu47 hliu47 mentioned this pull request Jul 26, 2022
Closed
1 of 2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uranusjr uranusjr uranusjr approved these changes
@kaxil kaxil kaxil left review comments
@ashb ashb ashb approved these changes
@XD-DENG XD-DENG Awaiting requested review from XD-DENG
@turbaszek turbaszek Awaiting requested review from turbaszek
@dimberman dimberman Awaiting requested review from dimberman
@mik-laj mik-laj Awaiting requested review from mik-laj
@potiuk potiuk Awaiting requested review from potiuk
@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton
@vikramkoka vikramkoka Awaiting requested review from vikramkoka

Assignees

No one assigned

Labels

area:scheduler full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Milestone

Airflow 2.1.3

Development

Successfully merging this pull request may close these issues.

Airflow 2.1.0 doesn't retry a task if it externally killed

4 participants

@ephraimbuddy @kaxil @ashb @uranusjr