Add `ray.io/originated-from` labels #1830

rueian · 2024-01-11T14:01:41Z

Why are these changes needed?

Previously, we didn't have a label for querying resources, such as the underlying ray cluster, created by a RayJob, while we had the ray.io/service label for querying resources created by a RayService.

We could introduce a new ray.io/job label for this purpose, and that would result in having three labels that look similar:

ray.io/job
ray.io/service
ray.io/cluster

However, the current usage of the ray.io/cluster label differs greatly from the first two.

After a discussion with @kevin85421, we think we could clarify the usage of the first two by replacing them with the following labels:

ray.io/originated-from-name
ray.io/originated-from-type

Examples:

# rayjob
ray.io/originated-from-name: myjob
ray.io/originated-from-type: rayjob
# rayservice
ray.io/originated-from-name: myservice
ray.io/originated-from-type: rayservice

To summarize, this PR removes the existing ray.io/service label and introduces ray.io/originated-from-name and ray.io/originated-from-type for both clarifying and unifying how to query resources created by a RayJob or a RayService.

Additionally, to make us more confident that ray.io/service is removable, I have searched its usage on GitHub and found that no one else uses this label.

Related issue number

Checks

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

andrewsykim · 2024-01-11T15:58:07Z

apiserver/pkg/util/config.go

