[History server] Update history server examples and defaults - #5080
[History server] Update history server examples and defaults#5080chiayi wants to merge 13 commits into
Conversation
| - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EVENTS_EXPORT_ADDR | ||
| value: "http://localhost:8084/v1/events" | ||
| - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES | ||
| - name: RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES |
There was a problem hiding this comment.
IIRC this is just a variable update starting from 2.55+ to tell which event to export.
There was a problem hiding this comment.
I think the change was reverted? ray-project/ray#59965
We should keep RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES here
| TASK_PROFILE_EVENT,DRIVER_JOB_DEFINITION_EVENT,DRIVER_JOB_LIFECYCLE_EVENT, | ||
| ACTOR_DEFINITION_EVENT,ACTOR_LIFECYCLE_EVENT,NODE_DEFINITION_EVENT,NODE_LIFECYCLE_EVENT" | ||
| image: rayproject/ray:2.52.0 | ||
| image: rayproject/ray:2.55.0 |
There was a problem hiding this comment.
Just wondering should we still keep this sample yaml or just change to use spec.historyServerOptions.collectorOptions instead, so the user don't need to manually set the envs and args.
There was a problem hiding this comment.
This requires RayClusterHistoryServer feature gate. I think we could have a separate sample for this?
There was a problem hiding this comment.
Got it, we already have one in the ray-operator samples.
| - name: collector | ||
| image: collector:v0.1.0 | ||
| image: quay.io/kuberay/collector:nightly | ||
| imagePullPolicy: IfNotPresent |
There was a problem hiding this comment.
If we use nightly, I think we need to set imagePullPolicy: Always, otherwise it will never pull the newest image. Also for other files.
Actually I think we can come back and update this after release to pin the specific version
There was a problem hiding this comment.
Could we also remove command here?
There was a problem hiding this comment.
Oh you know what, good point. So the env variables have not been converted on the history server side. History server will need to check for the RAY_ROOT_DIR env var. And I think it's small enough to add the change here (this PR) as well.
FYI: @andrewsykim
|
We'll need to update the ray docs after this is merge. Can do it together with #5085 |
|
CI failed with |
Future-Outlier
left a comment
There was a problem hiding this comment.
I am thining that we should combine historyserver/config/raycluster.yaml and historyserver/config/rayjob.yaml to RayJob example, WDYT?
and also these 2 files might be better if we turn then to rayjob example too.
historyserver/config/raycluster-azureblob.yaml
historyserver/config/raycluster-gcs.yaml
|
+1 to use ephemeral RayJob with |
Will convert these to rayjobs |
|
So historyserver/config/raycluster-azureblob.yaml is hard coded into the e2e tests and I think it will require more additional code changes. I think I will leave it as is as I am not familiar with azure configurations, |
|
Some of these change leading to more changes and bloating this PR. I will not be changing the image and the azureblob yaml to avoid adding to this PR. FYI: @machichima @andrewsykim |
| containers: | ||
| - env: | ||
| - name: RAY_TMP_ROOT | ||
| value: &rayTmpRoot /tmp/ray |
There was a problem hiding this comment.
we should add a comment at the top of this file that it must be run with envsubst with an example
| - name: STORAGE_BACKEND | ||
| value: "gcs" | ||
| - name: RAY_ROOT_DIR | ||
| value: "log" |
There was a problem hiding this comment.
This one seems a bit odd, why do we use "log" for RAY_ROOT_DIR
There was a problem hiding this comment.
Honestly do not remember the reason for setting root dir to logs. But we can probably just change it to root
There was a problem hiding this comment.
I don't think root is a valid value for RAY_ROOT_DIR either, what's the default value if we don't set it? If that works just don't set it here?
| if len(containers[i].Command) > 0 { | ||
| containers[i].Command = append( | ||
| containers[i].Command, | ||
| fmt.Sprintf("--ray-cluster-namespace=%s", rayClusterNamespace), | ||
| ) | ||
| } |
There was a problem hiding this comment.
While we set raycluster namespace through RAY_CLUSTER_NAMESPACE below, I think we do not need this part?
| if rayRootDir == "" { | ||
| if val := os.Getenv("RAY_ROOT_DIR"); val != "" { | ||
| rayRootDir = val | ||
| } | ||
| } |
There was a problem hiding this comment.
We unconditionally override with the env var in other places. I think can should do the same here to get the same behavior?
kuberay/historyserver/cmd/collector/main.go
Lines 81 to 83 in 7044c38
| command: | ||
| - collector |
There was a problem hiding this comment.
We updated historyserver/test/support/raycluster.go, so I think we can remove this here?
| - name: STORAGE_BACKEND | ||
| value: "gcs" | ||
| - name: RAY_ROOT_DIR | ||
| value: "root" |
There was a problem hiding this comment.
I don't think "root" will work either? Is RAY_ROOT_DIR referring to Ray's root directory or the root directory used in storage? If it's storage, this variable name is confusing.
There was a problem hiding this comment.
It's referring to the root directory in storage. So maybe STORAGE_ROOT_DIR instead?
There was a problem hiding this comment.
yeah I would rename to STORAGE_ROOT_DIR. Also when it's empty it should just use the specified bucket's root dir
| fieldPath: status.podIP | ||
| - name: FQ_RAY_IP | ||
| value: $(RAY_CLUSTER_NAME)-head-svc.$(RAY_CLUSTER_NAMESPACE).svc.cluster.local | ||
| - name: RAY_TMP_ROOT |
There was a problem hiding this comment.
Do we really need to set this? I think this will almost always be /tmp/ray
There was a problem hiding this comment.
I think we have it as an option because it's also an option for rayclusters. In case users decide to change the raycluster temp-dir.
| - name: code-sample | ||
| mountPath: /home/ray/samples | ||
| - name: collector | ||
| image: collector:v0.1.0 |
There was a problem hiding this comment.
This is so that the ci will use the PR's version of history server to run the tests. As @machichima mentioned here: #5080 (comment) we should separate it out so that there is a specific dir for test suite, but I think that should be a separate PR.
There was a problem hiding this comment.
Ooops. correction, this is rayjob and not raycluster. And raycluster-gcs isn't currently used in e2e either.
| mountPath: *rayTmpRoot | ||
| - name: collector | ||
| image: ${COLLECTOR_IMAGE} | ||
| image: collector:v0.1.0 |
There was a problem hiding this comment.
We would need to unify here. Now we have two versions, one setting collector:v0.1.0 and the other set the nightly image (in historyserver/config/rayjob-gcs.yaml).
Are we going to:
- keep
collector:v0.1.0here and update after release for the release version - use nightly version with image pull policy always
- Have an env var for user to inject the image
There was a problem hiding this comment.
What do you think about leaving it as is so ${COLLECTOR_IMAGE} and update once we release? so a combination of 3 and 1. So we don't change anything for the image here until release.
| imagePullSecrets: | ||
| affinity: |
There was a problem hiding this comment.
Why do we leave empty fields here?
There was a problem hiding this comment.
Good question, so the raycluster part of the spec is actually copied over from raycluster.yaml. I guess they are currently just empty placeholders.
There was a problem hiding this comment.
I see, should we just remove them?
| - name: STORAGE_BACKEND | ||
| value: "gcs" | ||
| - name: RAY_ROOT_DIR | ||
| value: "storage-root" |
There was a problem hiding this comment.
I think there's mismatch in ray root dir. Here we use storage-root but in other places (e.g. historyserver/config/raycluster-azureblob.yaml) we use log
There was a problem hiding this comment.
The log is actually hard coded in the e2e tests for azure and s3 and did not want to add to this PR even more. And I updated the raycluster-gcs because it is used in the ray doc/guide and did not want to cause confusion when users view them.
What we could do is when we open a RAY_ROOT_DIR fast followup change to STORAGE_ROOT_DIR, we can also make the changes to the e2e test as well? So no change here for the time being to avoid confusion.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit aa941ee. Configure here.
| TASK_PROFILE_EVENT,DRIVER_JOB_DEFINITION_EVENT,DRIVER_JOB_LIFECYCLE_EVENT, | ||
| ACTOR_DEFINITION_EVENT,ACTOR_LIFECYCLE_EVENT,NODE_DEFINITION_EVENT,NODE_LIFECYCLE_EVENT" | ||
| image: rayproject/ray:2.52.0 | ||
| value: "ALL" |
There was a problem hiding this comment.
Incomplete Ray 2.56 event export config
Medium Severity
After upgrading these samples to Ray 2.56.0 and switching exposable event types to ALL, only RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES is set. The GCS samples updated in the same change also set RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES, and the removed comments previously called out that newer Ray needs that publisher variable for HTTP export to the collector.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit aa941ee. Configure here.
There was a problem hiding this comment.
Yes it should be RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES instead of RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES @chiayi
| - name: historyserver | ||
| mountPath: *rayTmpRoot | ||
| - name: collector | ||
| image: quay.io/kuberay/collector:nightly |
There was a problem hiding this comment.
Related to #5080 (comment)
I think we also need to update here?
| - name: code-sample | ||
| mountPath: /home/ray/samples | ||
| - name: collector | ||
| image: quay.io/kuberay/collector:nightly |
| imagePullPolicy: IfNotPresent | ||
| securityContext: | ||
| allowPrivilegeEscalation: true | ||
| privileged: true |
There was a problem hiding this comment.
why do we need to use priviledged containers? I really hope this is no longer required for Beta
| - name: historyserver | ||
| mountPath: *rayTmpRoot | ||
| tolerations: | ||
| - key: ray |
There was a problem hiding this comment.
this is a weird toleration to have here, do we need it?
|
Followups:
|



Why are these changes needed?
This PR updates the event port default to 8084 from 8080 which will clash with dashboard port. Along with changing the default, this PR also update examples to use the env var instead of the flags. And since the entrypoint for the collector and historyserver is not part of the image, we no longer need the
command.It was also found that
RAY_ROOT_DIRwas not added to historyserver, this PR adds it.Edit: Update scope
Related issue number
Part of #4187
Labels
doc-updates-requiredlabel.breaking-changelabel.Checks
Manual test instructions
Ran the yaml against a live kubernetes cluster.