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

Fully working docker-compose file #6295

Merged
merged 64 commits into from Nov 25, 2019
Merged

Fully working docker-compose file #6295

merged 64 commits into from Nov 25, 2019

Conversation

@humitos
Copy link
Member

humitos commented Oct 16, 2019

This PR configures all Read the Docs following the architecture that we use on production (multiple processes for different services) and allows developers to have an environment very close to what production looks like.

There were other attempts to support docker-compose in the past (#3245, #3003, etc) but none of them worked for different reasons. 6 months ago, we started working again on this idea but got blocked because the configuration to sync files between servers was too complicated, but also because the famous permission issue and we weren't able to make it work. Although, we did the first step in that direction by solving the issue for the permissions.

I want to publicly thanks @raulcd for his help on the whole process.

Yesterday, I tried again now that we have "El Proxito" in front of everything and we use django-storages to upload the build's output to Cloud files, which reduces a lot the architecture needed for this. It works!

I'd like to have more people from @readthedocs/core team involve here and try to push this forward because it will reduce the barrier to contributors a lot (no need of #4608) but also because it will make our life easier, avoiding problems on deploy that we can detect while developing (#5976). Besides, it adds the opportunity to test some operation changes before deploying and reducing the risk of failure.

Services

This docker-compose starts:

  • nginx: serving the main app at dev.readthedocs.io and proxito at *.dev.readthedocs.io
  • proxito: serve docs from Cloud files (Azurite --Azure emulator)
  • web: main application (dashboard)
  • celery: celery application attending web,web01,reindex queues
  • build: celery application without access to the DB attending builder,celery,default,build01 queues
  • storage: Azurite to storage build's output and read them from proxito
  • cache: redis
  • search: elasticsearch
  • database: postgres

Changes needed to Python code

  • Docker shared volume used between build and the "docker in docker" container created by the build service itself. This avoid the permission issue.
  • Serve from NGINX when using a specific setting (RTD_DOCKER_COMPOSE)
  • Do not do Domain check when host is web (Docker service)
  • Define proxito as valid host to serve docs
  • Fallback to normal json module if orjson is not installed (wasn't able to compile it on alpine linux)

Testing this docker-compose file

  1. pull this branch (humitos/docker-compose)
  2. run GITHUB_TOKEN=<your-github-token> docker-compose up
  3. access to http://docs.dev.readthedocs.io

NOTE: if you don't have readthedocs/build:latest docker image already, you will need to pull it first running docker pull readthedocs/build:latest for a build to success

Improvements / ToDo

  • do not depend on readthedocs-ext (it's only used for custom AzureStorage classes but they can just be moved into .org)
  • find a way to automatically run az to create containers on first run
  • pass arguments to docker-compose up to avoid running collectstatic and others each time
  • properly set CORS on Azurite container
  • make custom 404 work properly (done at #6306)
  • (optional) configure celery-beat
  • (optional) setup cold storage for old builds
@humitos humitos requested review from davidfischer and readthedocs/core Oct 16, 2019
@stsewd stsewd mentioned this pull request Oct 17, 2019
Copy link
Contributor

davidfischer left a comment

I got an initial error during docker-compose up of the following:

TypeError: __init__() got an unexpected keyword argument 'token_credential'

It looks like it's related to the storage stuff.

Overall, you might find it easier to use some sort of S3 compatible storage in a setup like this. There might be something more minimal than the Azure setup. I'm not 100% sure. Perhaps something like the S3 portion of localstack

docker/Dockerfile Outdated Show resolved Hide resolved
readthedocs/search/parse_json.py Outdated Show resolved Hide resolved
docker-compose-search.yml Show resolved Hide resolved
@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Oct 19, 2019

TypeError: init() got an unexpected keyword argument 'token_credential'

Hrm... I thought I fixed this. There is a small incompatibility between django-storages and azure-storage-blob versions. I will double check this.

Overall, you might find it easier to use some sort of S3 compatible storage in a setup like this. There might be something more minimal than the Azure setup. I'm not 100% sure. Perhaps something like the S3 portion of localstack

I'm using MinIO on corporate and it works great. I tried Azure here to be closer to production, though.

@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Oct 19, 2019

TypeError: __init__() got an unexpected keyword argument 'token_credential'

I added a commit that fixes this.

raulcd and others added 20 commits Mar 31, 2019
Pass --user (`DOCKER_USER`) attribute when creating the container.

This has no effect in production since we are using the same user and
group than the one defined inside the Dockerfile image (docs:docs).
Although, this allow us to avoid permissions conflicts when running
the build with Docker locally (development) since we can pass our
current user.

That way, every file created/modified inside the container will be
done using the current UID and GID defined by the developer.

This can be done as,

local_settings.py
DOCKER_USER = f'{os.geteuid()}:{os.getegid()}'

With this change, there is no need to re-build the Docker image used
in production with our own custom `USER` instruction.

https://docs.docker.com/engine/reference/run/#user

Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
- add .dockerignore to exclude files from context
- mount current code into the container instead of a fixed commit
- prepare paths for readthedocs-ext
@ericholscher

This comment has been minimized.

Copy link
Member

ericholscher commented Nov 7, 2019

This seems to make my CPU sit around 80% on one core when idle, so I think there's likely something we need to do to make it do less work. I'm not even running ES in that case, and docker makes my fans turn on :/

@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Nov 11, 2019

@ericholscher there is a way to limit CPU and memory with YAML file version 2.2 (https://docs.docker.com/compose/compose-file/compose-file-v2/#cpu-and-other-resources). Although, this is not going to be implemented on v3 --just for swarm mode: https://docs.docker.com/compose/compose-file/#resources

There is also a way to convert v3 into its v2 using --compatibility (docker/compose#5684). It seems to stay in 2.x is the recommended way to use resource constraints with docker compose locally: docker/compose#4513 (comment)

@ericholscher

This comment has been minimized.

Copy link
Member

ericholscher commented Nov 13, 2019

I don't think limiting it's resources is the answer, it's likely either a docker bug or something we need to adjust in our scripts. Limiting the CPU or similar will just make things slower, when it's already slower than running on my local machine, so definitely not an outcome we want :/

humitos and others added 9 commits Nov 18, 2019
…humitos/docker-compose
…hedocs.org into humitos/docker-compose
We can't access docs.dev.readthedocs.io from celery container because
that domain points to 127.0.0.1 and we don't have the storage in that
IP. So, we need to override the AZURE_MEDIA_STORAGE_HOSTNAME in the
celery container to point to the storage.

We should do this directly in `docker_compose.py` settings file, but
since we can't configure CORS in Azurite we can't do it yet.
…humitos/docker-compose
@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Nov 21, 2019

@ericholscher when you have some time, please take another look at this PR (I added more commits after you looked last time) and let me know what you think. There may be some places where the code could be improved or decision can be made.

However, we can merge it as it is and continue improving it with smaller PRs.

@humitos humitos requested a review from ericholscher Nov 21, 2019
docker/settings/web.py Outdated Show resolved Hide resolved
humitos added 6 commits Nov 21, 2019
When using `pdb.set_trace()` we can use `docker attach
readthedocsorg_proxito` for example to jump into the container and
connect the stdout and stdin into that terminal, allowing us to debug
the process.
Now we are serving 404s via El Proxito and forcing DEBUG=False is not
needed anymore.
Copy link
Member

ericholscher left a comment

I'm 👍 on merging this, so we can keep testing it locally with other dev branches. The only thing I'd perhaps note is in the docker-compose file that it is not officially supported, perhaps also as a README in the docker/ directory.

@humitos

This comment has been minimized.

Copy link
Member Author

humitos commented Nov 25, 2019

I added a small note on the YAML file and a also a small README in the docker folder. I'll merge the branch once tests pass.

@humitos humitos merged commit 87369cd into master Nov 25, 2019
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.