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

Make sure postgres is healthy and not simply started #48

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

Conversation

Winnie-Fred
Copy link
Contributor

I have added healthchecks for postgres, redis and celery worker. The depends_on condition for postgres and redis has been changed from service_started to service_healthy to ensure the services are healthy and ready to accept connections.
This would fix the Django Operational error that is sometimes raised when running the network of containers, especially in development. This solution does not use polling to check health status and implements only a few tweaks to the existing docker compose configuration.

I have also added timeout config to the gunicorn config to fix gunicorn worker exiting due to [CRITICAL] WORKER TIMEOUT Error handling request (no URI read).

add healthchecks for postgres, redis and celery worker.
Change depends_on condition for postgres and redis to service_healthy.
Add timeout config to gunicorn config to fix gunicorn worker exiting due
to [CRITICAL] WORKER TIMEOUT Error handling request (no URI read).
@nickjj
Copy link
Owner

nickjj commented Jun 17, 2024 via email

@Winnie-Fred
Copy link
Contributor Author

Sure, no problem 👍 .

@nickjj
Copy link
Owner

nickjj commented Jun 24, 2024

I do like that pg_isready exists, I never used it before. Normally I've always connected and ran a SELECT 1 which demonstrates you can login as a specific user and the DB server can accept queries. Sounds like pg_isready does something comparable.

I think there's a few topics to tackle:

  • Do we want to do these health checks here
  • Adjusting the syntax of some of these changes

The 2nd one is easy, for example ["CMD-SHELL", "pg_isready -U $$POSTGRES_USER -d $$POSTGRES_DB"] can be changed to ["CMD-SHELL", "pg_isready", "-U", "$$POSTGRES_USER", "-d", "$$POSTGRES_DB"] to use the array syntax for all of the flags. When it comes to using $$foo, does ${$foo} also work? I haven't tested that. The same changes could be applied to the other health checks.

Also, maybe we can remove setting the 120s timeout on gunicorn from this PR as it's a different type of change.

@Winnie-Fred
Copy link
Contributor Author

Alright. I am looking into $$foo vs ${$foo} now. I am also adjusting the health check commands to use the array syntax.

You are right, the gunicorn timeout should be in a different PR and commit.

Also, can you explain what you mean when you asked if we want to perform the health checks here?

@nickjj
Copy link
Owner

nickjj commented Jun 24, 2024

Oh, I mean if we should merge the PR in general.

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