Fix: validation in right place

1. Remove validation logic in GetWorkerGroupDesiredReplicas (utils.go),
   and append to ValidateRayClusterSpec.
2. Add other validation logic for worker group specs.
3. Remove unnecessary test cases in GetWorkerGroupDesiredReplicas.

Author: nu1lspaxe

ray-operator/controllers/ray/raycluster_controller.go

@@ -642,7 +642,7 @@ func (r *RayClusterReconciler) reconcilePods(ctx context.Context, instance *rayv
 			continue
 		}
 		// workerReplicas will store the target number of pods for this worker group.
-		numExpectedWorkerPods := int(utils.GetWorkerGroupDesiredReplicas(ctx, worker))
+		numExpectedWorkerPods := int(utils.GetWorkerGroupDesiredReplicas(worker))
 		logger.Info("reconcilePods", "desired workerReplicas (always adhering to minReplicas/maxReplica)", numExpectedWorkerPods, "worker group", worker.GroupName, "maxReplicas", worker.MaxReplicas, "minReplicas", worker.MinReplicas, "replicas", worker.Replicas)
 
 		workerPods := corev1.PodList{}

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

@@ -335,23 +335,16 @@ func GenerateIdentifier(clusterName string, nodeType rayv1.RayNodeType) string {
 	return fmt.Sprintf("%s-%s", clusterName, nodeType)
 }
 
-func GetWorkerGroupDesiredReplicas(ctx context.Context, workerGroupSpec rayv1.WorkerGroupSpec) int32 {
-	log := ctrl.LoggerFrom(ctx)
+func GetWorkerGroupDesiredReplicas(workerGroupSpec rayv1.WorkerGroupSpec) int32 {
 	// Always adhere to min/max replicas constraints.
 	var workerReplicas int32
 	if workerGroupSpec.Suspend != nil && *workerGroupSpec.Suspend {
 		return 0
 	}
-	if *workerGroupSpec.MinReplicas > *workerGroupSpec.MaxReplicas {
-		log.Info("minReplicas is greater than maxReplicas, using maxReplicas as desired replicas. "+
-			"Please fix this to avoid any unexpected behaviors.", "minReplicas", *workerGroupSpec.MinReplicas, "maxReplicas", *workerGroupSpec.MaxReplicas)
-		workerReplicas = *workerGroupSpec.MaxReplicas
-	} else if workerGroupSpec.Replicas == nil || *workerGroupSpec.Replicas < *workerGroupSpec.MinReplicas {
-		// Replicas is impossible to be nil as it has a default value assigned in the CRD.
-		// Add this check to make testing easier.
+	// Validation for replicas/min/max should be enforced in validation.go before reconcile proceeds.
+	// Here we only compute the desired replicas within the already-validated bounds.
+	if workerGroupSpec.Replicas == nil {
 		workerReplicas = *workerGroupSpec.MinReplicas
-	} else if *workerGroupSpec.Replicas > *workerGroupSpec.MaxReplicas {
-		workerReplicas = *workerGroupSpec.MaxReplicas
 	} else {
 		workerReplicas = *workerGroupSpec.Replicas
 	}
@@ -362,7 +355,7 @@ func GetWorkerGroupDesiredReplicas(ctx context.Context, workerGroupSpec rayv1.Wo
 func CalculateDesiredReplicas(ctx context.Context, cluster *rayv1.RayCluster) int32 {
 	count := int32(0)
 	for _, nodeGroup := range cluster.Spec.WorkerGroupSpecs {
-		count += GetWorkerGroupDesiredReplicas(ctx, nodeGroup)
+		count += GetWorkerGroupDesiredReplicas(nodeGroup)
 	}
 
 	return count

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

@@ -550,7 +550,6 @@ func TestGenerateHeadServiceName(t *testing.T) {
 }
 
 func TestGetWorkerGroupDesiredReplicas(t *testing.T) {
-	ctx := context.Background()
 	// Test 1: `WorkerGroupSpec.Replicas` is nil.
 	// `Replicas` is impossible to be nil in a real RayCluster CR as it has a default value assigned in the CRD.
 	numOfHosts := int32(1)
@@ -562,37 +561,21 @@ func TestGetWorkerGroupDesiredReplicas(t *testing.T) {
 		MinReplicas: &minReplicas,
 		MaxReplicas: &maxReplicas,
 	}
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), minReplicas)
+	assert.Equal(t, GetWorkerGroupDesiredReplicas(workerGroupSpec), minReplicas)
 
 	// Test 2: `WorkerGroupSpec.Replicas` is not nil and is within the range.
 	replicas := int32(3)
 	workerGroupSpec.Replicas = &replicas
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), replicas)
+	assert.Equal(t, GetWorkerGroupDesiredReplicas(workerGroupSpec), replicas)
 
-	// Test 3: `WorkerGroupSpec.Replicas` is not nil but is more than maxReplicas.
-	replicas = int32(6)
-	workerGroupSpec.Replicas = &replicas
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), maxReplicas)
-
-	// Test 4: `WorkerGroupSpec.Replicas` is not nil but is less than minReplicas.
-	replicas = int32(0)
-	workerGroupSpec.Replicas = &replicas
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), minReplicas)
-
-	// Test 5: `WorkerGroupSpec.Replicas` is nil and minReplicas is less than maxReplicas.
-	workerGroupSpec.Replicas = nil
-	workerGroupSpec.MinReplicas = &maxReplicas
-	workerGroupSpec.MaxReplicas = &minReplicas
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), *workerGroupSpec.MaxReplicas)
-
-	// Test 6: `WorkerGroupSpec.Suspend` is true.
+	// Test 3: `WorkerGroupSpec.Suspend` is true.
 	suspend := true
 	workerGroupSpec.MinReplicas = &maxReplicas
 	workerGroupSpec.MaxReplicas = &minReplicas
 	workerGroupSpec.Suspend = &suspend
