[AIRFLOW-1235] Fix webserver's odd behaviour #2330

sekikn · 2017-05-27T22:18:40Z

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

My PR addresses the following Airflow JIRA issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-1235

Description

Here are some details about my PR, including screenshots of any UI changes:

In some cases, the gunicorn master shuts down
but the webserver monitor process doesn't.
This PR add timeout functionality to shutdown
all related processes in such cases.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

tests.core:CliTests.test_cli_webserver_shutdown_when_gunicorn_master_is_killed

Commits

My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

mention-bot · 2017-05-27T22:18:42Z

@sekikn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @plypaul, @mistercrunch and @saguziel to be potential reviewers.

sekikn · 2017-05-28T20:12:11Z

CI failed on test_trigger_dag_for_date, which is reported as AIRFLOW-1245. It's irrelevant to this fix since CI failed only on Python 2.7 x PostgreSQL backend.

icorbett · 2017-08-19T19:08:25Z

AIRFLOW-1245 has been merged: #2325

sekikn · 2017-10-01T17:27:22Z

Thanks for the information @icorbett. I've just rebased the PR.

codecov-io · 2017-10-01T17:50:52Z

Codecov Report

Merging #2330 into master will increase coverage by 0.49%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #2330      +/-   ##
==========================================
+ Coverage   72.98%   73.47%   +0.49%     
==========================================
  Files         180      180              
  Lines       12654    12668      +14     
==========================================
+ Hits         9235     9308      +73     
+ Misses       3419     3360      -59

