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.

Sign up for GitHub

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

Jump to bottom

Bugfix: correctly shutdown even without replication #554

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Bugfix: correctly shutdown even without replication #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6b05ff
bugfix: avoid killing replication process on shutdown if it was not s…
carlomion Apr 16, 2024
227361b
Dockerfile: use cache for apt and pip install
carlomion Apr 16, 2024
1992f00
Remove forced sleep in CI
carlomion Apr 16, 2024
04466a2
ci: use tail with grep
carlomion Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 31 additions & 16 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ jobs:
-p 8001:8080 \
--name nominatim \
nominatim &
sleep 120

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tried a few times to get rid of these sleeps but always failed. I'm going to run the tests a few times to see if there is any difference now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the python script, it should automatically retry failed connection with an exponential backoff.
Hard-coding a forced sleep may prolong the test more than necessary, while the container may be already ready before the timeout. IMHO a polling HTTP call done periodically by the script sounds more snappy than the forced sleep.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It still wasn't enough though. I will run it a few times and see.

./assert-non-empty-json "http://localhost:8001/search.php?q=avenue%20pasteur"
docker exec -i nominatim sudo -u nominatim nominatim replication --project-dir /nominatim --once
./assert-non-empty-json "http://localhost:8001/search.php?q=avenue%20pasteur"
Expand All @@ -70,7 +70,7 @@ jobs:
-v nominatim-data:/var/lib/postgresql/14/main \
-p 8002:8080 \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8002/search.php?q=avenue%20pasteur"

- name: Import with bind-mount, container restart & update
Expand All @@ -84,7 +84,7 @@ jobs:
-p 8003:8080 \
--name nominatim \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8003/search.php?q=avenue%20pasteur"

# Stop container
Expand All @@ -98,7 +98,7 @@ jobs:
-p 8004:8080 \
--name nominatim \
nominatim &
sleep 25

./assert-non-empty-json "http://localhost:8004/search.php?q=avenue%20pasteur"
docker exec -i nominatim sudo -u nominatim nominatim replication --project-dir /nominatim --once

Expand All @@ -116,10 +116,10 @@ jobs:
-p 8004:8080 \
--name nominatim \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8004/search.php?q=avenue%20pasteur"
echo -n "check replication log for Update completed. Count:"
docker exec -i nominatim grep -c 'Update completed.' /var/log/replication.log
docker exec nominatim /bin/bash -c 'tail -f /var/log/replication.log | grep -m 1 -c "Update completed."'

- name: UPDATE_MODE=continuous with bind-mount
commands: |-
Expand All @@ -133,10 +133,10 @@ jobs:
-p 8004:8080 \
--name nominatim \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8004/search.php?q=avenue%20pasteur"
echo -n "check replication log for Update completed. Count:"
docker exec -i nominatim grep -c 'Update completed.' /var/log/replication.log
docker exec nominatim /bin/bash -c 'tail -f /var/log/replication.log | grep -m 1 -c "Update completed."'

- name: Import full style
commands: |-
Expand All @@ -146,7 +146,7 @@ jobs:
-e IMPORT_STYLE=full \
-p 8005:8080 \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8005/search.php?q=hotel%20de%20paris"

- name: Import admin style
Expand All @@ -157,7 +157,7 @@ jobs:
-e IMPORT_STYLE=admin \
-p 8006:8080 \
nominatim &
sleep 120

./assert-empty-json "http://localhost:8006/search.php?q=hotel%20de%20paris"

- name: Import with PBF_PATH
Expand All @@ -171,7 +171,7 @@ jobs:
-p 8007:8080 \
--name nominatim \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8007/search.php?q=avenue%20pasteur"

- name: REPLICATION_URL is blank
Expand All @@ -180,7 +180,7 @@ jobs:
-e PBF_URL=http://download.geofabrik.de/europe/monaco-latest.osm.pbf \
-p 8008:8080 \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8008/search.php?q=avenue%20pasteur"

- name: FREEZE
Expand All @@ -190,7 +190,7 @@ jobs:
-e FREEZE="true" \
-p 8009:8080 \
nominatim &
sleep 120

./assert-non-empty-json "http://localhost:8009/search.php?q=avenue%20pasteur"

