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 Redis Sentinel configuration #2800

Open
seanrmurphy opened this issue Nov 15, 2022 · 2 comments
Open

Fix Redis Sentinel configuration #2800

seanrmurphy opened this issue Nov 15, 2022 · 2 comments
Labels

Comments

@seanrmurphy
Copy link
Contributor

seanrmurphy commented Nov 15, 2022

Describe the bug

Currently, renku services do not use Redis Sentinel correctly. It is necessary to modify the renku helm chart to enable the sentinels to be specified, to propagate this to the appropriate services and for them to pick up and apply this configuration. This leads to issues where services which are dependent on Redis lose connection to Redis in case of Redis restart and can be inoperational.

Link to project
N/A.

To Reproduce
Terminate the Redis master pod and see some services having issues.

Expected behavior
Services dependent on Redis should use the Sentinel mechanisms to become aware of a change in Redis master.

Screenshots and/or execution output
N/A.

Run environment (please complete the following information):
N/A.

Additional context
Have discussed this with @Panaetius and thought it best to include an issue here. There are related issues for the different renku components:

Proposal:

  • Add the field global.redis.sentinel.sentinelList to the values.yaml
  • This can contain a string of URIs as follows:
sentinelList: "redis-sentinel://renku-redis-node-0.renku-redis-headless:26379,redis-sentinel://renku-redis-node-1.renku-redis-headless:26379,redis-sentinel://renku-redis-node-2.renku-redis-headless:26379"
  • Notes:
    • the above sentinelList does not provide for passwords - it's not clear to me that there is any point/benefit in adding a password to the sentinel in our configuration (currently the sentinel is password protected).
    • redis-sentinel:// does not seem to be IANA recognized; however, it is clear and it is used by the Java lettuce library
    • using a string in this manner allows us to increase the number of sentinels quite easily if necessary; it seems a more natural solution than distributing lists of hosts and ports even if there is somehow a cost of ensuring no error in the string

Logic can be added to the individual services as follows:

  • if the sentinel list is provided, use this, otherwise operate as before
    • this will mean no breaking changes
  • we can revisit this subsequently and decide if we want to change the behaviour after this migration has been performed.
@olevski
Copy link
Member

olevski commented Nov 24, 2022

@seanrmurphy I think you mentioned this already but I think it is worth mentioning here again... Will the python redis clients be able to directly accept the proposed redis sentinel list string or do we need to do some parsing. I think I vaguely remember you mentioning using other libraries to parse this?

Currently the users of redis are:

  • gateway (using python)
  • core service (using python)
  • ui-server (using javascript)

@seanrmurphy
Copy link
Contributor Author

I found this python lib which more or less does the parsing:

https://github.com/exponea/redis-sentinel-url

I did not look for any specific JS/TS lib.

My view is that naming the service redis+sentinel seems a bit unnatural; redis-sentinel feels more comfortable to me (but I understand that's quite a weak reason).

Regarding which of our components use Redis, I provided links to issues above which have been created for each of these components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants