Resloved comments

Signed-off-by: win5923 <[email protected]>
ray-project · Jan 21, 2025 · 398357c · 398357c
1 parent 4abe1b1
commit 398357c
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 13 deletions.
diff --git a/ray-operator/controllers/ray/common/pod.go b/ray-operator/controllers/ray/common/pod.go
@@ -126,6 +126,7 @@ func configureGCSFaultTolerance(podTemplate *corev1.PodTemplateSpec, instance ra
 					Value: options.RedisAddress,
 				})
 				if options.RedisUsername != nil {
+					// Note that `redis-username` will be supported starting from Ray 2.41.
 					// If `GcsFaultToleranceOptions.RedisUsername` is set, it will be put into the
 					// `REDIS_USERNAME` environment variable later. Here, we use `$REDIS_USERNAME` in
 					// rayStartParams to refer to the environment variable.
@@ -148,17 +149,8 @@ func configureGCSFaultTolerance(podTemplate *corev1.PodTemplateSpec, instance ra
 					})
 				}
 			} else {
-				// If users directly set the `redis-username` in `rayStartParams` instead of referring
-				// to a K8s secret, we need to set the `REDIS_USERNAME` env var so that the Redis cleanup
-				// job can connect to Redis using the username.
-				if !utils.EnvVarExists(utils.REDIS_USERNAME, container.Env) {
-					// setting the REDIS_USERNAME env var from the params
-					redisUsernameEnv := corev1.EnvVar{Name: utils.REDIS_USERNAME}
-					if value, ok := instance.Spec.HeadGroupSpec.RayStartParams["redis-username"]; ok {
-						redisUsernameEnv.Value = value
-					}
-					container.Env = append(container.Env, redisUsernameEnv)
-				}
+				container.Env = append(container.Env, corev1.EnvVar{Name: utils.REDIS_USERNAME})
+
 				// If users directly set the `redis-password` in `rayStartParams` instead of referring
 				// to a K8s secret, we need to set the `REDIS_PASSWORD` env var so that the Redis cleanup
 				// job can connect to Redis using the password. This is not recommended.

diff --git a/ray-operator/controllers/ray/common/pod_test.go b/ray-operator/controllers/ray/common/pod_test.go
@@ -518,6 +518,19 @@ func TestConfigureGCSFaultToleranceWithGcsFTOptions(t *testing.T) {
 			},
 			isHeadPod: true,
 		},
+		{
+			name: "GCS FT enabled with redis username and password",
+			gcsFTOptions: &rayv1.GcsFaultToleranceOptions{
+				RedisAddress: "redis:6379",
+				RedisUsername: &rayv1.RedisCredential{
+					Value: "test-username",
+				},
+				RedisPassword: &rayv1.RedisCredential{
+					Value: "test-password",
+				},
+			},
+			isHeadPod: true,
+		},
 		{
 			name: "GCS FT enabled with redis password in secret",
 			gcsFTOptions: &rayv1.GcsFaultToleranceOptions{
@@ -533,7 +546,28 @@ func TestConfigureGCSFaultToleranceWithGcsFTOptions(t *testing.T) {
 			isHeadPod: true,
 		},
 		{
-			name: "GCS FT enabled with redis password in secret",
+			name: "GCS FT enabled with redis username and password in secret",
+			gcsFTOptions: &rayv1.GcsFaultToleranceOptions{
+				RedisAddress: "redis:6379",
+				RedisUsername: &rayv1.RedisCredential{
+					ValueFrom: &corev1.EnvVarSource{
+						FieldRef: &corev1.ObjectFieldSelector{
+							FieldPath: "spec.redisUsername",
+						},
+					},
+				},
+				RedisPassword: &rayv1.RedisCredential{
+					ValueFrom: &corev1.EnvVarSource{
+						FieldRef: &corev1.ObjectFieldSelector{
+							FieldPath: "spec.redisPassword",
+						},
+					},
+				},
+			},
+			isHeadPod: true,
+		},
+		{
+			name: "GCS FT enabled with redis external storage namespace",
 			gcsFTOptions: &rayv1.GcsFaultToleranceOptions{
 				RedisAddress:             "redis:6379",
 				ExternalStorageNamespace: "test-ns",
@@ -593,6 +627,12 @@ func TestConfigureGCSFaultToleranceWithGcsFTOptions(t *testing.T) {
 				env := getEnvVar(container, utils.RAY_REDIS_ADDRESS)
 				assert.Equal(t, env.Value, "redis:6379")
 
+				if test.gcsFTOptions.RedisUsername != nil {
+					env := getEnvVar(container, utils.REDIS_USERNAME)
+					assert.Equal(t, env.Value, test.gcsFTOptions.RedisUsername.Value)
+					assert.Equal(t, env.ValueFrom, test.gcsFTOptions.RedisUsername.ValueFrom)
+				}
+
 				if test.gcsFTOptions.RedisPassword != nil {
 					env := getEnvVar(container, utils.REDIS_PASSWORD)
 					assert.Equal(t, env.Value, test.gcsFTOptions.RedisPassword.Value)

diff --git a/ray-operator/controllers/ray/raycluster_controller.go b/ray-operator/controllers/ray/raycluster_controller.go
@@ -1237,7 +1237,7 @@ func (r *RayClusterReconciler) buildRedisCleanupJob(ctx context.Context, instanc
 			"parsed = urlparse(redis_address); ",
 	}
 	if utils.EnvVarExists(utils.REDIS_USERNAME, pod.Spec.Containers[utils.RayContainerIndex].Env) {
-		pod.Spec.Containers[utils.RayContainerIndex].Args[0] += "sys.exit(1) if not cleanup_redis_storage(host=parsed.hostname, port=parsed.port, username=os.getenv('REDIS_USERNAME', parsed.username), password=os.getenv('REDIS_PASSWORD', parsed.password), use_ssl=parsed.scheme=='rediss', storage_namespace=os.getenv('RAY_external_storage_namespace')) else None\""
+		pod.Spec.Containers[utils.RayContainerIndex].Args[0] += "sys.exit(1) if not cleanup_redis_storage(host=parsed.hostname, port=parsed.port, username=os.getenv('REDIS_USERNAME', parsed.username), password=os.getenv('REDIS_PASSWORD', parsed.password or ''), use_ssl=parsed.scheme=='rediss', storage_namespace=os.getenv('RAY_external_storage_namespace')) else None\""
 	} else {
 		pod.Spec.Containers[utils.RayContainerIndex].Args[0] += "sys.exit(1) if not cleanup_redis_storage(host=parsed.hostname, port=parsed.port, password=os.getenv('REDIS_PASSWORD', parsed.password or ''), use_ssl=parsed.scheme=='rediss', storage_namespace=os.getenv('RAY_external_storage_namespace')) else None\""
 	}