Skip to content

Commit

Permalink
Fix unhandled Redis exception in the /api/server/health/ endpoint (#7417
Browse files Browse the repository at this point in the history
)

<!-- Raise an issue to propose your change
(https://github.com/opencv/cvat/issues).
It helps to avoid duplication of efforts from multiple independent
contributors.
Discuss your ideas with maintainers to be sure that changes will be
approved and merged.
Read the [Contribution
guide](https://opencv.github.io/cvat/docs/contributing/). -->

<!-- Provide a general summary of your changes in the Title above -->

### Motivation and context
<!-- Why is this change required? What problem does it solve? If it
fixes an open
issue, please link to the issue here. Describe your changes in detail,
add
screenshots. -->
`CustomCacheBackend` does not handle `RedisError`, so if Redis is not
responding, the backend will propagate the exception, and the endpoint
will crash with a 500 response code.

This problem has been fixed upstream in django-health-check 3.18, so
just update to the latest version and remove our custom cache backend.

The upstream version, unlike ours, doesn't handle
`sqlite3.DatabaseError`, but we're not using SQLite for caching anymore,
so I don't think it matters.

### How has this been tested?
<!-- Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc. -->
I checked the endpoint with both working and non-working Redis.

### Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply.
If an item isn't applicable for some reason, then ~~explicitly
strikethrough~~ the whole
line. If you don't do that, GitHub will show incorrect progress for the
pull request.
If you're unsure about any of these, don't hesitate to ask. We're here
to help! -->
- [x] I submit my changes into the `develop` branch
- [x] I have created a changelog fragment <!-- see top comment in
CHANGELOG.md -->
- ~~[ ] I have updated the documentation accordingly~~
- ~~[ ] I have added tests to cover my changes~~
- ~~[ ] I have linked related issues (see [GitHub docs](

https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword))~~
- ~~[ ] I have increased versions of npm packages if it is necessary

([cvat-canvas](https://github.com/opencv/cvat/tree/develop/cvat-canvas#versioning),

[cvat-core](https://github.com/opencv/cvat/tree/develop/cvat-core#versioning),

[cvat-data](https://github.com/opencv/cvat/tree/develop/cvat-data#versioning)
and

[cvat-ui](https://github.com/opencv/cvat/tree/develop/cvat-ui#versioning))~~

### License

- [x] I submit _my code changes_ under the same [MIT License](
https://github.com/opencv/cvat/blob/develop/LICENSE) that covers the
project.
  Feel free to contact the maintainers if that's a concern.
  • Loading branch information
SpecLad authored Feb 5, 2024
1 parent e1ad07c commit edfafff
Show file tree
Hide file tree
Showing 7 changed files with 10 additions and 36 deletions.
4 changes: 4 additions & 0 deletions changelog.d/20240131_202605_roman_bump_health_check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Fixed

- Fix Redis exceptions crashing the `/api/server/health/` endpoint
(<https://github.com/opencv/cvat/pull/7417>)
6 changes: 1 addition & 5 deletions cvat/apps/health/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,12 @@
# SPDX-License-Identifier: MIT

from django.apps import AppConfig
from django.conf import settings

from health_check.plugins import plugin_dir

class HealthConfig(AppConfig):
name = 'cvat.apps.health'

def ready(self):
from .backends import OPAHealthCheck, CustomCacheBackend
from .backends import OPAHealthCheck
plugin_dir.register(OPAHealthCheck)

for backend in settings.CACHES:
plugin_dir.register(CustomCacheBackend, backend=backend)
27 changes: 0 additions & 27 deletions cvat/apps/health/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,11 @@
# SPDX-License-Identifier: MIT

import requests
import sqlite3

from health_check.backends import BaseHealthCheckBackend
from health_check.exceptions import HealthCheckException
from health_check.exceptions import ServiceReturnedUnexpectedResult, ServiceUnavailable

from django.conf import settings
from django.core.cache import CacheKeyWarning, caches

from cvat.utils.http import make_requests_session

Expand All @@ -28,27 +25,3 @@ def check_status(self):

def identifier(self):
return self.__class__.__name__

class CustomCacheBackend(BaseHealthCheckBackend):
def __init__(self, backend="default"):
super().__init__()
self.backend = backend

def identifier(self):
return f"Cache backend: {self.backend}"

def check_status(self):
try:
cache = caches[self.backend]

cache.set("djangohealtcheck_test", "itworks")
if not cache.get("djangohealtcheck_test") == "itworks":
raise ServiceUnavailable("Cache key does not match")
except CacheKeyWarning as e:
self.add_error(ServiceReturnedUnexpectedResult("Cache key warning"), e)
except ValueError as e:
self.add_error(ServiceReturnedUnexpectedResult("ValueError"), e)
except ConnectionError as e:
self.add_error(ServiceReturnedUnexpectedResult("Connection Error"), e)
except sqlite3.DatabaseError as e:
raise ServiceUnavailable("Cache error: {}".format(str(e)))
2 changes: 1 addition & 1 deletion cvat/requirements/base.in
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ django-compressor==4.3.1
django-cors-headers==3.5.0
django-crum==0.7.9
django-filter==2.4.0
django-health-check==3.17.0
django-health-check>=3.18.1,<4
django-rq==2.8.1
django-sendfile2==0.7.0
Django~=4.2.1
Expand Down
4 changes: 2 additions & 2 deletions cvat/requirements/base.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# SHA1:97dd55235acd1b8766b6bd1227ae6d776e9d8e7a
# SHA1:07743309d7b390659b762ca30db20ebc07ac81bc
#
# This file is autogenerated by pip-compile-multi
# To update, run:
Expand Down Expand Up @@ -105,7 +105,7 @@ django-crum==0.7.9
# via -r cvat/requirements/base.in
django-filter==2.4.0
# via -r cvat/requirements/base.in
django-health-check==3.17.0
django-health-check==3.18.1
# via -r cvat/requirements/base.in
django-rq==2.8.1
# via -r cvat/requirements/base.in
Expand Down
1 change: 1 addition & 0 deletions cvat/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ def generate_secret_key():
'corsheaders',
'allauth.socialaccount',
'health_check',
'health_check.cache',
'health_check.db',
'health_check.contrib.migrations',
'health_check.contrib.psutil',
Expand Down
2 changes: 1 addition & 1 deletion utils/dataset_manifest/requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# SHA1:2c4fe23872675e963864abe27e1644f42865f712
# SHA1:c60d1ed19f53b618c5528cd4b4fc708bc7ba404f
#
# This file is autogenerated by pip-compile-multi
# To update, run:
Expand Down

0 comments on commit edfafff

Please sign in to comment.