Fix reload gunicorn workers by AVMusorin · Pull Request #32102 · apache/airflow

AVMusorin · 2023-06-23T17:02:45Z

Since gunicorn can't reload a new code if starts with --preload setting, we need to check reload_on_plugin_change before set it up.

Gunicorn can't reload a new code because the code is preloaded into the master process and worker are launched with fork, they will still have the old code.

potiuk

I think there was a good reason we added --preload, https://github.com/apache/airflow/pull/27297/files and it was to remove lots of integrity warnings generated by parallel gunicorns trying to update the database in parallell and generating race conditions.

This cahnge means that when "reload_on_plugin_change" is turned on, we will start getting those warnings, unless the root cause of the problem has been fixed (I think it hasn't been fixed).

So I think this change only makes sense to be implemented if we find another way to prevent the integrity errors. For example adding some locking strategy for the DB operations that generated integrity warnings - without it, it will be a regression.

potiuk · 2023-06-28T10:35:40Z

cc: @ephraimbuddy ^^ would love your comment on that one

ephraimbuddy · 2023-06-28T11:10:28Z

I think there was a good reason we added --preload, https://github.com/apache/airflow/pull/27297/files and it was to remove lots of integrity warnings generated by parallel gunicorns trying to update the database in parallell and generating race conditions.

This cahnge means that when "reload_on_plugin_change" is turned on, we will start getting those warnings, unless the root cause of the problem has been fixed (I think it hasn't been fixed).

So I think this change only makes sense to be implemented if we find another way to prevent the integrity errors. For example adding some locking strategy for the DB operations that generated integrity warnings - without it, it will be a regression.

Exactly my thoughts. Without fixing the source of the IntegrityErrors, this change will be a regression.

@AVMusorin How about adding a new configuration that when set to True(default), enables this preloading instead of using the plugin's config? That way it won't be a breaking change.

AVMusorin · 2023-06-28T12:13:13Z

How about adding a new configuration that when set to True(default), enables this preloading instead of using the plugin's config? That way it won't be a breaking change.

Agree, no problem for me.
Thanks for review.

potiuk · 2023-06-29T13:32:11Z

How about adding a new configuration that when set to True(default), enables this preloading instead of using the plugin's config? That way it won't be a breaking change.

Agree, no problem for me. Thanks for review.

OK. Ping us when added. Fine for me too.

ephraimbuddy · 2023-07-27T20:28:45Z

After much debugging on this, I found that once --preload is used, you cannot reload plugins even if you set reload_on_plugin_change to True. The reason is that the flask app is loaded into gunicorn worker together with the plugins:

airflow/airflow/www/app.py

Line 162 in 9d68492

init_plugins(flask_app)

.

I'd suggest using the reload_on_plugin_change to toggle the --preload. What do you think @potiuk

potiuk · 2023-07-30T14:52:38Z

I'd suggest using the reload_on_plugin_change to toggle the --preload. What do you think @potiuk

Sure, if you think it's fine - but it will bring the previous behaviour if reload_on_plugin_change is true - not sure if the effect of it was devastating or just Logs with IntegrityError - this what we are opening to.

If you think It's fine for me - so I won't block it for 2.7.0 - reloading on plugin change is useful, and maybe having Integrity check errors is the price to pay. Maybe it would be a good idea to add a comment for that option that when it is enabled it will (or may? not sure if it happened always) produce those kind of errors and this is expected...

Removing my "require changes" after Ephraim's comments.

AVMusorin · 2023-08-01T04:40:12Z

Maybe it would be a good idea to add a comment for that option that when it is enabled it will (or may? not sure if it happened always) produce those kind of errors and this is expected...

There was a good description in comments, I moved it to warning message.

Since gunicorn can't reload a new code if starts with ``--preload`` setting, we need to check ``reload_on_plugin_change`` before set it up. Gunicorn can't reload a new code because the code is preloaded into the master process and worker are launched with ``fork``, they will still have the old code.

ephraimbuddy · 2023-08-01T10:16:43Z

I'd suggest using the reload_on_plugin_change to toggle the --preload. What do you think @potiuk

Sure, if you think it's fine - but it will bring the previous behaviour if reload_on_plugin_change is true - not sure if the effect of it was devastating or just Logs with IntegrityError - this what we are opening to.

If you think It's fine for me - so I won't block it for 2.7.0 - reloading on plugin change is useful, and maybe having Integrity check errors is the price to pay. Maybe it would be a good idea to add a comment for that option that when it is enabled it will (or may? not sure if it happened always) produce those kind of errors and this is expected...

Would like to hear what @jedcunningham @dstandish and @ashb think about this. I will request the changes back until we have agreed on this. In production, it doesn't make sense to reload plugin, so this gives me concern

ephraimbuddy

To prevent accidental merge

ashb · 2023-08-01T12:14:25Z

The whole point of preload is quite literally to pre-load all the code so that the workers start up quicker. The downside to that is they code changes anywhere won't be reflected.

If you want plugins to reload don't use preload.

The pr message as it stands looks good to me

Co-authored-by: Jed Cunningham <[email protected]>

uranusjr

I added a couple of commits to refine the message. The warning messag especially has some wording issues.

* Toggle gunicorn --preload with reload_on_plugin_change Since gunicorn can't reload a new code if starts with ``--preload`` setting, we need to check ``reload_on_plugin_change`` before set it up. Gunicorn can't reload a new code because the code is preloaded into the master process and worker are launched with ``fork``, they will still have the old code. * added warning message * Improve warning message Co-authored-by: Jed Cunningham <[email protected]> * Improve warning message Co-authored-by: Jed Cunningham <[email protected]> * Improve warning message Co-authored-by: Jed Cunningham <[email protected]> * Minor tweak of warning message * Mention prod issue in config description --------- Co-authored-by: Jed Cunningham <[email protected]> Co-authored-by: Tzu-ping Chung <[email protected]> (cherry picked from commit f78a836)

mujiannan · 2024-01-06T18:56:09Z

I think we should allow users to set auto-reload-when-plugins-change for scheduler also. The parsing_pre_import_modules doesn't take effect in some conditions, for example, a new-registered custom timetable in a new plugin.

potiuk · 2024-01-06T19:02:47Z

I think we should allow users to set auto-reload-when-plugins-change for scheduler also. The parsing_pre_import_modules doesn't take effect in some conditions, for example, a new-registered custom timetable in a new plugin.

Feel free to open PRs for that Or create a feature proposal. Commenting on closed issue is a straight way to your comment being completely ignored.

boring-cyborg Bot added area:CLI area:webserver Webserver related Issues labels Jun 23, 2023

AVMusorin force-pushed the fix-gunicorn-reload branch from 9edbb17 to 13f12b0 Compare June 23, 2023 17:03

potiuk requested a review from ephraimbuddy June 23, 2023 17:11

uranusjr approved these changes Jun 28, 2023

View reviewed changes

potiuk previously requested changes Jun 28, 2023

View reviewed changes

potiuk mentioned this pull request Jul 21, 2023

Make "--preload" flag optional #30910

Closed

2 tasks

potiuk linked an issue Jul 21, 2023 that may be closed by this pull request

Make "--preload" flag optional #30910

Closed

2 tasks

AVMusorin force-pushed the fix-gunicorn-reload branch from 13f12b0 to 3287fab Compare July 28, 2023 09:14

potiuk force-pushed the fix-gunicorn-reload branch from 3287fab to 1418f49 Compare July 30, 2023 14:53

AVMusorin added 2 commits August 1, 2023 08:21

added warning message

c37f687

AVMusorin force-pushed the fix-gunicorn-reload branch from c5080af to c37f687 Compare August 1, 2023 06:22

ephraimbuddy requested changes Aug 1, 2023

View reviewed changes

jedcunningham reviewed Aug 1, 2023

View reviewed changes

Comment thread airflow/cli/commands/webserver_command.py Outdated

Comment thread airflow/cli/commands/webserver_command.py Outdated

Comment thread airflow/config_templates/config.yml Outdated

AVMusorin and others added 3 commits August 2, 2023 08:44

Improve warning message

2435fea

Co-authored-by: Jed Cunningham <[email protected]>

Improve warning message

50a0405

Co-authored-by: Jed Cunningham <[email protected]>

Improve warning message

4a2f15d

Co-authored-by: Jed Cunningham <[email protected]>

jedcunningham approved these changes Aug 2, 2023

View reviewed changes

AVMusorin requested a review from ephraimbuddy August 2, 2023 19:15

ephraimbuddy approved these changes Aug 6, 2023

View reviewed changes

ephraimbuddy added this to the Airflow 2.7.0 milestone Aug 6, 2023

uranusjr added 2 commits August 7, 2023 12:39

Minor tweak of warning message

55fd413

Mention prod issue in config description

ebb0cb1

uranusjr approved these changes Aug 7, 2023

View reviewed changes

ephraimbuddy merged commit f78a836 into apache:main Aug 7, 2023

AVMusorin deleted the fix-gunicorn-reload branch August 7, 2023 08:17

ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 9, 2023

Uh oh!

Conversation

AVMusorin commented Jun 23, 2023

Uh oh!

potiuk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk commented Jun 28, 2023

Uh oh!

ephraimbuddy commented Jun 28, 2023

Uh oh!

AVMusorin commented Jun 28, 2023

Uh oh!

potiuk commented Jun 29, 2023

Uh oh!

ephraimbuddy commented Jul 27, 2023

Uh oh!

potiuk commented Jul 30, 2023

Uh oh!

AVMusorin commented Aug 1, 2023

Uh oh!

ephraimbuddy commented Aug 1, 2023

Uh oh!

ephraimbuddy left a comment

Choose a reason for hiding this comment

Uh oh!

ashb commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

mujiannan commented Jan 6, 2024

Uh oh!

potiuk commented Jan 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

potiuk left a comment •

edited

Loading

ashb commented Aug 1, 2023 •

edited

Loading