-
Notifications
You must be signed in to change notification settings - Fork 656
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
Ray logs persistence #4266
Ray logs persistence #4266
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4266 +/- ##
==========================================
- Coverage 59.53% 59.30% -0.23%
==========================================
Files 632 544 -88
Lines 53527 38974 -14553
==========================================
- Hits 31867 23114 -8753
+ Misses 19145 13582 -5563
+ Partials 2515 2278 -237
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! after adding config options to control that behavior as you suggested...
Name: "system-ray-state", | ||
MountPath: "/tmp/ray", | ||
} | ||
primaryContainer.VolumeMounts = append(primaryContainer.VolumeMounts, writeableVolMount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block should be included in the if
block, right? otherwise it'll double add that volumemount if one exists...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea this is wrong. I think we want to use info from an existing volume / volume mount where possible. I'll clean up.
// Ray logs integration | ||
foundTmpRayVolMount := false | ||
for _, vm := range primaryContainer.VolumeMounts { | ||
if vm.MountPath == "/tmp/ray" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that path hardcoded in ray driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea /tmp/ray
is a fixed path. I'll make this a const.
Signed-off-by: Jeev B <jeevb@users.noreply.github.com>
Describe your changes
Injects a logging sidecar to tail and expose RayJob logs to the container's stdout, and in turn, to a cluster's logging infrastructure. This will allow job logs to be persisted to Cloudwatch, Stackdriver, or other storage backend for perusal at a later time.
Also adds support for specifying multiple containers in a pod template for Ray tasks.
Check all the applicable boxes
Screenshots
Got this working with the following
fluent-bit.conf
config:Note that the
stdout
output plugin could work here as well if structured logs are desired, but I went with just exposing logs as plaintext for now.Sidecar stdout looks as follows:
Note to reviewers