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

Remove scripted waiting for db in favor of healthcheck #5976

Merged
merged 1 commit into from
Oct 4, 2024
+7 −12
Merged

Remove scripted waiting for db in favor of healthcheck #5976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions container/webui/docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,7 @@ services:
start_period: 5s

db:
# Teporary fix version https://progress.opensuse.org/issues/167524
image: postgres:16
image: postgres
restart: always
environment:
POSTGRES_PASSWORD: openqa
Expand All @@ -136,7 +135,12 @@ services:
volumes:
- ./workdir/db:/var/lib/postgresql/data:Z
healthcheck:
test: ["CMD", "sh", "-c", "echo 'select * from api_keys;' | psql -U openqa -v 'ON_ERROR_STOP=1' openqa"]
test:
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the functional difference here? Does this healthcheck somehow work different than before?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added pg_isready -U openqa -d openqa which should ensure it is possible to connect to the db.

Copy link
Contributor

@perlpunk perlpunk Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ok, but the previous check would also be retried until the db is ready, right? so this is just nicer, but it doesn't logically change anything that allows to remove the checks from the script, right?
or asked differently, is this change here needed for the PR?
I'm not questioning the change here, just wondering. Otherwise the checks in the script would have never been necessary, but the git log, e.g. 5232213, suggests that there can be a race condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

so this is just nicer

Yes, correct.

is this change here needed for the PR?

Probably not, I guess the former test would work as well. This (pg_isready) is a recommended way to check, so I added it.

From my understanding the presence of

    depends_on:
      db:
        condition: service_healthy

together with an appropriate health-check in the db container should be enough. And that's precisely what the linked commit adds. The check in the script I removed was added before that..

Otherwise the checks in the script would have never been necessary

If I am reading the history correctly, it was introduced in the opposite order and the script wasn't necessary (unless of course docker didn't know proper healthchecks back then).

We should also note, that the podman/docker was itself also improved in the meantime and maybe it was also not behaving correctly at that time. It could be also that I am utterly wrong and we will see the race condition in the near feature.

- CMD-SHELL
- >-
pg_isready -U openqa -d openqa
b10n1k marked this conversation as resolved.
Show resolved Hide resolved
&& echo 'select * from api_keys;'
| psql -U openqa -v 'ON_ERROR_STOP=1' openqa
interval: 10s
timeout: 10s
retries: 3
Expand Down
9 changes: 0 additions & 9 deletions container/webui/run_openqa.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
#!/bin/bash
set -e

function wait_for_db_creation() {
echo "Waiting for DB creation"
while ! su geekotest -c 'PGPASSWORD=openqa psql -h db -U openqa --list | grep -qe openqa'; do sleep .1; done
}

function upgradedb() {
wait_for_db_creation
su geekotest -c '/usr/share/openqa/script/upgradedb --upgrade_database'
}

Expand All @@ -20,17 +14,14 @@ function websockets() {
}

function gru() {
wait_for_db_creation
su geekotest -c /usr/share/openqa/script/openqa-gru
}

function livehandler() {
wait_for_db_creation
su geekotest -c /usr/share/openqa/script/openqa-livehandler-daemon
}

function webui() {
wait_for_db_creation
su geekotest -c /usr/share/openqa/script/openqa-webui-daemon
}

Expand Down
Loading