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.

Sign up for GitHub

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

Jump to bottom

Support Sentinel for Redis connections #132

Closed
sagepe opened this issue Apr 13, 2022 · 2 comments · Fixed by #133
Closed

Support Sentinel for Redis connections #132

sagepe opened this issue Apr 13, 2022 · 2 comments · Fixed by #133
Assignees
@sagepe

Comments

@sagepe
Copy link
Member

sagepe commented Apr 13, 2022

We need to be able to support using Sentinel to mediate Redis connections.
@sagepe sagepe self-assigned this Apr 13, 2022
@sagepe
Copy link
Member Author

sagepe commented Apr 14, 2022 •
edited
Loading

We use mockredis to, well, mock Redis in the tests. This is no longer maintained (and nor is the alternative they suggest using, fakeredis). Nor does it support mocking Sentinel. So if we want to add test coverage for this we'll need to take a wider look at this.

With REDIS_SENTINEL_HOSTS set, we do get the following error from the tests that suggest to me that it's not far off working:

redis.sentinel.MasterNotFoundError: No master found for 'data'

Set REDIS_SENTINEL_HOSTS to null in the exampel config file and tests pass using the usual connection, which at least suggests the logic to select Sentinel or direct connections works.

@sagepe
Copy link
Member Author

sagepe commented Apr 14, 2022

An alternative to mocking would be to add redis and redis-sentinel services to the GitHub action, that would be simple enough to do. It would remove a dependency on the unsupported mockredis (although I did note that this has been forked by RedisLabs with recent commits) while making local testing more complex.

sagepe added a commit that referenced this issue Apr 14, 2022
@sagepe
Redis: Initial support for Redis Sentinel
efc4058 
This adds basic support for using Redis Sentinel to mediate connections
to the primary Redis server used by the API functionality.

Setting `REDIS_SENTINEL_HOSTS` to a dict of "'host': port" key/values
will override any settings for `REDIS_DB_HOST` and `REDIS_DB_PORT` with
values provided by Sentinel.

Note that for the purposes of running tests, this will circumvent the
patching of `api_keys.utils.redis.StrictRedis` by mockredis as calls to
`redis_connection()` will use `sentinel.master_for` rather than
`redis.StrictRedis`, so you'll need functioning Redis and Sentinel
services in this case. Set `REDIS_SENTINEL_HOSTS` to `null` to fall-back
to the existing mocked connection.

Fixes #132
dracos pushed a commit that referenced this issue May 13, 2022
@sagepe @dracos
Redis: Initial support for Redis Sentinel
c07bbc2 
This adds basic support for using Redis Sentinel to mediate connections
to the primary Redis server used by the API functionality.

Setting `REDIS_SENTINEL_HOSTS` to a dict of "'host': port" key/values
will override any settings for `REDIS_DB_HOST` and `REDIS_DB_PORT` with
values provided by Sentinel.

Fixes #132
dracos pushed a commit that referenced this issue May 13, 2022
@sagepe @dracos
Redis: Initial support for Redis Sentinel
f7f5814 
This adds basic support for using Redis Sentinel to mediate connections
to the primary Redis server used by the API functionality.

Setting `REDIS_SENTINEL_HOSTS` to a dict of "'host': port" key/values
will override any settings for `REDIS_DB_HOST` and `REDIS_DB_PORT` with
values provided by Sentinel.

Fixes #132
@dracos dracos mentioned this issue May 13, 2022
Merged
@sagepe sagepe closed this as completed in 2144c39 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sagepe sagepe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add support for Redis Sentinel mysociety/mapit.mysociety.org
1 participant
@sagepe