[Refactor] Move validateRayClusterStatus function to `validation.go…

…` and move unit test to `validation_test.go` (ray-project#2780)
MortalHappiness · Jan 21, 2025 · 8dd2496 · 8dd2496
1 parent c0b6b0d
commit 8dd2496
Show file tree

Hide file tree

Showing 4 changed files with 107 additions and 89 deletions.
diff --git a/ray-operator/controllers/ray/raycluster_controller.go b/ray-operator/controllers/ray/raycluster_controller.go
@@ -211,15 +211,6 @@ func (r *RayClusterReconciler) deleteAllPods(ctx context.Context, filters common
 	return pods, nil
 }
 
-func validateRayClusterStatus(instance *rayv1.RayCluster) error {
-	suspending := meta.IsStatusConditionTrue(instance.Status.Conditions, string(rayv1.RayClusterSuspending))
-	suspended := meta.IsStatusConditionTrue(instance.Status.Conditions, string(rayv1.RayClusterSuspended))
-	if suspending && suspended {
-		return errstd.New("invalid RayCluster State: rayv1.RayClusterSuspending and rayv1.RayClusterSuspended conditions should not be both true")
-	}
-	return nil
-}
-
 // Validation for invalid Ray Cluster configurations.
 func validateRayClusterSpec(instance *rayv1.RayCluster) error {
 	if len(instance.Spec.HeadGroupSpec.Template.Spec.Containers) == 0 {
@@ -303,7 +294,7 @@ func (r *RayClusterReconciler) rayClusterReconcile(ctx context.Context, instance
 		return ctrl.Result{}, nil
 	}
 
-	if err := validateRayClusterStatus(instance); err != nil {
+	if err := utils.ValidateRayClusterStatus(instance); err != nil {
 		logger.Error(err, "The RayCluster status is invalid")
 		r.Recorder.Eventf(instance, corev1.EventTypeWarning, string(utils.InvalidRayClusterStatus),
 			"The RayCluster status is invalid %s/%s, %v", instance.Namespace, instance.Name, err)

diff --git a/ray-operator/controllers/ray/raycluster_controller_unit_test.go b/ray-operator/controllers/ray/raycluster_controller_unit_test.go
@@ -3832,82 +3832,3 @@ func TestValidateRayClusterSpecSuspendingWorkerGroup(t *testing.T) {
 		})
 	}
 }
-
-func TestValidateRayClusterStatus(t *testing.T) {
-	tests := []struct {
-		name        string
-		conditions  []metav1.Condition
-		expectError bool
-	}{
-		{
-			name: "Both suspending and suspended are true",
-			conditions: []metav1.Condition{
-				{
-					Type:   string(rayv1.RayClusterSuspending),
-					Status: metav1.ConditionTrue,
-				},
-				{
-					Type:   string(rayv1.RayClusterSuspended),
-					Status: metav1.ConditionTrue,
-				},
-			},
-			expectError: true,
-		},
-		{
-			name: "Only suspending is true",
-			conditions: []metav1.Condition{
-				{
-					Type:   string(rayv1.RayClusterSuspending),
-					Status: metav1.ConditionTrue,
-				},
-				{
-					Type:   string(rayv1.RayClusterSuspended),
-					Status: metav1.ConditionFalse,
-				},
-			},
-			expectError: false,
-		},
-		{
-			name: "Only suspended is true",
-			conditions: []metav1.Condition{
-				{
-					Type:   string(rayv1.RayClusterSuspending),
-					Status: metav1.ConditionFalse,
-				},
-				{
-					Type:   string(rayv1.RayClusterSuspended),
-					Status: metav1.ConditionTrue,
-				},
-			},
-			expectError: false,
-		},
-		{
-			name: "Both suspending and suspended are false",
-			conditions: []metav1.Condition{
-				{
-					Type:   string(rayv1.RayClusterSuspending),
-					Status: metav1.ConditionFalse,
-				},
-				{
-					Type:   string(rayv1.RayClusterSuspended),
-					Status: metav1.ConditionFalse,
-				},
-			},
-			expectError: false,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			instance := &rayv1.RayCluster{
-				Status: rayv1.RayClusterStatus{
-					Conditions: tt.conditions,
-				},
-			}
-			err := validateRayClusterStatus(instance)
-			if (err != nil) != tt.expectError {
-				t.Errorf("validateRayClusterStatus() error = %v, wantErr %v", err, tt.expectError)
-			}
-		})
-	}
-}
diff --git a/ray-operator/controllers/ray/utils/validation.go b/ray-operator/controllers/ray/utils/validation.go
@@ -0,0 +1,18 @@
+package utils
+
+import (
+	errstd "errors"
+
+	"k8s.io/apimachinery/pkg/api/meta"
+
+	rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
+)
+
+func ValidateRayClusterStatus(instance *rayv1.RayCluster) error {
+	suspending := meta.IsStatusConditionTrue(instance.Status.Conditions, string(rayv1.RayClusterSuspending))
+	suspended := meta.IsStatusConditionTrue(instance.Status.Conditions, string(rayv1.RayClusterSuspended))
+	if suspending && suspended {
+		return errstd.New("invalid RayCluster State: rayv1.RayClusterSuspending and rayv1.RayClusterSuspended conditions should not be both true")
+	}
+	return nil
+}
diff --git a/ray-operator/controllers/ray/utils/validation_test.go b/ray-operator/controllers/ray/utils/validation_test.go
@@ -0,0 +1,88 @@
+package utils
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
+)
+
+func TestValidateRayClusterStatus(t *testing.T) {
+	tests := []struct {
+		name        string
+		conditions  []metav1.Condition
+		expectError bool
+	}{
+		{
+			name: "Both suspending and suspended are true",
+			conditions: []metav1.Condition{
+				{
+					Type:   string(rayv1.RayClusterSuspending),
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Type:   string(rayv1.RayClusterSuspended),
+					Status: metav1.ConditionTrue,
+				},
+			},
+			expectError: true,
+		},
+		{
+			name: "Only suspending is true",
+			conditions: []metav1.Condition{
+				{
+					Type:   string(rayv1.RayClusterSuspending),
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Type:   string(rayv1.RayClusterSuspended),
+					Status: metav1.ConditionFalse,
+				},
+			},
+			expectError: false,
+		},
+		{
+			name: "Only suspended is true",
+			conditions: []metav1.Condition{
+				{
+					Type:   string(rayv1.RayClusterSuspending),
+					Status: metav1.ConditionFalse,
+				},
+				{
+					Type:   string(rayv1.RayClusterSuspended),
+					Status: metav1.ConditionTrue,
+				},
+			},
+			expectError: false,
+		},
+		{
+			name: "Both suspending and suspended are false",
+			conditions: []metav1.Condition{
+				{
+					Type:   string(rayv1.RayClusterSuspending),
+					Status: metav1.ConditionFalse,
+				},
+				{
+					Type:   string(rayv1.RayClusterSuspended),
+					Status: metav1.ConditionFalse,
+				},
+			},
+			expectError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			instance := &rayv1.RayCluster{
+				Status: rayv1.RayClusterStatus{
+					Conditions: tt.conditions,
+				},
+			}
+			err := ValidateRayClusterStatus(instance)
+			if (err != nil) != tt.expectError {
+				t.Errorf("ValidateRayClusterStatus() error = %v, wantErr %v", err, tt.expectError)
+			}
+		})
+	}
+}