Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix examples: bad K8s log config causing logs to be lost #501

Merged
merged 1 commit into from
Sep 1, 2022

Conversation

davidxia
Copy link
Contributor

across Container restarts

Ray logs are written to /tmp/ray by default. Right now
various RayCluster example YAMLs use volumeMounts for /var/log
which doesn't have anything written to it.

So right now /tmp/ray logs are deleted after Container restarts.

This change sets volumeMounts.mountPath to /tmp/ray so that
logs in this dir are persisted across Container restarts for the life
of the Pod.

This change also adds volumes for head Pods which was missing before.

Checks

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

across Container restarts

Ray logs are written to `/tmp/ray` by default. Right now
various RayCluster example YAMLs use `volumeMounts` for `/var/log`
which doesn't have anything written to it.

So right now `/tmp/ray` logs are deleted after Container restarts.

This change sets `volumeMounts.mountPath` to `/tmp/ray` so that
logs in this dir are persisted across Container restarts for the life
of the Pod.

This change also adds volumes for head Pods which was missing before.
@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Aug 31, 2022

Thanks for the fix! This looks good to me.
@davidxia just to confirm, is this PR ready for review? If so, could you click the button that marks it as such? :)
(It's currently marked as a draft.)

@davidxia davidxia marked this pull request as ready for review August 31, 2022 20:22
@davidxia
Copy link
Contributor Author

thanks, Moved out of draft. I wanted to add some tests for this. Do you think that'd be helpful? I can do that in another PR if so.

@DmitriGekhtman
Copy link
Collaborator

Tests are always appreciated! What kind of testing did you have in mind?

@davidxia
Copy link
Contributor Author

That the ray logs dir /tmp/ray is mounted in these example YAMLs. Are there existing tests for these example YAMLs that check a certain value is used correctly?

Also is /tmp/ray in some constant that a test can use?

@DmitriGekhtman
Copy link
Collaborator

DmitriGekhtman commented Aug 31, 2022

That the ray logs dir /tmp/ray is mounted in these example YAMLs. Are there existing tests for these example YAMLs that check a certain value is used correctly?

I don't think we systematically test the sample YAMLs, but this would be an excellent opportunity to start doing that.
(At the moment, it looks like all of the tests define their own RayCluster structs, rather than reading from the files.)
Just validating the volumes/mounts in the examples would be great.

Also is /tmp/ray in some constant that a test can use?

Looks like we added that here:

RayLogVolumeMountPath = "/tmp/ray"

@davidxia
Copy link
Contributor Author

davidxia commented Sep 1, 2022

Thanks, feel free to merge this PR first to unblock. I'll try to write tests for this soon.

@DmitriGekhtman DmitriGekhtman merged commit 3e789e3 into ray-project:master Sep 1, 2022
@davidxia davidxia deleted the fix-volume-mounts branch September 1, 2022 20:49
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…#501)

across Container restarts

Ray logs are written to `/tmp/ray` by default. Right now
various RayCluster example YAMLs use `volumeMounts` for `/var/log`
which doesn't have anything written to it.

So right now `/tmp/ray` logs are deleted after Container restarts.

This change sets `volumeMounts.mountPath` to `/tmp/ray` so that
logs in this dir are persisted across Container restarts for the life
of the Pod.

This change also adds volumes for head Pods which was missing before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants