RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates #1852

andrewsykim · 2024-01-19T02:49:03Z

Why are these changes needed?

See #1850 for details.

Related issue number

Closes #1850

Checks

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

kevin85421

Would you mind also updating the following code snippet to use FQ_RAY_IP for submitting a Ray job? Thank you!

kuberay/ray-operator/config/samples/ray_v1alpha1_rayjob.yaml

Lines 95 to 105 in 33f29f6

    
           # SubmitterPodTemplate is the template for the pod that will run the `ray job submit` command against the RayCluster. 
        
           # If SubmitterPodTemplate is specified, the first container is assumed to be the submitter container. 
        
           # submitterPodTemplate: 
        
           #   spec: 
        
           #     restartPolicy: Never 
        
           #     containers: 
        
           #       - name: my-custom-rayjob-submitter-pod 
        
           #         image: rayproject/ray:2.9.0 
        
           #         # If Command is not specified, the correct command will be supplied at runtime using the RayJob spec `entrypoint` field. 
        
           #         # Specifying Command is not recommended. 
        
           #         # command: ["ray job submit --address=http://rayjob-sample-raycluster-v6qcq-head-svc.default.svc.cluster.local:8265 -- echo hello world"]

kevin85421 · 2024-01-19T19:05:16Z

ray-operator/controllers/ray/rayjob_controller.go

@@ -327,6 +327,18 @@ func (r *RayJobReconciler) getSubmitterTemplate(rayJobInstance *rayv1.RayJob, ra
 		r.Log.Info("default submitter template is used")
 	} else {
 		submitterTemplate = *rayJobInstance.Spec.SubmitterPodTemplate.DeepCopy()
+


How about moving the logic to the same location as the PYTHONUNBUFFERED environment variable (code)?

We can always inject the env variable no matter whether users set SubmitterPodTemplate by themselves or not.

We can only inject FQ_RAY_IP into the Ray container, which is always the first container (i.e., submitterTemplate.Spec.Containers[utils.RayContainerIndex]) in a Pod in KubeRay. We can inject it into other sidecar containers later upon user request.

Do we need to check len(container.Env) == 0? Can we directly append to the slice instead?

Thanks, I updated to always inject the environment variable. I wasn't sure if it made sense to inject the env var in the default submitter template.

kevin85421 · 2024-01-19T19:09:56Z

Btw, you can add "Closes #1850" in the PR description so that the issue will be closed when the PR is merged.

andrewsykim · 2024-01-22T16:29:14Z

Updated sample manifest to use FQ_RAY_IP

kevin85421 · 2024-01-22T17:32:10Z

ray-operator/config/samples/ray_v1alpha1_rayjob.yaml

@@ -102,7 +102,7 @@ spec:
  #         image: rayproject/ray:2.9.0
  #         # If Command is not specified, the correct command will be supplied at runtime using the RayJob spec `entrypoint` field.
  #         # Specifying Command is not recommended.
-  #         # command: ["ray job submit --address=http://rayjob-sample-raycluster-v6qcq-head-svc.default.svc.cluster.local:8265 -- echo hello world"]
+  #         # command: ["ray job submit --address=http://$FQ_RAY_IP:8265 -- echo hello world"]


Would you mind trying whether it actually works or not? I forgot why I divided the command into a slice of multiple strings instead of a slice with just one string in rayjob_test.go.

The env injection works, but the original entrypoint was invalid. Updated to this:

command: ["sh", "-c", "ray job submit --address=http://$FQ_RAY_IP -- echo hello world"]

kevin85421 · 2024-01-22T17:35:54Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

+	submitterTemplate, err = r.getSubmitterTemplate(rayJobInstanceWithTemplate, nil)
+	assert.NoError(t, err)
+	found = false
+	for _, envVar := range submitterTemplate.Spec.Containers[utils.RayContainerIndex].Env {


Maybe we can make the function envVarExists public and reuse it? In the future, we can consider moving the function to ray-operator/utils instead.

Can I make this change in a follow-up PR? There are other tests and assertions that I would have to refactor

Sure. Would you mind opening an issue to track the progress? Thanks!

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

Would you mind rebasing with the master branch? Thanks!

andrewsykim · 2024-01-23T02:31:12Z

Btw, FQ_RAY_IP is a bit misleading because Dashboard URL in the status also contains the port. Something like FQ_RAY_HOST might be better but I won't try to fix that in this PR

kevin85421 · 2024-01-23T05:42:55Z

Btw, FQ_RAY_IP is a bit misleading because Dashboard URL in the status also contains the port. Something like FQ_RAY_HOST might be better but I won't try to fix that in this PR

Is there any reason not to address the issue in this PR? If it doesn't involve changes unrelated to this PR, I would prefer to resolve it here.

andrewsykim · 2024-01-23T14:52:12Z

Is there any reason not to address the issue in this PR? If it doesn't involve changes unrelated to this PR, I would prefer to resolve it here.

I didn't want to change other references to FQ_RAY_IP. But thinking about it more, can we just use RAY_ADDRESS instead which usually contains the port?

andrewsykim · 2024-01-23T16:12:50Z

@kevin85421 I updated the implementation to use RAY_ADDRESS, which means the submitter entrypoint doesn't even need to specify --address anymore

kevin85421 · 2024-01-23T16:24:48Z

can we just use RAY_ADDRESS instead which usually contains the port?

RAY_ADDRESS is set to $FQ_RAY_IP:$GCS_PORT, but DashboardURL is set to $FQ_RAY_IP:$DASHBOARD_PORT. Maybe we should use a different name?

andrewsykim · 2024-01-23T17:09:37Z

How about RAY_DASHBOARD_ADDRESS?

kevin85421 · 2024-01-23T18:24:36Z

ray-operator/controllers/ray/rayjob_controller.go

@@ -348,6 +348,12 @@ func (r *RayJobReconciler) getSubmitterTemplate(rayJobInstance *rayv1.RayJob, ra
 		Value: "1",
 	})

+	// Set RAY_ADDRESS so that the Ray address can be discovered at runtime


Update the comment.

kevin85421 · 2024-01-23T18:24:54Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

@@ -170,6 +170,19 @@ func TestGetSubmitterTemplate(t *testing.T) {
 		}
 	}
 	assert.True(t, found)
