-
Notifications
You must be signed in to change notification settings - Fork 29
Timeout before the openshift route times out #207
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
Timeout before the openshift route times out #207
Conversation
dsavineau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removed some } char from the nginx configuration
nginx: [emerg] unexpected end of file, expecting "}" in /etc/nginx/nginx.conf:92I think you also need to update the CRD otherwise users won't be able to customize the timeout value
|
Apologies folks, I meant to set this as a draft. |
16d3a49 to
3fb99a2
Compare
|
Confirmed I was able to sync and download a collection from my galaxy with these changes in play. |
|
@fao89 @dsavineau I have addressed your comments and gotten this into a non-draft stage, it's ready for your review. :) |
3fb99a2 to
33e483a
Compare
dsavineau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see changes in the CRD related to client_request_timeout so one won't be able to update that value.
not timing out before undercuts usefulness of our log-traceback-middleware in django-ansible-base that logs a traceback from requests that get timed out -- because uwsgi or gunicorn has to send the timeout signal to the worker handling the request. Also leads to issues where requests that envoy has already timed out are filling up queues of the workers of the components. Also, configure nginx to return a 503 if WSGI server doesn't respond. co-authored by: [email protected]
33e483a to
2e5e66c
Compare
|
@dsavineau yes, that is intentional. Based on conversation with @rooftopcellist , in order to increase that we'd have to add the feature to set annotations on routes, which we currently don't have. |
dsavineau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me but I just want to point out that GUNICORN_TIMEOUT_GRACE_PERIOD won't be honored for the api and content pods with the current modification.
There's a mapping within the start-api and start-content-app entrypoints to translate environment variables to gunicorn cli options and the timeout grace period isn't one of them
https://github.com/ansible/galaxy_ng/blob/main/docker/bin/start-api
https://github.com/ansible/galaxy_ng/blob/main/docker/bin/start-content-app
|
@dsavineau plan is to set env var GUNICORN_TIMEOUT_GRACE_PERIOD as its harmless and follow up with changes so it would be received |






SUMMARY
not timing out before undercuts usefulness of our log-traceback-middleware in django-ansible-base that logs a traceback from requests that get timed out -- because uwsgi or gunicorn has to send the timeout signal to the worker handling the request. Also leads to issues where requests that envoy has already timed out are filling up queues of the workers of the components.
Also, configure nginx to return a 503 if WSGI server doesn't respond.
ADDITIONAL INFORMATION
co-authored by: @kdelee