[RayJob] Enable job log streaming by setting `PYTHONUNBUFFERED` in job container #1375

architkulkarni · 2023-08-29T23:08:37Z

Why are these changes needed?

Previously, kubectl get logs job/my-rayjob and kubectl logs my-rayjob-xxxxx (getting the logs from the pod directly) would be empty until the job finished.

This PR sets PYTHONUNBUFFERED in the job container so that the logs can be viewed while the job is still running.

This improves the log viewing experience to match that of ordinary Kubernetes Jobs.

This PR adds unit tests, but adding an end-to-end test (e.g. submit a job that prints a log and then sleeps forever, and check that we can read the log) will require some followup work. The main issue is the following:

The RayJob sample YAML test framework waits for a job to "converge" (actually, to be SUCCEEDED) before running any Rules. But for this test, we just want the job to start before running the Rule.

Related issue number

Closes #1374

Checks

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

Signed-off-by: Archit Kulkarni <[email protected]>

This reverts commit a35a0d5.

This reverts commit 89f6d62.

Signed-off-by: Archit Kulkarni <[email protected]>

This reverts commit 4ebe88e.

kevin85421 · 2023-08-30T00:37:54Z

ray-operator/controllers/ray/rayjob_controller.go

@@ -375,6 +376,26 @@ func (r *RayJobReconciler) getSubmitterTemplate(rayJobInstance *rayv1alpha1.RayJ
 		r.Log.Info("User-provided command is used", "command", submitterTemplate.Spec.Containers[0].Command)
 	}

+	// Set PYTHONUNBUFFERED=1 for real-time logging, unless user already set it.


We can reuse envVarExists.

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

Line 670 in aa17363

