[Feature] Add headServiceLabels field to RayCluster CR

[Yicheng-Lu-llll]

Why are these changes needed?

Users want to set custom labels for the KubeRay Head Service. However, currently, KubeRay only supports custom annotations for the Head Service.
https://ray-distributed.slack.com/archives/C02GFQ82JPM/p1673647485229799

Implementation details and the reasons behind:

changes:

• In this PR, the newly added field name is HeadServiceCustomLabels.
• In this PR, the service selector and its label are decoupled. They are no longer the same.
• In this PR, custom labels can not set keys the same as the five default labels' keys.

reasons:

1. There is a reason to call HeadServiceCustomLabels instead of HeadServiceLabels. For kubeRay head service, labels = five default labels + user-defined custom labels. HeadServiceCustomLabels filed is only for user-defined custom labels. If we call HeadServiceLabels, users may think they should also add five default labels to this field.

2. kuberay/ray-operator/controllers/ray/common/service.go

   Line 49 in d4784a5

   Selector: labels,

   As shown above, in the current implementation, kubeRay head service's selector equals to its label. This fact is not true after adding custom service labels. Since the head pod will not contain these custom labels, a mismatch between the head pod and the head service will occur.

3. In the current implementation, the service selector uses the five default labels. And head pods will have exactly the same default labels. If users overwrite any of the five default labels by setting custom service labels, a mismatch between the head pod and the head service will occur. Note: this is unlike [Bug] Modification of nameOverride will cause label selector mismatch for head node #572, we can not change the head pod's labels.

when saying five default labels above, I am referring to

kuberay/ray-operator/controllers/ray/common/service.go

Line 12 in d4784a5

