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

fix: add healtchecks and probes #128

Closed
wants to merge 1 commit into from
Closed

fix: add healtchecks and probes #128

wants to merge 1 commit into from

Conversation

volker-raschek
Copy link


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

Starting with version 1.4.0 of bazarr, the configuration file format has been changed from ini to yaml. This make it easier to parse the config file for properties via yq. For example to implement a healthcheck for docker or probes for kubernetes. Neither a healthcheck nor probes is contained in the container image. This leads users to develop their own healthcheck like I developed in the past an awk script. This pull request adds default shell scripts for booth platforms to provide a healthchecks and probes.

Benefits of this PR and context:

This PR adds additional shell scripts which can be executed on different platforms. For example for docker and kubernetes. On docker there can be executed the healthcheck.sh and on kubernetes liveness.sh and readyness.sh. Currently are the scripts for kubernetes symbolic links to the healthcheck.sh file, but the symlink can be removed in the future, when the implementation between booth platforms are different.

I skipped the HEALTHCHECK definition in the Dockerfile, because it is docker specific staff and not supported by the official container spec. We can add a HEALTHCHECK definition as default, but I like to skip it, because maybe it can break user environments. For example, when the healthcheck requires to much CPU time and returns a failure, which leads to be an unhealthy container. Other applications can consume this state, like reverse proxy servers. This reverse proxy servers can skip the application and leads to an unreachable bazarr instance.

How Has This Been Tested?

Executed the healthcheck locally:

$ docker build -t localhost/bazarr:latest --no-cache .
$ docker run --rm --detach --health-cmd /usr/local/bin/healthcheck.sh --health-retries 3 --health-timeout 5s localhost/bazarr:latest`
$ docker ps
CONTAINER ID   IMAGE                               COMMAND   CREATED          STATUS                    PORTS      NAMES
ab2f74a6092a   localhost/bazarr:latest   "/init"   23 minutes ago   Up 23 minutes (healthy)   6767/tcp   frosty_neumann

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thanks for opening this pull request! Be sure to follow the pull request template!

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/bazarr/v1.4.0-pkg-c16d959c-dev-b235301c7007b19b37edafcc2eb5a495c7eb3041-pr-128/index.html
https://ci-tests.linuxserver.io/lspipepr/bazarr/v1.4.0-pkg-c16d959c-dev-b235301c7007b19b37edafcc2eb5a495c7eb3041-pr-128/shellcheck-result.xml

Tag Passed
amd64-v1.4.0-pkg-c16d959c-dev-b235301c7007b19b37edafcc2eb5a495c7eb3041-pr-128
arm64v8-v1.4.0-pkg-c16d959c-dev-b235301c7007b19b37edafcc2eb5a495c7eb3041-pr-128

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/bazarr/v1.4.0-pkg-c16d959c-dev-a6d2c871d51e9df4a6c7edf501d12947c082ef72-pr-128/index.html
https://ci-tests.linuxserver.io/lspipepr/bazarr/v1.4.0-pkg-c16d959c-dev-a6d2c871d51e9df4a6c7edf501d12947c082ef72-pr-128/shellcheck-result.xml

Tag Passed
amd64-v1.4.0-pkg-c16d959c-dev-a6d2c871d51e9df4a6c7edf501d12947c082ef72-pr-128
arm64v8-v1.4.0-pkg-c16d959c-dev-a6d2c871d51e9df4a6c7edf501d12947c082ef72-pr-128

@nemchik
Copy link
Member

nemchik commented Dec 15, 2023

We currently don't package healthchecks into any images (that I'm aware of) because the support for healthchecks has been inconsistent (at best) across various container platforms.

Edit:
If we were to add healthchecks, it would be something we would want to do across all images (or as many as possible) in a consistent way.

@LinuxServer-CI
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. This might be due to missing feedback from OP. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link

This pull request is locked due to inactivity

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants