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

fix: db container healthchecks that was earlier broken #6510

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

mahesh-naxa
Copy link
Collaborator

@mahesh-naxa mahesh-naxa commented Jul 19, 2024

What type of PR is this? (check all applicable)

  • πŸ› Bug Fix
  • πŸ• Feature
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Issue

Many users were experiencing failed tm-migration container that resulted in tm-backend service left in Created state and never to Started.

This is mainly caused due to healthcheck in tm-db being too light, and only checks if the postgresql server is started or not.

This was one of the reason deployment attempts by Bash from Technology without borders was not successful.
Errors he received:

tasking-manager-main-tm-migration-1  |     return self.loaded_dbapi.connect(*cargs, **cparams)
tasking-manager-main-tm-migration-1  |   File "/home/appuser/.local/lib/python3.10/site-packages/psycopg2/__init__.py", line 122, in connect
tasking-manager-main-tm-migration-1  |     conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
tasking-manager-main-tm-migration-1  | sqlalchemy.exc.OperationalError: (psycopg2.OperationalError) connection to server at "tm-db" (172.31.0.5), port 5432 failed: Connection refused
tasking-manager-main-tm-migration-1  | 	Is the server running on that host and accepting TCP/IP connections?
This second error also occurs after running the 'docker-compose up --detach' command:

    service "tm-migration" didn't complete successfully: exit 1

Describe this PR

I have updated healthcheck defination from

pg_isready -U ${POSTGRES_USER} -d ${POSTGRES_DB}

to

psql -h 0.0.0.0 -U ${POSTGRES_USER:-tm} -d ${POSTGRES_DB:-tasking-manager} -c 'SELECT 1;'

This verifies that the defined database & database role is created, and also performs a SELECT 1 that validates the initialization of the tm-db service.

Also to mention pg_isready doesn't do that, you could have pg_isready -U this_user_doesnt_exist -d this_db_doesnt_exist and it would still exit 0, which is unwanted.

Also, -h 0.0.0.0 verifies reachibility from other services, Ensuring the db now can be finally consumed.

Review Guide

Notes for the reviewer. How to test this change?

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

Copy link

sonarcloud bot commented Jul 19, 2024

@mahesh-naxa
Copy link
Collaborator Author

mahesh-naxa commented Jul 19, 2024

More discussion here: #6488 (comment)

@dakotabenjamin
Copy link
Member

@mahesh-naxa can you fix conflicts on this PR? Its ready to merge otherwise

Copy link

sonarcloud bot commented Sep 4, 2024

@mahesh-naxa
Copy link
Collaborator Author

@dakotabenjamin i have updated the branch.

@dakotabenjamin dakotabenjamin merged commit e26f3c7 into develop Sep 6, 2024
9 checks passed
@dakotabenjamin dakotabenjamin deleted the fix-docker-healthchecks branch September 6, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants