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

Add Docker Healthcheck #6680

Merged
merged 5 commits into from Oct 24, 2021
Merged

Add Docker Healthcheck #6680

merged 5 commits into from Oct 24, 2021

Conversation

MarcelCoding
Copy link
Contributor

@MarcelCoding MarcelCoding commented Oct 9, 2021

Changes
Added Docker Healthcheck to ensure jellyfin is always available. (e.g. This enables reverse proxies (traefik) to remove jellyfin from the routes if it is overloaded.)

Issues
Fixed #5760

PS: squash the pr, if the signature should be correct :) I did it in a codespace.

Dockerfile.arm Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile.arm Outdated Show resolved Hide resolved
Dockerfile.arm64 Outdated Show resolved Hide resolved
Co-authored-by: Cody Robibero <cody@robibe.ro>
Copy link
Member

@crobibero crobibero left a comment

Choose a reason for hiding this comment

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

This also may not work if the server the server has RequireHttps enabled

Dockerfile Outdated Show resolved Hide resolved
Dockerfile.arm Outdated Show resolved Hide resolved
Dockerfile.arm64 Outdated Show resolved Hide resolved
Co-authored-by: Cody Robibero <cody@robibe.ro>
@cvium
Copy link
Member

cvium commented Oct 9, 2021

This also may not work if the server the server has RequireHttps enabled

@crobibero Won't it also break if the server isn't listening on localhost?

@crobibero
Copy link
Member

This also may not work if the server the server has RequireHttps enabled

@crobibero Won't it also break if the server isn't listening on localhost?

Yeah that would also not work… maybe we should just have the health check url default to local host:8096 but allow it to be completely overridden using an ENV variable

@MarcelCoding
Copy link
Contributor Author

MarcelCoding commented Oct 10, 2021

Should I create a new env var? If there is a reverse proxy it must be the address and port without the reverse proxy.

@crobibero
Copy link
Member

Should I create a new env var? If there is a reverse proxy it must be the address and port without the reverse proxy.

I want to say yes, but it’ll have to be tested by someone who terminates ssl with Jellyfin and has RequireHttps enabled

@MarcelCoding
Copy link
Contributor Author

I personally use a reverse proxy but without https between jellyfin and the proxy. But I could test it.

@crobibero
Copy link
Member

I believe that’s how most people have Jellyfin set up. Additionally, with 10.8 (unstable) there is an automatic redirect to remove the baseUrl when getting health status

@MarcelCoding
Copy link
Contributor Author

As long as a trusted certificate is used and require https is enabled it is working. If you have any other edge cases that I should test, let me know.

@MarcelCoding
Copy link
Contributor Author

I believe that’s how most people have Jellyfin set up. Additionally, with 10.8 (unstable) there is an automatic redirect to remove the baseUrl when getting health status

Do you mean xxx/baseurl/health -> xxx/health or xxx/health -> xxx/baseurl/health?

@MarcelCoding
Copy link
Contributor Author

I also would like to add the option -L to curl that it follows redirects, just in case someone misses something in the configuration.

@crobibero
Copy link
Member

xxx/health -> xxx/baseurl/health
Implementation here: https://github.com/jellyfin/jellyfin/blob/master/Jellyfin.Server/Middleware/BaseUrlRedirectionMiddleware.cs

Dockerfile.arm Outdated Show resolved Hide resolved
Dockerfile.arm64 Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Co-authored-by: Cody Robibero <cody@robibe.ro>
Copy link
Member

@joshuaboniface joshuaboniface left a comment

Choose a reason for hiding this comment

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

Looks good. @MarcelCoding can you add the same to the https://github.com/jellyfin/jellyfin-metapackages Dockerfiles?

@MarcelCoding
Copy link
Contributor Author

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.

No CURL or WGET in official image to enable health-check
4 participants