Add validation logic

Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>

Author: ryanaoleary

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

@@ -3,6 +3,7 @@ package utils
 import (
 	errstd "errors"
 	"fmt"
+	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -33,16 +34,51 @@ func ValidateRayClusterMetadata(metadata metav1.ObjectMeta) error {
 	return nil
 }
 
+// validateRayGroupResourcesAndLabels checks for conflicting resource definitions and invalid labels.
+func validateRayGroupResourcesAndLabels(groupName string, rayStartParams, resources, labels map[string]string) error {
+	hasRayStartResources := rayStartParams["num-cpus"] != "" ||
+		rayStartParams["num-gpus"] != "" ||
+		rayStartParams["memory"] != "" ||
+		rayStartParams["resources"] != ""
+	if hasRayStartResources && len(resources) > 0 {
+		return fmt.Errorf("resource fields should not be set in both rayStartParams and Resources for %s group. Please use only one", groupName)
+	}
+
+	if _, ok := rayStartParams["labels"]; ok {
+		return fmt.Errorf("rayStartParams['labels'] is not supported for %s group. Please use the top-level Labels field instead", groupName)
+	}
+
+	// Validate syntax for the top-level Labels field is parsable.
+	for key, val := range labels {
+		if strings.Contains(key, ",") {
+			return fmt.Errorf("label key for %s group cannot contain commas, but found: '%s'", groupName, key)
+		}
+		if strings.Contains(val, ",") {
+			return fmt.Errorf("label value for key '%s' in %s group cannot contain commas, but found: '%s'", key, groupName, val)
+		}
+	}
+
+	return nil
+}
+
 // Validation for invalid Ray Cluster configurations.
 func ValidateRayClusterSpec(spec *rayv1.RayClusterSpec, annotations map[string]string) error {
 	if len(spec.HeadGroupSpec.Template.Spec.Containers) == 0 {
 		return fmt.Errorf("headGroupSpec should have at least one container")
 	}
 
+	if err := validateRayGroupResourcesAndLabels("Head", spec.HeadGroupSpec.RayStartParams, spec.HeadGroupSpec.Resources, spec.HeadGroupSpec.Labels); err != nil {
+		return err
+	}
+
 	for _, workerGroup := range spec.WorkerGroupSpecs {
 		if len(workerGroup.Template.Spec.Containers) == 0 {
 			return fmt.Errorf("workerGroupSpec should have at least one container")
 		}
+
+		if err := validateRayGroupResourcesAndLabels(workerGroup.GroupName, workerGroup.RayStartParams, workerGroup.Resources, workerGroup.Labels); err != nil {
+			return err
+		}
 	}
 
 	if annotations[RayFTEnabledAnnotationKey] != "" && spec.GcsFaultToleranceOptions != nil {

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

@@ -683,6 +683,127 @@ func TestValidateRayClusterSpecAutoscaler(t *testing.T) {
 	}
 }
 
+func TestValidateRayClusterSpec_ResourcesAndLabels(t *testing.T) {
+	// Util function to create a RayCluster spec.
+	createSpec := func() rayv1.RayClusterSpec {
+		return rayv1.RayClusterSpec{
+			HeadGroupSpec: rayv1.HeadGroupSpec{
+				Template: podTemplateSpec(nil, nil),
+			},
+			WorkerGroupSpecs: []rayv1.WorkerGroupSpec{
+				{
+					GroupName: "worker-group",
+					Template:  podTemplateSpec(nil, nil),
+				},
+			},
+		}
+	}
+
+	tests := []struct {
+		name         string
+		errorMessage string
+		spec         rayv1.RayClusterSpec
+		expectError  bool
+	}{
+		{
+			name: "Invalid: Head group has resources in both rayStartParams and top-level Resources",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.HeadGroupSpec.RayStartParams = map[string]string{"num-cpus": "1"}
+				s.HeadGroupSpec.Resources = map[string]string{"CPU": "1"}
+				return s
+			}(),
+			expectError:  true,
+			errorMessage: "resource fields should not be set in both rayStartParams and Resources for Head group. Please use only one",
+		},
+		{
+			name: "Invalid: Worker group has resources in both rayStartParams and .Resources",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.WorkerGroupSpecs[0].RayStartParams = map[string]string{"num-gpus": "1"}
+				s.WorkerGroupSpecs[0].Resources = map[string]string{"GPU": "1"}
+				return s
+			}(),
+			expectError:  true,
+			errorMessage: "resource fields should not be set in both rayStartParams and Resources for worker-group group. Please use only one",
+		},
+		{
+			name: "Valid: Only rayStartParams resources are set for head",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.HeadGroupSpec.RayStartParams = map[string]string{
+					"num-cpus":  "2",
+					"memory":    "4G",
+					"resources": "{\"TPU\": \"8\"}",
+				}
+				return s
+			}(),
+			expectError: false,
+		},
+		{
+			name: "Valid: Only .Resources field is set for worker",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.WorkerGroupSpecs[0].Resources = map[string]string{"CPU": "2", "memory": "4G", "TPU": "8"}
+				return s
+			}(),
+			expectError: false,
+		},
+		{
+			name: "Invalid: Head group has 'labels' in rayStartParams",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.HeadGroupSpec.RayStartParams = map[string]string{"labels": "ray.io/node-group=worker-group-1"}
+				return s
+			}(),
+			expectError:  true,
+			errorMessage: "rayStartParams['labels'] is not supported for Head group. Please use the top-level Labels field instead",
+		},
+		{
+			name: "Invalid: Worker group has 'labels' in rayStartParams",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.WorkerGroupSpecs[0].RayStartParams = map[string]string{"labels": "ray.io/node-group=worker-group-1"}
+				return s
+			}(),
+			expectError:  true,
+			errorMessage: "rayStartParams['labels'] is not supported for worker-group group. Please use the top-level Labels field instead",
+		},
+		{
+			name: "Valid: Only .Labels field is set",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.HeadGroupSpec.Labels = map[string]string{"ray.io/market-type": "on-demand"}
+				s.WorkerGroupSpecs[0].Labels = map[string]string{"ray.io/accelerator-type": "TPU-V6E"}
+				return s
+			}(),
+			expectError: false,
+		},
+		{
+			name: "Invalid: .Labels field values contain commas",
+			spec: func() rayv1.RayClusterSpec {
+				s := createSpec()
+				s.HeadGroupSpec.Labels = map[string]string{"invalid,labels,with,commas": "value"}
+				return s
+			}(),
+			expectError:  true,
+			errorMessage: "label key for Head group cannot contain commas, but found: 'invalid,labels,with,commas'",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateRayClusterSpec(&tt.spec, nil)
+			if tt.expectError {
+				require.Error(t, err)
+				assert.EqualError(t, err, tt.errorMessage)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 func TestValidateRayJobStatus(t *testing.T) {
 	tests := []struct {
 		name        string