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

feat(server): add Bash healthcheck script #14704

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

000yesnt
Copy link
Contributor

The current health check script starts up a Node JS script every time it's called. This becomes an issue in low-powered devices hosting Immich as the startup process can be CPU-intensive. The CPU usage bursts can compromise overall system stability.
#10312 (Orange Pi 5+) and #14364 (Raspberry Pi 4) are examples of users of such devices complaining about the usage spikes, where the only solution was to disable health checking entirely.

In testing with an old desktop PC (Intel Pentium E5400, 2GB of RAM + 4GB of ZRAM), replacing the JS healthcheck with a Bash one eliminated the spikes.
image

I have tried to replicate the behavior of the original healthcheck script as closely as I could, including an early exit when the api worker is excluded or left out of IMMICH_WORKERS_INCLUDE. I also passed it through Shellcheck to prevent bugs.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

bo0tzz
bo0tzz previously requested changes Dec 15, 2024
Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove the node healthcheck code?

@000yesnt
Copy link
Contributor Author

Should we remove the node healthcheck code?

I can delete the node script if there won't be any need for it. I should've done that eariler but I forgot to

Copy link
Contributor

@mmomjian mmomjian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just a few BASHism questions!

server/bin/immich-healthcheck Outdated Show resolved Hide resolved
server/bin/immich-healthcheck Outdated Show resolved Hide resolved
server/bin/immich-healthcheck Outdated Show resolved Hide resolved
server/bin/immich-healthcheck Outdated Show resolved Hide resolved
server/bin/immich-healthcheck Outdated Show resolved Hide resolved
if [ "$result" != "{\"res\":\"pong\"}" ]; then
echo "Fail: didn't reply with pong";
exit 1;
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explicitly exit 0 at the end of the script. Otherwise looks good to me. Ty!

@alextran1502 alextran1502 enabled auto-merge (squash) December 16, 2024 15:04
@alextran1502 alextran1502 merged commit f180ae7 into immich-app:main Dec 16, 2024
33 checks passed
@000yesnt 000yesnt deleted the bash-healthcheck branch December 18, 2024 09:19
@akostadinov
Copy link

Hmm, I was wondering why disk is so often queried... Is it possible to disable health check or make it not access storage so that HDDs can spin down?

yosit pushed a commit to yosit/immich that referenced this pull request Dec 20, 2024
* feat(server): add Bash healthcheck script

* fix(server): add 2 second timeout for healthcheck.js parity

* chore(server): delete old healthcheck Node script

* fix(server): feedback

---------

Co-authored-by: Alex <[email protected]>
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.

7 participants