func envVarExists(envName string, envVars []v1.EnvVar) bool {

kevin85421 · 2023-08-30T00:39:12Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

+	assert.NoError(t, err)
+	pythonUnbufferedSet := false
+	for _, envVar := range submitterTemplate.Spec.Containers[0].Env {
+		if envVar.Name == "PYTHONUNBUFFERED" {


Use PythonUnbufferedEnvVarName instead?

kevin85421 · 2023-08-30T00:40:12Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

+	userValue := "0"
+	rayJobInstanceWithTemplate.Spec.SubmitterPodTemplate.Spec.Containers[0].Env = []corev1.EnvVar{
+		{
+			Name:  "PYTHONUNBUFFERED",


Use PythonUnbufferedEnvVarName instead?

kevin85421 · 2023-08-30T00:40:33Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

+	submitterTemplate, err = r.getSubmitterTemplate(rayJobInstanceWithTemplate)
+	assert.NoError(t, err)
+	for _, envVar := range submitterTemplate.Spec.Containers[0].Env {
+		if envVar.Name == "PYTHONUNBUFFERED" {


Use PythonUnbufferedEnvVarName instead?

kevin85421 · 2023-08-30T00:41:28Z

ray-operator/controllers/ray/rayjob_controller_unit_test.go

+	submitterTemplate, err = r.getSubmitterTemplate(rayJobInstanceWithTemplate)
+	assert.NoError(t, err)
+	for _, envVar := range submitterTemplate.Spec.Containers[0].Env {
+		if envVar.Name == "PYTHONUNBUFFERED" {


It is possible for the test to pass without entering this if statement.

Good catch!

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-08-30T17:20:36Z

@kevin85421 Comments have been addressed, please take another look!

kevin85421 · 2023-08-30T17:36:40Z

ray-operator/controllers/ray/rayjob_controller.go

+	// Set PYTHONUNBUFFERED=1 for real-time logging, unless user already set it.
+	userProvidedValue := ""
+	if common.EnvVarExists(PythonUnbufferedEnvVarName, submitterTemplate.Spec.Containers[0].Env) {
+		for _, envVar := range submitterTemplate.Spec.Containers[0].Env {


It is a bit weird to iterate through the loop again after calling EnvVarExists.

We may consider returning bool, EnvVar.

kevin85421 · 2023-08-30T17:46:21Z

ray-operator/controllers/ray/rayjob_controller.go

+	if common.EnvVarExists(PythonUnbufferedEnvVarName, submitterTemplate.Spec.Containers[0].Env) {
+		for _, envVar := range submitterTemplate.Spec.Containers[0].Env {
+			if envVar.Name == PythonUnbufferedEnvVarName {
+				userProvidedValue = envVar.Value


The logic doesn't seem entirely correct to me, especially in cases where users can use ValueFrom instead of Value.

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni · 2023-08-31T23:05:57Z

@kevin85421 updated the PR to not check if the env var was already set, as discussed offline, because we don't anticipate any use case for PYTHONUNBUFFERED=0.

kevin85421

Maybe we can use submitterTemplate.Spec.Containers[common.RayContainerIndex] to replace submitterTemplate.Spec.Containers[0]. You can either update it in the PR or open an issue as a good first issue.

kevin85421 · 2023-09-05T18:50:03Z

ray-operator/controllers/ray/rayjob_controller.go

@@ -375,6 +376,12 @@ func (r *RayJobReconciler) getSubmitterTemplate(rayJobInstance *rayv1alpha1.RayJ
 		r.Log.Info("User-provided command is used", "command", submitterTemplate.Spec.Containers[0].Command)
 	}

+	// Set PYTHONUNBUFFERED=1 for real-time logging
+	submitterTemplate.Spec.Containers[0].Env = append(submitterTemplate.Spec.Containers[0].Env, v1.EnvVar{


Nit: What will happen if users specify PYTHONUNBUFFERED on their own? I am not sure whether defining the same env variable twice in a container will throw an error message or override the first value.

It seems the second definition will override the first one: https://www.baeldung.com/linux/kubernetes-pod-environment-variables

…fix-job-log-streaming

Signed-off-by: Archit Kulkarni <[email protected]>

kevin85421 · 2023-09-06T05:09:34Z

Maybe we can use submitterTemplate.Spec.Containers[common.RayContainerIndex] to replace submitterTemplate.Spec.Containers[0]. You can either update it in the PR or open an issue as a good first issue.

Open a good first issue #1397.

…b container (ray-project#1375) Enable job log streaming by setting `PYTHONUNBUFFERED` in job container

architkulkarni added 4 commits August 29, 2023 10:44

Add gke bucket yaml

a35a0d5

Signed-off-by: Archit Kulkarni <[email protected]>

Remove stray TODO

89f6d62

Signed-off-by: Archit Kulkarni <[email protected]>

Skip test

4ebe88e

Signed-off-by: Archit Kulkarni <[email protected]>

Set PYTHONUNBUFFERED in RayJob

bd81788

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni requested a review from kevin85421 August 29, 2023 23:08

architkulkarni added 4 commits August 29, 2023 16:09

Revert "Add gke bucket yaml"

fdc737a

This reverts commit a35a0d5.

Revert "Remove stray TODO"

2b63122

This reverts commit 89f6d62.

gofumpt

6e76a9a

Signed-off-by: Archit Kulkarni <[email protected]>

Revert "Skip test"

8b8fa2e

This reverts commit 4ebe88e.

kevin85421 reviewed Aug 30, 2023

View reviewed changes

Address comments

1e296ea

Signed-off-by: Archit Kulkarni <[email protected]>

kevin85421 reviewed Aug 30, 2023

View reviewed changes

Undo check for if the env var exists

95616ad

Signed-off-by: Archit Kulkarni <[email protected]>

architkulkarni requested a review from kevin85421 September 5, 2023 17:07

kevin85421 reviewed Sep 5, 2023

View reviewed changes

kevin85421 approved these changes Sep 5, 2023

View reviewed changes

architkulkarni added 3 commits September 5, 2023 12:44

Merge branch 'master' of https://github.com/ray-project/kuberay into …

0e2b6dd

…fix-job-log-streaming

Use RayContainerIndex

48fd324

Signed-off-by: Archit Kulkarni <[email protected]>

Fix typo

59e4fc5

Signed-off-by: Archit Kulkarni <[email protected]>

kevin85421 mentioned this pull request Sep 6, 2023

[Feature] Use RayContainerIndex instead of 0 #1397

Closed

2 tasks

kevin85421 merged commit 9382c1f into ray-project:master Sep 6, 2023
15 checks passed

z103cb pushed a commit to z103cb/kuberay that referenced this pull request Sep 11, 2023

[RayJob] Enable job log streaming by setting PYTHONUNBUFFERED in jo…

93c9c78

…b container (ray-project#1375) Enable job log streaming by setting `PYTHONUNBUFFERED` in job container

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

[RayJob] Enable job log streaming by setting PYTHONUNBUFFERED in jo…

71242ba

…b container (ray-project#1375) Enable job log streaming by setting `PYTHONUNBUFFERED` in job container

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RayJob] Enable job log streaming by setting `PYTHONUNBUFFERED` in job container #1375

[RayJob] Enable job log streaming by setting `PYTHONUNBUFFERED` in job container #1375

architkulkarni commented Aug 29, 2023 •

edited

Loading

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

architkulkarni Aug 30, 2023

architkulkarni commented Aug 30, 2023

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

kevin85421 Aug 30, 2023

architkulkarni commented Aug 31, 2023

kevin85421 left a comment

kevin85421 Sep 5, 2023

architkulkarni Sep 5, 2023

kevin85421 commented Sep 6, 2023

[RayJob] Enable job log streaming by setting PYTHONUNBUFFERED in job container #1375

[RayJob] Enable job log streaming by setting PYTHONUNBUFFERED in job container #1375

Conversation

architkulkarni commented Aug 29, 2023 • edited Loading

Why are these changes needed?

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

architkulkarni commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

architkulkarni commented Aug 31, 2023

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

kevin85421 commented Sep 6, 2023

[RayJob] Enable job log streaming by setting `PYTHONUNBUFFERED` in job container #1375

[RayJob] Enable job log streaming by setting `PYTHONUNBUFFERED` in job container #1375

architkulkarni commented Aug 29, 2023 •

edited

Loading