Radarr icon indicating copy to clipboard operation
Radarr copied to clipboard

Pull Sonarr commit 'New: /ping endpoint for verifying that Sonarr is running and able to access it's DB'

Open servarr[bot] opened this issue 4 years ago • 12 comments

Pull https://github.com/Sonarr/Sonarr/commit/75fc550a3fb44e528ca3c311e83075f79f987a36

git cherry-pick -ex 75fc550a3fb44e528ca3c311e83075f79f987a36

This commit has 2 conflict(s)

You can ask me to close all, open pr here or open pr everywhere

AB#2063

servarr[bot] avatar Dec 04 '21 01:12 servarr[bot]

  • [ ] - include https://github.com/Radarr/Radarr/issues/6787

bakerboy448 avatar Dec 12 '21 04:12 bakerboy448

Would like to see this one added, thanks!

onedr0p avatar Jan 13 '22 03:01 onedr0p

Not a fan of this one.

austinwbest avatar Jan 13 '22 03:01 austinwbest

Not a fan of this one.

Why's that? The issue linked to Sonarr is a very valid reason to support an endpoint like this.

See: https://github.com/Sonarr/Sonarr/issues/4766

onedr0p avatar Jan 13 '22 03:01 onedr0p

I'd personally like to have a ping endpoint without needing to use the API key. Happy to hear alternative views against such an endpoint though.

The current method is having to use something like /api/system/status which requires the API key, this API key needs to be stored securely in whatever client is making the request. This also has to be replicated for any other Arr app so that's multiple API keys needing to be stored securely. None of the information from this endpoint is really needed for basic health check purposes, it's really about getting a 200 OK status code from a reliable endpoint and that's it.

Monitoring the interface or TCP port might work, but it's not really testing the core app itself i.e. database connection OK, if you use third party authentication before something like Sonarr/Radarr you can't do anything like that anyway.

The ping endpoint is essentially designed for this but without the authentication layer. It is also more lightweight than querying /api/system/status given the information from this endpoint isn't really needed for automated test. This ping endpoint returns 200 "OK" for success and I assume a HTTP 500 error with "Error" given any further information i.e. stacktrace could reveal sensitive data, which I believe was originally included but since removed on these grounds.

Seems useful for monitoring without the hassle of needing API keys for this simple check.

jamesmacwhite avatar Feb 14 '22 19:02 jamesmacwhite

I don't think we'd ever want expose something totally unauthenticated

ta264 avatar Feb 14 '22 21:02 ta264

Monitoring the interface or TCP port might work, but it's not really testing the core app itself i.e. database connection OK, if you use third party authentication before something like Sonarr/Radarr you can't do anything like that anyway.

In my SRE experience, application healthchecks should not test the database for connectivity, that is a another system that usually has healthchecks of its own but since sqlite is embedded here that could be argued either way.

I would suggest putting the healthcheck on another port, one that is not exposed anywhere outside the container if database connectivity checks was a requirement. This would probably not be the easiest solution...

Normal: http://radarr:7878 Unauthenticated healthcheck: http://radarr:3000/healthz

Obviously healthchecks work best for container deployment methods otherwise people might complain of another port being expose if deployed on their Windows OS, so it would need to be optional and disabled by default via CLI args or whatever.

I don't think we'd ever want expose something totally unauthenticated

I think a simple unauthenticated (non-database called) /healthz, /ping or whatever endpoint which returns anything if the app is up is more than sufficient and no different that the login page being exposed.

onedr0p avatar Feb 14 '22 22:02 onedr0p

I have plans to stop the login page from using a db connection as well. not a fan of that either because it can also lead to someone looping db hits over and over and over just like this would

austinwbest avatar Feb 14 '22 22:02 austinwbest

Any movement on this? Having a simple /ping endpoint is usefull for monitoring.

hskrtich avatar Jun 26 '22 23:06 hskrtich

Any movement on this? Having a simple /ping endpoint is usefull for monitoring.

Negative.

austinwbest avatar Jun 27 '22 03:06 austinwbest

Any movement on this? Having a simple /ping endpoint is usefull for monitoring.

I need to make a few modifications to cache responses from DB for this endpoint for a short period of time to prevent the ability to load the server by hammering this endpoint. Then it should be going in.

It's on the list.

Qstick avatar Jun 27 '22 14:06 Qstick

Awesome, thank you for the update

hskrtich avatar Jun 27 '22 17:06 hskrtich