-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Feature request: add HEALTHCHECK #282
Comments
@rnavarropiris, you might be interested to know that I've just added healthchecks to the Postgres-XL Docker project, which provides Docker images for Postgres-XL, the open-source database cluster based on PostgreSQL. The healthchecks can certainly be improved, and would in any case be slightly different to PostgresSQL healthchecks themselves (the Coordinator definitions are closest). |
Copying comment from docker-library/cassandra#76 (comment)
|
@yosifkit
That's exactly what I meant in my comment when I said "images based on this one could define their own healthcheck". The image could provide a generic healthcheck with the semantic of "the server was successfully started and is ready". If someone needs a more refined semantic for the healthcheck, then overriding it in a new image based on the official is the way to go.
Fair enough, external network connectivity is hard to guarantee, but as long as the documentation states what the healthcheck does and its limitations to a local context, I don't see any problem with that. Again, if someone has special needs for healthcheck, he can rewrite it when building his own image or just ignore the healthcheck and use custom logic.
A single "SELECT 1" query executed using the command line client directly in the server every X seconds can hardly be considered "load on their systems" (imo). If this is indeed considered to be an issue, I find this proposal an adequate solution. |
@yosifkit, for what it's worth, I too respectfully disagree, and support @rnavarropiris 's comments. Healthchecks are really, really useful when running multiple containers in a stack, allowing not only for easy detection of basic health (yes, admittedly not through external access, but that doesn't matter so much to me; the container is more likely to be down than my entire network), but also for nice integration with restart policies. Simply having a healthcheck and a restart policy applied means that if the worst happens and my container because unresponsive, it will be restarted automatically after some time. When I wrote the healthchecks for my Postgres-XL Docker images (https://github.com/tiredpixel/postgres-xl-docker), I took a rather extreme approach, not only checking connections but also defining a healthcheck user automatically and connecting to a database without using superuser privileges (so as to avoid the typical reserved connections). The healthchecks I wrote can certainly be improved (since they currently report an error every time regarding a user which already exists, and indeed there's no need to perform setup in the main healthcheck execution path; I'll likely change this at some point), but they allow me to check even that there are enough connections free for a client to connect. This can cause an entire broken cluster to auto-heal within a few minutes. I can't stress enough how useful these healthchecks are. Admittedly, I could define them myself for main PostgreSQL installation-time, but then I would have to do so for every stack, and keep all of those healthchecks in sync. I would much sooner have a canonical, maintainer-supported healthcheck, which I could easily override or disable entirely if I wished. |
@yosifkit I also support the comments of @rnavarropiris. version: '3.3'
services:
postgresql:
image: postgres:9.6.6-alpine
web:
depends_on:
- postgresql which sadly does not work out of the box because postgresql does not come with a basic health check. If users are conerned by additional load they can override or deactivate the health check either
So there is a lot of flexibility in how users can customize any predefined health check. |
I tend to agree with @rnavarropiris I think a health check is quite interesting, and even tried to implement such a feature in #421 that should be compatible with this statement as long as the defintion fits in a SQL query to the database defined as environment variable
It does not solve the assertion that service is listening outside of localhost but can be done using non loopback tcp connection to postgres.
I would not think that, if using the default 30 seconds interval, selecting one value would have a noticeable impact on system load. |
I agree, I'm currently using postgres docker as part of my circleci testing and by default I'm having to bundle additional dependencies on the app side to detect if postgres is up, configured and ready (not just the port bound, but the specified role created, etc). Such a check covering the basics of "We're exposing a port, we created the user you asked for, and db you wanted" are bare basics. if you want anything more in depth, then @rnavarropiris is right, define your own health checks. Also if you're worried about the additional load a periodic healthcheck query is going to add, then you've got to be red-lining your setup pretty hard and so this container probably isn't for you in the first place. tl;dr: Healthcheck should just say if we're ready to provide what we've been asked for yet, no more, no less. |
@wglambert Is there any update on this issue? I don't mean to be pushy, but it's become a blocker for our team. We've been using the healthcheck/postgres image, but that only includes a |
The healthcheck repo is to serve as an example for creating your own image derived from the healthcheck-prototypes provided.
likewise the official-images are to serve that purpose as well
So in essence if users want added functionality, we try to demonstrate best practices such that users would have a reference for maintaining their own images for the specific functionality they want; as we try to maintain as close to upstream as possible to provide that base platform to build off of |
@wglambert Thanks for the clarification. I didn't see that in the documentation, and it explains why there are only two tags. I think there might be some confusion about this in a more general sense. Those images are listed in the CodeShip documentation, which is where we first found them, and they don't clarify that point. I realize that you guys aren't responsible for issues their documentation, but I'd just like to point out that there might be a larger misconception about how those images are intended to be used. The larger solution to this problem would still be to include a healthcheck in the official Postgres image. I know that my team would love to see that, and I think others would as well. |
My interpretation of the codeship passage is that they explained it well enough, but it does require further knowledge of the healthcheck repo's purpose
The "health checks built into a Dockerfile" seems to convey that sufficiently. It's broad sense encompasses the use cases of implementing the directive in any sense, which is really just adding two lines in the COPY docker-healthcheck /usr/local/bin/
HEALTHCHECK CMD ["docker-healthcheck"] I can see the confusion in the next mention: when they state "with minimal configuration", that doesn't explicitly mention the alternative of just adding two lines to a Dockerfile.
The discussions requesting healthchecks will remain open but our stance remains the same: "See docker-library/cassandra#76 (comment) for the rationale for why these don't get added to the official images directly." |
@wglambert Thanks again for the clarification, and for the quick response. I'll follow up with a comment in that repository. |
Is it possible to at least add healthcheck script into the image so that we are able to configure healthcheck without creating custom image ? The script may be even modified to accept some parameters (e.g. SQL query, that should be executed) so that it would be useful for different needs. |
From a docker-compose perspective this is working as expected for me
I put of a pull request to add a default Would love some input on desired |
See https://github.com/docker-library/faq#healthcheck for the official-images stance on adding explicit Closing this accordingly. |
Makes sense I'll close out the referenced MR as well. |
@tianon Any input on this tho?
|
@WhyNotHugo See #282 (comment) You can use |
Perfect, thanks! |
Docker supports defining a command to determine if the container is running as intended (healthy), broken for some reason (unhealthy) or still initializing (starting):
Adding this to this image with the semantic "the postgres server is online and functional" would be quite nice.
Since the last HEALTHCHECK defined by a Dockerfile is the one actually used, images based on this one could define their own healthcheck without "interference" by the one provided by the postgres image.
The text was updated successfully, but these errors were encountered: