Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rueian
Copy link
Contributor

@rueian rueian commented Jan 31, 2025

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

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

@rueian rueian force-pushed the refactor-rayservice-alwayssvc branch 6 times, most recently from b90ee6c to dde1cad Compare January 31, 2025 07:48
@@ -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 != "" {
Copy link
Contributor Author

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)
}
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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] == "" {
Copy link
Contributor Author

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.

@rueian rueian force-pushed the refactor-rayservice-alwayssvc branch from dde1cad to 4450ec6 Compare January 31, 2025 07:55
@rueian rueian marked this pull request as ready for review January 31, 2025 08:24
Copy link
Member

@kevin85421 kevin85421 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants