[Refactor] Move validateRayServiceSpec to validation.go and its unit …

…test to validation_test.go (#2816)
ray-project · Jan 25, 2025 · 5ccf361 · 5ccf361
1 parent 80cab41
commit 5ccf361
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 48 deletions.
diff --git a/ray-operator/controllers/ray/rayservice_controller.go b/ray-operator/controllers/ray/rayservice_controller.go
@@ -117,7 +117,7 @@ func (r *RayServiceReconciler) Reconcile(ctx context.Context, request ctrl.Reque
 	}
 	originalRayServiceInstance := rayServiceInstance.DeepCopy()
 
-	if err := validateRayServiceSpec(rayServiceInstance); err != nil {
+	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
@@ -245,21 +245,6 @@ func (r *RayServiceReconciler) reconcileServicesToReadyCluster(ctx context.Conte
 	return headSvc, serveSvc, nil
 }
 
-func validateRayServiceSpec(rayService *rayv1.RayService) error {
-	if headSvc := rayService.Spec.RayClusterSpec.HeadGroupSpec.HeadService; headSvc != nil && headSvc.Name != "" {
-		return fmt.Errorf("spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
-	}
-
-	// only NewCluster and None are valid upgradeType
-	if rayService.Spec.UpgradeStrategy != nil &&
-		rayService.Spec.UpgradeStrategy.Type != nil &&
-		*rayService.Spec.UpgradeStrategy.Type != rayv1.None &&
-		*rayService.Spec.UpgradeStrategy.Type != rayv1.NewCluster {
-		return fmt.Errorf("Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s or %s", *rayService.Spec.UpgradeStrategy.Type, rayv1.NewCluster, rayv1.None)
-	}
-	return nil
-}
-
 func (r *RayServiceReconciler) calculateStatus(ctx context.Context, rayServiceInstance *rayv1.RayService, headSvc, serveSvc *corev1.Service) error {
 	logger := ctrl.LoggerFrom(ctx)
 

diff --git a/ray-operator/controllers/ray/rayservice_controller_unit_test.go b/ray-operator/controllers/ray/rayservice_controller_unit_test.go
@@ -26,38 +26,6 @@ import (
 	"github.com/ray-project/kuberay/ray-operator/test/support"
 )
 
-func TestValidateRayServiceSpec(t *testing.T) {
-	err := validateRayServiceSpec(&rayv1.RayService{
-		Spec: rayv1.RayServiceSpec{
-			RayClusterSpec: rayv1.RayClusterSpec{
-				HeadGroupSpec: rayv1.HeadGroupSpec{
-					HeadService: &corev1.Service{
-						ObjectMeta: metav1.ObjectMeta{
-							Name: "my-head-service",
-						},
-					},
-				},
-			},
-		},
-	})
-	assert.Error(t, err, "spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
-
-	err = validateRayServiceSpec(&rayv1.RayService{
-		Spec: rayv1.RayServiceSpec{},
-	})
-	assert.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,
-			},
-		},
-	})
-	assert.Error(t, err, "spec.UpgradeSpec.Type is invalid")
-}
-
 func TestGenerateHashWithoutReplicasAndWorkersToDelete(t *testing.T) {
 	// `generateRayClusterJsonHash` will mute fields that will not trigger new RayCluster preparation. For example,
 	// Autoscaler will update `Replicas` and `WorkersToDelete` when scaling up/down. Hence, `hash1` should be equal to

diff --git a/ray-operator/controllers/ray/utils/validation.go b/ray-operator/controllers/ray/utils/validation.go
@@ -149,3 +149,18 @@ func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
 	}
 	return nil
 }
+
+func ValidateRayServiceSpec(rayService *rayv1.RayService) error {
+	if headSvc := rayService.Spec.RayClusterSpec.HeadGroupSpec.HeadService; headSvc != nil && headSvc.Name != "" {
+		return fmt.Errorf("spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
+	}
+
+	// only NewCluster and None are valid upgradeType
+	if rayService.Spec.UpgradeStrategy != nil &&
+		rayService.Spec.UpgradeStrategy.Type != nil &&
+		*rayService.Spec.UpgradeStrategy.Type != rayv1.None &&
+		*rayService.Spec.UpgradeStrategy.Type != rayv1.NewCluster {
+		return fmt.Errorf("Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s or %s", *rayService.Spec.UpgradeStrategy.Type, rayv1.NewCluster, rayv1.None)
+	}
+	return nil
+}
diff --git a/ray-operator/controllers/ray/utils/validation_test.go b/ray-operator/controllers/ray/utils/validation_test.go
@@ -686,3 +686,35 @@ func TestValidateRayJobSpec(t *testing.T) {
 	})
 	assert.ErrorContains(t, err, "shutdownAfterJobFinshes is set to 'true' while deletion policy is 'DeleteNone'")
 }
+
+func TestValidateRayServiceSpec(t *testing.T) {
+	err := ValidateRayServiceSpec(&rayv1.RayService{
+		Spec: rayv1.RayServiceSpec{
+			RayClusterSpec: rayv1.RayClusterSpec{
+				HeadGroupSpec: rayv1.HeadGroupSpec{
+					HeadService: &corev1.Service{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "my-head-service",
+						},
+					},
+				},
+			},
+		},
+	})
+	assert.Error(t, err, "spec.rayClusterConfig.headGroupSpec.headService.metadata.name should not be set")
+
+	err = ValidateRayServiceSpec(&rayv1.RayService{
+		Spec: rayv1.RayServiceSpec{},
+	})
+	assert.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,
+			},
+		},
+	})
+	assert.Error(t, err, "spec.UpgradeSpec.Type is invalid")
+}