-	assert.Zero(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec))
+	assert.Zero(t, GetWorkerGroupDesiredReplicas(workerGroupSpec))
 
-	// Test 7: `WorkerGroupSpec.NumOfHosts` is 4.
+	// Test 4: `WorkerGroupSpec.NumOfHosts` is 4.
 	numOfHosts = int32(4)
 	replicas = int32(5)
 	suspend = false
@@ -601,7 +584,7 @@ func TestGetWorkerGroupDesiredReplicas(t *testing.T) {
 	workerGroupSpec.Suspend = &suspend
 	workerGroupSpec.MinReplicas = &minReplicas
 	workerGroupSpec.MaxReplicas = &maxReplicas
-	assert.Equal(t, GetWorkerGroupDesiredReplicas(ctx, workerGroupSpec), replicas*numOfHosts)
+	assert.Equal(t, GetWorkerGroupDesiredReplicas(workerGroupSpec), replicas*numOfHosts)
 }
 
 func TestCalculateMinAndMaxReplicas(t *testing.T) {

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

@@ -43,6 +43,26 @@ func ValidateRayClusterSpec(spec *rayv1.RayClusterSpec, annotations map[string]s
 		if len(workerGroup.Template.Spec.Containers) == 0 {
 			return fmt.Errorf("workerGroupSpec should have at least one container")
 		}
+		if workerGroup.MinReplicas == nil || workerGroup.MaxReplicas == nil {
+			return fmt.Errorf("worker group %s must set both minReplicas and maxReplicas", workerGroup.GroupName)
+		}
+		if *workerGroup.MinReplicas < 0 {
+			return fmt.Errorf("worker group %s has negative minReplicas %d", workerGroup.GroupName, *workerGroup.MinReplicas)
+		}
+		if *workerGroup.MaxReplicas < 0 {
+			return fmt.Errorf("worker group %s has negative maxReplicas %d", workerGroup.GroupName, *workerGroup.MaxReplicas)
+		}
+		if *workerGroup.MinReplicas > *workerGroup.MaxReplicas {
+			return fmt.Errorf("worker group %s has minReplicas %d greater than maxReplicas %d", workerGroup.GroupName, *workerGroup.MinReplicas, *workerGroup.MaxReplicas)
+		}
+		if workerGroup.Replicas != nil {
+			if *workerGroup.Replicas < *workerGroup.MinReplicas {
+				return fmt.Errorf("worker group %s has replicas %d smaller than minReplicas %d", workerGroup.GroupName, *workerGroup.Replicas, *workerGroup.MinReplicas)
+			}
+			if *workerGroup.Replicas > *workerGroup.MaxReplicas {
+				return fmt.Errorf("worker group %s has replicas %d greater than maxReplicas %d", workerGroup.GroupName, *workerGroup.Replicas, *workerGroup.MaxReplicas)
+			}
+		}
 	}
 
 	if annotations[RayFTEnabledAnnotationKey] != "" && spec.GcsFaultToleranceOptions != nil {