search

gis-ops / docker-valhalla

This is our flexible Docker repository for the Valhalla routing engine
MIT License
228 stars 69 forks source link

fix signals being ignored #113

Closed not-my-profile closed 1 year ago

not-my-profile commented 1 year ago

Previously when running the Valhalla service with e.g.:

$ docker run -v $PWD/data:/data ghcr.io/gis-ops/docker-valhalla/valhalla
...
INFO: Found config file. Starting valhalla service!
....

You couldn't stop it with Ctrl+C (SIGINT) or kill $! (SIGTERM) when run in the background, because the run.sh shell script didn't use exec for the valhalla_service command, which resulted in an intermediary shell process (that didn't pass the signals to child processes because Docker gives it PID 1, which is special1). This is a well-known pitfall of Docker ENTRYPOINT shell scripts.2

This commit fixes that oversight.

Sidenote: It still takes ~30 seconds for a SIGTERM to shutdown the server because valhalla_service has configured such a graceful shutdown period for the prime_server HTTP server.

not-my-profile commented 1 year ago

If you like these changes, please merge the commit in a way that preserves the commit message.

nilsnolde commented 1 year ago

Huh weird, I was sure we already exec it. Thanks

not-my-profile commented 1 year ago

Thanks. Could you also merge the fix into the 3.4.0 branch? I'd like to get the fix without having to wait for the next valhalla version and don't want to use latest for reproducibility reasons.

not-my-profile commented 1 year ago

(I cannot open a PR for that because you apparently have to be a collaborator to create PRs between repo-internal branches.)

nilsnolde commented 1 year ago

I can do that as an exception, but usually I don’t. People can build themselves and there’s digests which pin the image to an exact build.

not-my-profile commented 1 year ago

Right I'd appreciate that. I am working on a test suite to compare the various OSM routers and each router is simply run via docker run ... ${image}:${tag} ..., I am not really planning on implementing digest support, since the tags are part of the user interface which I don't want to complicate. :sweat_smile: