Fix Celery executor getting stuck randomly because of reset_signals in multiprocessing #15989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuqian90
requested review from
ashb,
kaxil,
potiuk,
turbaszek and
XD-DENG
as code owners
boring-cyborg
bot
added
area:dev-tools
area:Scheduler
yuqian90
commented
May 21, 2021
yuqian90
force-pushed
the
celery_executor_stuck
branch
from
1e88cd4 to
d7e7035
Compare
|
We also had the same intermittent issues on our production servers. I wanted to help test this so I deployed this PR and the problem hasn't happened since. Previously, the scheduler would hang once or twice per day on average. It's been running fine for 48 hours now. I will update this comment if something happens, but you can assume that no news is good news. Thank you for figuring this out! |
|
Oh yes, will take a look. |
ashb
approved these changes
May 27, 2021
|
Could you just address the few small niggles (mostly to minimize the change so it is easier to backport, and easier to read.) |
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
yuqian90
force-pushed
the
celery_executor_stuck
branch
from
d7e7035 to
048c493
Compare
Thanks. These have been addressed. |
yuqian90
force-pushed
the
celery_executor_stuck
branch
from
048c493 to
7caba4f
Compare
yuqian90
added a commit
that referenced
this pull request
May 29, 2021
…n multiprocessing (#15989) Fixes #15938 multiprocessing.Pool is known to often become stuck. It causes celery_executor to hang randomly. This happens at least on Debian, Ubuntu using Python 3.8.7 and Python 3.8.10. The issue is reproducible by running test_send_tasks_to_celery_hang in this PR several times (with db backend set to something other than sqlite because sqlite disables some parallelization) The issue goes away once switched to concurrent.futures.ProcessPoolExecutor. In python 3.6 and earlier, ProcessPoolExecutor has no initializer argument. Fortunately, it's not needed because reset_signal is no longer needed because the signal handler now checks if the current process is the parent. (cherry picked from commit f75dd7a)
potiuk
added a commit
to potiuk/airflow
that referenced
this pull request
May 30, 2021
The test_send_tasks_to_celery_hang hangs on self-hosted runners more often than not. It's been introduced in apache#15989 and while the test does not usually hang on regular GitHub runners, or in case of running it locally (I could not make it fail), it does hang almost always when run on self-hosted runners. Marking it as quarantined for now. Issue apache#16168 created to keep track of it.
potiuk
added a commit
that referenced
this pull request
May 30, 2021
The test_send_tasks_to_celery_hang hangs on self-hosted runners more often than not. It's been introduced in #15989 and while the test does not usually hang on regular GitHub runners, or in case of running it locally (I could not make it fail), it does hang almost always when run on self-hosted runners. Marking it as quarantined for now. Issue #16168 created to keep track of it.
jhtimmins
pushed a commit
to astronomer/airflow
that referenced
this pull request
Jun 3, 2021
…n multiprocessing (apache#15989) Fixes apache#15938 multiprocessing.Pool is known to often become stuck. It causes celery_executor to hang randomly. This happens at least on Debian, Ubuntu using Python 3.8.7 and Python 3.8.10. The issue is reproducible by running test_send_tasks_to_celery_hang in this PR several times (with db backend set to something other than sqlite because sqlite disables some parallelization) The issue goes away once switched to concurrent.futures.ProcessPoolExecutor. In python 3.6 and earlier, ProcessPoolExecutor has no initializer argument. Fortunately, it's not needed because reset_signal is no longer needed because the signal handler now checks if the current process is the parent. (cherry picked from commit f75dd7a)
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15938
multiprocessing.Poolis known to often become stuck. It causes celery_executor to hang randomly. This happens at least on Debian, Ubuntu using Python 3.8.7 and Python 3.8.10. The issue is reproducible by runningtest_send_tasks_to_celery_hangin this PR several times (with db backend set to something other than sqlite because sqlite disables some parallelization)The issue goes away once switched to
concurrent.futures.ProcessPoolExecutor. In python 3.6 and earlier,ProcessPoolExecutorhas noinitializerargument. Fortunately, it's not needed becausereset_signalis no longer needed because the signal handler now checks if the current process is the parent.