[Feature] REP 54: Add PodName to the HeadInfo #2266

rueian · 2024-07-23T13:30:58Z

Why are these changes needed?

Closes #2236

This PR adds the new PodName to the HeadInfo status. And make RayService use the field to fetch the Head Pod.

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests

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

rueian · 2024-07-23T13:34:35Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -1257,7 +1257,7 @@ func (r *RayClusterReconciler) getHeadServiceIPAndName(ctx context.Context, inst
 		return "", "", fmt.Errorf("head service IP is empty. cluster name %s, filter labels %v", instance.Name, common.RayClusterHeadServiceListOptions(instance))
 	} else if runtimeServices.Items[0].Spec.ClusterIP == corev1.ClusterIPNone {
 		// We return Head Pod IP if the Head service is headless.
-		ip, err := r.getHeadPodIP(ctx, instance)
+		ip, _, err := r.getHeadPodIPAndName(ctx, instance)


This getHeadServiceIPAndName function returns the service name so we omit the returned Head name here.

rueian · 2024-07-23T13:35:35Z

ray-operator/controllers/ray/raycluster_controller.go

 	if err != nil {
 		return err
 	}
 	instance.Status.Head.PodIP = ip
+	instance.Status.Head.PodName = name


This is where we save the Head name into the HeadInfo status.

rueian · 2024-07-23T13:36:05Z

ray-operator/controllers/ray/raycluster_controller_unit_test.go

 			}

-			assert.Equal(t, tc.expectedIP, ip, "getHeadPodIP returned unexpected IP")
+			assert.Equal(t, tc.expectedIP, ip, "getHeadPodIPAndName returned unexpected IP")
+			assert.Equal(t, tc.expectedName, name, "getHeadPodIPAndName returned unexpected name")


Make sure that the Head name is saved.

rueian · 2024-07-23T13:37:25Z

ray-operator/controllers/ray/rayservice_controller.go

