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

Use nginx with uswgi #280

Closed
wants to merge 3 commits into from
Closed

Use nginx with uswgi #280

wants to merge 3 commits into from

Conversation

Kelvin-M
Copy link
Contributor

@Kelvin-M Kelvin-M commented Jul 3, 2020

@Kelvin-M
Copy link
Contributor Author

Kelvin-M commented Jul 3, 2020

@ClementGautier Can you review the nginx configuration. Should I add more options ?


# the upstream component nginx needs to connect to
upstream django {
server unix:///usr/src/app/backend/socket/substra-backend.sock; # for a file socket
Copy link
Member

@inelgnu inelgnu Jul 3, 2020

Choose a reason for hiding this comment

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

This should be passed as values and also should be the path to statics below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those paths are hardcoded in deployment-server, I don't think we should let them configurable or we need to make everything configurable (path to statics, socket and medias) which is not the aim of this PR.
I keep it like this for consistency :)

@@ -99,6 +95,8 @@ spec:
mountPath: /var/substra/servermedias
- name: statics
mountPath: /usr/src/app/backend/statics
- name: uwsgi-socket
mountPath: /usr/src/app/backend/socket
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: /usr/src is an uncommon folder for a socket, isn’t it?
What about a more standard folder, like /var/substra ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes !

Copy link
Contributor

Choose a reason for hiding this comment

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

sockets usually live in /var/run/xxx.sock if you are looking for something standard ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point !

Copy link
Contributor

@ClementGautier ClementGautier left a comment

Choose a reason for hiding this comment

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

I'm not completely confident we should add a webserver here... Every configuration (like max upload size, number of open connexion, forwarding X-forwarded headers etc.) will have to be duplicated between the loadbalancer and here and that gave me headache already.

I understand we need a WSGI server to translate the http request into something python can understand but I don't see the added value here. If you think uwsgi isn't performing well why not trying gunicorn for example ?

I would stick to http if it change nothing for you.

@Kelvin-M
Copy link
Contributor Author

Kelvin-M commented Jul 7, 2020

I'm not completely confident we should add a webserver here... Every configuration (like max upload size, number of open connexion, forwarding X-forwarded headers etc.) will have to be duplicated between the loadbalancer and here and that gave me headache already.

I understand we need a WSGI server to translate the http request into something python can understand but I don't see the added value here. If you think uwsgi isn't performing well why not trying gunicorn for example ?

I would stick to http if it change nothing for you.

In fact, we should not expose directly uwsgi (even gunicorn) directly. It always needs to have a webserver to server the request.

We run into something like this issue : https://medium.com/@gabriel.m.troy/aws-alb-docker-uwsgi-502-bad-gateway-16d0a36f6240

https://uwsgi-docs.readthedocs.io/en/latest/tutorials/Django_and_nginx.html

Now normally we won’t have the browser speaking directly to uWSGI. That’s a job for the webserver, which will act as a go-between.

But here nginx/kubernetes-ingress#143, it says that we do not need a webserver between ingress and nginx by adding more configuration in uwsgi.

@ClementGautier
Copy link
Contributor

You don't expose uwsgi directly, the Nginx ingress controller is the webserver in our case.
My point is the same as this: nginx/kubernetes-ingress#143 (comment)

If you still need to add a proxy Nginx, could you test that configuration put at ingress level isn't overwritten by this one:

# nginx controller chart values
config:
  server-tokens: 'false'
  use-forwarded-headers: 'true'
  compute-full-forwarded-for: 'true'
extraArgs:
  enable-ssl-passthrough: ''

# ingress annotations
nginx.ingress.kubernetes.io/client-body-buffer-size: 100m
nginx.ingress.kubernetes.io/proxy-body-size: 100m

@Kelvin-M
Copy link
Contributor Author

Kelvin-M commented Jul 7, 2020

@ClementGautier I will try to update the uwsgi settings along the github issue if it solves the 502 bad gateway.

@Kelvin-M
Copy link
Contributor Author

Kelvin-M commented Jul 8, 2020

We will try to handle it with ingress and uwsgi (http-socket directly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants