feat: add docker healthchecks to server and ml #9583

CodaBool · 2024-05-18T17:21:12Z

🐋 What is a docker healthcheck

the docker spec has a way to determine if a service is healthy. This is useful for servers. The command docker ps can be used to get a quick summary of if a container is responding positively.

Platforms like AWS have built in integration for this spec and show the health status in their UI when running containers in ECS Fargate.

There probably are other benefits but I only know of those. I personally like to use the lazydocker cli to manage my docker services. Which also displays if they are passing healthchecks

🏗️ My solution

One thing to know about the docker healthcheck is it uses CLI on the image to perform it. This can be tricky to accomplish in an era of distroless images. The images that Immich puts out seem to be in that category (which is a good thing). The following common CLI that allow docker healthchecks to easily be done are absent on the image for both immich_server and immich_machine_learning

common commands used for Docker healthchecks

curl
wget
netcat or nc
pgrep

This means to support docker healthchecks you have to go one of two routes. Add wget from the busybox image. Or use a programming language to script a healthcheck command.

If the end image has node or python, like is the case here, then the later is the most slim and secure way to do this.

So, that's what I've added for the two images

🧪 Tests

Lint

I ran npm run lint but got the following error

Healthchecks do need to follow the Docker spec and return exactly what Docker wants.

0: success - the container is healthy and ready for use
1: unhealthy - the container isn't working correctly
2: reserved - don't use this exit code

So, since using process.exit() is a requirement. I added this to the ignore patterns of eslint instead of resolving it.

Tests

passing

Something I haven't tested

Healthchecks by default will run every 30 seconds. This in some apps could cause an excess of logs. I did not check for this. If anyone wants to give that concern a look. That would be appreciated.

😶‍🌫️Keep Optional

It would be possible to add these to the installation docker-compose.yml file. But I don't think it would be good to force this out (by adding it to /docker/docker-compose.yml). It can always be something that people optionally add onto their compose file if they want it (it's more for people who know what they are doing anyways). For example postgres and redis have a built in way to do healthchecks. Which I'm adding to my docker compose file.

I guess something in the docs would be useful for this. But curious what the maintainers think on this.

An example of what my compose file looks like with healthchecks

services:
  immich-server:
    ...
    healthcheck:
      test: npm run healthcheck

  immich-microservices:
    ...
      # I don't even know where this code is. So, I didn't add a healthcheck

  machine-learning:
    ...
    healthcheck:
      test: python3 healthcheck.py

  redis:
    ...
    healthcheck:
      test: redis-cli ping

  database:
    ...
    healthcheck:
      test: ["CMD-SHELL", "pg_isready", "-d", "${DB_DATABASE_NAME}"]

mmomjian · 2024-05-18T17:29:43Z

I think this is a great idea. Currently I run healthchecks by using a static curl binary mounted inside the container. A built in healthcheck would be great.

mertalev

Thanks for the detailed explanation! I like the idea of using scripts to avoid adding dependencies.

immich-microservices doesn't have a separate codebase; it shares the same codebase as the server. It's also just been removed, so no need to worry about it :)

mertalev · 2024-05-19T05:13:11Z

server/src/utils/healthcheck.ts