@@ -13,7 +13,8 @@ const (
 	RayClusterUserLabelKey            = "ray.io/user"
 	RayClusterVersionLabelKey         = "ray.io/version"
 	RayClusterEnvironmentLabelKey     = "ray.io/environment"
-	RayServiceLabelKey                = "ray.io/service"
+	RayOriginatedFromNameLabelKey     = "ray.io/originated-from-name"


ray.io/owner-name might be better?

We have considered using owner-name. The question is that RayJob/RayService owns RayCluster, and RayCluster owns Ray Pods. We may need to associate the Ray Pods from both the RayJob/RayService level and the RayCluster level.

We don't have a good idea for the label name to represent the "root" of the owner/dependent tree. Do you have any good ideas?

We don't have a good idea for the label name to represent the "root" of the owner/dependent tree. Do you have any good ideas?

Would it be simpler to just have a single label ray.io/originiated-from that contains both the type and name?

ray.io/originated-from: rayjob/myjob ray.io/originated-from: raycluster/mycluster ray.io/originated-from: rayservice/myservice

It sounds good to me. cc @rueian WDYT?

That is a great idea. However, k8s will reject / in label value with an error like this:

metadata.labels: Invalid value: "rayjob/myjob": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')

How about using . instead?

ray.io/originated-from: rayjob.myjob ray.io/originated-from: raycluster.mycluster ray.io/originated-from: rayservice.myservice

Is it possible for CR name to have .? If it is impossible, using . makes sense to me.

Unfortunately, . is allowed in resource names, but _ is not.
So we are safe to use the following:

ray.io/originated-from: rayjob_myjob ray.io/originated-from: raycluster_mycluster ray.io/originated-from: rayservice_myservice

Does this sound good to you @andrewsykim @kevin85421?

Unfortunately, . is allowed in resource names, but _ is not.

The use of _ sounds good to me. What's the regex for resource names?

This one [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*

Source: https://github.com/kubernetes/kubernetes/blob/29bf09136ea034198f54d62e97e0d6f3af0722c3/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L205

andrewsykim · 2024-01-11T15:58:15Z

apiserver/pkg/util/config.go

@@ -13,7 +13,8 @@ const (
 	RayClusterUserLabelKey            = "ray.io/user"
 	RayClusterVersionLabelKey         = "ray.io/version"
 	RayClusterEnvironmentLabelKey     = "ray.io/environment"
-	RayServiceLabelKey                = "ray.io/service"
+	RayOriginatedFromNameLabelKey     = "ray.io/originated-from-name"
+	RayOriginatedFromTypeLabelKey     = "ray.io/originated-from-type"


ray.io/owner-resource?

As @kevin85421 mentioned, we prefer not to use the term owner as it frequently sets incorrect expectations.

kevin85421 · 2024-01-17T01:44:11Z

ray-operator/controllers/ray/utils/constant.go

@@ -75,7 +75,10 @@ const (
 	ComponentName = "kuberay-operator"

 	// The defaule RayService Identifier.


Could we use CRDType instead?

kuberay/ray-operator/controllers/ray/utils/util.go

Line 36 in 1634d70

type CRDType string

ray-operator/controllers/ray/utils/constant.go

apiserver/pkg/util/config.go

ray-operator/controllers/ray/utils/constant.go

ray-operator/controllers/ray/rayjob_controller.go

kevin85421 · 2024-01-18T17:43:52Z

@rueian could you rebase with the master branch? Thanks!

rueian · 2024-01-19T13:27:14Z

@rueian could you rebase with the master branch? Thanks!

Hi @kevin85421, rebased!

kevin85421

Could you summarize the current status of the ray.io/originated-from label? For instance, which Kubernetes resources will have this label if I create a RayService, RayJob, or RayCluster at this moment?
The tests are scattered across multiple locations. In the future, we may need to devise a better method for testing label propagations if we find them hard to maintain.

kevin85421 · 2024-01-19T18:42:18Z

ray-operator/controllers/ray/utils/constant.go

@@ -154,3 +155,16 @@ const (
 	HeadService    ServiceType = "headService"
 	ServingService ServiceType = "serveService"
 )
+
+// RayOriginatedFromServiceLabelValue generates a RayService value for the label RayOriginatedFromLabelKey


Will it be better if we unify the two functions RayOriginatedFromServiceLabelValue and RayOriginatedFromJobLabelValue into a single function?

func FUNC_NAME(crdType CRDType, crName string) string { return string(crdType) + "_" + crName }

rueian · 2024-01-22T16:01:12Z

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

-// BuildRouteForRayService Builds the route for head service dashboard for RayService.
-// This is used to expose dashboard for external traffic.
-// RayService controller updates the ingress whenever a new RayCluster serves the traffic.
-func BuildRouteForRayService(service rayv1.RayService, cluster rayv1.RayCluster) (*routev1.Route, error) {


This function is not used anywhere.

apiserver/pkg/util/service.go

rueian · 2024-01-22T16:03:18Z

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

@@ -589,7 +589,7 @@ func setContainerEnvVars(pod *corev1.Pod, rayNodeType rayv1.RayNodeType, rayStar
 		container.Env = append(container.Env, portEnv)
 	}

-	if strings.ToLower(creator) == utils.RayServiceCreatorLabelValue {
+	if strings.EqualFold(creator, string(utils.RayServiceCRD)) {


EqualFold reports whether s and t, interpreted as UTF-8 strings, are equal under simple Unicode case-folding, which is a more general form of case-insensitity.

This makes the linter happy.

rueian · 2024-01-22T16:08:19Z

ray-operator/controllers/ray/utils/constant.go

+	// RayOriginatedFromLabelKey is the label used to associate the root KubeRay Custom Resource.
+	// For example, if a RayCluster is created by a RayJob named myjob, then the cluster will have
+	// A ray.io/originated-from=RayService_mysvc label will be added to the following resources if they are originated from a RayService mysvc.
+	//   * Kubernetes Services
+	//   * RayClusters
+	// A ray.io/originated-from=RayJob_myjob label will be added to the following resources if they are originated from a RayJob myjob.
+	//   * Kubernetes Jobs
+	//   * RayClusters


Bulletin where the ray.io/originated-from will show up.

kevin85421 · 2024-01-23T01:47:33Z

Would you mind rebasing with the master branch? Thanks!

…s created by RayJob

…from-*` labels on k8s resources created by RayService

…from` labels on k8s resources created by RayService

…inated-from

kevin85421

I still have some comments for this PR, but since it has been pending for a while, I'll merge it now and immediately open a follow-up PR to address those comments.

rueian changed the title ~~Originated labels~~ Add ray.io/originated-from-name and ray.io/originated-from-type labels Jan 11, 2024

rueian force-pushed the originated-labels branch from 9caa2d8 to c3e0a56 Compare January 11, 2024 14:58

andrewsykim reviewed Jan 11, 2024

View reviewed changes

kevin85421 self-assigned this Jan 11, 2024

rueian force-pushed the originated-labels branch 5 times, most recently from 562455c to 68e76ea Compare January 14, 2024 11:14

rueian changed the title ~~Add ray.io/originated-from-name and ray.io/originated-from-type labels~~ Add ray.io/originated-from labels Jan 15, 2024

kevin85421 reviewed Jan 17, 2024

View reviewed changes

rueian force-pushed the originated-labels branch 2 times, most recently from bcc974e to 9714100 Compare January 17, 2024 14:52

andrewsykim reviewed Jan 17, 2024

View reviewed changes

ray-operator/controllers/ray/rayjob_controller.go Show resolved Hide resolved

rueian force-pushed the originated-labels branch from 9714100 to 53cc42c Compare January 19, 2024 00:16

kevin85421 reviewed Jan 19, 2024

View reviewed changes

rueian commented Jan 22, 2024

View reviewed changes

rueian added 5 commits January 23, 2024 19:37

[RayJob][Label] Add ray.io/originated-from-* labels to k8s resource…

0cc15d3

…s created by RayJob

[RayService][Label] Replace ray.io/service with `ray.io/originated-…

beb7f3f

…from-*` labels on k8s resources created by RayService

[RayService][Label] Replace ray.io/service with `ray.io/originated-…

03f0212

…from` labels on k8s resources created by RayService

[RayService][Label] Add comments to clarify the usages of ray.io/orig…

b218d1a

…inated-from

[RayService][Label] Add comments to clarify the usages of ray.io/orig…

780abbe

…inated-from

rueian force-pushed the originated-labels branch from 90b0a20 to 780abbe Compare January 23, 2024 11:38

kevin85421 approved these changes Jan 23, 2024

View reviewed changes

kevin85421 merged commit f05fa2e into ray-project:master Jan 23, 2024
18 of 24 checks passed

kevin85421 mentioned this pull request Jan 24, 2024

[Feature] Split ray.io/originated-from into ray.io/originated-from-cr-name and ray.io/originated-from-crd #1864

Merged

4 tasks

ryanaoleary pushed a commit to ryanaoleary/kuberay that referenced this pull request Feb 13, 2024

Add ray.io/originated-from labels (ray-project#1830)

6d67197

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ray.io/originated-from` labels #1830

Add `ray.io/originated-from` labels #1830

rueian commented Jan 11, 2024 •

edited

andrewsykim Jan 11, 2024

kevin85421 Jan 11, 2024

andrewsykim Jan 12, 2024 •

edited

kevin85421 Jan 12, 2024

rueian Jan 13, 2024 •

edited

kevin85421 Jan 14, 2024

rueian Jan 14, 2024 •

edited

kevin85421 Jan 15, 2024

rueian Jan 15, 2024 •

edited

andrewsykim Jan 11, 2024

rueian Jan 12, 2024

kevin85421 Jan 17, 2024

rueian Jan 17, 2024

kevin85421 commented Jan 18, 2024

rueian commented Jan 19, 2024

kevin85421 left a comment

kevin85421 Jan 19, 2024

rueian Jan 22, 2024

rueian Jan 22, 2024

rueian Jan 22, 2024

kevin85421 commented Jan 23, 2024

kevin85421 left a comment

		@@ -75,7 +75,10 @@ const (
		ComponentName = "kuberay-operator"

		// The defaule RayService Identifier.

Add ray.io/originated-from labels #1830

Add ray.io/originated-from labels #1830

Conversation

rueian commented Jan 11, 2024 • edited

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim Jan 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Jan 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Jan 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rueian Jan 15, 2024 • edited

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 Jan 18, 2024

rueian commented Jan 19, 2024

kevin85421 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

Choose a reason for hiding this comment

kevin85421 commented Jan 23, 2024

kevin85421 left a comment

Choose a reason for hiding this comment

Add `ray.io/originated-from` labels #1830

Add `ray.io/originated-from` labels #1830

rueian commented Jan 11, 2024 •

edited

andrewsykim Jan 12, 2024 •

edited

rueian Jan 13, 2024 •

edited

rueian Jan 14, 2024 •

edited

rueian Jan 15, 2024 •

edited