-
Notifications
You must be signed in to change notification settings - Fork 822
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
Docker development environment #2637
Conversation
Testing on Windows can be done quite easily nowadays, because one can download virtual MS Windows images (aka http://modern.ie). They are meant to test MSIE/Edge, but it's just plain Windows under the hood. On the other hand it's worth to remember that for Windows 7/8.1 there's only 32-bit version (however Windows 10 images are for x64) and the performance may be quite bad (common problem with virtualization). |
What would you suggest as appropriate behavior?
Please let me know some wishes, I'll try to do my best |
@Ircama I just pinged you because I thought you might be interested in testing this on Windows. Regarding get-shapefiles.py: It is a good script and I don't know of any necessary improvements yet, practice will have to show if it needs changes. The only difference here would be that the script now runs every time the container starts compared to manually as before. So I noticed the shapefile indexing. This depends on shapeindex and I don't know if you can detect if a shapefile has been already indexed. But the best solution would be to index only when it is needed. |
@nebulon42 OK, thanks, will do in the next few days |
|
@kocio-pl Ah, I missed that. Yes, that seems right although I think it should be |
I haven't had a chance to review this yet since I've been on vacation. |
Would this help us? |
Not specifically. I think the docker work is basically finished with this PR. What remains is streamlining. |
Case when using Docker Compose for Windows and if
It is suggested to position the openstreetmap-carto directory under the user's home directory (C:\users\username). Alternatively, the drive where the project is located (i.e., where the Dockerfile and volume are located) shall be shared. Runtime errors such as file not found, can't open scripts or cannot start service may indicate shared drives are needed. Additional recommendations for Docker Toolbox.
|
You can find here a new version of get-shapefiles.py (not yet pushed). In addition to only downloading archive updates by default, it now limits archive expansion to successfully downloaded archives and performs indexes only if they are missing or not up to date. Usage and command line options remain unchanged, with a slightly different meaning for |
With Windows, at the moment Docker can only be installed on 64-bit machines with hardware-assisted virtualization support enabled and where the operating system provides Hyper-V Host role (e.g., 64-bit Microsoft Windows 10 Professional. Enterprise, Education but not Windows 10 Home 64-bit, Windows 7 64-bit, etc.). For 64-bit OS not natively supporting Docker, Docker Toolbox can be installed, which uses Oracle Virtual Box instead of Hyper-V. Neither Docker nor Docker Toolbox can be installed on a 32-bit architecture. Nested virtualization scenarios are not generally supported. As by now I do not have Windows systems natively supporting Docker, I opted for Docker Toolbox and I tested the procedure on different PCs running Windows 7 Enterprise and Windows 10 Home, including one with MS Windows 10 Home x64 (i3 CPU with 4 GB RAM). It works, taking about 2.8 GB in C:\Users\<user>\.docker. It is advisable to use a faster machine (i5) with at least 8 GB RAM. This Docker configuration is definitively a great tool to allow developing openstreetmap-carto with a 64 bit Windows PC and testing the style through Kosmtik on the same machine. Kosmtik is transparently run in a VirtualBox VM, with all data (e.g., openstreetmap-carto directory) physically residing on the PC. Notice that the PostGIS database (with imported OSM data) is hosted within the VM. Procedure for x64 Windows versions requiring Docker Toolbox:
To maximize the performance of scripts/get-shapefiles.py, edit the scripts/docker-startup.sh script in the local directory and add the To stop Kosmtik, select the MINGW64 window and press Control C. To start again kosmtik without importing the OSM data:
To only import the OSM data (also within a separate Docker Quickstart Terminal window):
The default VM can be safely stopped to release OS resources when not needed.
Each time Docker Quickstart Terminal is run, the VM is automatically restarted if in power-off state (but this takes its time to wait for the IP address...). To totally remove the docker installation, delete the virtual machine named default and the directory C:\Users\<user>\.docker. While The RAM size available to the default VM should be increased for improved performance. To increase the VM memory size:
Change the memory of the default virtual machine though Oracle VM VirtualBox (and possibly also the number of cores).
|
While testing, I verified that both Dockerfile.import and Dockerfile do not check whether the db is running and this leads to sync errors between the db container and the other ones, as they start in parallel to the db instead of in sequence. A way that I found to fix this was the following:
I used
(But it can also be run locally through a Windows CMD, via scripts/get-shapefiles.py.) To import a PBF of OSM data with different name than data.osm.pbf (e.g., data1.pbf):
|
I'm afraid these VM cannot be used to test Docker or Docker Toolbox, as both need native 64-bit virtualization (to be physically set within the BIOS). Also I do not think that a Windows 10 x64 cloud instance can support Docker, as nested virtualization is not allowed generally. |
What is needed to merge this code? I think at least:
Is this TODO correct? Anything else missing here? |
DOCKER.md
Outdated
## Importing data | ||
|
||
openstreetmap-carto needs a database populated with rendering data to work. You first need a data file to import. | ||
It's probably easiest to grab an PBF of OSM data from [Mapzen](https://mapzen.com/metro-extracts/) or [geofabrik](http://download.geofabrik.de/). |
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.
Just small fixes here:
- https://mapzen.com/metro-extracts/ -> https://mapzen.com/data/metro-extracts/
- geofabrik -> Geofabrik
I have rewritten the path fix as in the new script version. I wanted to keep that in until the other PR is merged. Updated the doc with comments from @kocio-pl. I did not add documentation for Windows as I think this can be done after merge in a separate PR by e.g. @Ircama. |
I would be ready for merging once there is a review by a fellow maintainer. |
Great! I will most probably test if it works in a day or two, but I'm unable to do a code review. @pnorman - are you still interested in reviewing this PR? |
Thanks a lot! I will see if I can test this next week. Is the change in water.mss intended? |
@math1985 definitely not. hm, the diff looks weird. I'll try to move the commits into a clean branch, let's see if that helps. |
Diff looks better now, I probably forgot to rebase. But now I have recreated the branch from scratch. |
I'm afraid that something went wrong with merging and docker-startup.sh is now changed to DOS style (CR/LF) instead of UNIX style. I issued the following steps for testing with Windows 7 PRO: run "Docker QuickStart Terminal"; at command prompt:
It fails (needs UNIX style):
Anyway, if I change the file format of docker-startup.sh to UNIX, it works:
I guess that a new commit after converting docker-startup.sh might fix the issue. If you prefer to redo from scratch, the only files that I changed were Dockerfile, Dockerfile.import and scripts/docker-startup.sh (files under https://github.com/Ircama/openstreetmap-carto/tree/docker look correct to me). Anyway, the whole list of files needed for this PR should be: |
DOCKER.md
Outdated
openstreetmap-carto needs a database populated with rendering data to work. You first need a data file to import. | ||
It's probably easiest to grab an PBF of OSM data from [Mapzen](https://mapzen.com/data/metro-extracts/) or [Geofabrik](http://download.geofabrik.de/). | ||
Once you have that file put it into the openstreetmap-carto directory and run `docker-compose up import` in the openstreetmap-carto directory. | ||
This downloads (if not present) and starts the PostgreSQL container and builds (if not present) and starts a container that runs |
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.
The grammar is not clear to me, what is being downloaded exactly?
Testing this on Mac. I get the following errors, is this a problem? Attaching to openstreetmapcarto_kosmtik_1 |
So the procedure is basically:
Maybe we can add a 'Quick start' section on top that explains this? |
In the consideration that this Docker configuration might be of great help to quickly setup the Kosmtik dev environment, I would suggest to shortly mention it also in the INSTALL page. |
Confirmed that this works on Docker for Mac. Once again, great work! |
I get the following error message repeated on my terminal: kosmtik_1 | Mapnik LOG> 2017-07-06 09:54:40: could not create BreakIterator: U_MISSING_RESOURCE_ERROR Rendering seems to work fine, but does anybody knows what this means? I haven't seen it before. |
Thinking about the next step already: how hard would it be to extend this to a production-ready rendering server? It would be really great if people could create themselves a rendering server with a single docker command. Probably we don't need kosmtik for that, and would need an apache/mod_tile instead. Doesn't sound too hard? |
@@ -0,0 +1,2 @@ | |||
* |
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.
What does this do again? Why do we ignore all files?
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.
We don't need any of the files from openstreetmap-carto for building the containers. This avoids that those files are included in the build context. See https://docs.docker.com/engine/reference/builder/#dockerignore-file.
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've tested this and done a code review.
It would be great to have this. As far as I'm concerned, this can be merged now, and any minor improvements can be made after merge.
@@ -0,0 +1,12 @@ | |||
FROM debian:stretch | |||
|
|||
RUN apt-get update && apt-get install --no-install-recommends -y \ |
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.
We need to do the debian update now twice, once for the import step and once when we run. Doing the debian update is one of the slowest steps on my connection. Is there a way we can do the update only once?
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.
You mean for each container image import and kosmtik? No, there is no way to generalize that as both are completely independent from each other. But the containers are only build if you start them the first time, so this is only done once.
Ideally we would publish both images to the Docker registry so that users only would have to download the pre-build images and not have to build them themselves. But this opens up questions about versioning of the images. Separately or in line with the style? The images will not be updated often.
Also the import should be only done once if you don't change your data. You have to be careful not to type docker-compose up
but instead docker-compose up import
or docker-compose up kosmtik
. The first command without target will start both containers simultaneously.
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 what we can do is add a RUN apt-get update
line after the FROM line in both Dokerfile.import and Dockerfile. That way, we only need to do the update for the import container, and then we can use a cached container when doing the import for the main container.
In the line where we are doing apt-get install
we need to keep the apt-get update
as well (as is a Docker best practice), so we force docker to do an update whenever we change the dependencies in that line.
scripts/docker-startup.sh
Outdated
|
||
case "$1" in | ||
import) | ||
psql -c "SELECT 1 FROM pg_database WHERE datname = 'gis';" | grep -q 1 || createdb gis && \ |
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.
Tab instead of space
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 have fixed all instances of tabs now
There were quite some comments. I try to address them all in one reply:
Are you sure? On my system (Ubuntu) it tells me that the file is formatted with LF only? Maybe it has something to do with how Git is configured to handle line endings on your machine. Maybe this could point you in the right direction: https://help.github.com/articles/dealing-with-line-endings/
I have seen this too and have ignored it. It has something to do with the check if the database is ready. Since it is working I did not bother to investigate further.
This comes directly from Mapnik and I have seen this occasionally before (ref mapnik/mapnik#3193). In my experience it never affected rendering.
Not too hard probably. But I think this is out of scope for this repository. We could easily start a separate project for that if it not exists already. I have fixed tabs/spaces in docker-startup.sh |
I've just tested the quick start procedure under Ubuntu 17.04 and it works. I also think we can merge this PR soon and fine tune it later. The only thing I suppose would be good to do now, would be to have the database (and maybe some configuration directories?) mounted as a Docker volume, so the data would be independent from the code and survive removing/replacing the containers. Liechtenstein is small, but if I'm testing bigger country/continent/planet data, importing may be way too painful. Other nice thing would be to include Kosmtik plugins - probably all of them, but it's less important. |
The database runs in a separate container. So as long as you do not remove the database container your data is safe. You can delete and replace the import and kosmtik containers as much as you like. Your data should not be affected. I see no real benefit in syncing the database files out to the host as this only adds overhead and if you accidentally delete that directory it would be the same if you accidentally have deleted the database container. Kosmtik plugins would be good, but PRs for that would of course be possible also after merging. :) |
OK, it may be just me being paranoid about mixing code and data. ;-) So, in my opinion you can merge it now (I don't know if we should squeeze 4 commits into 1 or not). Thanks a lot for your work with this PR, Michael! |
if [ ! -e ".kosmtik-config.yml" ]; then | ||
touch .kosmtik-config.yml | ||
# this can be removed once https://github.com/kosmtik/kosmtik/issues/236 is resolved | ||
echo "plugins:" >> .kosmtik-config.yml |
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.
This issue is now resolved, so both lines can be removed.
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.
Thanks for noticing. :) Technically yes, but since a new release of Kosmtik is missing we have to leave it in for now.
@@ -0,0 +1,43 @@ | |||
#!/bin/sh | |||
|
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 we should add short description what is this script about, like:
# This script is used internally by Docker development environment, you can read more about it in DOCKER.md.
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.
added.
I'm not sure if it is outside scope. Purely technically there might be better places for it, on the other hand if it's only a small step compared to what we have already, I think we might just do it here. In a way it fits here, because it is a description for server admins to on how to deploy our stylesheet. |
Yes you could see it like that. But it boils down to if we want to maintain that too. :) |
To be able to backport this to 3.x we would have to change the osm2pgsl call inside docker-startup.sh. Do we want to do that? |
Personally I wouldnt spend time on that. |
@nebulon42 You are right and this issue is rather largely discussed with Docker for Windows, which allows intalling Git MSYS-git UNIX tools, used to help dealing with git-based projects and providing a UNIX style prompt to Docker Quickstart Terminal. When cloning a git project, by default Git for Windows replaces Linux Line Feed by Windows Carriage Return, dangerous for any repo being developed with Windows and delpoyed on Linux Dockers. For openstreetmap-carto I would suggest restoring the .gitattributes file for docker-startup.sh so that it will always have LF line endings. |
Fixes #657.
This adds necessary files and documentation to setup a development environment for openstreetmap-carto with Docker.
Builds upon work done by @pnorman.
We still have to work out
Would require testing on other platforms than Linux, which I don't have.
cc @Ircama