-
Notifications
You must be signed in to change notification settings - Fork 445
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
[RayCluster][Feature] add redis username to head pod from GcsFaultToleranceOptions #2760
Conversation
b786001
to
4793752
Compare
@rueian PTAL when you are free |
@win5923 would you mind rebasing with the master branch? |
2fceba0
to
833e95f
Compare
@kevin85421 Done! PTAL |
833e95f
to
5cadced
Compare
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.
@win5923 would you mind resolving the conflict? Thanks!
@@ -137,6 +148,15 @@ func configureGCSFaultTolerance(podTemplate *corev1.PodTemplateSpec, instance ra | |||
}) | |||
} | |||
} else { | |||
// If users directly set the `redis-username` in `rayStartParams` instead of referring |
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.
I am considering not adding any specific logic for redis-username
(i.e. remove the change from L151 to L159) in the old configurations to avoid maintenance costs and to provide an incentive for users to migrate to the new API, GcsFaultToleranceOptions
. HDYT @rueian?
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.
ok, then should we make the incentive clear in the validateRayClusterSpec
?
@@ -334,12 +336,26 @@ func TestConfigureGCSFaultToleranceWithAnnotations(t *testing.T) { | |||
redisPasswordEnv: "test-password", | |||
isHeadPod: true, | |||
}, | |||
{ |
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.
Please add tests in TestConfigureGCSFaultToleranceWithGcsFTOptions
too.
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.
Done.
c72577c
to
5cadced
Compare
…eranceOptions Signed-off-by: win5923 <[email protected]>
Signed-off-by: win5923 <[email protected]>
20f602f
to
f7080fe
Compare
398357c
to
b5acc94
Compare
Signed-off-by: win5923 <[email protected]>
b5acc94
to
152b126
Compare
instance.Spec.HeadGroupSpec.RayStartParams["redis-username"] = "$REDIS_USERNAME" | ||
container.Env = append(container.Env, corev1.EnvVar{ | ||
Name: utils.REDIS_USERNAME, | ||
Value: options.RedisUsername.Value, |
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.
I think only one of Value or ValurFrom can be set, not both
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.
(same for redis password)
Why are these changes needed?
This PR addresses the following selected part:
Currently, the example YAML file for GCS FT has Redis version 5.0.9, which does not support ACL. This causes a CrashLoopBackOff when setting redisUsername.
Ref: https://redis.io/docs/latest/operate/oss_and_stack/management/security/acl/
Manual Tests
Related issue number
Resolves #2720
Checks