Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 15 commits into from
Jan 14, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a1f0d05
feat: validation of RayFTEnabled == false and GcsFaultToleranceOption…
CheyuWu Jan 10, 2025
3cb26b0
fix: rm panic and set status in instance
CheyuWu Jan 10, 2025
a3d76d6
Revert "fix: rm panic and set status in instance"
CheyuWu Jan 11, 2025
b0edcd0
Revert "feat: validation of RayFTEnabled == false and GcsFaultToleran…
CheyuWu Jan 11, 2025
19625f5
feat: add validateRayClusterSpec in the beginning of rayClusterReconcile
CheyuWu Jan 11, 2025
392120e
fix: linter issue
CheyuWu Jan 11, 2025
ff818ef
feat: add additional case to reject, instance has RAY_REDIS_ADDRESS e…
CheyuWu Jan 12, 2025
8b50a8e
feat: add ValidateRayClusterSpec in raycluster webhook and add unit test
CheyuWu Jan 12, 2025
ef011c1
fix: linter issue
CheyuWu Jan 12, 2025
f4e5bd1
Revert "fix: linter issue"
CheyuWu Jan 13, 2025
ffa0bdf
Revert "feat: add ValidateRayClusterSpec in raycluster webhook and ad…
CheyuWu Jan 13, 2025
31b75f6
fix: validateRayClusterSpec not need to be the member of RayClusterRe…
CheyuWu Jan 13, 2025
987dbb2
chore: add rayCluseter Namespace and Name to logger and return nil to…
CheyuWu Jan 13, 2025
db6d00e
Merge remote-tracking branch 'upstream/master' into feat/gcsFT/valida…
CheyuWu Jan 14, 2025
2642edc
fix: rm Containers[utils.RayContainerIndex].Env (not necessary) and c…
CheyuWu Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions ray-operator/controllers/ray/raycluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,23 @@ func validateRayClusterStatus(instance *rayv1.RayCluster) error {
return nil
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

@rueian rueian Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, no problem

return fmt.Errorf("GcsFaultToleranceOptions should be nil when %s is set to false", utils.RayFTEnabledAnnotationKey)
}

if instance.Annotations[utils.RayFTEnabledAnnotationKey] != "true" && len(instance.Spec.HeadGroupSpec.Template.Spec.Containers) > 0 {
if utils.EnvVarExists(utils.RAY_REDIS_ADDRESS, instance.Spec.HeadGroupSpec.Template.Spec.Containers[utils.RayContainerIndex].Env) {
return fmt.Errorf(
"%s environment variable should not be set when %s annotation is not set to true",
utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey,
)
}
}
return nil
}

func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance *rayv1.RayCluster) (ctrl.Result, error) {
var reconcileErr error
logger := ctrl.LoggerFrom(ctx)
Expand All @@ -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, fmt.Sprintf("The RayCluster spec is invalid %s/%s", instance.Namespace, instance.Name))
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{}, nil
}

if err := validateRayClusterStatus(instance); err != nil {
logger.Error(err, "The RayCluster status is invalid")
r.Recorder.Eventf(instance, corev1.EventTypeWarning, string(utils.InvalidRayClusterStatus),
Expand Down
148 changes: 148 additions & 0 deletions ray-operator/controllers/ray/raycluster_controller_unit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package ray
import (
"context"
"errors"
"fmt"
"os"
"strconv"
"strings"
Expand Down Expand Up @@ -3472,6 +3473,153 @@ func Test_ReconcileManagedBy(t *testing.T) {
}
}

func TestValidateRayClusterSpec(t *testing.T) {
tests := []struct {
gcsFaultToleranceOptions *rayv1.GcsFaultToleranceOptions
annotations map[string]string
name string
errorMessage string
envVars []corev1.EnvVar
expectError bool
}{
{
name: "FT disabled with GcsFaultToleranceOptions set",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "false",
},
gcsFaultToleranceOptions: &rayv1.GcsFaultToleranceOptions{},
expectError: true,
errorMessage: fmt.Sprintf("GcsFaultToleranceOptions should be nil when %s is set to false", utils.RayFTEnabledAnnotationKey),
},
{
name: "FT disabled with RAY_REDIS_ADDRESS set",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "false",
},
envVars: []corev1.EnvVar{
{
Name: utils.RAY_REDIS_ADDRESS,
Value: "redis://127.0.0.1:6379",
},
},
expectError: true,
errorMessage: fmt.Sprintf(
"%s environment variable should not be set when %s annotation is not set to true",
utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey,
),
},
{
name: "FT not set with RAY_REDIS_ADDRESS set",
annotations: map[string]string{},
envVars: []corev1.EnvVar{
{
Name: utils.RAY_REDIS_ADDRESS,
Value: "redis://127.0.0.1:6379",
},
},
expectError: true,
errorMessage: fmt.Sprintf(
"%s environment variable should not be set when %s annotation is not set to true",
utils.RAY_REDIS_ADDRESS, utils.RayFTEnabledAnnotationKey,
),
},
{
name: "FT disabled with other environment variables set",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "false",
},
envVars: []corev1.EnvVar{
{
Name: "SOME_OTHER_ENV",
Value: "some-value",
},
},
expectError: false,
},
{
name: "FT enabled, GcsFaultToleranceOptions not nil",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "true",
},
gcsFaultToleranceOptions: &rayv1.GcsFaultToleranceOptions{
RedisAddress: "redis://127.0.0.1:6379",
},
expectError: false,
},
{
name: "FT enabled, GcsFaultToleranceOptions is nil",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "true",
},
expectError: false,
},
{
name: "FT enabled with with other environment variables set",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "true",
},
envVars: []corev1.EnvVar{
{
Name: "SOME_OTHER_ENV",
Value: "some-value",
},
},
expectError: false,
},
{
name: "FT enabled with RAY_REDIS_ADDRESS set",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "true",
},
envVars: []corev1.EnvVar{
{
Name: utils.RAY_REDIS_ADDRESS,
Value: "redis://127.0.0.1:6379",
},
},
expectError: false,
},
{
name: "FT disabled with no GcsFaultToleranceOptions and no RAY_REDIS_ADDRESS",
annotations: map[string]string{
utils.RayFTEnabledAnnotationKey: "false",
},
expectError: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
rayCluster := &rayv1.RayCluster{
ObjectMeta: metav1.ObjectMeta{
Annotations: tt.annotations,
},
Spec: rayv1.RayClusterSpec{
GcsFaultToleranceOptions: tt.gcsFaultToleranceOptions,
HeadGroupSpec: rayv1.HeadGroupSpec{
Template: corev1.PodTemplateSpec{
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Env: tt.envVars,
},
},
},
},
},
},
}
err := validateRayClusterSpec(rayCluster)
if tt.expectError {
assert.Error(t, err)
assert.EqualError(t, err, tt.errorMessage)
} else {
assert.Nil(t, err)
}
})
}
}

func TestValidateRayClusterStatus(t *testing.T) {
tests := []struct {
name string
Expand Down
1 change: 1 addition & 0 deletions ray-operator/controllers/ray/utils/constant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ type K8sEventType string
const (
// RayCluster event list
InvalidRayClusterStatus K8sEventType = "InvalidRayClusterStatus"
InvalidRayClusterSpec K8sEventType = "InvalidRayClusterSpec"
// Head Pod event list
CreatedHeadPod K8sEventType = "CreatedHeadPod"
FailedToCreateHeadPod K8sEventType = "FailedToCreateHeadPod"
Expand Down
Loading