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

Add Docker Compose Support #2406

Closed
wants to merge 51 commits into from

Conversation

jalessio
Copy link
Contributor

@jalessio jalessio commented Oct 20, 2019

This PR is primarily intended as a way of concretely demonstrating potential improvements / responding to reviewers in PR #2272 by @fazlerabbi37.

Notable changes

  • Reorganized the directory structure based on experience using this structure successfully on several other projects.
  • Use a postgis Docker image instead of postgres since we definitely need PostGIS.
  • Combine RUN commands to reduce the number of layers in Docker images.
  • Don't compile Ruby or install gems in the db image. We just need a single header file in order to build libpgosm.so.
  • Turns out the docker_postgres.sh script which installs the OSM-specific Postgres extensions wasn't actually being run because of a missing slash. Fixed that _(and renamed the file to include openstreetmap in the name to make its purpose clear).
  • Switched from ruby:2.5-slim to ruby:2.5 since we're installing so much stuff anyway (ok, fine - it's mostly because it has curl installed by default which saves some bootstrapping trouble 😛). I'm happy to be talked out of this one.

TODO

  • Review and update DOCKER.md. I haven't touched this other than moving it to the top-level directory.
  • I wonder if just using an Ubuntu image for web would be easier since we'd get sane Ruby and NodeJS defaults? Additionally, we could just copy/paste the current instructions from INSTALL.md and what is used for Vagrant instead of inventing a separate set of installation instructions.
  • Get feedback on the addition of a Makefile at the top level for running docker-compose commands (doesn't seem like that great of an idea).
  • Deal with Postgres persistence. Instructions for this included at https://hub.docker.com/_/postgres
  • "create dedicated user & group to run the server, don't just use root." per @mmd-osm in Add docker support #2272 (comment)
  • Get tests running
  • Get tests PASSING locally
  • Clean up commit history

@jalessio jalessio changed the title [WIP] Add Docker Compose Support Add Docker Compose Support Oct 20, 2019
@jalessio jalessio mentioned this pull request Oct 20, 2019
8 tasks
@simonpoole
Copy link
Contributor

* Use a `postgis` Docker image instead of `postgres` since we definitely need PostGIS.

What's the rationale for that? Given that the rails-port definitely doesn't require PostGIS.

@mmd-osm
Copy link
Contributor

mmd-osm commented Oct 20, 2019

With #2383 in place, most of docker/postgres/Dockerfile and docker/postgres/openstreetmap-postgres-init.sh will no longer be needed.

@jalessio
Copy link
Contributor Author

* Use a `postgis` Docker image instead of `postgres` since we definitely need PostGIS.

What's the rationale for that? Given that the rails-port definitely doesn't require PostGIS.

@simonpoole During the course of working on this yesterday I somehow managed to convince myself that PostGIS was in fact needed... but you're right, it's not!

I reverted back to Postgres in 3e5b8ce.

@jalessio
Copy link
Contributor Author

jalessio commented Oct 20, 2019

With #2383 in place, most of docker/postgres/Dockerfile and docker/postgres/openstreetmap-postgres-init.sh will no longer be needed.

@mmd-osm PR #2383 just merged by @tomhughes so I removed the additional PostgreSQL functions from the db Docker image. After rebase from master the changes are in commit b0d9d03.

@zerebubuth
Copy link
Contributor

Hi! Looks great so far. I was able to get a couple of containers up using your instructions. Please let me know if there's anything I can do to help 😄

@jalessio
Copy link
Contributor Author

Hi! Looks great so far. I was able to get a couple of containers up using your instructions. Please let me know if there's anything I can do to help 😄

@zerebubuth Any thoughts on how to handle persistence of Postgres data? There is some background information in the "Where to Store Data" section here: https://hub.docker.com/_/postgres

I personally prefer the "Let Docker manage the storage" approach but I have found that people find that confusing since their data vanishes when you run docker-compose down. For me, that's a feature but it seems to be surprising to people and can lead to unexpected data destruction which nobody likes.

Because of that I'm inclined to do with the "Create a data directory on the host system (outside the container)" option. I find it weird to have a PGDATA directory on my host system that is only used in a container, but I suspect I'm in the minority.

Thoughts?

btw, right now it's using the "Let Docker manage the storage" approach

@Firefishy
Copy link
Member

Use a docker volume (managed via docker-compose.yml) for postgres persistance. In "production" one would not use postgres in the same way. A volume is not automatically removed on a docker-compose down

@Firefishy
Copy link
Member

Quick example of docker-compose.yml volume with postgres: https://gist.github.com/Firefishy/c6ba34c1eab11d5c3426137011d4869a

apt-get install --no-install-recommends -y \
build-essential \
curl \
imagemagick \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why imagemagick is still in the list. GPX preview image generation was moved to gd2-ffij earlier this year, and I'm not aware of other parts of the website depending on imagemagick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The imagemagick requirements are still listed in the "Minimum Requirements" section of INSTALL.md so if they are dropped here they should probably be dropped there as well.

https://github.com/openstreetmap/openstreetmap-website/blob/master/INSTALL.md#minimum-requirements

Copy link
Contributor

@mmd-osm mmd-osm Oct 22, 2019

Choose a reason for hiding this comment

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

Right, it seems the INSTALL.md file also needs a bit of cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmd-osm is there anything in the test suite which would verify whether this dependency is needed? If not, any suggestions on a suitable test I could add?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we got rid of imagemagick, but it seems that 6c20244 introduced mini_magick, which depends on imagemagick. This is only used to resize avatar images and might be a bit heavyweight for the task at hand. Maybe @tomhughes knows more.

Copy link
Member

Choose a reason for hiding this comment

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

Well I don't believe there is any other supported approach. It only uses it via the command line though so there is no need for the development package.

imagemagick \
libarchive-dev \
libffi-dev \
libmagickwand-dev \
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should also be reviewed, not sure it is still needed.

@jalessio
Copy link
Contributor Author

Use a docker volume (managed via docker-compose.yml) for postgres persistance. In "production" one would not use postgres in the same way. A volume is not automatically removed on a docker-compose down

I switched over to use a Docker volume for Postgres in 8d84170

added Dockerfile for the ruby on rails app inside a separate docker directory. this will build a image from the official ruby (specifically ruby:2.5-slim).
…rror

postgresql-client throws a dpkg error when it can't create a softlink in for man page and aborts installation. found a solve at stack overflow and modified Dockerfile to add a comment to mention the problem with slove source.
…ed jobs from bundle install

modified Dockerfile to separate apt-get cache cleaning command, added a comment explaining why and how npm is added and removed --jobs flag from bundle install command
added docker_postgres.sh for PostgreSQL extension and functions that installs PostgreSQL Btree-gist Extension, compiles libpgosm.so and installs maptile_for_point, tile_for_point and xid_to_int4 functions
added Dockerfile.postgres for the postgres database app. this will build a image from the official postgres (specifically postgres:9.4) image.
…ntomjs via npm

modified Dockerfile to changes postgresql-client package from postgresql-client-10 to postgresql-client, and added phantomjs installation via npm
modified Dockerfile.postgres file to changes docker_postgres.sh location from db to docker
modified Dockerfile.postgres file to add app location setup and gem install via bundle
Until the Docker build context proves to be big enough
to actually be a performance problem.
Fallout from changing build context for this image
Without this there are permissions issues on the host
filesystem.
Make it easy to get up and running quickly
@jalessio
Copy link
Contributor Author

I'm closing this PR out and replacing it with PR #2409, which has a significantly cleaner commit history. For outstanding issues in the comments of this PR I propose we move them to Issues and address them in follow-on PRs if/when #2409 merges.

@jalessio jalessio closed this Oct 22, 2019
@tomhughes
Copy link
Member

It doesn't matter but if you're not aware you don't have to open a new PR just because you've rewritten the branch history - you can just force push and the existing PR will get the new clean history.

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.

None yet

7 participants