-		return nil, fmt.Errorf("Found %d head pods for RayCluster %s in the namespace %s", len(podList.Items), instance.Name, instance.Namespace)
+	if instance.Status.Head.PodName != "" {
+		pod := &corev1.Pod{}
+		if err := r.Get(ctx, types.NamespacedName{Namespace: instance.Namespace, Name: instance.Status.Head.PodName}, pod); err != nil {


According to the design doc, let RayService use the new .Status.Head.PodName to get the head.

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

ray-operator/controllers/ray/rayservice_controller.go

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

ray-operator/controllers/ray/rayservice_controller.go

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

ray-operator/controllers/ray/raycluster_controller.go

kevin85421 · 2024-07-24T03:36:17Z

ray-operator/controllers/ray/raycluster_controller.go

 	}
 	if len(runtimePods.Items) != 1 {
 		logger.Info(fmt.Sprintf("Found %d head pods. cluster name %s, filter labels %v", len(runtimePods.Items), instance.Name, filterLabels))
-		return "", nil
+		return "", "", nil


It is a bit weird to return nil when there is more than one head Pod. Can you check whether we need to return a non-nil error instead?

The behavior was intentional to keep the status update successful. But since we delete extra heads in the reconcilePods step, I think we are okay with returning an error here.

kevin85421 · 2024-07-24T03:41:31Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -1229,19 +1229,19 @@ func (r *RayClusterReconciler) calculateStatus(ctx context.Context, instance *ra
 }

 // Best effort to obtain the ip of the head node.
-func (r *RayClusterReconciler) getHeadPodIP(ctx context.Context, instance *rayv1.RayCluster) (string, error) {
+func (r *RayClusterReconciler) getHeadPodIPAndName(ctx context.Context, instance *rayv1.RayCluster) (string, string, error) {


Maybe we can use getHeadPod which returns the head Pod and an err instead

Hi @kevin85421, do you mean remove this function? We use this function to get the head IP and name and store them in the HeadInfo. It is actually this function that powers the getHeadPod.

Discussed offline: we moved the getHeadPodIPAndName into common.GetRayClusterHeadPod.

ray-operator/controllers/ray/rayservice_controller.go

rueian · 2024-07-24T09:53:56Z

ray-operator/controllers/ray/rayservice_controller.go

@@ -1220,17 +1220,3 @@ func (r *RayServiceReconciler) isHeadPodRunningAndReady(ctx context.Context, ins
 func isServeAppUnhealthyOrDeployedFailed(appStatus string) bool {
 	return appStatus == rayv1.ApplicationStatusEnum.UNHEALTHY || appStatus == rayv1.ApplicationStatusEnum.DEPLOY_FAILED
 }
-
-// TODO: Move this function to util.go and always use this function to retrieve the head Pod.
-func (r *RayServiceReconciler) getHeadPod(ctx context.Context, instance *rayv1.RayCluster) (*corev1.Pod, error) {


Moved to common.GetRayClusterHeadPod.

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

kevin85421 · 2024-07-24T16:15:44Z

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

@@ -159,3 +163,14 @@ func RayJobRayClusterNamespacedName(rayJob *rayv1.RayJob) types.NamespacedName {
 		Namespace: rayJob.Namespace,
 	}
 }
+
+func GetRayClusterHeadPod(ctx context.Context, reader client.Reader, instance *rayv1.RayCluster) (*corev1.Pod, error) {


Use common.RayClusterHeadPodsAssociationOptions instead.

kevin85421 · 2024-07-24T16:18:09Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -1309,13 +1313,14 @@ func (r *RayClusterReconciler) updateEndpoints(ctx context.Context, instance *ra
 }

 func (r *RayClusterReconciler) updateHeadInfo(ctx context.Context, instance *rayv1.RayCluster) error {
-	ip, err := r.getHeadPodIP(ctx, instance)
+	ip, name, err := r.getHeadPodIPAndName(ctx, instance)


headPod, err := common.GetRayClusterHeadPod(...) ... instance.Status.Head.PodIP = headPod.Status.PodIP

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

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

kevin85421 · 2024-07-24T18:31:28Z

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

+		return nil, err
+	}
+	if len(runtimePods.Items) == 0 {
+		logger.Info(fmt.Sprintf("Found %d head pods. cluster name %s, filter labels %v", len(runtimePods.Items), instance.Name, filterLabels))


Avoid using fmt.Sprintf. We can use:

logger.Info("Found 0 head Pod", "filter labels", filterLabels)

instead. I removed instance.Name because I think all KubeRay logs currently include the information about the CR name.

kevin85421 · 2024-07-24T18:31:59Z

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

+		return nil, nil
+	}
+	if len(runtimePods.Items) > 1 {
+		logger.Info(fmt.Sprintf("Found %d head pods. cluster name %s, filter labels %v", len(runtimePods.Items), instance.Name, filterLabels))


same

remove fmt.Sprintf

remove instance.Name

kevin85421 · 2024-07-24T18:32:47Z

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

+	}
+	if len(runtimePods.Items) > 1 {
+		logger.Info(fmt.Sprintf("Found %d head pods. cluster name %s, filter labels %v", len(runtimePods.Items), instance.Name, filterLabels))
+		return nil, fmt.Errorf("found multiple heads. cluster name %s, filter labels %v", instance.Name, filterLabels)


remove "cluster name"

kevin85421 · 2024-07-24T18:34:10Z

ray-operator/controllers/ray/raycluster_controller.go

@@ -1257,11 +1241,14 @@ func (r *RayClusterReconciler) getHeadServiceIPAndName(ctx context.Context, inst
 		return "", "", fmt.Errorf("head service IP is empty. cluster name %s, filter labels %v", instance.Name, common.RayClusterHeadServiceListOptions(instance))
 	} else if runtimeServices.Items[0].Spec.ClusterIP == corev1.ClusterIPNone {
 		// We return Head Pod IP if the Head service is headless.
-		ip, err := r.getHeadPodIP(ctx, instance)
+		head, err := common.GetRayClusterHeadPod(ctx, r, instance)


maybe use headPod instead to keep the naming consistent with the codebase.

Thanks for catching all these.

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

[Feat][RayCluster] Add PodName to the HeadInfo

4dad70b

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

rueian changed the title ~~Head info pod name~~ [Feature] REP 54: Add PodName to the HeadInfo Jul 23, 2024

rueian commented Jul 23, 2024

View reviewed changes

[Feat][RayService] Use the HeadInfo.PodName to get the Head Pod

6f40873

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

rueian force-pushed the head-info-pod-name branch from f19023e to 6f40873 Compare July 23, 2024 13:38

rueian marked this pull request as ready for review July 23, 2024 14:07

kevin85421 self-assigned this Jul 23, 2024

kevin85421 added the 1.2.0 label Jul 23, 2024

kevin85421 self-requested a review July 23, 2024 18:57

andrewsykim reviewed Jul 24, 2024

View reviewed changes

ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved

[Feat][RayService] Use the HeadInfo.PodName to get the Head Pod

aaa346a

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

andrewsykim reviewed Jul 24, 2024

View reviewed changes

ray-operator/controllers/ray/rayservice_controller.go Outdated Show resolved Hide resolved

[Feat][RayService] Use the HeadInfo.PodName to get the Head Pod

2da0db3

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

kevin85421 reviewed Jul 24, 2024

View reviewed changes

kevin85421 mentioned this pull request Jul 24, 2024

[Feature][RayCluster]: Implement the HeadReady condition #2261

Merged

3 tasks

rueian force-pushed the head-info-pod-name branch from a90e3b3 to cc3f369 Compare July 24, 2024 09:53

rueian commented Jul 24, 2024

View reviewed changes

[Feat][Association] Add GetRayClusterHeadPod

21d466d

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

rueian force-pushed the head-info-pod-name branch from cc3f369 to 21d466d Compare July 24, 2024 10:06

rueian requested a review from kevin85421 July 24, 2024 10:58

kevin85421 reviewed Jul 24, 2024

View reviewed changes

[Feat][RayService] Refactor GetRayClusterHeadPod to use RayClusterHea…

9ff0114

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

rueian force-pushed the head-info-pod-name branch from 34d462f to 9ff0114 Compare July 24, 2024 17:19

kevin85421 reviewed Jul 24, 2024

View reviewed changes

[Feat][RayService] Refactor GetRayClusterHeadPod to use RayClusterHea…

b0e9172

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

rueian requested a review from kevin85421 July 25, 2024 00:56

kevin85421 approved these changes Jul 25, 2024

View reviewed changes

kevin85421 merged commit 78b9828 into ray-project:master Jul 25, 2024
25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] REP 54: Add PodName to the HeadInfo #2266

[Feature] REP 54: Add PodName to the HeadInfo #2266

rueian commented Jul 23, 2024 •

edited

Loading

rueian Jul 23, 2024 •

edited

Loading

rueian Jul 23, 2024

rueian Jul 23, 2024 •

edited

Loading

rueian Jul 23, 2024

kevin85421 Jul 24, 2024

rueian Jul 24, 2024 •

edited

Loading

kevin85421 Jul 24, 2024

rueian Jul 24, 2024

rueian Jul 24, 2024

rueian Jul 24, 2024

kevin85421 Jul 24, 2024

kevin85421 Jul 24, 2024

kevin85421 Jul 24, 2024

kevin85421 Jul 24, 2024

kevin85421 Jul 24, 2024

kevin85421 Jul 24, 2024

rueian Jul 25, 2024

[Feature] REP 54: Add PodName to the HeadInfo #2266

[Feature] REP 54: Add PodName to the HeadInfo #2266

Conversation

rueian commented Jul 23, 2024 • edited Loading

Why are these changes needed?

Checks

rueian Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Jul 24, 2024 • edited Loading

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

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

rueian commented Jul 23, 2024 •

edited

Loading

rueian Jul 23, 2024 •

edited

Loading

rueian Jul 23, 2024 •

edited

Loading

rueian Jul 24, 2024 •

edited

Loading