[feat] validate the name length of RayCluster, RayService, and RayJob #3083

rueian · 2025-02-20T22:14:52Z

Why are these changes needed?

Part of the #3076, adding length validation to RayCluster, RayService, and RayJob to make sure we don't cut k8s service names:

RayCluster: 63 - len("-serve-svc") = 53 characters at most.
RayService: 53 - len("-xxxxx") = 47 characters at most.
RayJob: 53 - len("-xxxxx") = 47 characters at most.

After #3101, the longest suffix for a K8s service is now "-serve-svc", which takes 10 characters. Therefore, we can only use 63-10=53 characters for the name of a RayCluster.

After #3102, we now add a "-" and 5 random characters to the name of either RayService or RayJob to create the underlying RayCluster. Therefore, they can only have 53-6=47 characters for their name.

Any RayCluster, RayService, or RayJob with a name exceeding the above limit will not be reconciled further and will receive an invalid spec event, which can be observed by kubectl describe.

Related issue number

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

rueian · 2025-02-21T02:23:08Z

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

 	labels := map[string]string{
 		utils.RayClusterLabelKey:                cluster.Name,
-		utils.RayIDLabelKey:                     utils.GenerateIdentifier(cluster.Name, rayv1.HeadNode),
+		utils.RayIDLabelKey:                     utils.CheckLabel(utils.GenerateIdentifier(cluster.Name, rayv1.HeadNode)),


This is an old bug that forgets to chunk the value for utils.RayIDLabelKey.

rueian · 2025-02-21T02:23:35Z

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

 	labels := map[string]string{
 		utils.RayClusterLabelKey:                cluster.Name,
-		utils.RayIDLabelKey:                     utils.GenerateIdentifier(cluster.Name, rayv1.HeadNode),
+		utils.RayIDLabelKey:                     utils.CheckLabel(utils.GenerateIdentifier(cluster.Name, rayv1.HeadNode)),


This is an old bug that forgets to chunk the value for utils.RayIDLabelKey.

rueian · 2025-02-21T02:24:13Z

ray-operator/test/e2e/raycluster_gcs_ft_test.go

 )

-func TestRayClusterGCSFaultTolerence(t *testing.T) {
+func TestRayClusterGCSFaultTolerance(t *testing.T) {


ray-operator/test/e2e/raycluster_gcs_ft_test.go

rueian · 2025-03-02T23:14:15Z

ray-operator/controllers/ray/raycluster_controller.go

 	logger := ctrl.LoggerFrom(ctx)

-	// making sure the name is valid
-	svc.Name = utils.CheckName(svc.Name)


No more utils.CheckName to cut service names.

rueian · 2025-03-02T23:14:41Z

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

-		return CheckName(fmt.Sprintf("%s-%s-%s", ownerName, rayv1.HeadNode, "svc")), nil
+		return fmt.Sprintf("%s-%s-%s", ownerName, rayv1.HeadNode, "svc"), nil
 	case RayClusterCRD:
-		headSvcName := CheckName(fmt.Sprintf("%s-%s-%s", ownerName, rayv1.HeadNode, "svc"))
+		headSvcName := fmt.Sprintf("%s-%s-%s", ownerName, rayv1.HeadNode, "svc")


No more utils.CheckName to cut service names.

rueian · 2025-03-02T23:14:46Z

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

 // GenerateServeServiceName generates name for serve service.
 func GenerateServeServiceName(serviceName string) string {
-	return CheckName(fmt.Sprintf("%s-%s-%s", serviceName, ServeName, "svc"))
+	return fmt.Sprintf("%s-%s-%s", serviceName, ServeName, "svc")


No more utils.CheckName to cut service names.

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

davidxia · 2025-03-03T13:47:38Z

Would it be useful to also validate in webhooks like here?

kuberay/ray-operator/pkg/webhooks/v1/raycluster_webhook.go

Lines 79 to 84 in 6c4a77d

    
           func (w *RayClusterWebhook) validateName(rayCluster *rayv1.RayCluster) *field.Error { 
        
           	if !nameRegex.MatchString(rayCluster.Name) { 
        
           		return field.Invalid(field.NewPath("metadata").Child("name"), rayCluster.Name, "name must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')") 
        
           	} 
        
           	return nil 
        
           }

These webhooks can optionally be enabled by user (disabled by default). They prevent creation of invalid resources with a more obvious error than the controller events.

rueian · 2025-03-03T16:11:56Z

Hi @davidxia, while it would certainly be useful to replicate the validation to the webhook, we don’t do it now. Most Kuberay users don’t use the validation webhook.

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

kevin85421

I felt that adding E2E tests for this was a bit overkill. Does env tests check the K8s resource name length?

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

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

rueian · 2025-03-06T06:54:36Z

I felt that adding E2E tests for this was a bit overkill. Does env tests check the K8s resource name length?

Sure, I changed them to env tests.

kevin85421 · 2025-03-06T06:58:39Z

ray-operator/controllers/ray/raycluster_controller.go

 	}

+	if err := utils.ValidateRayClusterMetadata(instance.ObjectMeta); err != nil {
+		logger.Error(err, fmt.Sprintf("The RayCluster metadata is invalid %s/%s", instance.Namespace, instance.Name))


Avoid using fmt.Sprintf in logger.

kevin85421 · 2025-03-06T06:59:26Z

ray-operator/controllers/ray/raycluster_controller.go


+	if err := utils.ValidateRayClusterMetadata(instance.ObjectMeta); err != nil {
+		logger.Error(err, fmt.Sprintf("The RayCluster metadata is invalid %s/%s", instance.Namespace, instance.Name))
+		r.Recorder.Eventf(instance, corev1.EventTypeWarning, string(utils.InvalidRayClusterSpec),


InvalidRayClusterSpec -> InvalidaRayClusterMetadata?

kevin85421 · 2025-03-06T07:03:31Z

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

 }

 func ValidateRayJobSpec(rayJob *rayv1.RayJob) error {
+	if len(rayJob.Name) > MaxRayJobNameLength {


maybe we can (1) separate the logic into ValidateRayJobMetadata / ValidateRayServiceMetadata (2) only pass CR spec to the function ValidateXXXXSpec as a follow up PR. This will be a good first issue.

+1. It's weird that we have ValidateRayClusterMetadata and ValidateRayCluseterSpec, but only ValidateRayJobSpec and no ValidateRayJobMetadata.

done. I can open the issue for (2) after this is merged.

kevin85421 · 2025-03-06T07:06:49Z

ray-operator/controllers/ray/raycluster_controller_test.go

+		namespace := "default"
+		rayCluster := rayClusterTemplate(strings.Repeat("r", utils.MaxRayClusterNameLength), namespace)
+		numOfHosts := int32(4)
+		rayCluster.Spec.WorkerGroupSpecs[0].NumOfHosts = numOfHosts


why do we need to set numOfHosts?

We need it to create the -headless service. A comment is added.

kevin85421 · 2025-03-06T07:09:05Z

ray-operator/controllers/ray/raycluster_controller_test.go

+		numOfHosts := int32(4)
+		rayCluster.Spec.WorkerGroupSpecs[0].NumOfHosts = numOfHosts
+		rayCluster.Annotations = make(map[string]string)
+		rayCluster.Annotations[utils.EnableServeServiceKey] = "true"


would you mind adding comments to explain why this test requires this (i.e. -serve-svc is the longest suffix name)?

Sure, a comment is added.

kevin85421 · 2025-03-06T07:10:01Z

ray-operator/controllers/ray/raycluster_controller_test.go

+				time.Second*3, time.Millisecond*500).Should(BeNil(), "Should be able to see RayCluster: %v", rayCluster.Name)
+		})
+
+		It("Check RayCluster service names", func() {


maybe also check Pods are created

ok! Added above.

kevin85421 · 2025-03-06T07:17:50Z

ray-operator/controllers/ray/rayservice_controller_test.go


 var _ = Context("RayService env tests", func() {
+	Describe("RayService with a maximum name", Ordered, func() {
+		// If Autoscaler scales up the pending or active RayCluster, zero downtime upgrade should not be triggered.


why do we need this comment?

deleted. thanks!

kevin85421 · 2025-03-06T07:20:30Z

ray-operator/controllers/ray/rayservice_controller_test.go

+		It("Should create a pending RayCluster", func() {
+			Eventually(
+				getPreparingRayClusterNameFunc(ctx, rayService),
+				time.Second*15, time.Millisecond*500).Should(Not(BeEmpty()), "Pending RayCluster name: %v", rayService.Status.PendingServiceStatus.RayClusterName)


Change all time.Second*15 in this PR to time.Second*3 for consistency.

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

kevin85421

Just left some nit comments. Other LGTM

kevin85421 · 2025-03-07T20:44:40Z

ray-operator/controllers/ray/raycluster_controller.go

 	}

+	if err := utils.ValidateRayClusterMetadata(instance.ObjectMeta); err != nil {
+		logger.Error(err, "The RayCluster metadata is invalid", "namespace", instance.Namespace, "name", instance.Name)


Do we need namespace and name? The context information should be added automatically.

kevin85421 · 2025-03-07T20:47:37Z

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

+			errorMessage: fmt.Sprintf("RayCluster name should be no more than %d characters", MaxRayClusterNameLength),
+		},
+		{
+			name: "Both RayCluster name is ok (== MaxRayClusterNameLength)",


what does "both" refer to?

kevin85421 · 2025-03-07T20:48:26Z

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

+				ObjectMeta: metav1.ObjectMeta{
+					Name: strings.Repeat("A", MaxRayClusterNameLength),
+				},
+				Spec: rayv1.RayClusterSpec{


is cluster spec needed for this test?

kevin85421 · 2025-03-07T20:53:27Z

ray-operator/controllers/ray/raycluster_controller.go

+
 	if err := utils.ValidateRayClusterSpec(&instance.Spec, instance.Annotations); err != nil {
-		logger.Error(err, fmt.Sprintf("The RayCluster spec is invalid %s/%s", instance.Namespace, instance.Name))
+		logger.Error(err, "The RayCluster spec is invalid", "namespace", instance.Namespace, "name", instance.Name)


Do we need namespace and name?

kevin85421 · 2025-03-07T20:58:31Z

@MortalHappiness please review another iteration. Thanks!

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

rueian force-pushed the validate-raycluster-name branch from 1032d24 to 5cd981c Compare February 20, 2025 22:24

rueian changed the title ~~feat: validate the length of RayCluster name and worker group names~~ [feat][RayCluster] validate the length of RayCluster name and worker group names Feb 21, 2025

rueian force-pushed the validate-raycluster-name branch from d465e97 to b24ba93 Compare February 21, 2025 02:22

rueian commented Feb 21, 2025

View reviewed changes

rueian marked this pull request as ready for review February 21, 2025 05:09

kevin85421 self-assigned this Feb 21, 2025

rueian force-pushed the validate-raycluster-name branch 2 times, most recently from e6ee5ac to 8817876 Compare February 25, 2025 00:18

rueian marked this pull request as draft February 25, 2025 23:49

rueian force-pushed the validate-raycluster-name branch from 3426bc8 to e26affb Compare March 2, 2025 23:13

rueian commented Mar 2, 2025

View reviewed changes

rueian changed the title ~~[feat][RayCluster] validate the length of RayCluster name and worker group names~~ [feat] validate the name length of RayCluster, RayService, and RayJob Mar 3, 2025

rueian force-pushed the validate-raycluster-name branch from e26affb to c9c3e9c Compare March 3, 2025 04:40

feat: validate the name length for RayCluster, RayService, and RayJob

78f39a2

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

rueian force-pushed the validate-raycluster-name branch from c9c3e9c to 78f39a2 Compare March 3, 2025 05:18

rueian marked this pull request as ready for review March 3, 2025 06:51

Merge branch 'master' into validate-raycluster-name

32f719c

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

rueian force-pushed the validate-raycluster-name branch from af47a93 to 32f719c Compare March 3, 2025 16:48

kevin85421 reviewed Mar 6, 2025

View reviewed changes

ray-operator/controllers/ray/utils/validation.go Outdated Show resolved Hide resolved

feat: validate the name length for RayCluster, RayService, and RayJob

0be2088

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

rueian force-pushed the validate-raycluster-name branch 2 times, most recently from 342afe0 to 5371440 Compare March 6, 2025 06:49

kevin85421 reviewed Mar 6, 2025

View reviewed changes

kevin85421 assigned MortalHappiness and andrewsykim Mar 6, 2025

feat: validate the name length for RayCluster, RayService, and RayJob

d3a273c

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

rueian force-pushed the validate-raycluster-name branch from 5371440 to d3a273c Compare March 6, 2025 07:54

feat: validate the name length for RayCluster, RayService, and RayJob

a04fd62

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

kevin85421 approved these changes Mar 7, 2025

View reviewed changes

kevin85421 requested a review from MortalHappiness March 7, 2025 20:58

feat: validate the name length for RayCluster, RayService, and RayJob

ee6c895

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

rueian force-pushed the validate-raycluster-name branch from 7da6ea2 to ee6c895 Compare March 7, 2025 21:18

MortalHappiness approved these changes Mar 8, 2025

View reviewed changes

kevin85421 merged commit aeb2b57 into ray-project:master Mar 8, 2025
21 checks passed

This was referenced Mar 26, 2025

[Bug] [API Server] JobSubmission service does not work for cluster names >41 characters #2169

Open

[feat] enforce DNS1035 validations on RayCluster, RayService, and RayJob names #3239

Merged

rueian mentioned this pull request Apr 3, 2025

[Umbrella] Document the breaking changes for CR names in v1.4 #3271

Open

davidxia mentioned this pull request Jun 24, 2025

[feat][operator] validate Ray resource metadata in webhook #3831

Merged

4 tasks

[feat] validate the name length of RayCluster, RayService, and RayJob #3083

[feat] validate the name length of RayCluster, RayService, and RayJob #3083

Uh oh!

Conversation

rueian commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidxia commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rueian commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rueian commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevin85421 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rueian commented Feb 20, 2025 •

edited

Loading

davidxia commented Mar 3, 2025 •

edited

Loading

rueian commented Mar 3, 2025 •

edited

Loading