[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService #1110

kevin85421 · 2023-05-25T00:58:07Z

Why are these changes needed?

As mentioned in #1105, in Kubernetes 1.20.7, the selector in the Serve service of RayService cannot be updated, but Kubernetes 1.23.0 does not have this issue.

The root cause of this issue is the immutability of the ClusterIP field. If a service is created without specifying the ClusterIP, Kubernetes automatically assigns it. Therefore, in the reconcileServices function, the oldSvc object retrieved from the Kubernetes cluster contains the ClusterIP value.

Without this PR (as shown in the following code snippet), raySvc is derived from the CR spec, so the ClusterIP is "". However, the ClusterIP in the rayService variable is assigned by Kubernetes. Therefore, this update is invalid since the ClusterIP field is immutable.

kuberay/ray-operator/controllers/ray/rayservice_controller.go

Lines 804 to 830 in 7fe4050

    
           func (r *RayServiceReconciler) reconcileServices(ctx context.Context, rayServiceInstance *rayv1alpha1.RayService, rayClusterInstance *rayv1alpha1.RayCluster, serviceType common.ServiceType) error { 
        
           	// Creat Service Struct. 
        
           	var raySvc *corev1.Service 
        
           	var err error 
        
           	if serviceType == common.HeadService { 
        
           		raySvc, err = common.BuildHeadServiceForRayService(*rayServiceInstance, *rayClusterInstance) 
        
           	} else if serviceType == common.ServingService { 
        
           		raySvc, err = common.BuildServeServiceForRayService(*rayServiceInstance, *rayClusterInstance) 
        
           	} 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	raySvc.Name = utils.CheckName(raySvc.Name) 
        
           	// Get Service instance. 
        
           	rayService := &corev1.Service{} 
        
           	err = r.Get(ctx, client.ObjectKey{Name: raySvc.Name, Namespace: rayServiceInstance.Namespace}, rayService) 
        
           	if err == nil { 
        
           		// Update Service 
        
           		rayService.Spec = raySvc.Spec 
        
           		r.Log.V(1).Info("reconcileServices update service") 
        
           		if updateErr := r.Update(ctx, rayService); updateErr != nil { 
        
           			r.Log.Error(updateErr, "raySvc Update error!", "raySvc.Error", updateErr) 
        
           			return updateErr 
        
           		}

Optional: Why does Kubernetes 1.23.0 not have this issue?

I used binary search to identify that the issue was resolved between Kubernetes v1.21.2 and Kubernetes v1.21.10.

Trace code (Kubernetes v1.21.10):

Kubernetes Service Update RESTful API (code) -> Call BeforeUpdate() (code) -> Call strategy.PrepareForUpdate to prepare for update (code)
Kubernetes Service's PrepareForUpdate strategy (code) -> Call patchAllocatedValues (code)

patchAllocatedValues: if the new service does not specify a ClusterIP, the function will assign the ClusterIP of the old service to the new one. Hence, the update will not update ClusterIP from non-empty to empty.

 func patchAllocatedValues(newSvc, oldSvc *api.Service) {
     if needsClusterIP(oldSvc) && needsClusterIP(newSvc) {
 	    if newSvc.Spec.ClusterIP == "" {
 		    newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP
 	    }
 	    if len(newSvc.Spec.ClusterIPs) == 0 {
 		    newSvc.Spec.ClusterIPs = oldSvc.Spec.ClusterIPs
 	    }
     }
     ...
 }

The function patchAllocatedValues is added in Kubernetes v1.21.5.

Related issue number

Closes #1105

Checks

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

# Step 0: Create a Kind cluster
kind create cluster --image=kindest/node:v1.20.7

# Step 1: Build docker image for this PR (path: ray-operator/) and load into the Kind cluster
make docker-image
kind load docker-image controller:latest

# Step 2: Install a KubeRay operator
helm install kuberay-operator kuberay/kuberay-operator --version 0.5.0 --set image.repository=controller,image.tag=latest

# Step 3: Create a RayService
# path: ray-operator/config/samples
kubectl apply -f ray_v1alpha1_rayservice.yaml

# Step 4: Edit `spec.rayClusterConfig.rayVersion` from 2.4.0 to 2.100.0.
kubectl edit rayservices.ray.io rayservice-sample

# Step 5: Wait for the serve deployments on the new RayCluster becoming ready.
# Check the service's selector
kubectl describe svc rayservice-sample-head-svc

kevin85421 · 2023-05-25T04:13:40Z

ray-operator/apis/ray/v1alpha1/zz_generated.deepcopy.go

@@ -118,6 +118,11 @@ func (in *DashboardStatus) DeepCopy() *DashboardStatus {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *HeadGroupSpec) DeepCopyInto(out *HeadGroupSpec) {
 	*out = *in
+	if in.HeadService != nil {


This is unrelated to #1105. The generated code is for #1040. We should enhance the CI to detect such issues. The command make docker-image will not trigger the local zz_generated.deepcopy.go generation. We need to run make build to update zz_generated.deepcopy.go.

Open an issue: #1111

kevin85421 · 2023-05-25T04:29:04Z

ray-operator/controllers/ray/rayservice_controller.go

 	if err != nil {
 		return err
 	}
-	raySvc.Name = utils.CheckName(raySvc.Name)


The functions common.BuildHeadServiceForRayService and common.BuildServeServiceForRayService utilize utils.GenerateServiceName and utils.GenerateServeServiceName, respectively, which internally invoke CheckName. Hence, delete this line.

kevin85421 · 2023-05-25T04:37:02Z

ray-operator/controllers/ray/rayservice_controller.go

+		if newSvc.Spec.ClusterIP == "" {
+			newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP
+		}
+		oldSvc.Spec = *newSvc.Spec.DeepCopy()


We need to consider the updates of ObjectMeta in subsequent PRs. We currently only take Spec into consideration.

Shall we add a TODO in the code comment?

Added 3119e67

kevin85421 · 2023-05-25T05:41:57Z

cc @Yicheng-Lu-llll

kevin85421 · 2023-05-25T05:44:21Z

cc @jamm1985

jamm1985 · 2023-05-25T11:23:06Z

@kevin85421

It works fine. Is it possible to release v0.5.1 fix?

architkulkarni

Looks good to me, just a few minor questions (non-blocking)

architkulkarni · 2023-05-25T16:52:00Z

ray-operator/controllers/ray/rayservice_controller.go

-		r.Log.V(1).Info("reconcileServices update service")
-		if updateErr := r.Update(ctx, rayService); updateErr != nil {
-			r.Log.Error(updateErr, "raySvc Update error!", "raySvc.Error", updateErr)
+		// Only update the service if the RayCluster switches.


Is "Only update the service if the RayCluster switches" a new behavior change in this PR? What's the reasoning behind it? If the reason is easy to state it might be worth adding it to this code comment.

Is "Only update the service if the RayCluster switches" a new behavior change in this PR?

Yes

What's the reasoning behind it? If the reason is easy to state it might be worth adding it to this code comment.

Similar to #1065, the redundant Update invocations will cause an unnecessary burden on the Kubernetes API Server.

architkulkarni · 2023-05-25T16:52:03Z

ray-operator/controllers/ray/rayservice_controller.go

+		if newSvc.Spec.ClusterIP == "" {
+			newSvc.Spec.ClusterIP = oldSvc.Spec.ClusterIP
+		}
+		oldSvc.Spec = *newSvc.Spec.DeepCopy()


Shall we add a TODO in the code comment?

kevin85421 · 2023-05-25T18:51:09Z

cc @jamm1985 Is it acceptable for you to use the nightly build or image for a specific commit (DockerHub)? We anticipate releasing the first release candidate for v0.6.0 at the end of June, and currently, there are no plans for v0.5.1.

jamm1985 · 2023-05-26T06:25:48Z

@kevin85421 ok. Thank you a lot for the quick fix. I'm looking forward to the new v0.6.0 release and wish you a pleasant work on it.

…er RayService (ray-project#1110) [Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService

update

a5e366d

kevin85421 changed the title ~~[Bug] k8s v1.20.7 ClusterIP svc do not updated under RayService~~ [Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService May 25, 2023

kevin85421 added 4 commits May 25, 2023 03:50

update

c3aebc3

update

c41a402

update

a8ea880

update

7e35a10

kevin85421 commented May 25, 2023

View reviewed changes

update

beabca1

kevin85421 commented May 25, 2023

View reviewed changes

kevin85421 marked this pull request as ready for review May 25, 2023 05:40

kevin85421 requested review from architkulkarni, sihanwang41, gvspraveen, shrekris-anyscale and zcin May 25, 2023 16:40

architkulkarni approved these changes May 25, 2023

View reviewed changes

update

3119e67

kevin85421 merged commit f6a172f into ray-project:master May 26, 2023
19 checks passed

JoshKarpel mentioned this pull request Aug 8, 2023

[Bug] Only update ray.io/serve label on pods if its value changes #1303

Closed

2 tasks

lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023

[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated und…

3d19501

…er RayService (ray-project#1110) [Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService #1110

[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService #1110

kevin85421 commented May 25, 2023 •

edited

kevin85421 May 25, 2023

kevin85421 May 25, 2023

kevin85421 May 25, 2023

kevin85421 May 25, 2023

architkulkarni May 25, 2023

kevin85421 May 25, 2023

kevin85421 commented May 25, 2023

kevin85421 commented May 25, 2023

jamm1985 commented May 25, 2023

architkulkarni left a comment

architkulkarni May 25, 2023

kevin85421 May 25, 2023

architkulkarni May 25, 2023

kevin85421 commented May 25, 2023

jamm1985 commented May 26, 2023

	func (r RayServiceReconciler) reconcileServices(ctx context.Context, rayServiceInstance rayv1alpha1.RayService, rayClusterInstance *rayv1alpha1.RayCluster, serviceType common.ServiceType) error {
	// Creat Service Struct.
	var raySvc *corev1.Service
	var err error
	if serviceType == common.HeadService {
	raySvc, err = common.BuildHeadServiceForRayService(rayServiceInstance, rayClusterInstance)
	} else if serviceType == common.ServingService {
	raySvc, err = common.BuildServeServiceForRayService(rayServiceInstance, rayClusterInstance)
	}

	if err != nil {
	return err
	}
	raySvc.Name = utils.CheckName(raySvc.Name)

	// Get Service instance.
	rayService := &corev1.Service{}
	err = r.Get(ctx, client.ObjectKey{Name: raySvc.Name, Namespace: rayServiceInstance.Namespace}, rayService)

	if err == nil {
	// Update Service
	rayService.Spec = raySvc.Spec
	r.Log.V(1).Info("reconcileServices update service")
	if updateErr := r.Update(ctx, rayService); updateErr != nil {
	r.Log.Error(updateErr, "raySvc Update error!", "raySvc.Error", updateErr)
	return updateErr
	}

[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService #1110

[Bug][k8s compatibility] k8s v1.20.7 ClusterIP svc do not updated under RayService #1110

Conversation

kevin85421 commented May 25, 2023 • edited

Why are these changes needed?

Optional: Why does Kubernetes 1.23.0 not have this issue?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented May 25, 2023

kevin85421 commented May 25, 2023

jamm1985 commented May 25, 2023

architkulkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented May 25, 2023

jamm1985 commented May 26, 2023

kevin85421 commented May 25, 2023 •

edited