-
Notifications
You must be signed in to change notification settings - Fork 447
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
[Bug][RayCluster] Fix RAY_REDIS_ADDRESS parsing with redis scheme and… #1556
Merged
kevin85421
merged 1 commit into
ray-project:master
from
rueian:fix-redis-uri-in-cleanup-job
Oct 30, 2023
Merged
[Bug][RayCluster] Fix RAY_REDIS_ADDRESS parsing with redis scheme and… #1556
kevin85421
merged 1 commit into
ray-project:master
from
rueian:fix-redis-uri-in-cleanup-job
Oct 30, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… multiple addresses
rueian
force-pushed
the
fix-redis-uri-in-cleanup-job
branch
from
October 28, 2023 05:24
7f1949d
to
a36a45c
Compare
Hi @kevin85421, Here is the result of running
|
kevin85421
approved these changes
Oct 30, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. #1556 (comment) shows that ray-cluster.external-redis-uri.yaml
passes sample YAML tests.
This was referenced Oct 30, 2023
kevin85421
pushed a commit
to kevin85421/kuberay
that referenced
this pull request
Nov 2, 2023
… multiple addresses (ray-project#1556)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
The current parsing of RAY_REDIS_ADDRESS in a redis cleanup job is not correct and may result in the error reported in the slack channel: https://ray-distributed.slack.com/archives/C02GFQ82JPM/p1697556705340069
This is because the current parsing:
may result in more than two values in the following cases:
RAY_REDIS_ADDRESS
contains multiple addresses.RAY_REDIS_ADDRESS
contains IPv6 addresses.RAY_REDIS_ADDRESS
contains a username and a password.Related issues
#1557
A Potential Solution
We could use the private method
get_address
, which actually parses a redis address, from the upstream ray. This would let us stay consistent with the upstream. However, I think importing theget_address
is not suitable right now for the following reasons:get_address
is too vague. I believe it will be changed to something likeget_redis_address
soon.get_address
doesn't handle multiple addresses which are allowed in theRAY_REDIS_ADDRESS
according to here.get_address
implementation doesn't seem to be quite right to me. It doesn't consider most properties allowed in the redis uri spec, such as password, and database number. A valid redis uri looks like thisredis://user:secret@localhost:6379/0
and it will not be parsed corrected by theget_address
.get_address
was introduced only 2 months ago. I am afraid that it doesn't exist in most ray releases.Proposed Solution
I propose using the standard
urlparse
to handle the parsing. It will help us extract thescheme
,hostname
,port
, and evenusername
andpassword
more correctly. I also recommend contributing this method back to the upstream ray.In this PR, I did the following
.split(',')
and only pick the first part.redis://
scheme prefix to it. This is required by theurlparse
.urlparse
to extract scheme, hostname, port, and password from the address and ignore other properties that aren't supported by thecleanup_redis_storage
. Note that I also enable theuse_ssl
flag if the scheme isrediss
.Testing
To test this change, I added a new test, duplicated from the original
ray-operator/config/samples/ray-cluster.external-redis.yaml
, and added a redis schemeredis://
to itsRAY_REDIS_ADDRESS
.Checks