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.

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

feat: use mariadb client for healthchecks #221

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbergstroem
Copy link
Owner

@jbergstroem jbergstroem commented Mar 8, 2023

Instead of grepping for an existing daemon, we now run a query similar to how mysqladmin does it (connects to server/db and runs select 1).

Todo

  • review how others deal with then case of setting up a database with user/pass/root pass but then remove from a startup (conclusion: pass a mysql client config)
  • tests (painful! takes ~15s before it responds as healthy via docker inspect --format "{{json .State.Health }}" - I could just lean on similar query / logic)
  • add documentation about best practices in production environments

Instead of grepping for an existing daemon, we now run a query similar
to how `mysqladmin` does it (connects to server/db and runs `select 1`).
@jbergstroem
Copy link
Owner Author

jbergstroem commented Mar 8, 2023

Findings:

CHECK="mariadb"

# prefer root if available
if [ -n "${MYSQL_ROOT_PASSWORD}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

This ENV is not reliable since it can be changed right after the first warm up

Copy link
Owner Author

Choose a reason for hiding this comment

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

See my first point/todo and the first comment. MariaDB health check ignores auth and mysql ignores healthchecks altogether.

Copy link
Owner Author

@jbergstroem jbergstroem Mar 8, 2023

Choose a reason for hiding this comment

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

I think what we want is a sane default, if people do changes they should probably override the healthcheck as well. I can see this being very common in any kind of setup where you orchestrate a docker compose setup with many actors.

CHECK="mariadb"

# prefer root if available
if [ -n "${MYSQL_ROOT_PASSWORD}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth providing ENVs for the check, and if they are all empty then it does nothing ?

Copy link
Owner Author

@jbergstroem jbergstroem Mar 8, 2023

Choose a reason for hiding this comment

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

If they are all empty it would try to connect without credentials. This is what mariadb does.

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

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

I think this is a good idea only if you can have separate variables, this makes this image deviate from the original mariadb one a bit too much for me

@jbergstroem
Copy link
Owner Author

jbergstroem commented Mar 9, 2023

this makes this image deviate from the original mariadb one a bit too much for me

This is what the original does:

$ mariadb -h localhost --protocol tcp -e 'select 1'

If you set up this repo using the PR and remove your environment variables you'd see the same result.

Edit: if you check the original issue about adding this functionality to MariaDB; they are talking about the same thing / what I am trying to do as default: MariaDB/mariadb-docker#94

The cleanest approach to me from a production standpoint is to initialize your container, no longer pass credentials in your compose, then mount a config that contains authentication in a [mariadb-client] directive. Even with that, what I'm suggesting in this PR would work just fine (no variables are set -> just connect without passing credentials, reading from config instead).

I would add it to the documentation / best practices. That would be a great way to unit test this as well.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants