[WIP] feat: replace uwsgi with granian#1321
Conversation
mlabeeb03
force-pushed
the
labeeb/replace-uwsgi-with-granian
branch
from
481f368 to
e527b3e
Compare
|
Just a warning that I have a suspicion that MIT's switch to granian may have been responsible for exposing a multi-threading related bug in grading. This wouldn't be any fault of granian's, but more a case where granian's deployment strategy exposes already-existing problems. This is highly speculative on my part, and the investigation is ongoing. I just wanted to bring this to your attention early. The bug needs to be fixed regardless, so I don't expect it to block granian in the long term, even if this hypothesis turns out to be true. |
mlabeeb03
force-pushed
the
labeeb/replace-uwsgi-with-granian
branch
2 times, most recently
from
7becdfe to
c5d2e23
Compare
| --reload | ||
| --reload-paths /openedx/granian-reload-path |
There was a problem hiding this comment.
This is for watching a directory and reloading if there are file changes detected, right? Just out of curiosity, why enable this in a prod environment?
There was a problem hiding this comment.
We had a reload-uwsgi command as well. Here is the rational behind doing it. This is just a replacement for that. Now the reload-granian command puts current timestamp in the file mentioned in --reload-paths to trigger a reload.
|
Quick update: We believe the multi-threading issue has been fixed, though we're still waiting for MIT to deploy it on their instance next week to verify that it resolves the issues they've been seeing there. |
|
Belated update: The fix worked and the multi-threading issue has been resolved as far as we can tell. |
Abdul-Muqadim-Arbisoft
moved this from In Progress
to Backlog
in Tutor project management
|
I've bumped Granian to 2.7.4. I verified that it now serves static files correctly for both /media and /static. As for the I validated this with the same load test benchmark used in the earlier PR. Granian completed the test without any 502 responses, so I think we can mark this action item done as well.
The only remaining action items are the flags, and we still need to scope them and decide what to do with them. |
The version bump resolves the problem of serving multiple static paths
HammadYousaf01
force-pushed
the
labeeb/replace-uwsgi-with-granian
branch
from
8bb7a79 to
4dcfa6f
Compare
ormsbee
left a comment
ormsbee
left a comment
There was a problem hiding this comment.
A few more questions, but it's looking great. I'm so glad to hear that we can map multiple static dirs now.
The only remaining action items are the flags, and we still need to scope them and decide what to do with them.
Do you mean that you're still looking at the exact config values that should ship by default, or does this mean something else?
Thank you!
| cli(args=parse_args_file(), standalone_mode=False) | ||
|
|
||
| if __name__ == "__main__": | ||
| run() No newline at end of file |
There was a problem hiding this comment.
In general, please add a trailing new line in your files.
| @@ -0,0 +1,33 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Please add a docstring explaining why this is necessary (I realize it's because granian doesn't support a config file format, but this might be confusing to people who don't know that.)
|
|
||
|
|
||
| def run(): | ||
| cli(args=parse_args_file(), standalone_mode=False) |
There was a problem hiding this comment.
Likewise, please comment what standalone_mode=False is, so that people understand that it's related to click and the process, and not a separate arg for granian.
| --interface wsgi | ||
| --host 0.0.0.0 | ||
| --port 8000 | ||
| --workers $GRANIAN_WORKERS |
There was a problem hiding this comment.
Not blocking, but I'm curious if you folks are planning to experiment with different threading values?
| --static-path-mount /openedx/media/ | ||
| --reload | ||
| --reload-paths /openedx/granian-reload-path | ||
| --http1-buffer-size 8192 |
There was a problem hiding this comment.
This seems really low? Is this because we're not actually expecting to get HTTP 1.1 requests?
…with-granian # Conflicts: # tutor/templates/local/docker-compose.yml
7 participants
PLEASE REACH OUT TO @ahmed-arb FOR UPDATES ON THIS PR.
closes #937
This PR replaces uwsgi with granian.
All uwsgi related docs have been updated.
All uwsgi related patches and configs values have been renamed.
All uwsgi related files have been deleted and replaced with granian equivalents.
Here are the uwsgi configs/features we are currently using and their granian equivalents.
static-map = /static=/openedx/staticfiles/
static-map = /media=/openedx/media/
Even though
--static-path-route&--static-path-mountare available, they can only server from one path, in order to server both of them we must use caddy.http = 0.0.0.0:8000
--host&--portbuffer-size = 8192
--http1-buffer-size 8192
wsgi-file = $(SERVICE_VARIANT)/wsgi.py
Use this parameter
$SERVICE_VARIANT.wsgi:applicationprocesses = $(UWSGI_WORKERS)
--workersApparently granian does not have thundering herd problem, see Related Granian discussion
single-interpreter = true
enable-threads = true
Threads are enabled by default. You can explicitly set them with
--runtime-mode. Set the number of threads with--runtime-threads.http-keepalive = 1
--http1-keep-alive enabled by default
❌ add-header = Connection: Keep-Alive
Can not add arbitrary headers with granian, does this affect us if we have
--http1-keep-alive?die-on-term = true
Granian handles SIGTERM as a shutdown signal by default
lazy-apps = false
need-app = true
no-defer-accept = true
master = true
py-call-osafterfork = true
vacuum = true
Reload
We can define a file in the
--reload-pathsand changing that file will restart the server. We also need to install thereloaddependency and also unable the--reloadconfig.