Skip to content

Docker: replace wait_for_database with depends_on and healthcheck #1314

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

Merged

Conversation

openbrian
Copy link
Contributor

I just got started with umap using docker. Even though the entrypoint has a mechanism to wait for the database, it wasn't waiting.

Docker compose now supports depends_on with a healthcheck. I feel it's better to use this declarative configuration over imperative bash.

Side note, if this was the only reason "umap shell" exists, then please also close that entry point to remove the attack vector.

@yohanboniface
Copy link
Member

@openbrian Thanks for this!

One question: how would waiting for the database work in a "non compose" scenario (for example using kubernetes) ?

@openbrian
Copy link
Contributor Author

openbrian commented Sep 18, 2023 via email

@yohanboniface
Copy link
Member

I often find the complexity of Kubernetes doesn't make it worth using.

I can't argue ;)

But some people do use Kubernetes, and do deploy uMap with Kubernetes, so we cannot break the Docker image for them.

Maybe a first step would be to keep the script while adding the compose syntax ?

cc @jpetazzo in case you have some good practice to suggest here 🙏

@openbrian
Copy link
Contributor Author

Seems like the philosophy in Kubernetes is the app must stand on its own 2 feet. From what I've read so far, there is no equivalence to docker-compose depends_on.

https://saturncloud.io/blog/understanding-the-equivalent-of-dependson-in-kubernetes/

https://stackoverflow.com/questions/49368047/what-is-the-equivalent-for-depends-on-in-kubernetes

kubernetes/kubernetes#117359 - Implement "depends_on" for kind deployment

I'll wait for a response from @jpetazzo . If needed, I can put the wait-for script back. But really anything is better than bash, right?! How about a umap command? :)

@yohanboniface
Copy link
Member

Or something like that https://pypi.org/project/django-probes/? :)

@yohanboniface
Copy link
Member

@jpetazzo
Copy link
Contributor

Hi!

Until recently, depends_on did not wait for services to be up. It was only used to start subsets of containers (e.g., in a Compose file with 20 services, you could do docker-compose up -d auth and that would start auth as well as all the other services that auth would depends_on, instead of the whole stack of 20 services).

Now that Compose supports condition: service_healthy, it is possible to use depends_on to effectively wait for a service, as long as that service implements a healthcheck.

Unfortunately, Docker healthchecks lack nuance: there is only one type of healthcheck, whereas Kubernetes makes the difference between readinessProbe (when a container isn't ready, it should be removed from load balancer backends, and during a rolling update, we wait for containers to be ready), and livenessProbe (to detect containers that are completely dead and require a restart). To understand why we need both, consider e.g. a web app that absolutely requires a database to function: if the database isn't available, the web app is not ready (it shouldn't serve requests) but it is alive (it shouldn't be restarted either).

In Kubernetes, the recommended way to wait for other services is to use an initContainer, while remaining aware that the only purpose of that method is to avoid suprious error messages in the application container when it starts (since the application should be able to handle gracefully when the database becomes unavailable - even if it's by crashing, which will cause it to be automatically restarted once the database recovers).

My subjective opinion on the matter would be the following: if switching to a Docker healthcheck (+ depends_on) lets you get rid of the ENTRYPOINT script entirely, go for it, and advise Kubernetes users to use an initContainer. However, if you still need the ENTRYPOINT script (e.g. for migrations or whatnot), then in my humble opinion you might as well keep the dependency check there. Although, to be fair, someone deploying that on Kubernetes might just get rid of the ENTRYPOINT altogether and run migrations from a Job. (It looks like Compose is evolving to also support that workflow; but my hunch would be to wait 3-6 months before switching to that, to make sure that all potential users have upgraded to recent versions of Compose.)

@openbrian
Copy link
Contributor Author

In fact for another project, I have a docker compose service that only runs a database migration. The app service depends on that migration services on the condition that it successfully completes.

I'm going with the django_probes approach because it appears to be written with Kubernetes in mind.

@openbrian
Copy link
Contributor Author

openbrian commented Sep 20, 2023

While django-probes does add a dependency, I feel it's worth it. The core logic is in this file.

https://github.com/painless-software/django-probes/blob/main/django_probes/management/commands/wait_for_database.py

It checks for the database to be up and stable for 5 seconds. This can be reduced to 0 seconds, meaning if the db is up, just exit the command immediately. Feel free to just add --stable 0 to this command.

@openbrian
Copy link
Contributor Author

openbrian commented Sep 20, 2023

Here's a little tip for those using docker-compose... Instead of restarting the container, I often exec a command in there like to just restart (hang up) the app. Always seems to be pid 7 :)

docker compose exec app bash -c 'kill -HUP 7'

I should probably get my django app into proper debug mode with watching files. How do we do that?

@yohanboniface yohanboniface merged commit 5b7fec5 into umap-project:master Sep 21, 2023
@yohanboniface
Copy link
Member

Thanks for your work on this @openbrian! 👍

And thanks a lot @jpetazzo for your lights, clear as usual ♥

@yohanboniface yohanboniface changed the title Replace wait_for_database with depends_on and healthcheck. Docker: replace wait_for_database with depends_on and healthcheck Sep 21, 2023
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.

3 participants