Rayjob event can't set redis password in both GCSFaultTolerance and rayStartParam #3093

fscnick · 2025-02-22T13:49:47Z

Why are these changes needed?

If an user sets redis password in both GCSFaultTolerance and rayStartParam, it will prompt CR event in RayCluster.
However, it's not show in RayJob. Thus, the user need to find the CR event in RayCluster to know the cause.

The PR shows CR event on RayJob if it gets this kind of error.

Here is result of screenshot.

Related issue number

#2986

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

fscnick · 2025-02-22T13:52:52Z

ray-operator/controllers/ray/utils/validation.go

@@ -164,3 +137,47 @@ func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
 	}
 	return nil
 }
+
+func ValidateGCSFaultTolerance(spec *rayv1.RayClusterSpec, annotations map[string]string) error {


Put GCSFaultTolerance-related checks in a function to make it reusable in RayCluster and RayJob. The code in the function is mainly from

kuberay/ray-operator/controllers/ray/utils/validation.go

Lines 34 to 72 in 5ccf361

if instance.Annotations[RayFTEnabledAnnotationKey] != "" && instance.Spec.GcsFaultToleranceOptions != nil {

return fmt.Errorf("%s annotation and GcsFaultToleranceOptions are both set. "+

"Please use only GcsFaultToleranceOptions to configure GCS fault tolerance", RayFTEnabledAnnotationKey)

}

if !IsGCSFaultToleranceEnabled(*instance) {

if EnvVarExists(RAY_REDIS_ADDRESS, instance.Spec.HeadGroupSpec.Template.Spec.Containers[RayContainerIndex].Env) {

return fmt.Errorf("%s is set which implicitly enables GCS fault tolerance, "+

"but GcsFaultToleranceOptions is not set. Please set GcsFaultToleranceOptions "+

"to enable GCS fault tolerance", RAY_REDIS_ADDRESS)

}

}

headContainer := instance.Spec.HeadGroupSpec.Template.Spec.Containers[RayContainerIndex]

if instance.Spec.GcsFaultToleranceOptions != nil {

if redisPassword := instance.Spec.HeadGroupSpec.RayStartParams["redis-password"]; redisPassword != "" {

return fmt.Errorf("cannot set `redis-password` in rayStartParams when " +

"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisPassword instead")

}

if EnvVarExists(REDIS_PASSWORD, headContainer.Env) {

return fmt.Errorf("cannot set `REDIS_PASSWORD` env var in head Pod when " +

"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisPassword instead")

}

if EnvVarExists(RAY_REDIS_ADDRESS, headContainer.Env) {

return fmt.Errorf("cannot set `RAY_REDIS_ADDRESS` env var in head Pod when " +

"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisAddress instead")

}

if instance.Annotations[RayExternalStorageNSAnnotationKey] != "" {

return fmt.Errorf("cannot set `ray.io/external-storage-namespace` annotation when " +

"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.ExternalStorageNamespace instead")

}

}

if instance.Spec.HeadGroupSpec.RayStartParams["redis-username"] != "" || EnvVarExists(REDIS_USERNAME, headContainer.Env) {

return fmt.Errorf("cannot set redis username in rayStartParams or environment variables" +

" - use GcsFaultToleranceOptions.RedisUsername instead")

}

fscnick · 2025-02-22T13:56:25Z

ray-operator/controllers/ray/utils/validation_test.go

-									{
-										Env: tt.envVars,
-									},
+			err := ValidateGCSFaultTolerance(&rayv1.RayClusterSpec{


Change to ValidateGCSFaultTolerance because this test is testing GcsFaultToleranceOptions.

fscnick · 2025-02-23T12:53:39Z

@kevin85421 PTAL

kevin85421 · 2025-02-23T23:22:26Z

ray-operator/controllers/ray/utils/validation.go

@@ -112,6 +76,13 @@ func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
 	if rayJob.Spec.RayClusterSpec == nil && !isClusterSelectorMode {
 		return fmt.Errorf("one of RayClusterSpec or ClusterSelector must be set")
 	}
+
+	if rayJob.Spec.RayClusterSpec != nil {
+		if err := ValidateGCSFaultTolerance(rayJob.Spec.RayClusterSpec, rayJob.Annotations); err != nil {


It's better to validate the entire RayCluster spec in the RayJob controller instead of validating different parts one by one. That is,

if rayJob.Spec.RayClusterSpec != nil { if err := ValidateRayClusterSpec(rayJob.Spec.RayClusterSpec, rayJob.Annotations); err != nil { ... } }

In addition, this assumes that RayJob will propagate annotations to RayCluster. Could you please check whether we have tests to ensure this behavior? If not, you can add tests in this PR or open another PR to add tests.

It seems there is no such test to ensure that. I'll create another PR to add tests.

kevin85421 · 2025-02-23T23:25:57Z

ray-operator/controllers/ray/utils/validation.go

@@ -164,3 +135,47 @@ func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
 	}
 	return nil
 }
+
+func ValidateGCSFaultTolerance(spec *rayv1.RayClusterSpec, annotations map[string]string) error {


We can do the refactoring, but it's better to do it in a follow-up PR. Let's focus on calling ValidateRayClusterSpec in RayJob controller in this PR.

This function has been removed and the code has put back inside ValidateRayClusterSpec.

kevin85421

Split IsAutoscalingEnabled into a separate function to make it easier to review.

fscnick · 2025-02-28T15:37:29Z

@kevin85421 PTAL

kevin85421 · 2025-02-28T19:00:15Z

ray-operator/controllers/ray/utils/validation_test.go

@@ -201,30 +229,44 @@ func TestValidateRayClusterSpecGcsFaultToleranceOptions(t *testing.T) {
 			expectError:              true,
 			errorMessage:             errorMessageExternalStorageNamespaceConflict,
 		},
+		{


These two tests have already been tested in TestValidateRayClusterSpecRedisUsername.

kevin85421 · 2025-02-28T19:01:11Z

ray-operator/controllers/ray/utils/validation_test.go

@@ -156,6 +163,27 @@ func TestValidateRayClusterSpecGcsFaultToleranceOptions(t *testing.T) {
 			expectError:  true,
 			errorMessage: errorMessageRedisAddressSet,
 		},
+		{


These two tests have already been tested in TestValidateRayClusterSpecRedisPassword.

kevin85421 · 2025-02-28T19:03:22Z

ray-operator/controllers/ray/utils/validation_test.go

@@ -576,6 +618,14 @@ func TestValidateRayJobStatus(t *testing.T) {
 }

 func TestValidateRayJobSpec(t *testing.T) {


I would expect a test with a RayJob with an invalid RayClusterSpec

kevin85421 · 2025-03-02T08:10:52Z

Would you mind rebasing with the master branch? There is a CI fix in the master branch.

Signed-off-by: fscnick <[email protected]>

…tTolerance Signed-off-by: fscnick <[email protected]>

Signed-off-by: fscnick <[email protected]>

… RayJob Signed-off-by: fscnick <[email protected]>

fscnick · 2025-03-02T09:44:22Z

ray-operator/controllers/ray/utils/validation_test.go

+	err = ValidateRayJobSpec(&rayv1.RayJob{
+		Spec: rayv1.RayJobSpec{
+			RayClusterSpec: &rayv1.RayClusterSpec{
+				HeadGroupSpec: rayv1.HeadGroupSpec{},
+			},
+		},
+	})
+	require.ErrorContains(t, err, "headGroupSpec should have at least one container")


Here is the invalid RayClusterSpec in RayJob according to suggestion
#3093 (comment)

kevin85421 · 2025-03-02T18:47:43Z

Nice! Would you mind opening another PR to validate the cluster spec in RayService?

…ayStartParam (ray-project#3093)

fscnick · 2025-03-03T00:31:19Z

Nice! Would you mind opening another PR to validate the cluster spec in RayService?

Sure, I'll do that.

fscnick commented Feb 22, 2025

View reviewed changes

fscnick marked this pull request as ready for review February 23, 2025 12:21

kevin85421 reviewed Feb 23, 2025

View reviewed changes

kevin85421 self-assigned this Feb 23, 2025

fscnick requested a review from kevin85421 February 24, 2025 14:48

kevin85421 reviewed Feb 24, 2025

View reviewed changes

fscnick force-pushed the rayjob-event-cant-set-pwd branch from 864bb09 to 7615508 Compare February 25, 2025 14:20

fscnick requested a review from kevin85421 February 25, 2025 15:25

fscnick mentioned this pull request Feb 26, 2025

[RayCluster] IsAutoscalingEnabled takes RayClusterSpec #3111

Merged

4 tasks

fscnick force-pushed the rayjob-event-cant-set-pwd branch from 7615508 to 7dc75c2 Compare February 27, 2025 11:36

kevin85421 reviewed Feb 28, 2025

View reviewed changes

kevin85421 approved these changes Mar 2, 2025

View reviewed changes

fscnick added 10 commits March 2, 2025 16:41

[RayJob] validate GCSFaultTolerance

c7217c5

Signed-off-by: fscnick <[email protected]>

[RayCluster][Test] reduce test dependences

c760664

Signed-off-by: fscnick <[email protected]>

[RayCluster][Test] add more test cases for GCSFaultTolerance

d79b5f5

Signed-off-by: fscnick <[email protected]>

[RayJob] check RayClusterSpec is nil or not before validating GCSFaul…

b55c19f

…tTolerance Signed-off-by: fscnick <[email protected]>

[RayJob] use ValidateRayClusterSpec instead

9ae8ebd

Signed-off-by: fscnick <[email protected]>

[RayCluster] separate IsAutoscalingEnabled

ad1e72f

Signed-off-by: fscnick <[email protected]>

[RayCluster] apply changing IsAutoscalingEnabled from another pr

26226d8

Signed-off-by: fscnick <[email protected]>

[RayCluster] remove ValidateGCSFaultTolerance

dedd307

Signed-off-by: fscnick <[email protected]>

[RayCluster][Test] remove duplicated tests

43d678c

Signed-off-by: fscnick <[email protected]>

[RayJob][Test] add a test error on ValidateRayClusterSpec for testing…

6d4d001

… RayJob Signed-off-by: fscnick <[email protected]>

fscnick force-pushed the rayjob-event-cant-set-pwd branch from 5b61cb9 to 6d4d001 Compare March 2, 2025 08:42

fscnick commented Mar 2, 2025

View reviewed changes

kevin85421 merged commit 6c4a77d into ray-project:master Mar 2, 2025
21 checks passed

tinaxfwu pushed a commit to tinaxfwu/kuberay that referenced this pull request Mar 2, 2025

Rayjob event can't set redis password in both GCSFaultTolerance and r…

e610caa

…ayStartParam (ray-project#3093)

fscnick deleted the rayjob-event-cant-set-pwd branch March 3, 2025 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rayjob event can't set redis password in both GCSFaultTolerance and rayStartParam #3093

Rayjob event can't set redis password in both GCSFaultTolerance and rayStartParam #3093

fscnick commented Feb 22, 2025 •

edited

Loading

fscnick Feb 22, 2025 •

edited

Loading

fscnick Feb 22, 2025

fscnick commented Feb 23, 2025

kevin85421 Feb 23, 2025

fscnick Feb 24, 2025

kevin85421 Feb 23, 2025

fscnick Feb 24, 2025

kevin85421 left a comment

fscnick commented Feb 28, 2025

kevin85421 Feb 28, 2025

kevin85421 Feb 28, 2025

kevin85421 Feb 28, 2025

kevin85421 commented Mar 2, 2025

fscnick Mar 2, 2025

kevin85421 commented Mar 2, 2025

fscnick commented Mar 3, 2025

	if instance.Annotations[RayFTEnabledAnnotationKey] != "" && instance.Spec.GcsFaultToleranceOptions != nil {
	return fmt.Errorf("%s annotation and GcsFaultToleranceOptions are both set. "+
	"Please use only GcsFaultToleranceOptions to configure GCS fault tolerance", RayFTEnabledAnnotationKey)
	}

	if !IsGCSFaultToleranceEnabled(*instance) {
	if EnvVarExists(RAY_REDIS_ADDRESS, instance.Spec.HeadGroupSpec.Template.Spec.Containers[RayContainerIndex].Env) {
	return fmt.Errorf("%s is set which implicitly enables GCS fault tolerance, "+
	"but GcsFaultToleranceOptions is not set. Please set GcsFaultToleranceOptions "+
	"to enable GCS fault tolerance", RAY_REDIS_ADDRESS)
	}
	}

	headContainer := instance.Spec.HeadGroupSpec.Template.Spec.Containers[RayContainerIndex]
	if instance.Spec.GcsFaultToleranceOptions != nil {
	if redisPassword := instance.Spec.HeadGroupSpec.RayStartParams["redis-password"]; redisPassword != "" {
	return fmt.Errorf("cannot set `redis-password` in rayStartParams when " +
	"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisPassword instead")
	}

	if EnvVarExists(REDIS_PASSWORD, headContainer.Env) {
	return fmt.Errorf("cannot set `REDIS_PASSWORD` env var in head Pod when " +
	"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisPassword instead")
	}

	if EnvVarExists(RAY_REDIS_ADDRESS, headContainer.Env) {
	return fmt.Errorf("cannot set `RAY_REDIS_ADDRESS` env var in head Pod when " +
	"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.RedisAddress instead")
	}

	if instance.Annotations[RayExternalStorageNSAnnotationKey] != "" {
	return fmt.Errorf("cannot set `ray.io/external-storage-namespace` annotation when " +
	"GcsFaultToleranceOptions is enabled - use GcsFaultToleranceOptions.ExternalStorageNamespace instead")
	}
	}
	if instance.Spec.HeadGroupSpec.RayStartParams["redis-username"] != "" \|\| EnvVarExists(REDIS_USERNAME, headContainer.Env) {
	return fmt.Errorf("cannot set redis username in rayStartParams or environment variables" +
	" - use GcsFaultToleranceOptions.RedisUsername instead")
	}

		@@ -576,6 +618,14 @@ func TestValidateRayJobStatus(t *testing.T) {
		}

		func TestValidateRayJobSpec(t *testing.T) {

Rayjob event can't set redis password in both GCSFaultTolerance and rayStartParam #3093

Rayjob event can't set redis password in both GCSFaultTolerance and rayStartParam #3093

Conversation

fscnick commented Feb 22, 2025 • edited Loading

Why are these changes needed?

Related issue number

Checks

fscnick Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fscnick commented Feb 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 left a comment

Choose a reason for hiding this comment

fscnick commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Mar 2, 2025

Choose a reason for hiding this comment

kevin85421 commented Mar 2, 2025

fscnick commented Mar 3, 2025

fscnick commented Feb 22, 2025 •

edited

Loading

fscnick Feb 22, 2025 •

edited

Loading