-
Notifications
You must be signed in to change notification settings - Fork 450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RayService] Always create RayService services and remove RayServiceInitializing #2867
base: master
Are you sure you want to change the base?
Conversation
b90ee6c
to
dde1cad
Compare
@@ -222,7 +222,7 @@ func BuildServeService(ctx context.Context, rayService rayv1.RayService, rayClus | |||
|
|||
if isRayService { | |||
// We are invoked from RayService | |||
if len(ports) == 0 && rayService.Spec.ServeService == nil { | |||
if len(ports) == 0 && rayService.Spec.ServeService == nil && rayCluster.Name != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rayCluster.Name != ""
is essential for building a service without RayCluster.
message := "RayService is initializing" | ||
setCondition(rayServiceInstance, rayv1.RayServiceReady, metav1.ConditionFalse, rayv1.RayServiceInitializing, message) | ||
setCondition(rayServiceInstance, rayv1.UpgradeInProgress, metav1.ConditionFalse, rayv1.RayServiceInitializing, message) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove RayServiceInitializing
for simplicity.
case utils.ServingService: | ||
newSvc, err = common.BuildServeServiceForRayService(ctx, *rayServiceInstance, *rayClusterInstance) | ||
if rayClusterInstance == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it clear that rayClusterInstance
could be nil.
} | ||
|
||
// Retrieve the Service from the Kubernetes cluster with the name and namespace. | ||
oldSvc := &corev1.Service{} | ||
err = r.Get(ctx, client.ObjectKey{Name: newSvc.Name, Namespace: rayServiceInstance.Namespace}, oldSvc) | ||
|
||
if err == nil { | ||
// Do not drop cluster selector on the existing service. | ||
if newSvc.Spec.Selector[utils.RayClusterLabelKey] == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not change the existing cluster selector to "" for not dropping requests.
…nitializing Signed-off-by: Rueian <[email protected]>
dde1cad
to
4450ec6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is much more complex than my imagination. Maybe we should defer this PR a bit and discuss whether we should support it in v1.3.0 or not.
Why are these changes needed?
Currently, the RayService controller only creates k8s services for RayService when the RayServe is ready. This PR changes the behavior to create k8s services first even when the RayServe is not ready, and later, the selector is updated to the ready RayServe. The new behaivor will help users expose k8s services as soon as possible.
Related issue number
Checks