+
+	// Test 5: Check default FQ_RAY_IP env var


Update the comment.

kevin85421 · 2024-01-25T00:11:27Z

There is a conflict. Would you mind rebasing with the master branch? Thanks!

…ter templates Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

…vided submiter templates (ray-project#1852)

andrewsykim mentioned this pull request Jan 19, 2024

[Feature][RayJob] Set the environment variable FQ_RAY_IP for the submitter Kubernetes Job #1850

Closed

2 tasks

kevin85421 reviewed Jan 19, 2024

View reviewed changes

kevin85421 self-assigned this Jan 19, 2024

andrewsykim force-pushed the 1850 branch 2 times, most recently from 9bb7a79 to 032a052 Compare January 22, 2024 16:20

kevin85421 reviewed Jan 22, 2024

View reviewed changes

andrewsykim force-pushed the 1850 branch from 032a052 to 32e4ac2 Compare January 23, 2024 02:18

andrewsykim force-pushed the 1850 branch from 32e4ac2 to 2fe3917 Compare January 23, 2024 16:12

andrewsykim force-pushed the 1850 branch from 2fe3917 to 986c769 Compare January 23, 2024 17:18

kevin85421 approved these changes Jan 23, 2024

View reviewed changes

andrewsykim force-pushed the 1850 branch from 986c769 to 0222d8c Compare January 23, 2024 18:48

andrewsykim changed the title ~~RayJob: inject FQ_RAY_IP envariable variable for user provided submiter templates~~ RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates Jan 23, 2024

andrewsykim force-pushed the 1850 branch 2 times, most recently from 156433c to 635ed70 Compare January 24, 2024 01:54

andrewsykim added 2 commits January 25, 2024 15:31

RayJob: inject FQ_RAY_IP envariable variable for user provided submit…

96a53ab

…ter templates Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

use RAY_DASHBOARD_ADDRESS instead

e789779

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>

andrewsykim force-pushed the 1850 branch from 635ed70 to e789779 Compare January 25, 2024 15:31

kevin85421 merged commit 0216b33 into ray-project:master Jan 25, 2024
22 of 23 checks passed

kevin85421 mentioned this pull request Jan 26, 2024

[RayJob] Inject RAY_SUBMISSION_ID env variable for user provided submitter template #1868

Merged

4 tasks

andrewsykim mentioned this pull request Jan 26, 2024

rayjob controller: refactor environment variable check in unit tests #1870

Merged

4 tasks

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

RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user pro…

9ef3a65

…vided submiter templates (ray-project#1852)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates #1852

RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates #1852

andrewsykim commented Jan 19, 2024 •

edited

kevin85421 left a comment

kevin85421 Jan 19, 2024 •

edited

andrewsykim Jan 22, 2024

kevin85421 commented Jan 19, 2024

andrewsykim commented Jan 22, 2024

kevin85421 Jan 22, 2024

andrewsykim Jan 23, 2024

kevin85421 Jan 22, 2024

andrewsykim Jan 22, 2024

kevin85421 Jan 22, 2024

kevin85421 commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

kevin85421 commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

kevin85421 commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

kevin85421 Jan 23, 2024

andrewsykim Jan 23, 2024

kevin85421 Jan 23, 2024

andrewsykim Jan 23, 2024

kevin85421 commented Jan 25, 2024

	# SubmitterPodTemplate is the template for the pod that will run the `ray job submit` command against the RayCluster.
	# If SubmitterPodTemplate is specified, the first container is assumed to be the submitter container.
	# submitterPodTemplate:
	# spec:
	# restartPolicy: Never
	# containers:
	# - name: my-custom-rayjob-submitter-pod
	# image: rayproject/ray:2.9.0
	# # If Command is not specified, the correct command will be supplied at runtime using the RayJob spec `entrypoint` field.
	# # Specifying Command is not recommended.
	# # command: ["ray job submit --address=http://rayjob-sample-raycluster-v6qcq-head-svc.default.svc.cluster.local:8265 -- echo hello world"]

RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates #1852

RayJob: inject RAY_DASHBOARD_ADDRESS envariable variable for user provided submiter templates #1852

Conversation

andrewsykim commented Jan 19, 2024 • edited

Why are these changes needed?

Related issue number

Checks

kevin85421 left a comment

Choose a reason for hiding this comment

kevin85421 Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin85421 commented Jan 19, 2024

andrewsykim commented Jan 22, 2024

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

andrewsykim commented Jan 23, 2024

kevin85421 commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

kevin85421 commented Jan 23, 2024

andrewsykim commented Jan 23, 2024

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 25, 2024

andrewsykim commented Jan 19, 2024 •

edited

kevin85421 Jan 19, 2024 •

edited