[Feature] Forward Kubernetes Node infrastructure events to Ray custom resources - #4978
[Feature] Forward Kubernetes Node infrastructure events to Ray custom resources#4978richabanker wants to merge 4 commits into
Conversation
Future-Outlier
left a comment
Future-Outlier
left a comment
There was a problem hiding this comment.
is it possible to have e2e test for this controller? tks!
dc2fd2b to
14e5384
Compare
0dd59fa to
8ec334d
Compare
@Future-Outlier yes, added! |
richabanker
changed the title
|
Failing build associated with unrelated changes addressed by #5084 |
8ec334d to
2ddbb71
Compare
|
cc @andrewsykim |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Hi @richabanker, could you rebase on or merge the master? The build error should be fixed then. |
2ddbb71 to
1c6bb3f
Compare
| | eventForwarder.enabled | bool | `false` | Whether KubeRay operator should forward Kubernetes Node events to Ray custom resources. | | ||
| | eventForwarder.sources | list | `[]` | Only forward Node events emitted by these components, matched against both source.component and reportingController. Empty means all sources. | | ||
| | eventForwarder.reasons | list | `[]` | Only forward Node events with these reasons, e.g. ["XIDError"]. Empty means all reasons. | | ||
| | eventForwarder.types | list | `[]` | Only forward Node events with these types. Valid values are "Warning" and "Normal". The operator refuses to start on any other value. Empty defaults to "Warning" only. | |
There was a problem hiding this comment.
shouldn't empty default to all types?
There was a problem hiding this comment.
Intentionally kept "Normal" outside of the default value, since that could lead to high event-volume (including the ones that may not even be critical enough for troubleshooting)
| @@ -10,3 +10,4 @@ spec: | |||
| - name: kuberay-operator | |||
| args: | |||
| - --feature-gates=RayClusterStatusConditions=true,RayJobDeletionPolicy=true,RayMultiHostIndexing=true,RayCronJob=true,RayServiceIncrementalUpgrade=true,SidecarSubmitterRestart=true,GCSFaultToleranceEmbeddedStorage=true,RayClusterMTLS=true | |||
There was a problem hiding this comment.
I think we still want a feature gate to indicate the maturity of the feature, WDYT?
There was a problem hiding this comment.
Good idea, added a gate while keeping the config as well, to support the future case of when the feature graduates to GA/stable, but we still want the node-event-forwarding to be optional. Does it make sense to have this dual gating for the feature ?
| where the operator is otherwise granted namespaced Roles only. Without it the | ||
| Event informer never syncs and the operator fails to start. | ||
| */ -}} | ||
| {{- if and .Values.rbacEnable .Values.eventForwarder.enabled }} |
There was a problem hiding this comment.
{{- if and .Values.rbacEnable .Values.eventForwarder.enabled .Values.singleNamespaceInstall }}
?
| t = e.EventTime.Time | ||
| } | ||
| if t.IsZero() { | ||
| t = e.CreationTimestamp.Time |
There was a problem hiding this comment.
what about e.FirstTimestamp?
There was a problem hiding this comment.
Good catch, added a guard for it as well, finally resorting to e.CreationTimestamp if t.IsZero() continues to be zero.
| {{- if .Values.eventForwarder.sources -}} | ||
| {{- $argList = append $argList (printf "--event-forwarder-sources=%s" (join "," .Values.eventForwarder.sources)) -}} | ||
| {{- end -}} | ||
| {{- if .Values.eventForwarder.reasons -}} |
There was a problem hiding this comment.
consider guarding the suboptions with if .Values.eventForwarder.enabled
|
cc @fscnick @AndySung320 @justinyeh1995 to take a look |
6604951 to
e6309c3
Compare
Signed-off-by: Richa Banker <richabanker@google.com>
eceb372 to
8cb9470
Compare
4c9e33f to
3c6eaba
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 3c6eaba. Configure here.
3c6eaba to
0611913
Compare
0611913 to
c24f928
Compare
|
|
||
| // NodeEventForwarderTypes restricts event forwarding to Node events of these types | ||
| // ("Warning", "Normal"). Empty defaults to "Warning" only. | ||
| NodeEventForwarderTypes []string `json:"nodeEventForwarderTypes,omitempty"` |
There was a problem hiding this comment.
While defaulting to Warning makes sense, it feels a bit counterintuitive since no value is explicitly passed in the argument, yet it still acts as a specific value. Additionally, this is inconsistent with the previous two arguments, where an empty value applies to all.
8 participants
Why are these changes needed?
This PR implements a Selective Event Forwarder controller to KubeRay operator. It surfaces critical hardware and infrastructure failures (such as GPU XID errors or NCCL communication crashes) directly within the Ray Dashboard. By re-emitting Kubernetes Node events onto RayCluster and RayJob custom resources, Ray users can distinguish between application-level issues and underlying infrastructure faults without leaving their primary monitoring interface.
Design proposal: https://docs.google.com/document/d/1Isn6vbV3bugcmYn0uc_uH5To0zohiYmfnDDszaJ5KEw/edit?tab=t.0#heading=h.xcoop4webpbh
Related issue number
Checks