+const http = require("http");
+
+const options = {
+  host: "localhost",


Would it be better to use the /api/server-info/ping endpoint?

Yes, and we should probably also assert on the response body, not just the status code.

Expected value

I first tested with a wget call for a successful call to /api/server-info/ping gave. It responded with this:

2024-05-19 20:47:35 URL: http://localhost:3001/api/server-info/ping 200 OK

Next I attempted to read that data inside the healthcheck.ts file

What I tried

I'm having some trouble using node.js http.request() function for the purpose of getting the response body. Looking at their docs. It seems something like this should work. When placed inside the callback function of http.request():

res.on('data', (chunk) => { console.log(`BODY: ${chunk}`); });

but this never gets triggered. Not sure what I'm doing wrong here. If anyone is familiar with http.request(), I need some help on reading the body. 🙇

Alternative

I didn't see any http requesting packages. So, I would like to make http.request() work here.

This only other useful piece of data I saw in the response was statusMessage. Which has the value of "OK". This seems to be like it could be used as a replacement for the body. Any thoughts on this idea?

Just use fetch instead

oh nice fetch does work. Seems I have to do a weird signal thing to implement a timeout 😮‍💨

But I was able to read the body this way.

mertalev · 2024-05-19T05:14:49Z

machine-learning/app/healthcheck.py

+port = os.getenv("IMMICH_PORT", 3003)
+
+try:
+  response = requests.get(f"http://localhost:{port}")


Similarly /ping here?

mertalev · 2024-05-19T05:17:41Z

machine-learning/app/healthcheck.py

+port = os.getenv("IMMICH_PORT", 3003)
+
+try:
+  response = requests.get(f"http://localhost:{port}")


The Node.js health check has a timeout set, but this one doesn't. They should probably be the same.

mertalev · 2024-05-19T05:24:44Z

Looks like you need to format the node script through Prettier and the Python script through ruff.

mmomjian · 2024-05-19T05:54:33Z

I have drafted some documentation. According to the PG docs pg_isready can return values other than 0 or 1. Can we add a || exit 1 to that test?

I see some threads saying redis-cli may have the same issue, may be smart to add the same there.

I personally end all of my healthchecks (not just Immich) with || exit 1.

server/.eslintrc.js

server/package.json

server/src/utils/healthcheck.ts

CodaBool · 2024-05-19T22:35:15Z

Thanks for the help everyone. I have implemented your suggestions. With the exception of @bo0tzz. I was not able to find a way to read the response body with node.js http.request() callback. I left a reply to your comment with more details

mertalev · 2024-05-20T21:56:30Z

I believe you can fix the process.exit linting error by adding #!/usr/bin/env node to the top of the file.

mertalev · 2024-05-20T21:58:26Z

May as well add it to the Python script too for consistency (except python3 instead of node).

bo0tzz

Can you rebase on main to fix the status checks?

I'm also thinking we should probably document the existence of these healthchecks somewhere, though I'm not sure where would be a good place at the moment 🤔

zackpollard · 2024-05-21T08:30:38Z

Can you rebase on main to fix the status checks?

I'm also thinking we should probably document the existence of these healthchecks somewhere, though I'm not sure where would be a good place at the moment 🤔

Dockerfiles have a HEALTHCHECK instruction, we should add these endpoints into there so that by default everyone gets healthchecks. I don't see a reason anyone wouldn't want them and if they specifically didn't they could override and disable them. @CodaBool

jrasm91 · 2024-05-21T11:52:38Z

@zackpollard @bo0tzz #9590

mmomjian · 2024-05-21T14:06:22Z

@zackpollard I agree with adding it to the Dockerfile. That won't cover Redis or Postgres. One option (my preference) is to add it to the docker-compose.yml for those two services (non-breaking). Thoughts?

mmomjian · 2024-05-21T16:26:50Z

I have added an enhanced PG healthcheck in #9590 that uses database checksums. My preference would still be to modify that PR into one that just adds the checks to the docker-compose.yml

CodaBool · 2024-05-21T16:52:46Z

One option is to add it to the docker-compose.yml

I'm happy either way

zackpollard · 2024-05-21T17:01:29Z

I have added an enhanced PG healthcheck in #9590 that uses database checksums. My preference would still be to modify that PR into one that just adds the checks to the docker-compose.yml

Happy to add that into the docker compose and have our own ones directly in the dockerfiles

mmomjian · 2024-05-21T17:12:57Z

I have added an enhanced PG healthcheck in #9590 that uses database checksums. My preference would still be to modify that PR into one that just adds the checks to the docker-compose.yml

Happy to add that into the docker compose and have our own ones directly in the dockerfiles

#9590 updated.

mmomjian · 2024-05-22T15:09:03Z

server/src/utils/healthcheck.ts

+    if (response.ok) {
+      const body = await response.json();
+      if (body.res === 'pong') {
+        console.log('Server is running');


Will this write to the docker logs every time the HC is run (default 30 seconds)? Is that desired?

Doing a watch on the immich_server container for 2 minutes and nothing showed up (healthchecks should by default happen every 30 seconds). There is no other logs than the container's stdout right?

Hm, interesting. I guess that should be fine then

Imo just remove it anyway, if it doesn't get written anywhere then there's no point having it

removed 2 of the 3 console calls. I left the last one in because there is a very minor usefulness to them. If healthchecks are failing when they shouldn't be you could exec in and run the file with node. Then look at the output for debugging.

Let me know what you think. I think either way is fine. Docker is likely redirecting stdout to dev/null anyways but that's speculation

Looks good to me, thanks for making all these changes!

It's not really important here, but for future reference, healthcheck logs are captured separately by docker and listed in the container status next to healthcheck status changes.

changes were made

add healthcheck

d26ba62

CodaBool requested review from mertalev and danieldietzler as code owners May 18, 2024 17:21

mmomjian added deployment Deployment related tasks maintenance 🗄️server and removed maintenance labels May 18, 2024

danieldietzler requested review from jrasm91 and bo0tzz May 18, 2024 17:41

CodaBool mentioned this pull request May 19, 2024

Built-in Docker Healthcheck cloudflare/cloudflared#1126

Open

mertalev reviewed May 19, 2024

View reviewed changes

mmomjian mentioned this pull request May 19, 2024

feat(server): healthchecks for PG and redis #9590

Merged

jrasm91 requested changes May 19, 2024

View reviewed changes

server/.eslintrc.js Outdated Show resolved Hide resolved

server/package.json Outdated Show resolved Hide resolved

server/src/utils/healthcheck.ts Show resolved Hide resolved

server/src/utils/healthcheck.ts Outdated Show resolved Hide resolved

format, import, IMMICH_PORT, and eslint change

83bf56d

chore: clean up nodejs healthcheck

38ffce6

jrasm91 approved these changes May 20, 2024

View reviewed changes

fix ruff formating

cb3d215

mertalev approved these changes May 21, 2024

View reviewed changes

bo0tzz previously requested changes May 21, 2024

View reviewed changes

CodaBool and others added 3 commits May 21, 2024 07:53

add healthcheck

6f2f3d1

format, import, IMMICH_PORT, and eslint change

3ad2a46

chore: clean up nodejs healthcheck

715214a

fix ruff formating

daec35d

jrasm91 force-pushed the main branch from cb3d215 to daec35d Compare May 21, 2024 11:54

CodaBool added 2 commits May 22, 2024 11:05

merge + add to Dockerfile

f6f5491

add healthcheck to dockerfile

5082756

mmomjian reviewed May 22, 2024

View reviewed changes

poetry run ruff check --fix

21e6f18

zackpollard changed the title ~~feat(server, ml) add ability to docker healthcheck~~ feat(server, ml): add ability to docker healthcheck May 22, 2024

zackpollard changed the title ~~feat(server, ml): add ability to docker healthcheck~~ feat: add ability to docker healthcheck May 22, 2024

zackpollard changed the title ~~feat: add ability to docker healthcheck~~ feat: add docker healthchecks to server and ml May 22, 2024

removed 2 of 3 console calls

7246bd3

zackpollard approved these changes May 22, 2024

View reviewed changes

zackpollard enabled auto-merge (squash) May 22, 2024 16:48

zackpollard merged commit 6a4c2e9 into immich-app:main May 22, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add docker healthchecks to server and ml #9583

feat: add docker healthchecks to server and ml #9583

CodaBool commented May 18, 2024 •

edited

mmomjian commented May 18, 2024

mertalev left a comment •

edited

mertalev May 19, 2024

bo0tzz May 19, 2024

CodaBool May 19, 2024

jrasm91 May 20, 2024

CodaBool May 20, 2024

mertalev May 19, 2024

mertalev May 19, 2024

mertalev commented May 19, 2024

mmomjian commented May 19, 2024 •

edited

CodaBool commented May 19, 2024 •

edited

mertalev commented May 20, 2024

mertalev commented May 20, 2024

bo0tzz left a comment

zackpollard commented May 21, 2024 •

edited

jrasm91 commented May 21, 2024

mmomjian commented May 21, 2024

mmomjian commented May 21, 2024 •

edited

CodaBool commented May 21, 2024

zackpollard commented May 21, 2024

mmomjian commented May 21, 2024

mmomjian May 22, 2024

CodaBool May 22, 2024

mmomjian May 22, 2024

zackpollard May 22, 2024

CodaBool May 22, 2024 •

edited

zackpollard May 22, 2024

bo0tzz May 22, 2024

feat: add docker healthchecks to server and ml #9583

feat: add docker healthchecks to server and ml #9583

Conversation

CodaBool commented May 18, 2024 • edited

🐋 What is a docker healthcheck

🏗️ My solution

🧪 Tests

Lint

Tests

Something I haven't tested

😶‍🌫️Keep Optional

mmomjian commented May 18, 2024

mertalev left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Expected value

What I tried

Alternative

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertalev commented May 19, 2024

mmomjian commented May 19, 2024 • edited

CodaBool commented May 19, 2024 • edited

mertalev commented May 20, 2024

mertalev commented May 20, 2024

bo0tzz left a comment

Choose a reason for hiding this comment

zackpollard commented May 21, 2024 • edited

jrasm91 commented May 21, 2024

mmomjian commented May 21, 2024

mmomjian commented May 21, 2024 • edited

CodaBool commented May 21, 2024

zackpollard commented May 21, 2024

mmomjian commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaBool May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaBool commented May 18, 2024 •

edited

mertalev left a comment •

edited

mmomjian commented May 19, 2024 •

edited

CodaBool commented May 19, 2024 •

edited

zackpollard commented May 21, 2024 •

edited

mmomjian commented May 21, 2024 •

edited

CodaBool May 22, 2024 •

edited