-
-
Notifications
You must be signed in to change notification settings - Fork 266
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 healthcheck to MariaDB container to ensure it is ready to accept external connections before reporting as ready #758
Conversation
Wow @wotnak this has been an issue for so long! Thank you for contributing this fix!! The tricky part has always been testing, because the failures are not consistent. I'll requeue the tests on this PR a few times to test it out... |
Hmm MariaDB tests failed on attempt #3, but I don't think it was related to the issue this PR is fixing: https://github.com/farmOS/farmOS/actions/runs/7221675735/job/19681595880?pr=758
I will continue running tests a few more times. |
Hmm - just failed again (on attempt #8) which seems to be the issue this PR is trying to prevent:
https://github.com/farmOS/farmOS/actions/runs/7221675735/job/19684915488 |
Yes, looks like adding healthcheck doesn't fix the issue. Will later try to further investigate the cause of it. |
Thanks @wotnak! It was a good thing to try! |
Done some more testing, and it looks like the health check works as it should, but by default dependency between docker compose services waits only on the dependent service start not for when it is declared as healthy. Pushed an additional change that configures www service to wait for db service to be declared as healthy (for the healthcheck to pass, https://docs.docker.com/compose/startup-order/#control-startup) before starting www service and executing tests that require connection to the database. From my local testing, this consistently fixes the issue with tests failing because of MariaDB database connection failure. |
Ah ha! That makes perfect sense. Thanks @wotnak! I'll re-run the PR tests a bunch of times and see if this does the trick. 👍 |
I ran tests 10 times and all of them passed! Requesting @paul121 and @symbioquine's reviews, but this looks good to me! Thanks @wotnak!!! |
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.
Amazing!
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.
Looks good to me!
Worth noting that this has two caveats;
- It means the farmOS container can't start up in parallel with the DB starting up so it would be expected to increase the test run time marginally (As opposed to a strategy where just the tests themselves wait to start until both the DB and farmOS containers are ready. I think the strategy in this PR is more maintainable though.)
- The
depends_on.condition
functionality is part of docker-compose V2 so it might not work for some folks on older versions of Docker/Docker-compose
I don't think either of those is a show-stopper, just worth calling out.
Good points @symbioquine - thanks for the review! Improving the performance and overall run time of tests would be a great thing to focus on next, especially given the fact that we ended up dropping In the meantime, this fix is welcome! Thanks again @wotnak! |
…external connections before reporting as ready farmOS#758
9bb2f26
to
a2bd713
Compare
Currently, tests running in GitHub Actions sometimes randomly fail when connection to MariaDB server fails. It usually fails only in first tests, with later ones running without problems. A recent example can be seen in #751 (comment).
The problem is with MariaDB container being reported as ready before the actual MariaDB server is ready to accept external connections.
It can be fixed by adding a healthcheck to the MariaDB container that checks if MariaDB server accepts external connections before reporting the container as ready. To do that, MariaDB container image provides a helper script that can be used in a healthcheck.
This pr adds a healthcheck as described at https://mariadb.org/mariadb-server-docker-official-images-healthcheck-without-mysqladmin/ to the MariaDB container used in tests, which should prevent random failures caused by connectivity issues between MariaDB server and farmOS when MariaDB server is starting.