Impacted Files	Coverage Δ
airflow/exceptions.py	`100% <100%> (ø)`	⬆️
airflow/bin/cli.py	`62.73% <41.86%> (+7.71%)`	⬆️
airflow/jobs.py	`80.08% <0%> (+0.44%)`	⬆️
airflow/www/app.py	`100% <0%> (+1.04%)`	⬆️
airflow/utils/helpers.py	`54.02% <0%> (+2.87%)`	⬆️
airflow/task/task_runner/bash_task_runner.py	`100% <0%> (+6.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8754cb1...70fc3f3. Read the comment docs.

sekikn · 2018-02-24T09:41:20Z

Rebased on master.

Fokko · 2018-02-24T14:49:21Z

Why isn't this test executed on Travis?

sekikn · 2018-02-25T19:21:24Z

Thanks for the review @Fokko, I have some reasons to disable it on Travis:

AFAIR accessing pidfile doesn't work as expected on Travis for some environmental reason. Strangely, when I tried to run this test on Travis, it seemed to cause another test failure.
This test uses another thread to kill gunicorn with some delay. It's non-deterministic and can occur transient test failure.

Instead of enabling this on Travis, I ran it locally and confirmed that it succeeded.

Fokko · 2018-02-26T08:13:30Z

@sekikn Thanks for elaboration on the problem. I think we should find a way of isolating these tests. For me, a test that isn't automated, is as valuable as no test. I hope you understand, and this is something at the core of Airflow, so we have to make sure that it is tested well.

sekikn · 2018-02-26T23:51:15Z

I understand your concern. I'll update the PR if I come up with some good solution. Thanks :)

Fokko · 2018-02-27T11:02:21Z

@sekikn Thanks!

sekikn · 2018-03-03T07:00:57Z

Updated PR. Instead of killing gunicorn actually using pidfile, it uses mock.patch to emulate the situation that gunicorn doesn't respond. @Fokko would you take a look?

sekikn · 2018-03-13T23:01:03Z

Updated PR. Removed unnecessary code in newly added test as #3123 was merged.

Fokko · 2018-03-14T08:25:18Z

@sekikn Could you rebase on master?

sekikn · 2018-03-14T12:49:34Z

Rebased on master. CI failure occurred on test_authorized and it's irrelevant to this PR. It's continued for at least 3 days (e.g. https://travis-ci.org/apache/incubator-airflow/builds/351858876).

artwr

Aside from one variable which seems to need renaming, this looks good. Let's fix this and merge. @sekikn

artwr · 2018-03-21T14:36:32Z

airflow/bin/cli.py

+            if 0 < timeout and timeout <= time.time() - t:
+                raise AirflowWebServerTimeout(
+                    "No response from gunicorn master within {0} seconds"
+                    .format(master_timeout))


Should this be called just timeout?

Exactly. I'll update the PR. Thanks!

In some cases, the gunicorn master shuts down but the webserver monitor process doesn't. This PR add timeout functionality to shutdown all related processes in such cases.

johnarnold · 2018-03-22T00:20:07Z

+1 I like it.

johnarnold · 2018-03-22T18:43:54Z

@artwr merge?

artwr · 2018-03-22T18:52:15Z

Done

sekikn · 2018-03-23T00:32:46Z

@artwr @johnarnold Thanks a lot!

In some cases, the gunicorn master shuts down but the webserver monitor process doesn't. This PR add timeout functionality to shutdown all related processes in such cases. Dear Airflow maintainers, Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below! ### JIRA - [x] My PR addresses the following [Airflow JIRA] (https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR" - https://issues.apache.org/jira/browse/AIRFLOW-1235 ### Description - [x] Here are some details about my PR, including screenshots of any UI changes: In some cases, the gunicorn master shuts down but the webserver monitor process doesn't. This PR add timeout functionality to shutdown all related processes in such cases. ### Tests - [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason: tests.core:CliTests.test_cli_webserver_shutdown_wh en_gunicorn_master_is_killed ### Commits - [x] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git- commit/)": 1. Subject is separated from body by a blank line 2. Subject is limited to 50 characters 3. Subject does not end with a period 4. Subject uses the imperative mood ("add", not "adding") 5. Body wraps at 72 characters 6. Body explains "what" and "why", not "how" Closes apache#2330 from sekikn/AIRFLOW-1235

sekikn force-pushed the AIRFLOW-1235 branch from 8dc0b62 to ff9c63e Compare October 1, 2017 17:24

sekikn force-pushed the AIRFLOW-1235 branch 2 times, most recently from f10acf9 to ba1c283 Compare February 24, 2018 07:58

sekikn force-pushed the AIRFLOW-1235 branch from ba1c283 to a93b833 Compare March 3, 2018 06:09

sekikn force-pushed the AIRFLOW-1235 branch 2 times, most recently from 14065d2 to 52c944d Compare March 13, 2018 22:59

sekikn force-pushed the AIRFLOW-1235 branch from 52c944d to e363fb8 Compare March 14, 2018 11:55

sekikn mentioned this pull request Mar 21, 2018

[AIRFLOW-1235] In webserver, when gunicorn master dies, bubble up exc #3144

Closed

2 tasks

artwr reviewed Mar 21, 2018

View reviewed changes

[AIRFLOW-1235] Fix webserver's odd behaviour

70fc3f3

In some cases, the gunicorn master shuts down but the webserver monitor process doesn't. This PR add timeout functionality to shutdown all related processes in such cases.

sekikn force-pushed the AIRFLOW-1235 branch from e363fb8 to 70fc3f3 Compare March 21, 2018 14:53

asfgit closed this in 7e762d4 Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-1235] Fix webserver's odd behaviour #2330

[AIRFLOW-1235] Fix webserver's odd behaviour #2330

sekikn commented May 27, 2017

mention-bot commented May 27, 2017

sekikn commented May 28, 2017

icorbett commented Aug 19, 2017 •

edited

sekikn commented Oct 1, 2017

codecov-io commented Oct 1, 2017 •

edited

sekikn commented Feb 24, 2018

Fokko commented Feb 24, 2018

sekikn commented Feb 25, 2018 •

edited

Fokko commented Feb 26, 2018

sekikn commented Feb 26, 2018

Fokko commented Feb 27, 2018

sekikn commented Mar 3, 2018

sekikn commented Mar 13, 2018

Fokko commented Mar 14, 2018

sekikn commented Mar 14, 2018

artwr left a comment

artwr Mar 21, 2018

sekikn Mar 21, 2018

johnarnold commented Mar 22, 2018

johnarnold commented Mar 22, 2018

artwr commented Mar 22, 2018 •

edited

sekikn commented Mar 23, 2018

[AIRFLOW-1235] Fix webserver's odd behaviour #2330

[AIRFLOW-1235] Fix webserver's odd behaviour #2330

Conversation

sekikn commented May 27, 2017

JIRA

Description

Tests

Commits

mention-bot commented May 27, 2017

sekikn commented May 28, 2017

icorbett commented Aug 19, 2017 • edited

sekikn commented Oct 1, 2017

codecov-io commented Oct 1, 2017 • edited

Codecov Report

sekikn commented Feb 24, 2018

Fokko commented Feb 24, 2018

sekikn commented Feb 25, 2018 • edited

Fokko commented Feb 26, 2018

sekikn commented Feb 26, 2018

Fokko commented Feb 27, 2018

sekikn commented Mar 3, 2018

sekikn commented Mar 13, 2018

Fokko commented Mar 14, 2018

sekikn commented Mar 14, 2018

artwr left a comment

Choose a reason for hiding this comment

artwr Mar 21, 2018

Choose a reason for hiding this comment

sekikn Mar 21, 2018

Choose a reason for hiding this comment

johnarnold commented Mar 22, 2018

johnarnold commented Mar 22, 2018

artwr commented Mar 22, 2018 • edited

sekikn commented Mar 23, 2018

icorbett commented Aug 19, 2017 •

edited

codecov-io commented Oct 1, 2017 •

edited

sekikn commented Feb 25, 2018 •

edited

artwr commented Mar 22, 2018 •

edited