[feat] validate the name length of RayCluster, RayService, and RayJob #3083
+347
−16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rueian
force-pushed
the
validate-raycluster-name
branch
from
1032d24 to
5cd981c
Compare
rueian
changed the title
rueian
force-pushed
the
validate-raycluster-name
branch
from
d465e97 to
b24ba93
Compare
rueian
commented
Feb 21, 2025
| @@ -15,7 +15,7 @@ import ( | |||
| func BuildRouteForHeadService(cluster rayv1.RayCluster) (*routev1.Route, error) { | |||
| 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)), | |||
There was a problem hiding this comment.
This is an old bug that forgets to chunk the value for utils.RayIDLabelKey.
| @@ -21,7 +21,7 @@ func BuildIngressForHeadService(ctx context.Context, cluster rayv1.RayCluster) ( | |||
|
|
|||
| 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)), | |||
There was a problem hiding this comment.
This is an old bug that forgets to chunk the value for utils.RayIDLabelKey.
| @@ -21,7 +22,7 @@ const ( | |||
| redisAddress = "redis:6379" | |||
| ) | |||
|
|
|||
| func TestRayClusterGCSFaultTolerence(t *testing.T) { | |||
| func TestRayClusterGCSFaultTolerance(t *testing.T) { | |||
rueian
force-pushed
the
validate-raycluster-name
branch
2 times, most recently
from
e6ee5ac to
8817876
Compare
rueian
force-pushed
the
validate-raycluster-name
branch
from
3426bc8 to
e26affb
Compare
rueian
commented
Mar 2, 2025
| @@ -1019,8 +1019,6 @@ func (r *RayClusterReconciler) createHeadRoute(ctx context.Context, route *route | |||
| func (r *RayClusterReconciler) createService(ctx context.Context, svc *corev1.Service, instance *rayv1.RayCluster) error { | |||
| logger := ctrl.LoggerFrom(ctx) | |||
|
|
|||
| // making sure the name is valid | |||
| svc.Name = utils.CheckName(svc.Name) | |||
There was a problem hiding this comment.
No more utils.CheckName to cut service names.
Comment on lines
-265
to
+267
| 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") |
There was a problem hiding this comment.
No more utils.CheckName to cut service names.
| @@ -293,7 +293,7 @@ func ExtractRayIPFromFQDN(fqdnRayIP string) string { | |||
|
|
|||
| // 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") | |||
There was a problem hiding this comment.
No more utils.CheckName to cut service names.
rueian
changed the title
rueian
force-pushed
the
validate-raycluster-name
branch
from
e26affb to
c9c3e9c
Compare
Signed-off-by: Rueian <[email protected]>
rueian
force-pushed
the
validate-raycluster-name
branch
from
c9c3e9c to
78f39a2
Compare
|
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 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
53 charactersat most.47 charactersat most.47 charactersat 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