Refactor ValidateRayClusterUpgradeOptions

Signed-off-by: win5923 <ken89@kimo.com>

Author: win5923

ray-operator/controllers/ray/common/pod.go

@@ -168,11 +168,14 @@ func DefaultHeadPodTemplate(ctx context.Context, instance rayv1.RayCluster, head
 	// headPort is passed into setMissingRayStartParams but unused there for the head pod.
 	// To mitigate this awkwardness and reduce code redundancy, unify head and worker pod configuration logic.
 
+	log := ctrl.LoggerFrom(ctx)
 	// Calculate the pod template hash before any modifications
 	// This ensures the hash reflects the original user-defined template for upgrade detection
 	templateHash := ""
 	if hash, err := GeneratePodTemplateHash(instance.Spec.HeadGroupSpec.Template); err == nil {
 		templateHash = hash
+	} else {
+		log.Error(err, "Failed to generate pod template hash for head group")
 	}
 
 	podTemplate := headSpec.Template
@@ -315,13 +318,16 @@ func getEnableProbesInjection() bool {
 
 // DefaultWorkerPodTemplate sets the config values
 func DefaultWorkerPodTemplate(ctx context.Context, instance rayv1.RayCluster, workerSpec rayv1.WorkerGroupSpec, podName string, fqdnRayIP string, headPort string, replicaGrpName string, replicaIndex int, numHostIndex int) corev1.PodTemplateSpec {
+	log := ctrl.LoggerFrom(ctx)
 	// Calculate the pod template hash before any modifications
 	// This ensures the hash reflects the original user-defined template for upgrade detection
 	templateHash := ""
 	for _, wg := range instance.Spec.WorkerGroupSpecs {
 		if wg.GroupName == workerSpec.GroupName {
 			if hash, err := GeneratePodTemplateHash(wg.Template); err == nil {
 				templateHash = hash
+			} else {
+				log.Error(err, "Failed to generate pod template hash for worker group", "groupName", workerSpec.GroupName)
 			}
 			break
 		}

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

@@ -38,14 +38,19 @@ func ValidateRayClusterMetadata(metadata metav1.ObjectMeta) error {
 }
 
 func ValidateRayClusterUpgradeOptions(instance *rayv1.RayCluster) error {
-	strategy := instance.Spec.UpgradeStrategy
-	if strategy == nil || strategy.Type == nil || *strategy.Type == rayv1.RayClusterUpgradeNone {
-		return nil
+	// only Recreate and None are valid upgradeType
+	if instance.Spec.UpgradeStrategy != nil && instance.Spec.UpgradeStrategy.Type != nil &&
+		*instance.Spec.UpgradeStrategy.Type != rayv1.Recreate &&
+		*instance.Spec.UpgradeStrategy.Type != rayv1.RayClusterUpgradeNone {
+		return fmt.Errorf("The RayCluster spec is invalid: Spec.UpgradeStrategy.Type value %s is invalid, valid options are %s or %s", *instance.Spec.UpgradeStrategy.Type, rayv1.Recreate, rayv1.RayClusterUpgradeNone)
 	}
 
-	creatorCRDType := GetCRDType(instance.Labels[RayOriginatedFromCRDLabelKey])
-	if creatorCRDType == RayJobCRD || creatorCRDType == RayServiceCRD {
-		return fmt.Errorf("upgradeStrategy cannot be set when RayCluster is created by %s", creatorCRDType)
+	// only allow UpgradeStrategy to be set when RayCluster is created directly by user
+	if instance.Spec.UpgradeStrategy != nil && instance.Spec.UpgradeStrategy.Type != nil {
+		creatorCRDType := GetCRDType(instance.Labels[RayOriginatedFromCRDLabelKey])
+		if creatorCRDType == RayJobCRD || creatorCRDType == RayServiceCRD {
+			return fmt.Errorf("upgradeStrategy cannot be set when RayCluster is created by %s", creatorCRDType)
+		}
 	}
 	return nil
 }

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

@@ -1930,6 +1930,15 @@ func TestValidateRayClusterUpgradeOptions(t *testing.T) {
 			expectError:       true,
 			errorMessage:      "upgradeStrategy cannot be set when RayCluster is created by RayService",
 		},
+		{
+			name: "Invalid upgrade strategy value",
+			upgradeStrategy: &rayv1.RayClusterUpgradeStrategy{
+				Type: ptr.To(rayv1.RayClusterUpgradeType("InvalidStrategy")),
+			},
+			originatedFromCRD: string(RayClusterCRD),
+			expectError:       true,
+			errorMessage:      "Spec.UpgradeStrategy.Type value InvalidStrategy is invalid, valid options are Recreate or None",
+		},
 	}
 
 	for _, tt := range tests {