RayService event can't set redis password in both GCSFaultTolerance and rayStartParam #3153

fscnick · 2025-03-04T14:54:43Z

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 RayService. Thus, the user need to find the CR event in RayCluster to know the cause.

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-03-04T14:56:20Z

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

@@ -158,6 +159,12 @@ func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
 }

 func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
+	if !reflect.DeepEqual(rayService.Spec.RayClusterSpec, rayv1.RayClusterSpec{}) {


Is this acceptable that it uses reflect.DeepEqual to verify if RayClusterSpec or not?

Hi @fscnick, can’t we use ValidateRayClusterSpec directly? What will happen if use it directly?

Suggested change

if !reflect.DeepEqual(rayService.Spec.RayClusterSpec, rayv1.RayClusterSpec{}) {

if !reflect.ValueOf(rayService.Spec.RayClusterSpec).IsZero() {

Hi @fscnick, can’t we use ValidateRayClusterSpec directly? What will happen if use it directly?

Hi @rueian , it seems that the following test cases implies an empty RayClusterSpec is acceptable.

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

Lines 735 to 748 in a721d60

err = ValidateRayServiceSpec(&rayv1.RayService{

Spec: rayv1.RayServiceSpec{},

})

require.NoError(t, err, "The RayService spec is valid.")

var upgradeStrat rayv1.RayServiceUpgradeType = "invalidStrategy"

err = ValidateRayServiceSpec(&rayv1.RayService{

Spec: rayv1.RayServiceSpec{

UpgradeStrategy: &rayv1.RayServiceUpgradeStrategy{

Type: &upgradeStrat,

},

},

})

require.Error(t, err, "spec.UpgradeSpec.Type is invalid")

However, if trying to apply a yaml without RayClusterSpec. It would pass the creation of RayService but fail on creating RayCluster.

@kevin85421 Should we make RayClusterSpec to required field for RayService CRD?

Should we make RayClusterSpec to required field for RayService CRD?

Wow, I wasn’t aware that RayClusterSpec isn’t a required field in the RayService CRD. It should be required. @fscnick would you mind opening a PR to make it become required?

Remove omitempty and then generate the new CRD YAMLs.

kuberay/ray-operator/apis/ray/v1/rayservice_types.go

Line 78 in a721d60

RayClusterSpec RayClusterSpec `json:"rayClusterConfig,omitempty"`

SG. I think we should make this field required first and remove the corresponding tests. And then we can remove this check completely in this PR.

Should we make RayClusterSpec to required field for RayService CRD?

Wow, I wasn’t aware that RayClusterSpec isn’t a required field in the RayService CRD. It should be required. @fscnick would you mind opening a PR to make it become required?

Remove omitempty and then generate the new CRD YAMLs.

kuberay/ray-operator/apis/ray/v1/rayservice_types.go

Line 78 in a721d60

RayClusterSpec RayClusterSpec `json:"rayClusterConfig,omitempty"`

Sure, I'll create another PR to address this.

fscnick · 2025-03-04T14:57:21Z

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

+	err = ValidateRayServiceSpec(&rayv1.RayService{
+		Spec: rayv1.RayServiceSpec{
+			RayClusterSpec: rayv1.RayClusterSpec{
+				HeadServiceAnnotations: map[string]string{
+					"foo": "bar",
+				},
+				HeadGroupSpec: rayv1.HeadGroupSpec{},
+			},
+		},
+	})
+	require.Error(t, err, "headGroupSpec should have at least one container")


Add an expected error case on ValidateRayClusterSpec inside ValidateRayServiceSpec.

You should also add a testcase for empty RayClusterSpec. Also, please convert this test into subtests and then run them with t.Run. You can reference the TestValidateRayClusterSpecRedisPassword function above.

Follow-up PR: Also convert TestValidateRayJobSpec

kevin85421 · 2025-03-05T17:28:27Z

cc @MortalHappiness @rueian would you mind reviewing this PR?

rueian · 2025-03-12T06:19:45Z

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

@@ -158,6 +159,12 @@ func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
 }

 func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
+	if !reflect.ValueOf(rayService.Spec.RayClusterSpec).IsZero() {


Can this be removed now?

Suggested change

if !reflect.ValueOf(rayService.Spec.RayClusterSpec).IsZero() {

MortalHappiness · 2025-03-12T08:44:18Z

Hi. Please resolve the conflicts. Thanks.

Signed-off-by: fscnick <fscnick.dev@gmail.com>

…terSpec Signed-off-by: fscnick <fscnick.dev@gmail.com>

Signed-off-by: fscnick <fscnick.dev@gmail.com>

kevin85421 · 2025-03-12T18:29:44Z

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

+			name: "headGroupSpec should have at least one container",
+			spec: rayv1.RayServiceSpec{
+				RayClusterSpec: rayv1.RayClusterSpec{
+					HeadServiceAnnotations: map[string]string{


Is HeadServiceAnnotations necessary?

fscnick commented Mar 4, 2025

View reviewed changes

fscnick force-pushed the rayservice-event-cant-set-pwd branch from 9d79335 to 09dfae8 Compare March 4, 2025 15:03

kevin85421 self-assigned this Mar 4, 2025

fscnick marked this pull request as ready for review March 5, 2025 00:27

kevin85421 assigned MortalHappiness Mar 5, 2025

fscnick mentioned this pull request Mar 8, 2025

[RayService] make RayClusterSpec required #3169

Merged

4 tasks

kevin85421 requested review from rueian and MortalHappiness March 12, 2025 06:09

rueian reviewed Mar 12, 2025

View reviewed changes

fscnick added 5 commits March 12, 2025 20:42

[RayService] validate RayClusterSpec on ValidateRayServiceSpec

4b2a631

Signed-off-by: fscnick <fscnick.dev@gmail.com>

[RayService][Test] add an expected error test case on ValidateRayClus…

3bdd681

…terSpec Signed-off-by: fscnick <fscnick.dev@gmail.com>

[RayService] use IsZero to check empty RayClusterSpec

b894a48

Signed-off-by: fscnick <fscnick.dev@gmail.com>

[RayService][Test] refactor TestValidateRayServiceSpec with table test.

2077b45

Signed-off-by: fscnick <fscnick.dev@gmail.com>

[RayService][Test] resolve conflict and apply RayClusterSpec required

445bcb6

Signed-off-by: fscnick <fscnick.dev@gmail.com>

fscnick force-pushed the rayservice-event-cant-set-pwd branch from 113c824 to 445bcb6 Compare March 12, 2025 13:12

fscnick requested review from rueian and kevin85421 March 12, 2025 14:14

rueian approved these changes Mar 12, 2025

View reviewed changes

kevin85421 reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RayService event can't set redis password in both GCSFaultTolerance and rayStartParam #3153

RayService event can't set redis password in both GCSFaultTolerance and rayStartParam #3153

fscnick commented Mar 4, 2025

fscnick Mar 4, 2025

rueian Mar 5, 2025

MortalHappiness Mar 6, 2025

fscnick Mar 6, 2025

fscnick Mar 6, 2025

MortalHappiness Mar 6, 2025

kevin85421 Mar 6, 2025

MortalHappiness Mar 6, 2025

fscnick Mar 7, 2025

fscnick Mar 4, 2025

MortalHappiness Mar 6, 2025 •

edited

Loading

kevin85421 commented Mar 5, 2025

rueian Mar 12, 2025

MortalHappiness commented Mar 12, 2025

kevin85421 Mar 12, 2025

	if !reflect.DeepEqual(rayService.Spec.RayClusterSpec, rayv1.RayClusterSpec{}) {
	if !reflect.ValueOf(rayService.Spec.RayClusterSpec).IsZero() {

	err = ValidateRayServiceSpec(&rayv1.RayService{
	Spec: rayv1.RayServiceSpec{},
	})
	require.NoError(t, err, "The RayService spec is valid.")

	var upgradeStrat rayv1.RayServiceUpgradeType = "invalidStrategy"
	err = ValidateRayServiceSpec(&rayv1.RayService{
	Spec: rayv1.RayServiceSpec{
	UpgradeStrategy: &rayv1.RayServiceUpgradeStrategy{
	Type: &upgradeStrat,
	},
	},
	})
	require.Error(t, err, "spec.UpgradeSpec.Type is invalid")

RayService event can't set redis password in both GCSFaultTolerance and rayStartParam #3153

Are you sure you want to change the base?

RayService event can't set redis password in both GCSFaultTolerance and rayStartParam #3153

Conversation

fscnick commented Mar 4, 2025

Why are these changes needed?

Related issue number

Checks

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

kevin85421 commented Mar 5, 2025

Choose a reason for hiding this comment

MortalHappiness commented Mar 12, 2025

Choose a reason for hiding this comment

MortalHappiness Mar 6, 2025 •

edited

Loading