Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(docker): Make Gunicorn max_requests and max_requests_jitter adjustable #20733

Merged
merged 1 commit into from
Jul 17, 2022
Merged

fix(docker): Make Gunicorn max_requests and max_requests_jitter adjustable #20733

merged 1 commit into from
Jul 17, 2022

Conversation

mdeshmu
Copy link
Contributor

@mdeshmu mdeshmu commented Jul 17, 2022

SUMMARY

This PR allows to adjust Gunicorn max_requests and max_requests_jitter which allows to restart the Gunicorn worker process periodically.

This is a simple method to help limit the damage of memory leaks like one reported in 15132.

For more details, Please refer to: https://docs.gunicorn.org/en/stable/settings.html#max-requests

Default values are set to zero to match gunicorn's default i.e. automatic worker restarts are disabled by default.

Note: This change is non-disruptive, no end user will be impacted unless they turn on settings.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

memory1

TESTING INSTRUCTIONS

Set the values like:
gunicorn --max-requests 1024 --max-requests-jitter 100 ....

ADDITIONAL INFORMATION

  • Has associated issue: 15132
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@mdeshmu
Copy link
Contributor Author

mdeshmu commented Jul 17, 2022

@zhaoyongjie please review and approve.

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, it's a useful workaround for memory leaks.

@zhaoyongjie zhaoyongjie merged commit 8832410 into apache:master Jul 17, 2022
@mdeshmu mdeshmu deleted the fix-make-gunicorn-max-requests-adjustable branch July 17, 2022 13:01
@zac-roberts
Copy link

Seeing this in logs during docker compose (posting in case related):

superset_app | gunicorn: error: argument --max-requests: expected one argument

@zhaoyongjie
Copy link
Member

zhaoyongjie commented Jul 18, 2022

Seeing this in logs during docker compose (posting in case related):

superset_app | gunicorn: error: argument --max-requests: expected one argument

getting default value should be ${parameter:-word} instead of ${parameter:word}. I will push the fix.

@mdeshmu
Copy link
Contributor Author

mdeshmu commented Jul 18, 2022

sorry. my bad, i missed the hyphen. Thanks for reporting @z6cr0b3rts and thanks for the quick fix @zhaoyongjie.

@evkuzin
Copy link

evkuzin commented Jul 18, 2022

What's the timeline on the docker hub image? How long would it take to release a new image? The one published is broken now. @zhaoyongjie

@mdeshmu
Copy link
Contributor Author

mdeshmu commented Jul 18, 2022

@evkuzin updated build is available now.

@low-on-mana
Copy link

We have tried this solution sometime back with custom docker image, but faced lot of intermittent http504s.
Our setup to serve requests is aws alb -> superset pods in k8.
Please share your findings as well @mdeshmu @zhaoyongjie @evkuzin

@mdeshmu
Copy link
Contributor Author

mdeshmu commented Jul 21, 2022

@low-on-mana We have it enabled for a while now but I don’t see any 504's.

For you, Is it 502 bad gateway or 504 gateway time out?

@low-on-mana
Copy link

504 gateway timeout @mdeshmu .

@mdeshmu
Copy link
Contributor Author

mdeshmu commented Jul 22, 2022

@low-on-mana Please make sure you have appropriate jitter parameter value as it is needed to avoid simultaneous restarts of workers.

Another possibility, Maybe your queries are occasionally taking longer time. Please make sure you are having large enough timeout values at all places.

This is what i have,

ALB idle timeout (120s) >
GUNICORN_TIMEOUT (115s) =
SUPERSET_WEBSERVER_TIMEOUT

I also have ecs task (gunicorn + superset) directly under alb and was intermittently getting 502 bad gateway because of low keep alive of gunicorn. Then i set it below and all errors were gone.

GUNICORN_KEEPALIVE (125s) >
ALB idle timeout (120s)

@low-on-mana
Copy link

@mdeshmu Thanks for sharing this, I will check these configuration out.

eschutho pushed a commit that referenced this pull request Sep 20, 2022
Co-authored-by: Multazim Deshmukh <multazim.deshmukh@morningstar.com>
(cherry picked from commit 8832410)
Fahrenheit35 pushed a commit to Fahrenheit35/superset that referenced this pull request Nov 11, 2022
Co-authored-by: Multazim Deshmukh <multazim.deshmukh@morningstar.com>
(cherry picked from commit 8832410)
@mistercrunch mistercrunch added 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS v2.0 v2.0.1 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants