[Feature] Validation of RayFTEnabled is false and GcsFaultToleranceOption is not nil #2726

CheyuWu · 2025-01-10T13:42:41Z

Why are these changes needed?

The annotation explicitly disables GCS FT (i.e., ray.io/ft-enabled: "false") and GcsFaultToleranceOptions is not nil. This case is invalid.
Therefore, I add a validation to check

Related issue number

Closes #2694

Checks

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

CheyuWu · 2025-01-10T13:43:03Z

@MortalHappiness @kevin85421 PTAL

MortalHappiness · 2025-01-10T13:48:52Z

ray-operator/controllers/ray/common/pod.go

-
+	// Validation for invalid case: FT explicitly enabled and GcsFaultToleranceOptions is not nil
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] == "false" && instance.Spec.GcsFaultToleranceOptions != nil {
+		panic("Invalid configuration: GCS Fault Tolerance is explicitly disabled (ray.io/ft-enabled: false) while GcsFaultToleranceOptions is set")


Don't panic here. It is user's error instead of our error. Make the instance in failed state instead.

@MortalHappiness
It is confirmed that panic needs to be removed.

I’d like to discuss the initTemplateAnnotations function. If I remove the panic and instead set the instance to a failed state, since it’s not using a pointer, it might not be possible to propagate the failed state back to the higher-level instance.

If I modify initTemplateAnnotations to pass the instance as a pointer, the failed state will propagate back to the calling function, e.g., DefaultWorkerPodTemplate. However, the instance passed to it is also not using a pointer, so the original instance wouldn’t be affected. If I were to change that, it would involve many modifications.

I want to confirm if it’s okay just to push forward and make these changes directly.

Or did I actually misunderstand something?

Don't pass pointer. You can either:

Change the function signature to returns error. And let the caller handle it. Propogate the error the callers.

Don't validate spec here. Validate it in function like validateRayClusterSpec.

OK, no problem

CheyuWu · 2025-01-10T18:18:14Z

Added new rayClusterType -> InvalidConfiguration
Use instance as a pointer and pass the parameter into initTemplateAnnotations

If there is anything that needs to be changed, please let me know.

kevin85421 · 2025-01-10T19:29:02Z

ray-operator/controllers/ray/common/pod.go

 	if podTemplate.Annotations == nil {
 		podTemplate.Annotations = make(map[string]string)
 	}
-
+	// Validation for invalid case: FT explicitly enabled and GcsFaultToleranceOptions is not nil
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] == "false" && instance.Spec.GcsFaultToleranceOptions != nil {


can you add a function validateRayClusterSpec and add the check in the very beginning of rayClusterReconcile?

You can refer to the following links:

kuberay/ray-operator/controllers/ray/raycluster_controller.go

Line 232 in 9068102

if err := r.validateRayClusterStatus(instance); err != nil {

kuberay/ray-operator/controllers/ray/rayjob_controller.go

Line 132 in 9068102

if err := validateRayJobSpec(rayJobInstance); err != nil {

kevin85421 · 2025-01-10T19:35:04Z

also cc @rueian for review.

CheyuWu · 2025-01-11T14:35:50Z

I reverted the previous commit and added the validateRayClusterSpec function
add unit test for validateRayClusterSpec

rueian · 2025-01-11T20:11:10Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -220,10 +220,26 @@ func (r *RayClusterReconciler) validateRayClusterStatus(instance *rayv1.RayClust
 	return nil
 }

+// Validation for invalid Ray Cluster configurations.
+func (r *RayClusterReconciler) validateRayClusterSpec(instance *rayv1.RayCluster) error {
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] == "false" && instance.Spec.GcsFaultToleranceOptions != nil {


Besides this case, we also need to reject the case where the instance has RAY_REDIS_ADDRESS env set but has no instance.Annotations[utils.RayFTEnabledAnnotationKey] == "true".

And this validateRayClusterSpec validation should also be invoked in the optional validation webhook:

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

Line 51 in 9068102

func (r *RayCluster) validateRayCluster() error {

Note that the validation webhook is usually the recommended way to validate a k8s CR, but users can choose not to enable it. That is the reason we need to do validation in both reconciliation and the webhook.

OK, no problem

rueian · 2025-01-11T20:17:07Z

ray-operator/controllers/ray/raycluster_controller.go

+		logger.Error(err, "The RayCluster spec is invalid")
+		r.Recorder.Eventf(instance, corev1.EventTypeWarning, string(utils.InvalidRayClusterSpec),
+			"The RayCluster spec is invalid %s/%s: %v", instance.Namespace, instance.Name, err)
+		return ctrl.Result{RequeueAfter: DefaultRequeueDuration}, err


I think we don't need to requeue the invalid RayCluster.

…s no nil

This reverts commit c2da53c.

…ceOptions no nil" This reverts commit b42c48f.

…nv set but has no instance.Annotations[utils.RayFTEnabledAnnotationKey] == "true"

rueian · 2025-01-12T18:46:51Z

ray-operator/apis/ray/v1/raycluster_webhook.go

+	if r.Annotations[RayFTEnabledAnnotationKey] == "false" && r.Spec.GcsFaultToleranceOptions != nil {
+		return field.Invalid(field.NewPath("spec").Child("gcsFaultToleranceOptions"), r.Spec.GcsFaultToleranceOptions, "GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is disabled")
+	}
+	if r.Annotations[RayFTEnabledAnnotationKey] != "true" && r.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {


Suggested change

if r.Annotations[RayFTEnabledAnnotationKey] != "true" && r.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {

if r.Annotations[RayFTEnabledAnnotationKey] != "true" && utils.EnvVarExists("RAY_REDIS_ADDRESS ", r.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env) {

And, is it possible that r.Spec.HeadGroupSpec.Template.Spec.Containers doesn't exist at this point?

If I use utils, it will cause import cycle.
I thinks we need to move ray-operator/controllers/ray/utils/constant.go to other place to avoid import cycle?

And, is it possible that r.Spec.HeadGroupSpec.Template.Spec.Containers doesn't exist at this point?

OK, I will handle this

I thinks we need to move ray-operator/controllers/ray/utils/constant.go to other place to avoid import cycle?

One thing we could try is moving the EnvVarExists and RayContainerIndex to the /ray/v1 while keeping their references in the utils.

CheyuWu · 2025-01-12T18:49:37Z

I removed the action of requeue the invalid RayCluster.
Add a condition for reject the case where the instance has RAY_REDIS_ADDRESS env set but has no instance.Annotations[utils.RayFTEnabledAnnotationKey] == "true"
Create a new function and relative parameters in validateRayClusterSpec

If I use functions in utils, it will cause import cycle.

kevin85421

Maybe we can leave the webhook change for another PR, so this PR will be easier to review.

kevin85421 · 2025-01-12T22:56:23Z

ray-operator/controllers/ray/raycluster_controller.go

 func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance *rayv1.RayCluster) (ctrl.Result, error) {
 	var reconcileErr error
 	logger := ctrl.LoggerFrom(ctx)

+	// Validate that GcsFaultToleranceOptions is nil when FT is explicitly disabled


Move it below utils.ManagedByExternalController.

kevin85421 · 2025-01-12T22:57:18Z

ray-operator/controllers/ray/raycluster_controller.go

 func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance *rayv1.RayCluster) (ctrl.Result, error) {
 	var reconcileErr error
 	logger := ctrl.LoggerFrom(ctx)

+	// Validate that GcsFaultToleranceOptions is nil when FT is explicitly disabled


Remove the comment. validateRayClusterSpec is to validate the whole RayClusterSpec instead of GcsFaultToleranceOptions` only.

Suggested change

// Validate that GcsFaultToleranceOptions is nil when FT is explicitly disabled

kevin85421 · 2025-01-12T22:59:11Z

ray-operator/controllers/ray/raycluster_controller.go

+// Validation for invalid Ray Cluster configurations.
+func (r *RayClusterReconciler) validateRayClusterSpec(instance *rayv1.RayCluster) error {
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] == "false" && instance.Spec.GcsFaultToleranceOptions != nil {
+		return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is disabled")


Suggested change

return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is disabled")

return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is set to false")

kevin85421 · 2025-01-12T23:01:26Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -220,10 +220,33 @@ func (r *RayClusterReconciler) validateRayClusterStatus(instance *rayv1.RayClust
 	return nil
 }

+// Validation for invalid Ray Cluster configurations.
+func (r *RayClusterReconciler) validateRayClusterSpec(instance *rayv1.RayCluster) error {


This function doesn't need to be the member function of RayClusterReconciler. You can open another PR to update validateRayClusterStatus.

Suggested change

func (r *RayClusterReconciler) validateRayClusterSpec(instance *rayv1.RayCluster) error {

func validateRayClusterSpec(instance *rayv1.RayCluster) error {

kevin85421 · 2025-01-12T23:03:01Z

ray-operator/controllers/ray/raycluster_controller.go

+		return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is disabled")
+	}
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] != "true" && instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {
+		for _, env := range instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env {


Use utils.EnvVarExists

kevin85421 · 2025-01-12T23:05:42Z

ray-operator/controllers/ray/raycluster_controller.go

+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] != "true" && instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {
+		for _, env := range instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env {
+			if env.Name == "RAY_REDIS_ADDRESS" {
+				return fmt.Errorf("RAY_REDIS_ADDRESS should not be set when ray.io/ft-enabled is disabled")


Suggested change

return fmt.Errorf("RAY_REDIS_ADDRESS should not be set when ray.io/ft-enabled is disabled")

return fmt.Errorf("RAY_REDIS_ADDRESS should not be set when ray.io/ft-enabled is not set or set to false")

kevin85421 · 2025-01-12T23:06:02Z

ray-operator/controllers/ray/raycluster_controller.go

+	}
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] != "true" && instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {
+		for _, env := range instance.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env {
+			if env.Name == "RAY_REDIS_ADDRESS" {


Add RAY_REDIS_ADDRESS to constant.go.

This reverts commit ef011c1.

…d unit test" This reverts commit 8b50a8e.

…conciler, to avoid HeadGroupSpec container count < 0, use utils.EnvVarExists to check env variables

CheyuWu · 2025-01-13T14:36:50Z

Reverted the webhook changes in this PR
Moved the webhook changes to [Feature] Add ValidateRayClusterSpec to Webhook #2739
Move validateRayClusterStatus out of RayClusterReconciler [Refactor] move validateRayClusterStatus out of RayClusterReconciler #2738
Fix the code review issue related to validateRayClusterSpec

PTAL thx

rueian · 2025-01-13T15:18:42Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -229,6 +246,13 @@ func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance
 		return ctrl.Result{}, nil
 	}

+	if err := validateRayClusterSpec(instance); err != nil {
+		logger.Error(err, "The RayCluster spec is invalid")


Could you also add instance.Namespace and instance.Name into the message?

rueian · 2025-01-13T15:22:24Z

ray-operator/controllers/ray/raycluster_controller.go

+		logger.Error(err, "The RayCluster spec is invalid")
+		r.Recorder.Eventf(instance, corev1.EventTypeWarning, string(utils.InvalidRayClusterSpec),
+			"The RayCluster spec is invalid %s/%s: %v", instance.Namespace, instance.Name, err)
+		return ctrl.Result{}, err


Suggested change

return ctrl.Result{}, err

return ctrl.Result{}, nil

I guess returning an err here still requeues the invalid CR. Could you double check on that?

… avoid rescheduling in `validateRayClusterSpec`

CheyuWu · 2025-01-13T17:18:01Z

Add instance.Namespace and instance.Name into the logger message
return nil in validateRayClusterSpec to avoid requeues

rueian · 2025-01-13T17:53:11Z

ray-operator/controllers/ray/raycluster_controller.go

+
+	if instance.Annotations[utils.RayFTEnabledAnnotationKey] != "true" &&
+		len(instance.Spec.HeadGroupSpec.Template.Spec.Containers) > 0 &&
+		instance.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env != nil {


instance.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env != nil check is not necessary.

rueian · 2025-01-13T17:53:59Z

ray-operator/controllers/ray/raycluster_controller.go

+		instance.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env != nil {
+
+		if utils.EnvVarExists(utils.RAY_REDIS_ADDRESS, instance.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env) {
+			return fmt.Errorf("%s should not be set when %s is set to false", utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey)


Suggested change

return fmt.Errorf("%s should not be set when %s is set to false", utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey)

return fmt.Errorf("%s environment variable should not be set when %s annotation is not set to true", utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey)

kevin85421 · 2025-01-13T23:18:57Z

@CheyuWu would you mind fixing the conflicts?

…tion

…hange log message

CheyuWu · 2025-01-14T14:35:06Z

fix merge conflict
change error log message
remove not necessary check

kevin85421

I will open a follow up PR to address some issues directly.

dismiss

…tion is not nil (ray-project#2726)

CheyuWu changed the title ~~feat: validation of RayFTEnabled == false and GcsFaultToleranceOption…~~ [Feature] Vvalidation of RayFTEnabled is false and GcsFaultToleranceOption is not nil Jan 10, 2025

CheyuWu changed the title ~~[Feature] Vvalidation of RayFTEnabled is false and GcsFaultToleranceOption is not nil~~ [Feature] Validation of RayFTEnabled is false and GcsFaultToleranceOption is not nil Jan 10, 2025

MortalHappiness previously requested changes Jan 10, 2025

View reviewed changes

kevin85421 reviewed Jan 10, 2025

View reviewed changes

kevin85421 self-assigned this Jan 10, 2025

rueian reviewed Jan 11, 2025

View reviewed changes

CheyuWu added 6 commits January 13, 2025 00:38

feat: validation of RayFTEnabled == false and GcsFaultToleranceOption…

a1f0d05

…s no nil

fix: rm panic and set status in instance

3cb26b0

Revert "fix: rm panic and set status in instance"

a3d76d6

This reverts commit c2da53c.

Revert "feat: validation of RayFTEnabled == false and GcsFaultToleran…

b0edcd0

…ceOptions no nil" This reverts commit b42c48f.

feat: add validateRayClusterSpec in the beginning of rayClusterReconcile

19625f5

fix: linter issue

392120e

CheyuWu force-pushed the feat/gcsFT/validation branch from f85d476 to 392120e Compare January 12, 2025 16:38

CheyuWu added 2 commits January 13, 2025 01:16

feat: add additional case to reject, instance has RAY_REDIS_ADDRESS e…

ff818ef

…nv set but has no instance.Annotations[utils.RayFTEnabledAnnotationKey] == "true"

feat: add ValidateRayClusterSpec in raycluster webhook and add unit test

8b50a8e

rueian reviewed Jan 12, 2025

View reviewed changes

fix: linter issue

ef011c1

kevin85421 reviewed Jan 12, 2025

View reviewed changes

CheyuWu mentioned this pull request Jan 13, 2025

[Feature] Add Validate RayCluster spec in webhook #2737

Closed

2 tasks

CheyuWu added 2 commits January 13, 2025 21:18

Revert "fix: linter issue"

f4e5bd1

This reverts commit ef011c1.

Revert "feat: add ValidateRayClusterSpec in raycluster webhook and ad…

ffa0bdf

…d unit test" This reverts commit 8b50a8e.

CheyuWu mentioned this pull request Jan 13, 2025

[Refactor] move validateRayClusterStatus out of RayClusterReconciler #2738

Merged

4 tasks

fix: validateRayClusterSpec not need to be the member of RayClusterRe…

31b75f6

…conciler, to avoid HeadGroupSpec container count < 0, use utils.EnvVarExists to check env variables

CheyuWu mentioned this pull request Jan 13, 2025

[Feature] Add ValidateRayClusterSpec to Webhook #2739

Open

4 tasks

rueian reviewed Jan 13, 2025

View reviewed changes

chore: add rayCluseter Namespace and Name to logger and return nil to…

987dbb2

… avoid rescheduling in `validateRayClusterSpec`

rueian approved these changes Jan 13, 2025

View reviewed changes

rueian reviewed Jan 13, 2025

View reviewed changes

CheyuWu added 2 commits January 14, 2025 22:22

Merge remote-tracking branch 'upstream/master' into feat/gcsFT/valida…

db6d00e

…tion

fix: rm Containers[utils.RayContainerIndex].Env (not necessary) and c…

2642edc

…hange log message

rueian approved these changes Jan 14, 2025

View reviewed changes

kevin85421 approved these changes Jan 14, 2025

View reviewed changes

kevin85421 merged commit 92c2907 into ray-project:master Jan 14, 2025
24 checks passed

kevin85421 mentioned this pull request Jan 14, 2025

[RayCluster] Validate RayClusterSpec for empty containers and GCS FT #2749

Merged

4 tasks

win5923 pushed a commit to win5923/kuberay that referenced this pull request Jan 15, 2025

[Feature] Validation of RayFTEnabled is false and GcsFaultToleranceOp…

1b7d7b6

…tion is not nil (ray-project#2726)

kevin85421 mentioned this pull request Jan 22, 2025

[Umbrella] GCS FT API improvement #2696

Closed

2 tasks

	if r.Annotations[RayFTEnabledAnnotationKey] != "true" && r.Spec.HeadGroupSpec.Template.Spec.Containers[0].Env != nil {
	if r.Annotations[RayFTEnabledAnnotationKey] != "true" && utils.EnvVarExists("RAY_REDIS_ADDRESS ", r.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env) {

	return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is disabled")
	return fmt.Errorf("GcsFaultToleranceOptions should be nil when ray.io/ft-enabled is set to false")

	func (r RayClusterReconciler) validateRayClusterSpec(instance rayv1.RayCluster) error {
	func validateRayClusterSpec(instance *rayv1.RayCluster) error {

	return fmt.Errorf("RAY_REDIS_ADDRESS should not be set when ray.io/ft-enabled is disabled")
	return fmt.Errorf("RAY_REDIS_ADDRESS should not be set when ray.io/ft-enabled is not set or set to false")

	return fmt.Errorf("%s should not be set when %s is set to false", utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey)
	return fmt.Errorf("%s environment variable should not be set when %s annotation is not set to true", utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey)

[Feature] Validation of RayFTEnabled is false and GcsFaultToleranceOption is not nil #2726

[Feature] Validation of RayFTEnabled is false and GcsFaultToleranceOption is not nil #2726

Conversation

CheyuWu commented Jan 10, 2025 • edited by MortalHappiness Loading

Why are these changes needed?

Related issue number

Checks

CheyuWu commented Jan 10, 2025

Choose a reason for hiding this comment

CheyuWu Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CheyuWu commented Jan 10, 2025

Choose a reason for hiding this comment

kevin85421 commented Jan 10, 2025

CheyuWu commented Jan 11, 2025

Choose a reason for hiding this comment

rueian Jan 11, 2025 • edited Loading

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

CheyuWu commented Jan 12, 2025 • edited Loading

kevin85421 left a 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

CheyuWu commented Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CheyuWu commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Jan 13, 2025

CheyuWu commented Jan 14, 2025

kevin85421 left a comment

Choose a reason for hiding this comment

CheyuWu commented Jan 10, 2025 •

edited by MortalHappiness

Loading

CheyuWu Jan 10, 2025 •

edited

Loading

rueian Jan 11, 2025 •

edited

Loading

CheyuWu commented Jan 12, 2025 •

edited

Loading

CheyuWu commented Jan 13, 2025 •

edited

Loading