func HeadServiceLabels(cluster rayiov1alpha1.RayCluster) map[string]string {

What next?

In the current implementation, pod labels and service labels are assigned in different places and there is no consistency check. In other words, when modifying the code, we need to manually make sure the value of the default five labels in the service selector and pods are the same.

[TODO] unit tests: whether the labels and selector match or not.

Related issue number

Closes #868

Checks

Step1:
Clone my repo and checkout to AddCustomLabels branch

Step2:
Follow DEVELOPMENT.md to build Docker image and install kuberay operator.

Step3:
Add the following to the last line of kuberay/ray-operator/config/samples/ray-cluster.complete.yaml
(As @kevin85421 mentions, add app.kubernetes.io/name: "fake_name" to test)

 headServiceCustomLabels:
   test_key0: "test_value0"
   test_key1: "test_value1"
   test_key2: "test_value2"
   app.kubernetes.io/name: "fake_name"

Step4:

#path: kuberay/
kubectl apply -f ray-operator/config/samples/ray-cluster.complete.yaml

Step5:

kubectl get svc
kubectl describe svc raycluster-complete-head-svc

Step6: Set up port-forwarding
kubectl port-forward svc/raycluster-complete-head-svc 8265:8265

Step7: Submit a simple job
ray job submit --address http://localhost:8265 -- python -c "import ray; ray.init(); print(ray.cluster_resources())"

Step 8: In helm-chart/ray-cluster/values.yaml, add:

  # annotations passed on for the Head Service
  headServiceAnnotations: 
    test_ano_key0: "test_ano_value0"
  # # custom labels passed on for the Head Service
  # # to avoid conflicts, custom labels should not start with either of the prefixes `app.kubernetenes.io/` or `ray.io/`
  headServiceCustomLabels:
    test_key0: "test_value0"
    test_key1: "test_value1"
    test_key2: "test_value2"
    app.kubernetes.io/name: "fake_name"

Step 9

helm install raycluster ./helm-chart/ray-cluster
kubectl get svc
kubectl describe svc raycluster-kuberay-head-svc

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

[kevin85421]

Thank you for your contribution!

[kevin85421]

Would you mind adding some comments about this field in RayCluster chart, ray-cluster.complete.yaml, and ray-cluster.complete.large.yaml?

[kevin85421]

Is it possible to test whether the labels and selector match or not with unit tests?

[kevin85421]

We need to have a clear story about labels for the Head service.

Without custom labels for Head service

• Head Pod: labels (head_pod_labels)
• Head Service:
  • selector: default_labels (If KubernetesApplicationNameLabelKey is defined by head_pod_labels, overwrite the default value.)
  • labels: default_labels (If KubernetesApplicationNameLabelKey is defined by head_pod_labels, overwrite the default value.)
    

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

    Lines 309 to 312 in 2600854

    	labels := make(map[string]string)
    	if val, ok := instance.Spec.HeadGroupSpec.Template.ObjectMeta.Labels[common.KubernetesApplicationNameLabelKey]; ok {
    	labels[common.KubernetesApplicationNameLabelKey] = val
    	}

Current status of this PR

• Head Pod: labels (head_pod_labels)
• Head Service:
  • selector: default_labels (If KubernetesApplicationNameLabelKey is defined by head_pod_labels, overwrite the default value.)
  • labels: default_labels (If KubernetesApplicationNameLabelKey is defined by head_pod_labels, overwrite the default value.) + custom_svc_labels

Need to discuss

• Q: Does HeadService.labels need to contain default_labels?
  • For me, it seems unnecessary.
• Q: How to handle overlaps between head_pod_labels, default_labels, and custom_svc_labels?
  • In my opinion, for HeadService.selector, default_labels should always respect head_pod_labels and ignore custom_svc_labels to avoid a mismatch between label and selector. Currently, we only respect head_pod_labels when the key is KubernetesApplicationNameLabelKey.

My current thought

• Head Pod: labels (head_pod_labels)
• Head Service:
  • selector: default_labels (if overlaps exist, respect head_pod_labels and ignore custom_svc_labels.)
  • labels: custom_svc_labels

cc @DmitriGekhtman @architkulkarni @Jeffwan for more inputs.

[Jeffwan]

I feel we should not update CRD for this kind of change. Let's change to use annotations instead?? This won't be scalable and will constantly introduce more api changes. WDYT?

[Jeffwan]

Let' have a short discussion on future api changes?

[DmitriGekhtman]

I feel we should not update CRD for this kind of change. Let's change to use annotations instead?? This won't be scalable and will constantly introduce more api changes. WDYT?

The request in this case was specifically to expose head service labels (we didn't ask what for)

[kevin85421]

@Jeffwan

I followed up with the user on slack. In his words, "just FYI I'm not actively working on this, but the original need was due to use of labels for cross-cluster service discovery".

Let's change to use annotations instead??

It seems annotations will not work in this case.
"Labels can be used to select objects and to find collections of objects that satisfy certain conditions. In contrast, annotations are not used to identify and select objects." (Kubernetes official doc)

This won't be scalable and will constantly introduce more api changes. WDYT?

• We have already exposed custom annotations for the Head service ([RayCluster controller] Add headServiceAnnotations field to RayCluster CR #841).
• I think we should expose custom labels for the Head service due to the following reasons:
  • Keep consistency with the custom annotations for the Head service.
  • We cannot directly copy the labels to the Head service, and we will check the labels and attach new labels for the Head service. You can see the discussion about label overlaps above.
• We plan to expose the entire Head service except for some fields, including selector, annotations, labels, ... etc.
  • [Feature] Expose loadBalancerIP for head service to users #877 (comment)
  • With this, we do not need to introduce any CRD update for the Head service.

[Jeffwan]

@kevin85421 @Yicheng-Lu-llll sorry for late response. I am kind of overwhelmed by internal stuff.

For this api change, I feel we can make it with next API version like v1beta1. I will double check your notes and come back to you soon

[kevin85421]

@Jeffwan no worries. We may close this PR in favor of exposing the entire service (#877 (comment)) based on community sync on Jan. 31 2023.