[RayService] Directly fail CR if is invalid (#4228)

* [RayService] Directly fail CR if is invalid

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

* nit: set the name with strings.Repeat(a, 48)

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

---------

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

Author: win5923

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

@@ -206,6 +206,7 @@ const (
 	BothActivePendingClustersExist RayServiceConditionReason = "BothActivePendingClustersExist"
 	NoPendingCluster               RayServiceConditionReason = "NoPendingCluster"
 	NoActiveCluster                RayServiceConditionReason = "NoActiveCluster"
+	RayServiceValidationFailed     RayServiceConditionReason = "ValidationFailed"
 )
 
 // +kubebuilder:object:root=true

ray-operator/controllers/ray/rayservice_controller.go

@@ -115,15 +115,21 @@ func (r *RayServiceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 	}
 	originalRayServiceInstance := rayServiceInstance.DeepCopy()
 
-	if err := utils.ValidateRayServiceMetadata(rayServiceInstance.ObjectMeta); err != nil {
-		r.Recorder.Eventf(rayServiceInstance, corev1.EventTypeWarning, string(utils.InvalidRayServiceMetadata),
-			"The RayService metadata is invalid %s/%s: %v", rayServiceInstance.Namespace, rayServiceInstance.Name, err)
-		return ctrl.Result{RequeueAfter: ServiceDefaultRequeueDuration}, err
-	}
-	if err := utils.ValidateRayServiceSpec(rayServiceInstance); err != nil {
-		r.Recorder.Eventf(rayServiceInstance, corev1.EventTypeWarning, string(utils.InvalidRayServiceSpec),
-			"The RayService spec is invalid %s/%s: %v", rayServiceInstance.Namespace, rayServiceInstance.Name, err)
-		return ctrl.Result{RequeueAfter: ServiceDefaultRequeueDuration}, err
+	// Perform all validations and directly fail the RayService if any of the validation fails
+	errType, err := validateRayService(ctx, rayServiceInstance)
+	// Immediately update the status after validation
+	if err != nil {
+		r.Recorder.Eventf(rayServiceInstance, corev1.EventTypeWarning, string(errType),
+			"%s/%s: %v", rayServiceInstance.Namespace, rayServiceInstance.Name, err)
+
+		setCondition(rayServiceInstance, rayv1.RayServiceReady, metav1.ConditionFalse, rayv1.RayServiceValidationFailed, err.Error())
+		rayServiceInstance.Status.LastUpdateTime = &metav1.Time{Time: time.Now()}
+
+		if updateErr := r.Status().Update(ctx, rayServiceInstance); updateErr != nil {
+			logger.Info("Failed to update RayService status", "error", updateErr)
+			return ctrl.Result{RequeueAfter: ServiceDefaultRequeueDuration}, updateErr
+		}
+		return ctrl.Result{}, nil
 	}
 
 	r.cleanUpServeConfigCache(ctx, rayServiceInstance)
@@ -265,6 +271,25 @@ func (r *RayServiceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 	return ctrl.Result{RequeueAfter: ServiceDefaultRequeueDuration}, nil
 }
 
+func validateRayService(ctx context.Context, rayServiceInstance *rayv1.RayService) (utils.K8sEventType, error) {
+	logger := ctrl.LoggerFrom(ctx)
+	validationRules := []struct {
+		validate func() error
+		errType  utils.K8sEventType
+	}{
+		{func() error { return utils.ValidateRayServiceMetadata(rayServiceInstance.ObjectMeta) }, utils.InvalidRayServiceMetadata},
+		{func() error { return utils.ValidateRayServiceSpec(rayServiceInstance) }, utils.InvalidRayServiceSpec},
+	}
+
+	for _, validation := range validationRules {
+		if err := validation.validate(); err != nil {
+			logger.Error(err, err.Error())
+			return validation.errType, err
+		}
+	}
+	return "", nil
+}
+
 func (r *RayServiceReconciler) reconcileServicesToReadyCluster(ctx context.Context, rayServiceInstance *rayv1.RayService, rayClusterInstance *rayv1.RayCluster) (*corev1.Service, *corev1.Service, error) {
 	// Create K8s services if they don't exist. If they do exist, update the services to point to the RayCluster passed in.
 	headSvc, err := r.reconcileServices(ctx, rayServiceInstance, rayClusterInstance, utils.HeadService)

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

@@ -311,15 +311,15 @@ func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
 
 func ValidateRayServiceMetadata(metadata metav1.ObjectMeta) error {
 	if len(metadata.Name) > MaxRayServiceNameLength {
-		return fmt.Errorf("RayService name should be no more than %d characters", MaxRayServiceNameLength)
+		return fmt.Errorf("The RayService metadata is invalid: RayService name should be no more than %d characters", MaxRayServiceNameLength)
 	}
 	if errs := validation.IsDNS1035Label(metadata.Name); len(errs) > 0 {
-		return fmt.Errorf("RayService name should be a valid DNS1035 label: %v", errs)
+		return fmt.Errorf("The RayService metadata is invalid: RayService name should be a valid DNS1035 label: %v", errs)
 	}
 
 	// Validate initializing timeout annotation if present
 	if err := validateInitializingTimeout(metadata.Annotations); err != nil {
-		return fmt.Errorf("RayService annotations is invalid: %w", err)
+		return fmt.Errorf("The RayService metadata is invalid: RayService annotations is invalid: %w", err)
 	}
 
 	return nil
@@ -359,11 +359,11 @@ func validateInitializingTimeout(annotations map[string]string) error {
 
 func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
 	if err := ValidateRayClusterSpec(&rayService.Spec.RayClusterSpec, rayService.Annotations); err != nil {
-		return err
+		return fmt.Errorf("The RayService spec is invalid: %w", err)
 	}
 
 	if headSvc := rayService.Spec.RayClusterSpec.HeadGroupSpec.HeadService; headSvc != nil && headSvc.Name != "" {
-		return fmt.Errorf("spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
+		return fmt.Errorf("The RayService spec is invalid: spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
 	}
 
 	// only NewClusterWithIncrementalUpgrade, NewCluster, and None are valid upgradeType
@@ -372,17 +372,19 @@ func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
 		*rayService.Spec.UpgradeStrategy.Type != rayv1.None &&
 		*rayService.Spec.UpgradeStrategy.Type != rayv1.NewCluster &&
 		*rayService.Spec.UpgradeStrategy.Type != rayv1.NewClusterWithIncrementalUpgrade {
-		return fmt.Errorf("Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s, %s, or %s", *rayService.Spec.UpgradeStrategy.Type, rayv1.NewClusterWithIncrementalUpgrade, rayv1.NewCluster, rayv1.None)
+		return fmt.Errorf("The RayService spec is invalid: Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s, %s, or %s", *rayService.Spec.UpgradeStrategy.Type, rayv1.NewClusterWithIncrementalUpgrade, rayv1.NewCluster, rayv1.None)
 	}
 
 	if rayService.Spec.RayClusterDeletionDelaySeconds != nil &&
 		*rayService.Spec.RayClusterDeletionDelaySeconds < 0 {
-		return fmt.Errorf("Spec.RayClusterDeletionDelaySeconds should be a non-negative integer, got %d", *rayService.Spec.RayClusterDeletionDelaySeconds)
+		return fmt.Errorf("The RayService spec is invalid: Spec.RayClusterDeletionDelaySeconds should be a non-negative integer, got %d", *rayService.Spec.RayClusterDeletionDelaySeconds)
 	}
 
 	// If type is NewClusterWithIncrementalUpgrade, validate the ClusterUpgradeOptions
 	if IsIncrementalUpgradeEnabled(&rayService.Spec) {
-		return ValidateClusterUpgradeOptions(rayService)
+		if err := ValidateClusterUpgradeOptions(rayService); err != nil {
+			return fmt.Errorf("The RayService spec is invalid: %w", err)
+		}
 	}
 
 	return nil

ray-operator/test/e2erayservice/rayservice_validation_test.go

@@ -0,0 +1,39 @@
+package e2erayservice
+
+import (
+	"strings"
+	"testing"
+
+	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/api/meta"
+
+	rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
+	rayv1ac "github.com/ray-project/kuberay/ray-operator/pkg/client/applyconfiguration/ray/v1"
+	. "github.com/ray-project/kuberay/ray-operator/test/support"
+)
+
+func TestRayServiceValidation(t *testing.T) {
+	test := With(t)
+	g := NewWithT(t)
+
+	// Create a namespace
+	namespace := test.NewTestNamespace()
+
+	test.T().Run("RayService name too long with 48 characters", func(_ *testing.T) {
+		rayServiceAC := rayv1ac.RayService(strings.Repeat("a", 48), namespace.Name).
+			WithSpec(RayServiceSampleYamlApplyConfiguration())
+
+		rayService, err := test.Client().Ray().RayV1().RayServices(namespace.Name).Apply(test.Ctx(), rayServiceAC, TestApplyOptions)
+		g.Expect(err).NotTo(HaveOccurred())
+		LogWithTimestamp(test.T(), "Created RayService %s/%s successfully", rayService.Namespace, rayService.Name)
+
+		g.Eventually(RayService(test, rayService.Namespace, rayService.Name), TestTimeoutShort).
+			Should(WithTransform(func(rs *rayv1.RayService) bool {
+				condition := meta.FindStatusCondition(rs.Status.Conditions, string(rayv1.RayServiceReady))
+				if condition == nil {
+					return false
+				}
+				return condition.Status == "False" && condition.Reason == string(rayv1.RayServiceValidationFailed)
+			}, BeTrue()))
+	})
+}