- name: GB postcode import
Expand All @@ -200,7 +200,7 @@ jobs:
-e IMPORT_GB_POSTCODES="true" \
-p 8010:8080 \
nominatim &
sleep 600

./assert-non-empty-json "http://localhost:8010/search.php?postalcode=LE15+8TX"
./assert-non-empty-json "http://localhost:8010/search.php?postalcode=PE9+3SY"
./assert-non-empty-json "http://localhost:8010/search.php?postalcode=PE9+4ES"
Expand All @@ -212,10 +212,25 @@ jobs:
-e REVERSE_ONLY="true" \
-p 8011:8080 \
nominatim &
sleep 120

./assert-reverse-only "http://localhost:8011/search.php?q=avenue%20pasteur"
./assert-non-empty-json "http://localhost:8011/reverse.php?lat=43.734&lon=7.42&format=jsonv2"


- name: Check for clean shutdown
commands: |-
docker run --detach --rm \
-e PBF_URL=http://download.geofabrik.de/europe/monaco-latest.osm.pbf \
-p 8009:8080 \
--name nominatim \
nominatim

./assert-non-empty-json "http://localhost:8009/search.php?q=avenue%20pasteur"
# Shutdown the container
docker stop -t 60 nominatim
# Verify that the exit code is zero
CONTAINER_EXIT_CODE=$(docker inspect nominatim --format='{{.State.ExitCode}}')
test "$CONTAINER_EXIT_CODE" -eq 0

steps:
- name: Download artifact
uses: actions/download-artifact@v4
Expand Down
8 changes: 7 additions & 1 deletion 4.3/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ replicationpid=0
stopServices() {
service apache2 stop
service postgresql stop
kill $replicationpid
# Check if the replication process is active
if [ $replicationpid -ne 0 ]; then
echo "Shutting down replication process"
kill $replicationpid
fi
kill $tailpid
# Force exit code 0 to signal a succesfull shutdown to Docker
exit 0
}
trap stopServices SIGTERM TERM INT

Expand Down
10 changes: 5 additions & 5 deletions 4.4/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ ENV LANG=C.UTF-8

WORKDIR /app

RUN true \
RUN \
--mount=type=cache,target=/var/cache/apt \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain what is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a feature introduces (relatively) recently by Docker buildkit: https://docs.docker.com/build/cache/#use-the-dedicated-run-cache.
It instructs the image build process to mount that directory as a volume inside the container, only for the duration of the build. This way subsequent invocation of docker build will find the cached file locally, without leaving them inside the final image, thus reducing its final size.

# Do not start daemons after installation.
&& echo '#!/bin/sh\nexit 101' > /usr/sbin/policy-rc.d \
echo '#!/bin/sh\nexit 101' > /usr/sbin/policy-rc.d \
&& chmod +x /usr/sbin/policy-rc.d \
# Install all required packages.
&& apt-get -y update -qq \
Expand Down Expand Up @@ -75,7 +76,8 @@ RUN true \
&& echo "listen_addresses='*'" >> /etc/postgresql/14/main/postgresql.conf

# Osmium install to run continuous updates.
RUN pip3 install osmium
RUN --mount=type=cache,target=/root/.cache/pip \
pip3 install osmium

# Nominatim install.
ARG NOMINATIM_VERSION
Expand Down Expand Up @@ -107,12 +109,10 @@ RUN true \
liblua*-dev \
postgresql-server-dev-14 \
nlohmann-json3-dev \
&& apt-get clean \
# Clear temporary files and directories.
&& rm -rf \
/tmp/* \
/var/tmp/* \
/root/.cache \
/app/src/.git \
/var/lib/apt/lists/* \
# Remove nominatim source and build directories
Expand Down
8 changes: 7 additions & 1 deletion 4.4/start.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ replicationpid=0
stopServices() {
service apache2 stop
service postgresql stop
kill $replicationpid
# Check if the replication process is active
if [ $replicationpid -ne 0 ]; then
echo "Shutting down replication process"
kill $replicationpid
fi
kill $tailpid
# Force exit code 0 to signal a succesfull shutdown to Docker
exit 0
}
trap stopServices SIGTERM TERM INT

Expand Down