gunicorn icon indicating copy to clipboard operation
gunicorn copied to clipboard

Feat/docker

Open nanaones opened this issue 4 years ago • 13 comments

If you are using a Docker container, you can deploy Django and Flask using Gunicorn.

nanaones avatar Jul 14 '20 20:07 nanaones

Thanks for the PR. Is there a reason this wasn't put in the existing examples/ directory?

jamadden avatar Jul 14 '20 20:07 jamadden

Thanks for the PR. Is there a reason this wasn't put in the existing examples/ directory?

Simple instructions are written in the README, but I will add more guides if needed. What should I specifically add?

nanaones avatar Jul 16 '20 12:07 nanaones

No, I'm saying this is just an example. It should go in the existing examples/ directory. It should not be a new top-level directory.

jamadden avatar Jul 16 '20 13:07 jamadden

I have just a few thoughts, but I really appreciate this.

Should we commit to making all or most of our configuration possible to define through the environment? I think I would like that. It would remove the need for the script that builds the command line.

Should we provide the requirements files or let users handle that themselves? For Tornado, users should already have it as a requirement because I think they need to import its APIs. It is sometimes the case that users can write code that is worker agnostic, but other times there is a need to perform concurrent, non-blocking work within a single request and users then import and depend on gevent or eventlet directly already. If we didn't have the requirements files, then the dockerfile itself gets very small.

Building on this, we could have the dockerfile install the user-supplied requirements.txt, which should also contain the framework requirement.

Some users will need to apt-get install additional libraries for their applications to work, but I don't see a great way to handle that and maybe we shouldn't.

I think what I'm seeing from the PR, my comments, and the comment from @jamadden is that this is written as if it could be an official gunicorn quick-start image, but I worry that the set of users for whom that is practical is small. With some changes, I feel like an example becomes sufficient because configuring gunicorn through the environment would be easy enough that a fairly boilerplate dockerfile would be a better starting point than a stock image.

tilgovi avatar Jul 16 '20 17:07 tilgovi

No, I'm saying this is just an example. It should go in the existing examples/ directory. It should not be a new top-level directory.

This idea was benchmarked on Celery's Github. Like Celery, we put GunicornDockerimage in a new top-level directory that Gunicorn can officially support. I think you can put it in the /example folder as you said.

Django and Celery officially support Docker images.

Django https://hub.docker.com/_/django

nanaones avatar Jul 21 '20 00:07 nanaones

@tilgovi what about having it in this week release. I like the idea. Btw any idea where we coudl host an "official" build? apparently docker hub has became too much restrictive in term of pricing. Thoughts?

benoitc avatar Aug 26 '20 09:08 benoitc

I don't think we should publish an image. The Django image that is referenced has been deprecated for reasons similar to my objection. A user likely needs their own requirements.txt anyway, so it doesn't help to pre-install Gunicorn in the image. That means the image is really just a Python image.

I think it's useful to have as an example, but I will leave a separate review comment asking for some changes.

We should not hold up release for this because I think most users will find the examples in the repository. They can be added after releasing.

tilgovi avatar Aug 27 '20 02:08 tilgovi

@nanaones what do you think about the suggested change by @tilgovi ?

benoitc avatar Sep 17 '20 15:09 benoitc

I don't think we should publish an image. The Django image that is referenced has been deprecated for reasons similar to my objection. A user likely needs their own requirements.txt anyway, so it doesn't help to pre-install Gunicorn in the image. That means the image is really just a Python image.

I think it's useful to have as an example, but I will leave a separate review comment asking for some changes.

We should not hold up release for this because I think most users will find the examples in the repository. They can be added after releasing.

Fear enough. I will make the release today. I think it would be good anyway to have a good template for docker images, so anyone can install it. Including on windows for a basic dev environment. That would solve a lot of issues for some developers today that doesn't use UNIX / Linux systems.

benoitc avatar Sep 17 '20 15:09 benoitc

@nanaones what do you think about the suggested change by @tilgovi ?

Sorry to answer now. I thought I was rejected because I didn't receive a reply longer than I thought. I will post after modifying according to the above modifications.

nanaones avatar Oct 21 '20 16:10 nanaones

I worry that the config.sh script complicates the example and makes it less accessible as an educational tool. A gunicorn.conf.py file in the example that sets values from the environment directly might be more helpful. Something like this would also make sense in any kind of Gunicorn application template. This would be analogous to how the Rails templates install a Puma config that reads from the environment, but sets defaults: https://github.com/rails/rails/blob/6-0-stable/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt

I think the example can be simplified by removing REQUIREMENTS_FILE_PATH. It made sense when the idea was to ship a base image, because there was a separation between the user's requirements and the base requirements. I would just expect now a single requirements.txt in the work path.

I don't think a separation between /config and /work is helpful, nor is CONFIG_FILE_PATH. Gunicorn accepts an argument on the command line to set the config file path, so if the default execution is gunicorn then the user can docker run app --config <path>. The entrypoint script should not be necessary.

I sympathize enough. I would appreciate it if you think of it as a sample rather than an official image. I also agree with what you said is more useful to configure using the gunicorn.conf.py file. The config.sh file was originally created to set environment variables that should be in gunicorn.conf.py. The reason is that when using containers through Docker or creating Kubernetes environments through Helm charts, it was easier to apply configuration values ​​as environment variables than injecting them into a file. (This is purely from my experience.). There were also concerns that users would have a hard time using config.sh. But in this DockerFile, the user does not need to modify the config.sh file. Also, the DockerFile posted by PR will read the contents of gunicorn.conf.py if it exists.

nanaones avatar Nov 15 '20 05:11 nanaones

I worry that the config.sh script complicates the example and makes it less accessible as an educational tool. A gunicorn.conf.py file in the example that sets values from the environment directly might be more helpful. Something like this would also make sense in any kind of Gunicorn application template. This would be analogous to how the Rails templates install a Puma config that reads from the environment, but sets defaults: https://github.com/rails/rails/blob/6-0-stable/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt

I think the example can be simplified by removing REQUIREMENTS_FILE_PATH. It made sense when the idea was to ship a base image, because there was a separation between the user's requirements and the base requirements. I would just expect now a single requirements.txt in the work path.

I don't think a separation between /config and /work is helpful, nor is CONFIG_FILE_PATH. Gunicorn accepts an argument on the command line to set the config file path, so if the default execution is gunicorn then the user can docker run app --config <path>. The entrypoint script should not be necessary.

The reason I put the config file in the /config folder is because the user could overwrite files like start.sh that should be in /config, which could cause the container not to work properly.

nanaones avatar Nov 15 '20 07:11 nanaones

@tilgovi I have reflected the changes you requested. Please check. Please forgive me for being late.

nanaones avatar Nov 15 '20 08:11 nanaones