[History Server] Poll Serve, placement group, and Ray Data endpoints by default - #5093
[History Server] Poll Serve, placement group, and Ray Data endpoints by default#5093Future-Outlier wants to merge 9 commits into
Conversation
…by default The head collector now polls these endpoints itself every RAY_COLLECTOR_POLL_INTERVAL (default 30s); RAY_COLLECTOR_ADDITIONAL_ENDPOINTS still adds more on top.
RayServiceReconciler.Recorder is events.EventRecorder, so record.NewFakeRecorder does not compile. Every other test in this file already uses events.
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 34e5a50. Configure here.
A periodic poll now stores an empty Serve response since that is the live truth, while the final shutdown poll still cannot erase a converged snapshot. Periodic polling stops on the shutdown signal instead of ShutdownChan so no tick can overwrite the final pass.
- Cap the shutdown wait on the periodic poller so a stuck storage write cannot consume the termination grace period, and skip periodic writes once shutdown starts so the final poll owns the store. - Drop the placement-group replay from the collector suite: testDeadClusterPlacementGroups already covers the same round trip. - Kill and reap the e2e kubectl port-forward on cleanup: a leaked forward keeps the port and the next test silently talks to a deleted namespace. Signed-off-by: Future-Outlier <eric901201@gmail.com>
|
cc @win5923 @machichima to take a look, tks |
machichima
left a comment
There was a problem hiding this comment.
Only went through part of the PR, will finish the review tomorrow!
| // Fall back instead of exiting: crash-looping this sidecar would take the head pod | ||
| // out of its Service endpoints. |
There was a problem hiding this comment.
nit: maybe move this to line 114 above, as this is the behavior of all env var parsing logic
There was a problem hiding this comment.
Env parsing isn't all in one place — RAY_COLLECTOR_EVENT_* (131-155), ADDITIONAL_ENDPOINTS (166), STORAGE_BACKEND (197) and RAY_DASHBOARD_ADDRESS (235) all come after 113. The block ending at 113 is the core flags validateFlags consumes, while this one pairs with ADDITIONAL_ENDPOINTS (both endpoint-polling knobs, adjacent config fields).
Keeping it here, but happy to move all the trailing blocks in a separate cleanup if you prefer.
| // PollAdditionalEndpointsPeriodically fetches the built-in endpoints, plus anything from | ||
| // RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, on a timer; each cycle overwrites the previous one. | ||
| // It stops on the shutdown signal, not ShutdownChan: ShutdownChan closes only after the | ||
| // final shutdown poll, and a tick in between could overwrite that final snapshot. | ||
| // Run joins this goroutine before that final poll, so the ctx cancels at stop to keep a | ||
| // blocked resolve or in-flight cycle from stalling shutdown. |
There was a problem hiding this comment.
| // PollAdditionalEndpointsPeriodically fetches the built-in endpoints, plus anything from | |
| // RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, on a timer; each cycle overwrites the previous one. | |
| // It stops on the shutdown signal, not ShutdownChan: ShutdownChan closes only after the | |
| // final shutdown poll, and a tick in between could overwrite that final snapshot. | |
| // Run joins this goroutine before that final poll, so the ctx cancels at stop to keep a | |
| // blocked resolve or in-flight cycle from stalling shutdown. | |
| // PollAdditionalEndpointsPeriodically fetches the built-in endpoints, plus anything from | |
| // RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, on a timer; each cycle overwrites the previous one. | |
| // It stops when stop closes and cancels any blocked resolve or in-flight request at that point. |
Should we move the shutdown specific comment to the call site and just describe the function behavior here
There was a problem hiding this comment.
Done in 17626bc — the shutdown rationale moved to the goroutine launch in collector.go.
| logrus.Info("Finished processing polled endpoints") | ||
| } | ||
|
|
||
| // finalPoll marks the shutdown pass, where an empty Serve response is distrusted. |
There was a problem hiding this comment.
| // finalPoll marks the shutdown pass, where an empty Serve response is distrusted. | |
| // pollAllEndpoints fetches the polled endpoints plus per-job dataset endpoints and stores | |
| // their responses, stopping early once ctx is canceled. | |
| // finalPoll marks the shutdown pass, which distrusts empty Serve responses (see isEmptyPayload). |
update docstring to describe what this function is doing
| func (r *RayLogHandler) polledEndpoints() []string { | ||
| endpoints := make([]string, 0, len(staticPolledEndpoints)+len(r.AdditionalEndpoints)) | ||
| seen := make(map[string]struct{}, cap(endpoints)) | ||
| for _, list := range [][]string{staticPolledEndpoints, r.AdditionalEndpoints} { |
There was a problem hiding this comment.
Could we use slices.Concat, which merge the two into a slice, then we can simplify this to a single loop
There was a problem hiding this comment.
Done in 17626bc — slices.Concat plus a single dedup loop.
- Match the sibling env blocks when parsing RAY_COLLECTOR_POLL_INTERVAL. - Merge the built-in and configured endpoints with slices.Concat. - Describe what PollAdditionalEndpointsPeriodically and pollAllEndpoints do, and keep the shutdown-ordering rationale at the call site. Signed-off-by: Future-Outlier <eric901201@gmail.com>
0805dee to
17626bc
Compare
Signed-off-by: Future-Outlier <eric901201@gmail.com>
machichima
left a comment
There was a problem hiding this comment.
Overall LGTM, just few comments
| } | ||
|
|
||
| storageKey := utils.EndpointPathToStorageKey(endpoint) | ||
| objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) |
There was a problem hiding this comment.
nit: could be in follow-up. Could we add a new function in historyserver/pkg/storage/clusterlogs/clusterlogs.go like following?
func FetchedEndpointsDir(prefix, sessionName string) string {
return path.Join(prefix, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME)
}| defer cancel() | ||
|
|
||
| // Fresh state, so this final pass re-captures every job. | ||
| r.pollAllEndpoints(ctx, sessionName, newDatasetPollState(), true) |
There was a problem hiding this comment.
We use a fresh state here, which will also re-capture terminal jobs that are already marked as done. Is this intended? With many jobs this could burn through the whole shutdownPollBudget (10s) on jobs that don't need re-fetching.
| value: "TASK_DEFINITION_EVENT,TASK_LIFECYCLE_EVENT,ACTOR_TASK_DEFINITION_EVENT, | ||
| TASK_PROFILE_EVENT,DRIVER_JOB_DEFINITION_EVENT,DRIVER_JOB_LIFECYCLE_EVENT, | ||
| ACTOR_DEFINITION_EVENT,ACTOR_LIFECYCLE_EVENT,NODE_DEFINITION_EVENT,NODE_LIFECYCLE_EVENT" |
There was a problem hiding this comment.
Let's update the ray image version to 2.56.0 and use "ALL" here
| - name: FQ_RAY_IP | ||
| value: "localhost" | ||
| - name: RAY_TMP_ROOT | ||
| value: *rayTmpRoot |
There was a problem hiding this comment.
Do we also want to set RAY_COLLECTOR_POLL_INTERVAL to 5 sec here so test historyserver/test/support/rayservice.go can end quicker?
| # Optional extras on top of the built-in endpoints (Serve, placement groups, Ray Data). | ||
| # Paths must match the frontend request URI, query string included. | ||
| # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS | ||
| # value: "/nodes?view=summary" |
There was a problem hiding this comment.
I'm curious why the sample value is /nodes?view=summary. Only endpoints under /api/** that don't have dedicated handlers can be replayed.
| case pollFailed: | ||
| // Retried next cycle. | ||
| } | ||
| } |
| r.processSessionLatestLogs() | ||
| wg.Wait() | ||
|
|
||
| // Only now: pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. |
There was a problem hiding this comment.
| // Only now: pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. | |
| // Close after all writers have finished. |
I think we can update the comment, since ShutdownChan is no longer used to cancel in-flight requests. At this point, all writers have already finished before it's closed.
| // the response to storage. | ||
| func (r *RayLogHandler) pollSingleEndpoint(endpoint, sessionName string) { | ||
| // fetchEndpoint GETs one dashboard endpoint; in-flight requests are canceled on shutdown. | ||
| func (r *RayLogHandler) fetchEndpoint(parent context.Context, endpoint string) ([]byte, error) { |
There was a problem hiding this comment.
I think we can simplify this by relying on the request context directly:
func (r *RayLogHandler) fetchEndpoint(parent context.Context, endpoint string) ([]byte, error) {
ctx, cancel := context.WithTimeout(parent, defaultRequestTimeout)
defer cancel()
req, err := http.NewRequestWithContext(ctx, http.MethodGet, r.DashboardAddress+endpoint, nil)
...
Since parent (pollSingleEndpoint, pollDataDatasets) is already canceled during shutdown, ctx.Done() will be triggered before ShutdownChan is closed, so the extra goroutine waiting on ShutdownChan doesn't seem necessary.

Why are these changes needed?
The History Server rebuilds the UI from Ray events, but Serve applications, placement groups, and Ray Data datasets are not in the event stream yet. The head collector now polls these endpoints by default so replayed clusters show them:
/api/serve/applications//api/v0/placement_groups?detail=1&limit=10000/api/data/datasets/{job_id}(per job discovered via/api/jobs/)You can set the interval with
RAY_COLLECTOR_POLL_INTERVALif you want (default 30s).RAY_COLLECTOR_ADDITIONAL_ENDPOINTSstill adds more endpoints on top. Ray Train can follow once its frontend schema is settled.Screenshots
Dead clusters replayed through the History Server, each endpoint with and without data.
Serve applications:
Serve applications, cluster that never ran Serve:
Ray Data datasets, row expanded into its operators:
Placement groups; this job used no Ray Data, so no
Ray Data Overviewsection:Placement groups, job that created none:
Related issue number
Labels
doc-updates-requiredlabel.breaking-changelabel.Checks
Manual test instructions
cd historyserver && make test.cd historyserver && go test -count=1 ./test/e2e/ -run 'TestCollector$'— 8/8 passing.0without a Prometheus server (on live clusters too); the screenshot was taken with one installed.