From 54251e87bb4174cbad0a0ea7a0a70f73883289ff Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 17:30:29 -0500 Subject: [PATCH 01/10] [History Server] Poll Serve, placement group, and Ray Data endpoints 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. --- historyserver/cmd/collector/main.go | 31 +- historyserver/config/ray-data.yaml | 126 +++++ .../config/raycluster-azureblob.yaml | 16 + historyserver/config/raycluster-gcs.yaml | 37 +- historyserver/config/raycluster.yaml | 37 +- historyserver/config/rayservice.yaml | 129 +++++ .../runtime/logcollector/collector.go | 15 +- .../logcollector/runtime/logcollector/poll.go | 339 +++++++++--- .../runtime/logcollector/poll_test.go | 514 ++++++++++++++++++ historyserver/pkg/collector/types/types.go | 1 + historyserver/pkg/historyserver/router.go | 13 +- historyserver/test/e2e/collector_test.go | 229 +++++++- historyserver/test/support/rayjob.go | 28 + historyserver/test/support/rayservice.go | 55 ++ 14 files changed, 1448 insertions(+), 122 deletions(-) create mode 100644 historyserver/config/ray-data.yaml create mode 100644 historyserver/config/rayservice.yaml create mode 100644 historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go create mode 100644 historyserver/test/support/rayservice.go diff --git a/historyserver/cmd/collector/main.go b/historyserver/cmd/collector/main.go index 7810aebe63a..bd886ce5722 100644 --- a/historyserver/cmd/collector/main.go +++ b/historyserver/cmd/collector/main.go @@ -2,6 +2,7 @@ package main import ( "encoding/json" + "errors" "flag" "fmt" "os" @@ -143,6 +144,8 @@ func main() { } } + // RAY_COLLECTOR_ADDITIONAL_ENDPOINTS is optional: the head collector always + // polls its built-in endpoints, and anything listed here is polled on top. var additionalEndpoints []string if epStr := os.Getenv("RAY_COLLECTOR_ADDITIONAL_ENDPOINTS"); epStr != "" { for _, ep := range strings.Split(epStr, ",") { @@ -153,16 +156,20 @@ func main() { } } + // An unusable poll interval falls back to the default instead of exiting: the + // collector is a sidecar in the Ray head pod, so crash-looping on a bad + // observability knob would take the head out of its Service endpoints. endpointPollInterval := 30 * time.Second - if intervalStr := os.Getenv("RAY_COLLECTOR_POLL_INTERVAL"); intervalStr != "" { - parsed, parseErr := time.ParseDuration(intervalStr) - if parseErr != nil { - logrus.Fatalf("Failed to parse RAY_COLLECTOR_POLL_INTERVAL: %v", parseErr) + if v := os.Getenv("RAY_COLLECTOR_POLL_INTERVAL"); v != "" { + parsed, err := time.ParseDuration(v) + if err == nil && parsed <= 0 { + err = errors.New("must be positive") } - if parsed <= 0 { - logrus.Fatalf("RAY_COLLECTOR_POLL_INTERVAL must be positive, got: %s", intervalStr) + if err != nil { + logrus.Warnf("Invalid RAY_COLLECTOR_POLL_INTERVAL=%s (%v), using default %s", v, err, endpointPollInterval) + } else { + endpointPollInterval = parsed } - endpointPollInterval = parsed } jsonData := make(map[string]interface{}) @@ -210,6 +217,14 @@ func main() { sessionName := path.Base(activeSessionDir) + // The collector always runs as a sidecar in the Ray head pod, so the dashboard is + // reachable on localhost at Ray's default port. Only the head collector uses this. + // Override it when the dashboard listens on a non-default port. + dashboardAddress := "http://localhost:8265" + if v := os.Getenv("RAY_DASHBOARD_ADDRESS"); v != "" { + dashboardAddress = v + } + globalConfig := types.RayCollectorConfig{ RootDir: rayRootDir, SessionDir: activeSessionDir, @@ -219,7 +234,7 @@ func main() { RayClusterNamespace: rayClusterNamespace, PushInterval: pushInterval, LogBatching: logBatching, - DashboardAddress: os.Getenv("RAY_DASHBOARD_ADDRESS"), + DashboardAddress: dashboardAddress, OwnerKind: ownerKind, OwnerName: ownerName, diff --git a/historyserver/config/ray-data.yaml b/historyserver/config/ray-data.yaml new file mode 100644 index 00000000000..18526a57950 --- /dev/null +++ b/historyserver/config/ray-data.yaml @@ -0,0 +1,126 @@ +apiVersion: ray.io/v1 +kind: RayJob +metadata: + name: rayjob-ray-data +spec: + # Self-contained: this RayJob brings up its own cluster instead of attaching to an + # existing one via clusterSelector, so it also exercises the collector's shutdown path. + shutdownAfterJobFinishes: true + # Keeps the cluster alive long enough for at least one polling cycle after the job + # succeeds. Without it the cluster is deleted immediately and the datasets would only + # be captured by the collector's best-effort final poll during shutdown. + ttlSecondsAfterFinished: 30 + entrypoint: | + python -c " + import ray + ray.init() + + # materialize() is required: an unexecuted Dataset produces no stats, so + # /api/data/datasets/{job_id} would stay empty. + ds = ray.data.range(100).map_batches(lambda batch: batch).materialize() + print(f'Dataset rows: {ds.count()}') + " + rayClusterSpec: + # Head-only on purpose: worker collectors need the head Service FQDN in FQ_RAY_IP, + # which cannot be written here because KubeRay generates the cluster name. + headGroupSpec: + rayStartParams: + dashboard-host: 0.0.0.0 + serviceType: ClusterIP + template: + spec: + containers: + - env: + - name: RAY_TMP_ROOT + value: &rayTmpRoot /tmp/ray + - name: RAY_enable_ray_event + value: "true" + - name: RAY_enable_core_worker_ray_event_to_aggregator + value: "true" + - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EVENTS_EXPORT_ADDR + value: "http://localhost:8084/v1/events" + # in ray 2.52.0, we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES + # in ray 2.53.0 (noy yet done). we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES + - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES + 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" + image: rayproject/ray:2.52.0 + imagePullPolicy: IfNotPresent + name: ray-head + securityContext: + allowPrivilegeEscalation: true + privileged: true + resources: + limits: + cpu: "5" + memory: 10G + requests: + cpu: "50m" + memory: 1G + volumeMounts: + - name: historyserver + mountPath: *rayTmpRoot + - name: collector + image: collector:v0.1.0 + imagePullPolicy: IfNotPresent + env: + - name: POD_IP + valueFrom: + fieldRef: + fieldPath: status.podIP + # KubeRay generates the cluster name, so these are read back from the labels + # it stamps on the pod rather than hardcoded. + - name: RAY_CLUSTER_NAME + valueFrom: + fieldRef: + fieldPath: metadata.labels['ray.io/cluster'] + - name: RAY_CLUSTER_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + # Hardcoded, unlike the cluster name: KubeRay puts ray.io/originated-from-* + # on the RayCluster but not on the pod, so the downward API cannot read them. + # Must match metadata.name above. + - name: OWNER_KIND + value: "RayJob" + - name: OWNER_NAME + value: "rayjob-ray-data" + # Only used to look up this pod's Ray NodeID, and the dashboard is in this + # same pod. A worker collector would need the head Service FQDN instead. + - name: FQ_RAY_IP + value: "localhost" + - name: RAY_TMP_ROOT + value: *rayTmpRoot + # Shorter than the 30s default so a full cycle fits inside + # ttlSecondsAfterFinished above. + - name: RAY_COLLECTOR_POLL_INTERVAL + value: "5s" + - name: S3DISABLE_SSL + value: "true" + - name: AWS_ACCESS_KEY_ID + value: minioadmin + - name: AWS_SECRET_ACCESS_KEY + value: minioadmin + - name: AWS_SESSION_TOKEN + value: "" + - name: S3_BUCKET + value: "ray-historyserver" + - name: S3_ENDPOINT + value: "minio-service.minio-dev:9000" + - name: S3_REGION + value: "test" + - name: S3FORCE_PATH_STYLE + value: "true" + command: + - collector + - --role=Head + - --runtime-class-name=s3 + - --ray-root-dir=log + - --events-port=8084 + volumeMounts: + - name: historyserver + mountPath: *rayTmpRoot + volumes: + - name: historyserver + emptyDir: {} diff --git a/historyserver/config/raycluster-azureblob.yaml b/historyserver/config/raycluster-azureblob.yaml index 9de3a42e8b7..11bab489c5c 100644 --- a/historyserver/config/raycluster-azureblob.yaml +++ b/historyserver/config/raycluster-azureblob.yaml @@ -60,6 +60,22 @@ spec: value: raycluster-historyserver-head-svc.default.svc.cluster.local - name: RAY_TMP_ROOT value: *rayTmpRoot + # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same + # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard + # listens on a non-default port. Worker collectors do not use it. + # - name: RAY_DASHBOARD_ADDRESS + # value: "http://localhost:9265" + # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray + # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # - name: RAY_COLLECTOR_POLL_INTERVAL + # value: "1m" + # The head collector always polls its built-in endpoints (Serve applications, + # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # is optional and adds more on top; uncomment to use it. Each comma-separated path + # must match what the dashboard frontend requests, query string included, because + # the storage key is derived from the request URI. + # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # value: "/nodes?view=summary" # reference: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite#connect-to-the-emulator-by-using-the-azure-storage-explorer - name: AZURE_STORAGE_CONNECTION_STRING value: "DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://azurite-service.azurite-dev.svc.cluster.local:10000/devstoreaccount1;" diff --git a/historyserver/config/raycluster-gcs.yaml b/historyserver/config/raycluster-gcs.yaml index 4739b79d509..4de79f1a242 100644 --- a/historyserver/config/raycluster-gcs.yaml +++ b/historyserver/config/raycluster-gcs.yaml @@ -63,27 +63,22 @@ spec: value: *rayTmpRoot - name: GCS_BUCKET value: "${GCS_BUCKET}" - # RAY_DASHBOARD_ADDRESS is used by the head collector to fetch endpoints' results - # (e.g., /api/v0/cluster_metadata) from the Ray Dashboard running in the same pod. - # Only the head collector uses this; worker collectors do not need it. - # If your Ray Dashboard uses a non-default port (not 8265), update this value accordingly. - - name: RAY_DASHBOARD_ADDRESS - value: "http://localhost:8265" - # RAY_COLLECTOR_ADDITIONAL_ENDPOINTS is a comma-separated list of Ray Dashboard - # API endpoint paths that the head collector will periodically poll and store. - # Use this for endpoints whose data cannot be obtained via Ray events. - # You can add more endpoints, e.g., "/api/v0/placement_groups,/api/serve/applications/" - # Note: Only static endpoints (no dynamic path parameters like {job_id}) are supported. - - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - value: "/api/v0/placement_groups?detail=1&limit=10000" - # Query params must match the Ray Dashboard frontend request exactly - # (see https://github.com/ray-project/ray/blob/cb9c80fee6a700efe61ea97987248ce82e3fa2e2/python/ray/dashboard/client/src/service/placementGroup.ts): - # detail=1 — include bundles and stats fields required by PlacementGroupTable - # limit=10000 — match the frontend's default limit to avoid truncation - # RAY_COLLECTOR_POLL_INTERVAL controls how often the collector polls the additional - # endpoints above. Accepts Go duration format (e.g., "30s", "1m", "5m"). - - name: RAY_COLLECTOR_POLL_INTERVAL - value: "30s" + # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same + # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard + # listens on a non-default port. Worker collectors do not use it. + # - name: RAY_DASHBOARD_ADDRESS + # value: "http://localhost:9265" + # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray + # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # - name: RAY_COLLECTOR_POLL_INTERVAL + # value: "1m" + # The head collector always polls its built-in endpoints (Serve applications, + # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # is optional and adds more on top; uncomment to use it. Each comma-separated path + # must match what the dashboard frontend requests, query string included, because + # the storage key is derived from the request URI. + # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # value: "/nodes?view=summary" command: - collector - --role=Head diff --git a/historyserver/config/raycluster.yaml b/historyserver/config/raycluster.yaml index 1acf4b94462..e76ac243ff4 100644 --- a/historyserver/config/raycluster.yaml +++ b/historyserver/config/raycluster.yaml @@ -66,27 +66,22 @@ spec: value: raycluster-historyserver-head-svc.default.svc.cluster.local - name: RAY_TMP_ROOT value: *rayTmpRoot - # RAY_DASHBOARD_ADDRESS is used by the head collector to fetch endpoints' results - # (e.g., /api/v0/cluster_metadata) from the Ray Dashboard running in the same pod. - # Only the head collector uses this; worker collectors do not need it. - # If your Ray Dashboard uses a non-default port (not 8265), update this value accordingly. - - name: RAY_DASHBOARD_ADDRESS - value: "http://localhost:8265" - # RAY_COLLECTOR_ADDITIONAL_ENDPOINTS is a comma-separated list of Ray Dashboard - # API endpoint paths that the head collector will periodically poll and store. - # Use this for endpoints whose data cannot be obtained via Ray events. - # You can add more endpoints, e.g., "/api/v0/placement_groups,/api/serve/applications/" - # Note: Only static endpoints (no dynamic path parameters like {job_id}) are supported. - - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # Query params must match the Ray Dashboard frontend request exactly - # (see https://github.com/ray-project/ray/blob/cb9c80fee6a700efe61ea97987248ce82e3fa2e2/python/ray/dashboard/client/src/service/placementGroup.ts): - # detail=1 — include bundles and stats fields required by PlacementGroupTable - # limit=10000 — match the frontend's default limit to avoid truncation - value: "/api/v0/placement_groups?detail=1&limit=10000" - # RAY_COLLECTOR_POLL_INTERVAL controls how often the collector polls the additional - # endpoints above. Accepts Go duration format (e.g., "30s", "1m", "5m"). - - name: RAY_COLLECTOR_POLL_INTERVAL - value: "30s" + # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same + # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard + # listens on a non-default port. Worker collectors do not use it. + # - name: RAY_DASHBOARD_ADDRESS + # value: "http://localhost:9265" + # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray + # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # - name: RAY_COLLECTOR_POLL_INTERVAL + # value: "1m" + # The head collector always polls its built-in endpoints (Serve applications, + # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # is optional and adds more on top; uncomment to use it. Each comma-separated path + # must match what the dashboard frontend requests, query string included, because + # the storage key is derived from the request URI. + # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS + # value: "/nodes?view=summary" - name: S3DISABLE_SSL value: "true" - name: AWS_ACCESS_KEY_ID diff --git a/historyserver/config/rayservice.yaml b/historyserver/config/rayservice.yaml new file mode 100644 index 00000000000..ba2eae56cb0 --- /dev/null +++ b/historyserver/config/rayservice.yaml @@ -0,0 +1,129 @@ +apiVersion: ray.io/v1 +kind: RayService +metadata: + name: rayservice-historyserver +spec: + serveConfigV2: | + proxy_location: EveryNode + applications: + - name: history-e2e + route_prefix: / + import_path: microbenchmarks.no_ops:app_builder + args: + num_forwards: 0 + runtime_env: + working_dir: https://github.com/ray-project/serve_workloads/archive/a9f184f4d9ddb7f9a578502ae106470f87a702ef.zip + deployments: + - name: NoOp + num_replicas: 1 + ray_actor_options: + num_cpus: 0.5 + rayClusterConfig: + # Head-only on purpose: worker collectors need the head Service FQDN in FQ_RAY_IP, + # which cannot be written here because KubeRay generates the cluster name. + headGroupSpec: + rayStartParams: + dashboard-host: 0.0.0.0 + serviceType: ClusterIP + template: + spec: + containers: + - env: + - name: RAY_TMP_ROOT + value: &rayTmpRoot /tmp/ray + - name: RAY_enable_ray_event + value: "true" + - name: RAY_enable_core_worker_ray_event_to_aggregator + value: "true" + - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EVENTS_EXPORT_ADDR + value: "http://localhost:8084/v1/events" + # in ray 2.52.0, we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES + # in ray 2.53.0 (noy yet done). we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES + - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES + 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" + image: rayproject/ray:2.52.0 + imagePullPolicy: IfNotPresent + name: ray-head + securityContext: + allowPrivilegeEscalation: true + privileged: true + ports: + - containerPort: 6379 + name: gcs-server + - containerPort: 8265 + name: dashboard + - containerPort: 10001 + name: client + - containerPort: 8000 + name: serve + resources: + limits: + cpu: "5" + memory: 10G + requests: + cpu: "50m" + memory: 1G + volumeMounts: + - name: historyserver + mountPath: *rayTmpRoot + - name: collector + image: collector:v0.1.0 + imagePullPolicy: IfNotPresent + env: + - name: POD_IP + valueFrom: + fieldRef: + fieldPath: status.podIP + # KubeRay generates the cluster name, so these are read back from the labels + # it stamps on the pod rather than hardcoded. + - name: RAY_CLUSTER_NAME + valueFrom: + fieldRef: + fieldPath: metadata.labels['ray.io/cluster'] + - name: RAY_CLUSTER_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + # Hardcoded, unlike the cluster name: KubeRay puts ray.io/originated-from-* + # on the RayCluster but not on the pod, so the downward API cannot read them. + # Must match metadata.name above. + - name: OWNER_KIND + value: "RayService" + - name: OWNER_NAME + value: "rayservice-historyserver" + # Only used to look up this pod's Ray NodeID, and the dashboard is in this + # same pod. A worker collector would need the head Service FQDN instead. + - name: FQ_RAY_IP + value: "localhost" + - name: RAY_TMP_ROOT + value: *rayTmpRoot + - name: S3DISABLE_SSL + value: "true" + - name: AWS_ACCESS_KEY_ID + value: minioadmin + - name: AWS_SECRET_ACCESS_KEY + value: minioadmin + - name: AWS_SESSION_TOKEN + value: "" + - name: S3_BUCKET + value: "ray-historyserver" + - name: S3_ENDPOINT + value: "minio-service.minio-dev:9000" + - name: S3_REGION + value: "test" + - name: S3FORCE_PATH_STYLE + value: "true" + command: + - collector + - --role=Head + - --runtime-class-name=s3 + - --ray-root-dir=log + - --events-port=8084 + volumeMounts: + - name: historyserver + mountPath: *rayTmpRoot + volumes: + - name: historyserver + emptyDir: {} diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index 8d3e96274af..55e08bfbe86 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -91,13 +91,18 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { <-stop logrus.Info("Received stop signal, processing all logs...") - r.processSessionLatestLogs() - // Perform one final poll of additional endpoints before shutting down. - // This must happen before close(r.ShutdownChan) because pollSingleEndpoint - // uses ShutdownChan to cancel in-flight HTTP requests. + + // The final endpoint poll races the Ray head's own shutdown: once the dashboard is + // gone the data is unrecoverable, while log files stay on local disk. Run it + // concurrently so it never queues behind a slow log upload. + var wg sync.WaitGroup if r.IsHead { - r.processAdditionalEndpoints() + wg.Go(r.processAdditionalEndpoints) } + r.processSessionLatestLogs() + wg.Wait() + + // Only now, because pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. close(r.ShutdownChan) return nil diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index 18179444624..71b9b5bbe29 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -3,10 +3,13 @@ package logcollector import ( "bytes" "context" + "encoding/json" + "fmt" "io" "net/http" "path" "path/filepath" + "strings" "time" "github.com/sirupsen/logrus" @@ -14,34 +17,110 @@ import ( "github.com/ray-project/kuberay/historyserver/pkg/utils" ) -// PollAdditionalEndpointsPeriodically periodically fetches user-configured additional -// endpoints from the Ray Dashboard and stores their responses in storage. Unlike -// FetchAndStoreClusterMetadata (which runs once), this runs continuously on a timer -// until shutdown. -// -// Each endpoint response is stored at: {ClusterDir}/{sessionName}/fetched_endpoints/{storageKey} -// where storageKey is derived from the endpoint path (e.g., "/api/v0/nodes/summary" -// becomes "restful__api__v0__nodes__summary"). Each poll cycle overwrites the -// previous response. -func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { - if len(r.AdditionalEndpoints) == 0 { - logrus.Info("No additional endpoints configured, skipping polling") - return +// These are polled periodically, unlike the one-shot startup endpoints in +// startup_endpoints.go. Each string must match the frontend's request URI, query +// string included: the history server derives the storage key from that URI, so a +// mismatch silently makes the stored object unreachable. +const ( + // Paths mirror the Ray Dashboard frontend: serve.ts, placementGroup.ts, data.ts. + serveApplicationsEndpoint = "/api/serve/applications/" + + // detail=1 adds the bundle and stats fields PlacementGroupTable needs; + // limit=10000 matches the frontend default. + placementGroupsEndpoint = "/api/v0/placement_groups?detail=1&limit=10000" + + // Only used to discover job IDs. Its response is not stored: the history + // server rebuilds the job list from Ray events. + jobsEndpoint = "/api/jobs/" + + // Requested per job, where job_id is the Ray core job ID in hex (e.g. + // "01000000"), not the submission ID. + dataDatasetsEndpointPrefix = "/api/data/datasets/" +) + +// dataDatasetsEndpointPrefix is absent here: it needs a job ID, so pollDataDatasets +// handles it. +var staticPolledEndpoints = []string{ + serveApplicationsEndpoint, + placementGroupsEndpoint, +} + +// polledEndpoints deduplicates so that listing a built-in endpoint in +// RAY_COLLECTOR_ADDITIONAL_ENDPOINTS does not fetch and store it twice per cycle. +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} { + for _, endpoint := range list { + if _, ok := seen[endpoint]; ok { + continue + } + seen[endpoint] = struct{}{} + endpoints = append(endpoints, endpoint) + } } + return endpoints +} - // Note: session name staleness is not a concern here because the log collector runs as a - // sidecar in the Ray head pod. If the Ray head process restarts (creating a new session), - // the entire pod — including this sidecar container — restarts, so resolveSessionName() - // always runs in a fresh container lifecycle with the current session. +// terminalJobStatuses are the /api/jobs/ statuses a job never leaves, so its Ray +// Data datasets only need to be stored once. +// Ref: https://github.com/ray-project/ray/blob/ray-2.54.1/python/ray/dashboard/modules/job/common.py#L38-L50 +var terminalJobStatuses = map[string]bool{ + "SUCCEEDED": true, + "FAILED": true, + "STOPPED": true, +} + +// pollOutcome distinguishes "nothing worth storing" from "could not store", which +// decides whether a terminal job is worth polling again. +type pollOutcome int + +const ( + pollFailed pollOutcome = iota + pollStored + pollSkippedEmpty +) + +// A job's stats can appear slightly after it reports terminal, so giving up on the +// first empty response would lose them permanently. +const terminalEmptyPollsBeforeGivingUp = 2 + +// The final poll shares the pod's termination grace period (30s by default) with the +// log upload, so it gives up rather than risking a SIGKILL partway through. +const shutdownPollBudget = 10 * time.Second + +// datasetPollState remembers across cycles which jobs no longer need their datasets +// fetched. The polling loop owns it exclusively, so it needs no lock. +type datasetPollState struct { + done map[string]struct{} + emptyRuns map[string]int +} + +func newDatasetPollState() *datasetPollState { + return &datasetPollState{ + done: make(map[string]struct{}), + emptyRuns: make(map[string]int), + } +} + +// PollAdditionalEndpointsPeriodically fetches the built-in endpoints, plus anything +// from RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, on a timer until shutdown. Each response +// is stored at {ClusterDir}/{sessionName}/fetched_endpoints/{storageKey}, and each +// cycle overwrites the previous one. +func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { + // Blocking resolve is fine here but not in the loop below: on startup there is + // nothing to poll until session_latest exists. sessionName, err := r.resolveSessionName() if err != nil { - logrus.Errorf("Failed to resolve session name for additional endpoints polling: %v", err) + logrus.Errorf("Failed to resolve session name for endpoint polling: %v", err) return } - logrus.Infof("Starting additional endpoints polling (interval=%v, endpoints=%v)", r.EndpointPollInterval, r.AdditionalEndpoints) + logrus.Infof("Starting endpoint polling (interval=%v, endpoints=%v)", r.EndpointPollInterval, r.polledEndpoints()) + + state := newDatasetPollState() // Perform an initial poll immediately on startup. - r.pollAllEndpoints(sessionName) + r.pollAllEndpoints(context.Background(), sessionName, state) ticker := time.NewTicker(r.EndpointPollInterval) defer ticker.Stop() @@ -49,15 +128,44 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { for { select { case <-r.ShutdownChan: - logrus.Info("Shutdown signaled, stopping additional endpoints polling") + logrus.Info("Shutdown signaled, stopping endpoint polling") return case <-ticker.C: - r.pollAllEndpoints(sessionName) + sessionName, state = r.pollCycle(context.Background(), sessionName, state) } } } -// processAdditionalEndpoints performs a final poll of all additional endpoints +// pollCycle runs one polling pass, re-resolving the session first. +// +// The Ray head container can restart on its own (an OOMKill restarts that container, not +// the pod), which starts a new session while this sidecar keeps running. Job IDs restart +// with it, so a stale state would both write into the dead session's directory and skip +// the new session's jobs as already captured. +func (r *RayLogHandler) pollCycle(ctx context.Context, sessionName string, state *datasetPollState) (string, *datasetPollState) { + switch current, err := currentSessionName(); { + case err != nil: + logrus.Warnf("Failed to re-resolve session name, polling into %s: %v", sessionName, err) + case current != sessionName: + logrus.Infof("Session changed from %s to %s, resetting dataset polling state", sessionName, current) + sessionName, state = current, newDatasetPollState() + } + + r.pollAllEndpoints(ctx, sessionName, state) + return sessionName, state +} + +// currentSessionName resolves session_latest without retrying, for callers that must not +// block: the polling loop and the shutdown path. +func currentSessionName() (string, error) { + sessionRealDir, err := filepath.EvalSymlinks(utils.GetRaySessionLatestPath()) + if err != nil { + return "", err + } + return filepath.Base(sessionRealDir), nil +} + +// processAdditionalEndpoints performs a final poll of all polled endpoints // before shutdown. This mirrors processSessionLatestLogs as a shutdown cleanup step. // // Unlike PollAdditionalEndpointsPeriodically, this does NOT retry session name @@ -65,37 +173,160 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { // Ray head already exited), retrying would hang forever since ShutdownChan has // not been closed yet. func (r *RayLogHandler) processAdditionalEndpoints() { - if len(r.AdditionalEndpoints) == 0 { + logrus.Info("Processing polled endpoints before shutdown") + + sessionName, err := currentSessionName() + if err != nil { + logrus.Errorf("Failed to resolve session name for final endpoint poll: %v", err) return } - logrus.Info("Processing additional endpoints before shutdown") - // Resolve session name directly without retry — this is a shutdown path. - sessionLatestDir := utils.GetRaySessionLatestPath() - sessionRealDir, err := filepath.EvalSymlinks(sessionLatestDir) + // One budget for the whole pass, not per request: the endpoints are fetched + // serially, so per-request timeouts would add up past the grace period. + ctx, cancel := context.WithTimeout(context.Background(), shutdownPollBudget) + defer cancel() + + // Fresh state, so this final pass re-captures every job rather than trusting + // what the polling loop already stored. + r.pollAllEndpoints(ctx, sessionName, newDatasetPollState()) + logrus.Info("Finished processing polled endpoints") +} + +func (r *RayLogHandler) pollAllEndpoints(ctx context.Context, sessionName string, state *datasetPollState) { + for _, endpoint := range r.polledEndpoints() { + if ctx.Err() != nil { + logrus.Warnf("Stopped polling before %s: %v", endpoint, ctx.Err()) + return + } + r.pollSingleEndpoint(ctx, endpoint, sessionName) + } + r.pollDataDatasets(ctx, sessionName, state) +} + +// pollDataDatasets stores one datasets object per job discovered via jobsEndpoint. +// Terminal jobs are dropped from future cycles once settled: without that the +// per-cycle cost would grow with the cluster's total job count forever, and each +// datasets request also makes the dashboard query Prometheus. +func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string, state *datasetPollState) { + body, err := r.fetchEndpoint(ctx, jobsEndpoint) if err != nil { - logrus.Errorf("Failed to resolve session name for final additional endpoints poll: %v", err) + logrus.Warnf("Failed to fetch %s for dataset polling: %v", jobsEndpoint, err) return } - sessionName := filepath.Base(sessionRealDir) - r.pollAllEndpoints(sessionName) - logrus.Info("Finished processing additional endpoints") + var jobs []struct { + JobID string `json:"job_id"` + Status string `json:"status"` + } + if err := json.Unmarshal(body, &jobs); err != nil { + logrus.Warnf("Failed to parse %s response: %v", jobsEndpoint, err) + return + } + + for i, job := range jobs { + // Bail out rather than letting every remaining job log its own failure. + if ctx.Err() != nil { + logrus.Warnf("Stopped dataset polling after %d/%d jobs: %v", i, len(jobs), ctx.Err()) + return + } + // job_id is empty for submission jobs whose driver has not started yet. + if job.JobID == "" { + continue + } + if _, ok := state.done[job.JobID]; ok { + continue + } + + outcome := r.pollSingleEndpoint(ctx, dataDatasetsEndpointPrefix+job.JobID, sessionName) + if !terminalJobStatuses[job.Status] { + continue + } + switch outcome { + case pollStored: + state.done[job.JobID] = struct{}{} + case pollSkippedEmpty: + state.emptyRuns[job.JobID]++ + if state.emptyRuns[job.JobID] >= terminalEmptyPollsBeforeGivingUp { + state.done[job.JobID] = struct{}{} + } + case pollFailed: + // Retry next cycle so a transient dashboard error does not lose datasets. + } + } } -// pollAllEndpoints fetches all configured additional endpoints and stores their responses. -func (r *RayLogHandler) pollAllEndpoints(sessionName string) { - for _, endpoint := range r.AdditionalEndpoints { - r.pollSingleEndpoint(endpoint, sessionName) +func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessionName string) pollOutcome { + body, err := r.fetchEndpoint(ctx, endpoint) + if err != nil { + logrus.Warnf("Failed to poll endpoint %s: %v", endpoint, err) + return pollFailed + } + + if isEmptyPayload(endpoint, body) { + logrus.Debugf("Skipping %s: nothing to store", endpoint) + return pollSkippedEmpty } + + storageKey := utils.EndpointPathToStorageKey(endpoint) + objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) + if err := r.Writer.WriteFile(objectKey, bytes.NewReader(body)); err != nil { + logrus.Errorf("Failed to store endpoint %s at %s: %v", endpoint, objectKey, err) + return pollFailed + } + + logrus.Infof("Successfully stored endpoint %s at %s (%d bytes)", endpoint, objectKey, len(body)) + return pollStored } -// pollSingleEndpoint fetches a single endpoint from the Ray Dashboard and writes -// the response to storage. -func (r *RayLogHandler) pollSingleEndpoint(endpoint, sessionName string) { +// isEmptyPayload reports whether a response carries nothing worth storing. +// +// Overwriting a converged snapshot with an empty one is worse than not writing at all: a +// Ray head that is shutting down answers 200 with an empty body, and on replay that is +// indistinguishable from a cluster that never used the feature. The history server +// already synthesizes empty responses for these paths, so skipping the write costs +// nothing. +func isEmptyPayload(endpoint string, body []byte) bool { + switch { + case strings.HasPrefix(endpoint, dataDatasetsEndpointPrefix): + return !hasDatasets(body) + case endpoint == serveApplicationsEndpoint: + return !hasServeApplications(body) + default: + return false + } +} + +// hasServeApplications reports whether a Serve response lists at least one application. +// Unparsable bodies count as non-empty so unexpected shapes are stored, not dropped. +func hasServeApplications(body []byte) bool { + var resp struct { + Applications map[string]json.RawMessage `json:"applications"` + } + if err := json.Unmarshal(body, &resp); err != nil { + return true + } + return len(resp.Applications) > 0 +} + +// hasDatasets reports whether a datasets response carries at least one dataset. +// Unparsable bodies count as non-empty so unexpected shapes are stored, not dropped. +func hasDatasets(body []byte) bool { + var resp struct { + Datasets []json.RawMessage `json:"datasets"` + } + if err := json.Unmarshal(body, &resp); err != nil { + return true + } + return len(resp.Datasets) > 0 +} + +// fetchEndpoint performs a single GET against the Ray Dashboard and returns the +// response body. In-flight requests are canceled on shutdown. +func (r *RayLogHandler) fetchEndpoint(parent context.Context, endpoint string) ([]byte, error) { url := r.DashboardAddress + endpoint - ctx, cancel := context.WithTimeout(context.Background(), defaultRequestTimeout) + ctx, cancel := context.WithTimeout(parent, defaultRequestTimeout) + defer cancel() go func() { select { case <-r.ShutdownChan: @@ -106,37 +337,21 @@ func (r *RayLogHandler) pollSingleEndpoint(endpoint, sessionName string) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { - cancel() - logrus.Errorf("Failed to create request for additional endpoint %s: %v", endpoint, err) - return + return nil, fmt.Errorf("failed to create request: %w", err) } resp, err := r.HttpClient.Do(req) if err != nil { - cancel() - logrus.Warnf("Failed to fetch additional endpoint %s: %v", endpoint, err) - return + return nil, err } + defer resp.Body.Close() body, err := io.ReadAll(resp.Body) - resp.Body.Close() - cancel() if err != nil { - logrus.Warnf("Failed to read response body for additional endpoint %s: %v", endpoint, err) - return + return nil, fmt.Errorf("failed to read response body: %w", err) } - if resp.StatusCode != http.StatusOK { - logrus.Warnf("Additional endpoint %s returned status %d", endpoint, resp.StatusCode) - return + return nil, fmt.Errorf("unexpected status %d", resp.StatusCode) } - - storageKey := utils.EndpointPathToStorageKey(endpoint) - objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) - if err := r.Writer.WriteFile(objectKey, bytes.NewReader(body)); err != nil { - logrus.Errorf("Failed to store additional endpoint %s at %s: %v", endpoint, objectKey, err) - return - } - - logrus.Infof("Successfully stored additional endpoint %s at %s (%d bytes)", endpoint, objectKey, len(body)) + return body, nil } diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go new file mode 100644 index 00000000000..f5c7a4f9417 --- /dev/null +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go @@ -0,0 +1,514 @@ +package logcollector + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "sort" + "strings" + "sync" + "testing" + + . "github.com/onsi/gomega" + + "github.com/ray-project/kuberay/historyserver/pkg/utils" +) + +// fakeDashboard stands in for the Ray Dashboard, recording every path requested +// so tests can assert which endpoints the collector polled. +type fakeDashboard struct { + mu sync.Mutex + requests []string + // jobs is the /api/jobs/ response body. + jobs string + // datasets maps a job ID to its /api/data/datasets/{job_id} response body. + datasets map[string]string +} + +func (f *fakeDashboard) start(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + f.requests = append(f.requests, r.URL.RequestURI()) + f.mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == jobsEndpoint: + _, _ = w.Write([]byte(f.jobs)) + case r.URL.Path == serveApplicationsEndpoint: + // Non-empty by default: an empty Serve response is deliberately not stored. + _, _ = w.Write([]byte(`{"applications": {"app": {"status": "RUNNING"}}}`)) + case strings.HasPrefix(r.URL.Path, dataDatasetsEndpointPrefix): + jobID := strings.TrimPrefix(r.URL.Path, dataDatasetsEndpointPrefix) + body, ok := f.datasets[jobID] + if !ok { + body = `{"datasets": []}` + } + _, _ = w.Write([]byte(body)) + default: + _, _ = w.Write([]byte(`{}`)) + } + })) + t.Cleanup(srv.Close) + return srv +} + +func (f *fakeDashboard) requestsFor(prefix string) []string { + f.mu.Lock() + defer f.mu.Unlock() + var out []string + for _, req := range f.requests { + if strings.HasPrefix(req, prefix) { + out = append(out, req) + } + } + return out +} + +func newPollTestHandler(t *testing.T, dashboardAddr string) (*RayLogHandler, *MockStorageWriter) { + t.Helper() + writer := NewMockStorageWriter() + return &RayLogHandler{ + Writer: writer, + HttpClient: &http.Client{}, + ShutdownChan: make(chan struct{}), + ClusterDir: "cluster-dir", + DashboardAddress: dashboardAddr, + IsHead: true, + }, writer +} + +func writtenKeys(writer *MockStorageWriter) []string { + writer.mu.Lock() + defer writer.mu.Unlock() + keys := make([]string, 0, len(writer.writtenFiles)) + for k := range writer.writtenFiles { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// TestPollDataDatasetsFansOutPerJob verifies that job IDs are discovered from +// /api/jobs/, that blank IDs are skipped, and that each job with datasets gets +// its own storage object keyed by the frontend's request URI. +func TestPollDataDatasetsFansOutPerJob(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{ + jobs: `[ + {"job_id": "01000000", "status": "RUNNING"}, + {"job_id": null, "status": "PENDING"}, + {"job_id": "02000000", "status": "RUNNING"} + ]`, + datasets: map[string]string{ + "01000000": `{"datasets": [{"dataset": "ds_a", "job_id": "01000000"}]}`, + "02000000": `{"datasets": [{"dataset": "ds_b", "job_id": "02000000"}]}`, + }, + } + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState()) + + // The blank job_id is skipped. + g.Expect(dash.requestsFor(dataDatasetsEndpointPrefix)).To(ConsistOf( + "/api/data/datasets/01000000", + "/api/data/datasets/02000000", + )) + g.Expect(writtenKeys(writer)).To(Equal([]string{ + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__02000000", + })) +} + +// TestPollDataDatasetsSkipsEmptyResponse verifies that a job reporting no Ray +// Data datasets does not get an object written. Most jobs never use Ray Data, +// and storing the empty response would also let a stats-actor eviction replace +// datasets that were captured earlier. +func TestPollDataDatasetsSkipsEmptyResponse(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{ + jobs: `[{"job_id": "01000000", "status": "RUNNING"}]`, + // No entry, so the fake dashboard replies {"datasets": []}. + datasets: map[string]string{}, + } + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState()) + + g.Expect(dash.requestsFor(dataDatasetsEndpointPrefix)).To(HaveLen(1)) + g.Expect(writtenKeys(writer)).To(BeEmpty()) +} + +// TestPollDataDatasetsStopsPollingTerminalJobs verifies that a terminal job is +// fetched once and then skipped, so the per-cycle cost does not grow with the +// cluster's total job count, while a running job keeps being refreshed. +func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{ + jobs: `[ + {"job_id": "01000000", "status": "SUCCEEDED"}, + {"job_id": "02000000", "status": "RUNNING"} + ]`, + datasets: map[string]string{ + "01000000": `{"datasets": [{"dataset": "ds_done"}]}`, + "02000000": `{"datasets": [{"dataset": "ds_live"}]}`, + }, + } + srv := dash.start(t) + handler, _ := newPollTestHandler(t, srv.URL) + + state := newDatasetPollState() + handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state) + + g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(state.done).NotTo(HaveKey("02000000")) + // The terminal job is fetched only on the first cycle; the running one every cycle. + g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(1)) + g.Expect(dash.requestsFor("/api/data/datasets/02000000")).To(HaveLen(3)) +} + +// TestPollDataDatasetsRetriesFailedTerminalJob verifies that a terminal job +// whose fetch failed is retried on the next cycle rather than being marked +// captured, so a transient dashboard error does not lose its datasets. +func TestPollDataDatasetsRetriesFailedTerminalJob(t *testing.T) { + g := NewWithT(t) + + var mu sync.Mutex + attempts := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == jobsEndpoint { + _, _ = w.Write([]byte(`[{"job_id": "01000000", "status": "SUCCEEDED"}]`)) + return + } + mu.Lock() + attempts++ + first := attempts == 1 + mu.Unlock() + if first { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "ds_done"}]}`)) + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + + state := newDatasetPollState() + handler.pollDataDatasets(context.Background(), "session_1", state) + g.Expect(state.done).To(BeEmpty()) + g.Expect(writtenKeys(writer)).To(BeEmpty()) + + handler.pollDataDatasets(context.Background(), "session_1", state) + g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(writtenKeys(writer)).To(Equal([]string{ + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", + })) +} + +// TestPollDataDatasetsRetriesTerminalJobWithLateStats verifies a terminal job whose +// first datasets response is empty is polled again, because Ray Data registers its +// stats slightly after the job reports SUCCEEDED. +func TestPollDataDatasetsRetriesTerminalJobWithLateStats(t *testing.T) { + g := NewWithT(t) + + var mu sync.Mutex + attempts := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == jobsEndpoint { + _, _ = w.Write([]byte(`[{"job_id": "01000000", "status": "SUCCEEDED"}]`)) + return + } + mu.Lock() + attempts++ + first := attempts == 1 + mu.Unlock() + if first { + _, _ = w.Write([]byte(`{"datasets": []}`)) + return + } + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "ds_late"}]}`)) + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + + state := newDatasetPollState() + handler.pollDataDatasets(context.Background(), "session_1", state) + g.Expect(state.done).To(BeEmpty()) + g.Expect(writtenKeys(writer)).To(BeEmpty()) + + handler.pollDataDatasets(context.Background(), "session_1", state) + g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(writtenKeys(writer)).To(Equal([]string{ + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", + })) +} + +// TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob verifies the retry above is +// bounded, so jobs that never touch Ray Data stop costing a request every cycle. +func TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{ + jobs: `[{"job_id": "01000000", "status": "SUCCEEDED"}]`, + // No entry, so the fake dashboard always replies {"datasets": []}. + datasets: map[string]string{}, + } + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + state := newDatasetPollState() + for i := 0; i < 4; i++ { + handler.pollDataDatasets(context.Background(), "session_1", state) + } + + g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(terminalEmptyPollsBeforeGivingUp)) + g.Expect(writtenKeys(writer)).To(BeEmpty()) +} + +// TestPollAllEndpointsStoresStaticEndpoints verifies the built-in static +// endpoints are polled with the exact URIs the dashboard frontend requests, so +// the storage keys match what the history server looks up on replay. +func TestPollAllEndpointsStoresStaticEndpoints(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{jobs: `[]`} + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + + g.Expect(dash.requestsFor("/api/serve/applications/")).To(HaveLen(1)) + g.Expect(dash.requestsFor("/api/v0/placement_groups?detail=1&limit=10000")).To(HaveLen(1)) + g.Expect(writtenKeys(writer)).To(Equal([]string{ + "cluster-dir/session_1/fetched_endpoints/restful__api__serve__applications", + "cluster-dir/session_1/fetched_endpoints/restful__api__v0__placement_groups?detail=1&limit=10000", + })) +} + +// TestPollCycleFollowsSessionChange verifies that a Ray head restart, which starts a new +// session without restarting this sidecar, moves polling to the new session and resets +// the dataset state. Job IDs restart with the session, so a stale state would write into +// the dead session's directory and skip the new session's jobs as already captured. +func TestPollCycleFollowsSessionChange(t *testing.T) { + g := NewWithT(t) + + tmpRoot := t.TempDir() + t.Setenv("RAY_TMP_ROOT", tmpRoot) + symlink := filepath.Join(tmpRoot, "session_latest") + pointAt := func(session string) { + g.Expect(os.MkdirAll(filepath.Join(tmpRoot, session), 0o755)).To(Succeed()) + _ = os.Remove(symlink) + g.Expect(os.Symlink(filepath.Join(tmpRoot, session), symlink)).To(Succeed()) + } + + dash := &fakeDashboard{ + jobs: `[{"job_id": "01000000", "status": "SUCCEEDED"}]`, + datasets: map[string]string{"01000000": `{"datasets": [{"dataset": "ds"}]}`}, + } + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + pointAt("session_old") + session, state := handler.pollCycle(context.Background(), "session_old", newDatasetPollState()) + g.Expect(session).To(Equal("session_old")) + g.Expect(state.done).To(HaveKey("01000000")) + + pointAt("session_new") + session, state = handler.pollCycle(context.Background(), session, state) + g.Expect(session).To(Equal("session_new")) + g.Expect(state.done).To(HaveKey("01000000"), "the new session's job must be captured, not skipped") + + // The same job ID is stored once per session, not once overall. + g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(2)) + g.Expect(writtenKeys(writer)).To(ContainElements( + "cluster-dir/session_old/fetched_endpoints/restful__api__data__datasets__01000000", + "cluster-dir/session_new/fetched_endpoints/restful__api__data__datasets__01000000", + )) +} + +// TestPollAllEndpointsStopsWhenContextExpires verifies the shutdown budget bounds the +// whole pass. Without it, an unresponsive dashboard would cost one request timeout per +// endpoint and per job on the way out, overrunning the pod's grace period. +func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{jobs: `[{"job_id": "01000000", "status": "SUCCEEDED"}]`} + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + handler.pollAllEndpoints(ctx, "session_1", newDatasetPollState()) + + g.Expect(dash.requestsFor("/")).To(BeEmpty()) + g.Expect(writtenKeys(writer)).To(BeEmpty()) +} + +// TestPolledEndpointsAppendsConfiguredOnes verifies RAY_COLLECTOR_ADDITIONAL_ENDPOINTS +// adds to the built-in set rather than replacing it, and that repeating a +// built-in endpoint does not make it fetched twice per cycle. +func TestPolledEndpointsAppendsConfiguredOnes(t *testing.T) { + g := NewWithT(t) + + handler, _ := newPollTestHandler(t, "http://unused") + g.Expect(handler.polledEndpoints()).To(Equal(staticPolledEndpoints)) + + handler.AdditionalEndpoints = []string{ + "/nodes?view=summary", + serveApplicationsEndpoint, // already built in + "/nodes?view=summary", // repeated by the user + } + g.Expect(handler.polledEndpoints()).To(Equal([]string{ + serveApplicationsEndpoint, + placementGroupsEndpoint, + "/nodes?view=summary", + })) + + // The built-in list itself must not be mutated by the append. + g.Expect(staticPolledEndpoints).To(Equal([]string{ + serveApplicationsEndpoint, + placementGroupsEndpoint, + })) +} + +// TestPollAllEndpointsStoresConfiguredEndpoint verifies a configured endpoint is +// actually fetched and stored alongside the built-in ones. +func TestPollAllEndpointsStoresConfiguredEndpoint(t *testing.T) { + g := NewWithT(t) + + dash := &fakeDashboard{jobs: `[]`} + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + handler.AdditionalEndpoints = []string{"/nodes?view=summary"} + + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + + g.Expect(dash.requestsFor("/nodes?view=summary")).To(HaveLen(1)) + g.Expect(writtenKeys(writer)).To(ContainElement( + "cluster-dir/session_1/fetched_endpoints/restful__nodes?view=summary")) + g.Expect(writtenKeys(writer)).To(HaveLen(3)) +} + +// TestPollAllEndpointsKeepsConvergedServeSnapshot verifies an empty Serve response never +// replaces a converged one. A Ray head that is shutting down still answers 200 with no +// applications, and the shutdown pass writes to the same storage key, so without this the +// last thing written before deletion would wipe the snapshot the replay depends on. +func TestPollAllEndpointsKeepsConvergedServeSnapshot(t *testing.T) { + g := NewWithT(t) + + var mu sync.Mutex + converged := true + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == jobsEndpoint { + _, _ = w.Write([]byte(`[]`)) + return + } + mu.Lock() + defer mu.Unlock() + if converged { + _, _ = w.Write([]byte(`{"applications": {"app": {"status": "RUNNING"}}}`)) + return + } + _, _ = w.Write([]byte(`{"applications": {}}`)) + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + serveKey := "cluster-dir/session_1/fetched_endpoints/" + + utils.EndpointPathToStorageKey(serveApplicationsEndpoint) + + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + writer.mu.Lock() + stored := writer.writtenFiles[serveKey] + writer.mu.Unlock() + g.Expect(stored).To(ContainSubstring("RUNNING")) + + // The Serve controller stops before the dashboard does, so the next poll sees nothing. + mu.Lock() + converged = false + mu.Unlock() + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + + writer.mu.Lock() + defer writer.mu.Unlock() + g.Expect(writer.writtenFiles[serveKey]).To(Equal(stored), "the converged snapshot must survive") +} + +// TestHasServeApplications covers the guard that decides whether a Serve response is +// worth storing, including the deliberate choice to store unparsable bodies. +func TestHasServeApplications(t *testing.T) { + g := NewWithT(t) + + g.Expect(hasServeApplications([]byte(`{"applications": {}}`))).To(BeFalse()) + g.Expect(hasServeApplications([]byte(`{"applications": {"a": {}}}`))).To(BeTrue()) + g.Expect(hasServeApplications([]byte(`{}`))).To(BeFalse()) + // An unexpected shape is stored rather than silently dropped. + g.Expect(hasServeApplications([]byte(`not json`))).To(BeTrue()) +} + +// TestIsEmptyPayloadOnlyGuardsKnownEndpoints verifies the guard never suppresses an +// endpoint it does not understand, which would silently stop storing it. +func TestIsEmptyPayloadOnlyGuardsKnownEndpoints(t *testing.T) { + g := NewWithT(t) + + g.Expect(isEmptyPayload(serveApplicationsEndpoint, []byte(`{"applications": {}}`))).To(BeTrue()) + g.Expect(isEmptyPayload(dataDatasetsEndpointPrefix+"01000000", []byte(`{"datasets": []}`))).To(BeTrue()) + g.Expect(isEmptyPayload(placementGroupsEndpoint, []byte(`{}`))).To(BeFalse()) + g.Expect(isEmptyPayload("/nodes?view=summary", []byte(`{}`))).To(BeFalse()) +} + +// TestHasDatasets covers the guard that decides whether a datasets response is +// worth storing, including the deliberate choice to store unparsable bodies. +func TestHasDatasets(t *testing.T) { + g := NewWithT(t) + + g.Expect(hasDatasets([]byte(`{"datasets": []}`))).To(BeFalse()) + g.Expect(hasDatasets([]byte(`{"datasets": [{"dataset": "a"}]}`))).To(BeTrue()) + g.Expect(hasDatasets([]byte(`{}`))).To(BeFalse()) + // An unexpected shape is stored rather than silently dropped. + g.Expect(hasDatasets([]byte(`not json`))).To(BeTrue()) +} + +// TestTerminalJobStatusesMatchRay guards the status strings against drift from +// Ray's JobStatus enum, which is what /api/jobs/ serializes. +func TestTerminalJobStatusesMatchRay(t *testing.T) { + g := NewWithT(t) + + for _, status := range []string{"SUCCEEDED", "FAILED", "STOPPED"} { + g.Expect(terminalJobStatuses[status]).To(BeTrue(), "%s should be terminal", status) + } + for _, status := range []string{"PENDING", "RUNNING", ""} { + g.Expect(terminalJobStatuses[status]).To(BeFalse(), "%q should not be terminal", status) + } +} + +// TestFakeDashboardJobsShapeMatchesRay documents the /api/jobs/ fields the +// collector depends on, so a Ray-side rename shows up as a decode failure here. +func TestFakeDashboardJobsShapeMatchesRay(t *testing.T) { + g := NewWithT(t) + + var jobs []struct { + JobID string `json:"job_id"` + Status string `json:"status"` + } + body := `[{"job_id": "01000000", "submission_id": "raysubmit_x", "status": "SUCCEEDED", "type": "SUBMISSION"}]` + g.Expect(json.Unmarshal([]byte(body), &jobs)).To(Succeed()) + g.Expect(jobs).To(HaveLen(1)) + g.Expect(jobs[0].JobID).To(Equal("01000000")) + g.Expect(jobs[0].Status).To(Equal("SUCCEEDED")) +} diff --git a/historyserver/pkg/collector/types/types.go b/historyserver/pkg/collector/types/types.go index 9358841f38e..0f90cd08e21 100644 --- a/historyserver/pkg/collector/types/types.go +++ b/historyserver/pkg/collector/types/types.go @@ -24,6 +24,7 @@ type RayCollectorConfig struct { PushInterval time.Duration DashboardAddress string + // AdditionalEndpoints are polled on top of the collector's built-in set. AdditionalEndpoints []string EndpointPollInterval time.Duration diff --git a/historyserver/pkg/historyserver/router.go b/historyserver/pkg/historyserver/router.go index 28eb9046ffd..194dcf1d477 100644 --- a/historyserver/pkg/historyserver/router.go +++ b/historyserver/pkg/historyserver/router.go @@ -1121,8 +1121,9 @@ func (s *ServerHandler) getAdditionalEndpoint(req *restful.Request, resp *restfu clusterLogPathPrefix := s.getClusterLogPathPrefix(req) // Use the full request URI (path + query) for storage key lookup. - // The collector stores keys using the full endpoint URL from RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, - // which may include query params (e.g., "/api/v0/placement_groups?detail=1&limit=10000"). + // The collector stores keys using the full endpoint URI it polls (see the + // collector's poll.go), which may include query params (e.g., + // "/api/v0/placement_groups?detail=1&limit=10000"). // RequestURI() includes query params when present, and equals URL.Path when absent. storageKey := utils.EndpointPathToStorageKey(req.Request.URL.RequestURI()) endpointPath := path.Join(sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) @@ -1173,6 +1174,14 @@ func emptyResponseForEndpoint(urlPath string) []byte { }) return data default: + // The collector stores nothing for a job that never used Ray Data, so replay + // must synthesize what a live dashboard returns instead of 404ing. + if strings.HasPrefix(trimmed, "/api/data/datasets/") { + data, _ := json.Marshal(map[string]interface{}{ + "datasets": []interface{}{}, + }) + return data + } return nil } } diff --git a/historyserver/test/e2e/collector_test.go b/historyserver/test/e2e/collector_test.go index 26d24f810d0..9266647fb4a 100644 --- a/historyserver/test/e2e/collector_test.go +++ b/historyserver/test/e2e/collector_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "fmt" "io" + "net/http" + "path" "path/filepath" "strings" "testing" @@ -80,6 +82,14 @@ func TestCollector(t *testing.T) { name: "Timezone: collector should fetch and store timezone endpoint data once on startup", testFunc: testCollectorStoresTimezone, }, + { + name: "Serve applications: collector should poll and store a converged Serve snapshot", + testFunc: testCollectorStoresServeApplications, + }, + { + name: "Ray Data datasets: collector should store per-job datasets only for the job that used Ray Data", + testFunc: testCollectorStoresDataDatasets, + }, } for _, tt := range tests { @@ -435,8 +445,8 @@ func testCollectorStoresTimezone(test Test, g *WithT, namespace *corev1.Namespac // testCollectorStoresPlacementGroups verifies that the Head collector periodically polls // /api/v0/placement_groups from the Ray Dashboard and stores the result in S3. // -// The placement_groups endpoint is configured via RAY_COLLECTOR_ADDITIONAL_ENDPOINTS in -// raycluster.yaml and polled by PollAdditionalEndpointsPeriodically. +// The placement_groups endpoint is one of the collector's built-in polled endpoints +// (see staticPolledEndpoints in poll.go), polled by PollAdditionalEndpointsPeriodically. // // The test case follows these steps: // 1. Prepare test environment by applying a Ray cluster with the collector @@ -454,7 +464,8 @@ func testCollectorStoresPlacementGroups(test Test, g *WithT, namespace *corev1.N ApplyRayJobAndWaitForCompletion(test, g, namespace, rayCluster) sessionID := GetSessionIDFromHeadPod(test, g, rayCluster) - // The collector stores the endpoint with query params (as configured in RAY_COLLECTOR_ADDITIONAL_ENDPOINTS). + // The collector stores the endpoint with query params (matching the built-in + // placementGroupsEndpoint constant in the collector's poll.go). storageKey := utils.EndpointPathToStorageKey("/api/v0/placement_groups?detail=1&limit=10000") sessionDir := clusterlogs.SessionDir("log", "", "", rayCluster.Namespace, rayCluster.Name, sessionID) pgKey := fmt.Sprintf("%s/%s/%s", sessionDir, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) @@ -649,3 +660,215 @@ func assertAllEventTypesCovered(test Test, g Gomega, events []rayEvent) { g.Expect(foundEventTypes[string(eventType)]).To(BeTrue(), "Event type %s not found", eventType) } } + +// readS3Object reads an object body, failing the enclosing Eventually if it is absent. +func readS3Object(g Gomega, s3Client *s3.S3, key string) []byte { + result, err := s3Client.GetObject(&s3.GetObjectInput{ + Bucket: aws.String(S3BucketName), + Key: aws.String(key), + }) + g.Expect(err).NotTo(HaveOccurred()) + defer result.Body.Close() + + body, err := io.ReadAll(result.Body) + g.Expect(err).NotTo(HaveOccurred()) + return body +} + +// listFetchedEndpoints returns the keys of polled-endpoint objects whose storage key +// starts with storageKeyPrefix, anywhere under clusterPrefix. +// +// Listing beats constructing the key: the session name sits between the two and would +// otherwise have to be read from the head pod, which these fixtures delete on their own. +func listFetchedEndpoints(g Gomega, s3Client *s3.S3, clusterPrefix, storageKeyPrefix string) []string { + marker := "/" + utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME + "/" + + var keys []string + err := s3Client.ListObjectsV2Pages(&s3.ListObjectsV2Input{ + Bucket: aws.String(S3BucketName), + Prefix: aws.String(clusterPrefix + "/"), + }, func(page *s3.ListObjectsV2Output, _ bool) bool { + for _, obj := range page.Contents { + key := aws.StringValue(obj.Key) + idx := strings.Index(key, marker) + if idx >= 0 && strings.HasPrefix(key[idx+len(marker):], storageKeyPrefix) { + keys = append(keys, key) + } + } + return true + }) + g.Expect(err).NotTo(HaveOccurred()) + return keys +} + +// enterClusterForOwner points the client's session cookie at a cluster owned by a RayJob +// or RayService. setClusterContext cannot be reused: it hardcodes the raycluster kind, +// and for the other two kinds enter_cluster takes the owner's name and resolves the +// generated cluster name itself. +func enterClusterForOwner(test Test, g *WithT, client *http.Client, historyServerURL, namespace, ownerKind, ownerName, clusterName, session string) { + enterURL := fmt.Sprintf("%s/enter_cluster/%s/%s/%s/%s", historyServerURL, namespace, ownerKind, ownerName, session) + LogWithTimestamp(test.T(), "Setting cluster context: %s", enterURL) + + g.Eventually(func(gg Gomega) { + var result map[string]any + gg.Expect(json.Unmarshal(getHistoryServerJSON(gg, client, enterURL), &result)).To(Succeed()) + gg.Expect(result["result"]).To(Equal("success")) + gg.Expect(result["name"]).To(Equal(clusterName), "enter_cluster should resolve the owner to its generated cluster") + gg.Expect(result["session"]).To(Equal(session)) + }, TestTimeoutShort).Should(Succeed()) +} + +// getHistoryServerJSON GETs a history server URL and returns the body, requiring 200. +func getHistoryServerJSON(g Gomega, client *http.Client, url string) []byte { + resp, err := client.Get(url) + g.Expect(err).NotTo(HaveOccurred()) + defer resp.Body.Close() + g.Expect(resp.StatusCode).To(Equal(http.StatusOK)) + + body, err := io.ReadAll(resp.Body) + g.Expect(err).NotTo(HaveOccurred()) + return body +} + +// testCollectorStoresServeApplications verifies that the Head collector polls +// /api/serve/applications/ and stores a snapshot in which the Serve app has actually +// converged, not merely one that parses. +// +// Waiting for the object to exist is not a sufficient assertion: the collector writes +// every cycle, so it will happily store a valid response taken while the app is still +// DEPLOYING. The assertion therefore requires application status RUNNING and deployment +// status HEALTHY inside the stored bytes. +// +// A RayService is used rather than a RayJob because Serve outlives the driver: the app +// must still be reported once nothing is actively submitting work. +// +// The second half replays the dead cluster through the history server. Storing and +// serving both derive the storage key with EndpointPathToStorageKey but from different +// inputs — a constant in the collector, the live request URI in the server — so only a +// round trip proves the two agree. A mismatch would not even surface as an error: +// emptyResponseForEndpoint answers /api/serve/applications with {"applications": {}} and +// HTTP 200, which the frontend renders as "Serve not started". +func testCollectorStoresServeApplications(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { + rayService := ApplyRayServiceAndWaitForRunning(test, g, namespace) + clusterName := rayService.Status.ActiveServiceStatus.RayClusterName + clusterPrefix := clusterlogs.Prefix("log", utils.RayServiceKind, rayService.Name, namespace.Name, clusterName) + + // Matches serveApplicationsEndpoint in the collector's poll.go. + storageKey := utils.EndpointPathToStorageKey("/api/serve/applications/") + LogWithTimestamp(test.T(), "Waiting for a converged Serve snapshot under S3 prefix: %s", clusterPrefix) + + g.Eventually(func(gg Gomega) { + keys := listFetchedEndpoints(gg, s3Client, clusterPrefix, storageKey) + gg.Expect(keys).To(HaveLen(1), "the head collector stores exactly one Serve snapshot per session") + assertServeAppConverged(gg, readS3Object(gg, s3Client, keys[0])) + }, TestTimeoutMedium).Should(Succeed()) + + DeleteRayServiceAndWait(test, g, namespace.Name, rayService.Name, clusterName) + + ApplyHistoryServer(test, g, namespace, "") + historyServerURL := GetHistoryServerURL(test, g, namespace) + clusterInfo := getClusterFromList(test, g, historyServerURL, clusterName, namespace.Name) + g.Expect(clusterInfo.SessionName).NotTo(Equal(LiveSessionName), "Cluster should be a dead session after deletion") + + client := CreateHTTPClientWithCookieJar(g) + enterClusterForOwner(test, g, client, historyServerURL, namespace.Name, + utils.RayServiceKind, rayService.Name, clusterName, clusterInfo.SessionName) + + LogWithTimestamp(test.T(), "Replaying /api/serve/applications/ through the history server") + g.Eventually(func(gg Gomega) { + assertServeAppConverged(gg, getHistoryServerJSON(gg, client, historyServerURL+"/api/serve/applications/")) + }, TestTimeoutShort).Should(Succeed()) + + DeleteS3Bucket(test, g, s3Client) +} + +// assertServeAppConverged requires the snapshot to show the app actually running, not +// merely to parse: the collector writes every cycle, so a response captured mid-deploy +// is still valid JSON, and the history server's empty fallback is valid JSON too. +func assertServeAppConverged(g Gomega, body []byte) { + var response struct { + Applications map[string]struct { + Status string `json:"status"` + Deployments map[string]struct { + Status string `json:"status"` + } `json:"deployments"` + } `json:"applications"` + } + g.Expect(json.Unmarshal(body, &response)).To(Succeed(), "Serve response should be valid JSON") + + app, ok := response.Applications["history-e2e"] + g.Expect(ok).To(BeTrue(), "applications should contain history-e2e, got %v", response.Applications) + g.Expect(app.Status).To(Equal("RUNNING"), "history-e2e should have converged") + + deployment, ok := app.Deployments["NoOp"] + g.Expect(ok).To(BeTrue(), "history-e2e should contain the NoOp deployment") + g.Expect(deployment.Status).To(Equal("HEALTHY"), "NoOp replica should be healthy") +} + +// testCollectorStoresDataDatasets verifies that the Head collector discovers jobs via +// /api/jobs/ and stores /api/data/datasets/{job_id} for the job that used Ray Data. +// +// The job ID is assigned by Ray, so the object is located by prefix rather than guessed. +// Expecting exactly one object doubles as coverage for the empty-response rule: the +// dashboard and agent register their own jobs, and jobs without datasets must not be +// stored at all. +// +// The fixture sets shutdownAfterJobFinishes, so this also covers a cluster that tears +// itself down: the stored objects have to survive the cluster that produced them, and +// the history server then has to serve them back for the URI the frontend requests. +func testCollectorStoresDataDatasets(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { + rayJob := ApplyRayDataJobAndWaitForCompletion(test, g, namespace) + clusterName := rayJob.Status.RayClusterName + clusterPrefix := clusterlogs.Prefix("log", utils.RayJobKind, rayJob.Name, namespace.Name, clusterName) + + // "/api/data/datasets/" maps to "restful__api__data__datasets"; per-job keys append + // "__{job_id}", so this prefix matches every stored job. + storageKeyPrefix := utils.EndpointPathToStorageKey("/api/data/datasets/") + LogWithTimestamp(test.T(), "Waiting for a non-empty datasets object under S3 prefix: %s", clusterPrefix) + + var jobID string + g.Eventually(func(gg Gomega) { + keys := listFetchedEndpoints(gg, s3Client, clusterPrefix, storageKeyPrefix) + gg.Expect(keys).To(HaveLen(1), + "exactly one job used Ray Data, so exactly one datasets object should exist") + assertDatasetsNonEmpty(gg, readS3Object(gg, s3Client, keys[0])) + + // Ray assigns the job ID, so recover it from the key the collector chose. + jobID = strings.TrimPrefix(path.Base(keys[0]), storageKeyPrefix+"__") + gg.Expect(jobID).NotTo(BeEmpty()) + }, TestTimeoutMedium).Should(Succeed()) + + // shutdownAfterJobFinishes tears the cluster down on its own, which is what turns + // the session into a replayable one. + g.Eventually(func() error { + _, err := GetRayCluster(test, namespace.Name, clusterName) + return err + }, TestTimeoutMedium).Should(WithTransform(k8serrors.IsNotFound, BeTrue())) + + ApplyHistoryServer(test, g, namespace, "") + historyServerURL := GetHistoryServerURL(test, g, namespace) + clusterInfo := getClusterFromList(test, g, historyServerURL, clusterName, namespace.Name) + g.Expect(clusterInfo.SessionName).NotTo(Equal(LiveSessionName), "Cluster should be a dead session after shutdown") + + client := CreateHTTPClientWithCookieJar(g) + enterClusterForOwner(test, g, client, historyServerURL, namespace.Name, + utils.RayJobKind, rayJob.Name, clusterName, clusterInfo.SessionName) + + datasetsURL := fmt.Sprintf("%s/api/data/datasets/%s", historyServerURL, jobID) + LogWithTimestamp(test.T(), "Replaying %s through the history server", datasetsURL) + g.Eventually(func(gg Gomega) { + assertDatasetsNonEmpty(gg, getHistoryServerJSON(gg, client, datasetsURL)) + }, TestTimeoutShort).Should(Succeed()) + + DeleteS3Bucket(test, g, s3Client) +} + +// assertDatasetsNonEmpty requires real Ray Data stats: an empty response is never +// stored, so an empty one coming back means the object was not found. +func assertDatasetsNonEmpty(g Gomega, body []byte) { + var response struct { + Datasets []json.RawMessage `json:"datasets"` + } + g.Expect(json.Unmarshal(body, &response)).To(Succeed(), "datasets response should be valid JSON") + g.Expect(response.Datasets).NotTo(BeEmpty(), "datasets must be non-empty") +} diff --git a/historyserver/test/support/rayjob.go b/historyserver/test/support/rayjob.go index f6d7d059c70..7653daf84d6 100644 --- a/historyserver/test/support/rayjob.go +++ b/historyserver/test/support/rayjob.go @@ -11,6 +11,9 @@ import ( const ( rayJobManifestPath = "../../config/rayjob.yaml" + // rayDataManifestPath is self-contained: it brings up its own cluster, so the + // cluster name is only known from the RayJob status afterwards. + rayDataManifestPath = "../../config/ray-data.yaml" ) // ApplyRayJobAndWaitForCompletion applies a Ray job to the existing Ray cluster and waits for it to complete successfully. @@ -35,3 +38,28 @@ func ApplyRayJobAndWaitForCompletion(test Test, g *WithT, namespace *corev1.Name return rayJob } + +// ApplyRayDataJobAndWaitForCompletion applies the self-contained Ray Data RayJob and +// waits for it to succeed. The returned RayJob carries Status.RayClusterName, which is +// the only way to learn the cluster name KubeRay generated. +func ApplyRayDataJobAndWaitForCompletion(test Test, g *WithT, namespace *corev1.Namespace) *rayv1.RayJob { + rayJobFromYaml := DeserializeRayJobYAML(test, rayDataManifestPath) + rayJobFromYaml.Namespace = namespace.Name + + rayJob, err := test.Client().Ray().RayV1(). + RayJobs(namespace.Name). + Create(test.Ctx(), rayJobFromYaml, metav1.CreateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + LogWithTimestamp(test.T(), "Created RayJob %s/%s successfully", rayJob.Namespace, rayJob.Name) + + LogWithTimestamp(test.T(), "Waiting for RayJob %s/%s to complete successfully", rayJob.Namespace, rayJob.Name) + g.Eventually(RayJob(test, rayJob.Namespace, rayJob.Name), TestTimeoutMedium). + Should(WithTransform(RayJobStatus, Equal(rayv1.JobStatusSucceeded))) + + rayJob, err = GetRayJob(test, rayJob.Namespace, rayJob.Name) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rayJob.Status.RayClusterName).NotTo(BeEmpty()) + LogWithTimestamp(test.T(), "RayJob %s/%s succeeded on cluster %s", rayJob.Namespace, rayJob.Name, rayJob.Status.RayClusterName) + + return rayJob +} diff --git a/historyserver/test/support/rayservice.go b/historyserver/test/support/rayservice.go new file mode 100644 index 00000000000..1a456e9489c --- /dev/null +++ b/historyserver/test/support/rayservice.go @@ -0,0 +1,55 @@ +package support + +import ( + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1" + . "github.com/ray-project/kuberay/ray-operator/test/support" +) + +// rayServiceManifestPath is self-contained: it brings up its own cluster, so the +// cluster name is only known from the RayService status afterwards. +const rayServiceManifestPath = "../../config/rayservice.yaml" + +// ApplyRayServiceAndWaitForRunning applies the RayService and waits until Serve reports +// it Running. The returned RayService carries Status.ActiveServiceStatus.RayClusterName, +// which is the only way to learn the cluster name KubeRay generated. +func ApplyRayServiceAndWaitForRunning(test Test, g *WithT, namespace *corev1.Namespace) *rayv1.RayService { + rayServiceFromYaml := DeserializeRayServiceYAML(test, rayServiceManifestPath) + rayServiceFromYaml.Namespace = namespace.Name + + rayService, err := test.Client().Ray().RayV1(). + RayServices(namespace.Name). + Create(test.Ctx(), rayServiceFromYaml, metav1.CreateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + LogWithTimestamp(test.T(), "Created RayService %s/%s successfully", rayService.Namespace, rayService.Name) + + LogWithTimestamp(test.T(), "Waiting for RayService %s/%s to be running", rayService.Namespace, rayService.Name) + g.Eventually(RayService(test, rayService.Namespace, rayService.Name), TestTimeoutMedium). + Should(WithTransform(RayServiceStatus, Equal(rayv1.Running))) + + rayService, err = GetRayService(test, rayService.Namespace, rayService.Name) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(rayService.Status.ActiveServiceStatus.RayClusterName).NotTo(BeEmpty()) + LogWithTimestamp(test.T(), "RayService %s/%s is running on cluster %s", + rayService.Namespace, rayService.Name, rayService.Status.ActiveServiceStatus.RayClusterName) + + return rayService +} + +// DeleteRayServiceAndWait deletes a RayService and waits until its RayCluster is gone, +// which is what turns the session from live into a replayable one. +func DeleteRayServiceAndWait(test Test, g *WithT, namespace, name, clusterName string) { + err := test.Client().Ray().RayV1().RayServices(namespace).Delete(test.Ctx(), name, metav1.DeleteOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + LogWithTimestamp(test.T(), "Deleted RayService %s/%s", namespace, name) + + g.Eventually(func() error { + _, err := GetRayCluster(test, namespace, clusterName) + return err + }, TestTimeoutMedium).Should(WithTransform(k8serrors.IsNotFound, BeTrue())) + LogWithTimestamp(test.T(), "RayCluster %s/%s fully deleted", namespace, clusterName) +} From 34e5a50a5efc2f6dd6c14608b218270e5dca516d Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 17:31:33 -0500 Subject: [PATCH 02/10] [Fix] Use events.NewFakeRecorder in rayservice suspend unit test RayServiceReconciler.Recorder is events.EventRecorder, so record.NewFakeRecorder does not compile. Every other test in this file already uses events. --- ray-operator/controllers/ray/rayservice_controller_unit_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ray-operator/controllers/ray/rayservice_controller_unit_test.go b/ray-operator/controllers/ray/rayservice_controller_unit_test.go index aa19ff63cdf..5065fc76080 100644 --- a/ray-operator/controllers/ray/rayservice_controller_unit_test.go +++ b/ray-operator/controllers/ray/rayservice_controller_unit_test.go @@ -2634,7 +2634,7 @@ func TestHandleSuspendResumeResetsReadyToInitializing(t *testing.T) { r := &RayServiceReconciler{ Client: fakeClient, Scheme: scheme.Scheme, - Recorder: record.NewFakeRecorder(10), + Recorder: events.NewFakeRecorder(10), } _, err := r.handleSuspend(ctx, rs) From 29d50887131eee3aa4326854d0aee294c7a59863 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 17:59:53 -0500 Subject: [PATCH 03/10] [History Server] Mirror live Serve state and stop polling at shutdown 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. --- historyserver/cmd/collector/main.go | 9 +- historyserver/config/ray-data.yaml | 26 +-- .../config/raycluster-azureblob.yaml | 14 +- historyserver/config/raycluster-gcs.yaml | 14 +- historyserver/config/raycluster.yaml | 14 +- historyserver/config/rayservice.yaml | 13 +- .../runtime/logcollector/collector.go | 8 +- .../logcollector/runtime/logcollector/poll.go | 128 +++++-------- .../runtime/logcollector/poll_test.go | 180 +++++++++--------- historyserver/pkg/historyserver/router.go | 3 +- historyserver/test/e2e/collector_test.go | 57 ++---- historyserver/test/support/rayjob.go | 8 +- historyserver/test/support/rayservice.go | 10 +- 13 files changed, 188 insertions(+), 296 deletions(-) diff --git a/historyserver/cmd/collector/main.go b/historyserver/cmd/collector/main.go index bd886ce5722..ba56eb836ac 100644 --- a/historyserver/cmd/collector/main.go +++ b/historyserver/cmd/collector/main.go @@ -156,9 +156,8 @@ func main() { } } - // An unusable poll interval falls back to the default instead of exiting: the - // collector is a sidecar in the Ray head pod, so crash-looping on a bad - // observability knob would take the head out of its Service endpoints. + // Fall back instead of exiting: crash-looping this sidecar would take the head pod + // out of its Service endpoints. endpointPollInterval := 30 * time.Second if v := os.Getenv("RAY_COLLECTOR_POLL_INTERVAL"); v != "" { parsed, err := time.ParseDuration(v) @@ -217,9 +216,7 @@ func main() { sessionName := path.Base(activeSessionDir) - // The collector always runs as a sidecar in the Ray head pod, so the dashboard is - // reachable on localhost at Ray's default port. Only the head collector uses this. - // Override it when the dashboard listens on a non-default port. + // The head collector shares a pod with the dashboard; override for non-default ports. dashboardAddress := "http://localhost:8265" if v := os.Getenv("RAY_DASHBOARD_ADDRESS"); v != "" { dashboardAddress = v diff --git a/historyserver/config/ray-data.yaml b/historyserver/config/ray-data.yaml index 18526a57950..877711e980b 100644 --- a/historyserver/config/ray-data.yaml +++ b/historyserver/config/ray-data.yaml @@ -3,26 +3,21 @@ kind: RayJob metadata: name: rayjob-ray-data spec: - # Self-contained: this RayJob brings up its own cluster instead of attaching to an - # existing one via clusterSelector, so it also exercises the collector's shutdown path. + # Self-contained: brings up its own cluster; also exercises the collector's shutdown path. shutdownAfterJobFinishes: true - # Keeps the cluster alive long enough for at least one polling cycle after the job - # succeeds. Without it the cluster is deleted immediately and the datasets would only - # be captured by the collector's best-effort final poll during shutdown. + # Long enough for one polling cycle after the job succeeds. ttlSecondsAfterFinished: 30 entrypoint: | python -c " import ray ray.init() - # materialize() is required: an unexecuted Dataset produces no stats, so - # /api/data/datasets/{job_id} would stay empty. + # materialize() is required: an unexecuted Dataset produces no stats. ds = ray.data.range(100).map_batches(lambda batch: batch).materialize() print(f'Dataset rows: {ds.count()}') " rayClusterSpec: - # Head-only on purpose: worker collectors need the head Service FQDN in FQ_RAY_IP, - # which cannot be written here because KubeRay generates the cluster name. + # Head-only: worker collectors would need the generated head Service FQDN in FQ_RAY_IP. headGroupSpec: rayStartParams: dashboard-host: 0.0.0.0 @@ -69,8 +64,7 @@ spec: valueFrom: fieldRef: fieldPath: status.podIP - # KubeRay generates the cluster name, so these are read back from the labels - # it stamps on the pod rather than hardcoded. + # KubeRay generates the cluster name; read it back from the pod label. - name: RAY_CLUSTER_NAME valueFrom: fieldRef: @@ -79,21 +73,17 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - # Hardcoded, unlike the cluster name: KubeRay puts ray.io/originated-from-* - # on the RayCluster but not on the pod, so the downward API cannot read them. - # Must match metadata.name above. + # ray.io/originated-from-* labels are on the RayCluster, not the pod; must match metadata.name. - name: OWNER_KIND value: "RayJob" - name: OWNER_NAME value: "rayjob-ray-data" - # Only used to look up this pod's Ray NodeID, and the dashboard is in this - # same pod. A worker collector would need the head Service FQDN instead. + # Only used to look up this pod's Ray NodeID; the dashboard is in this pod. - name: FQ_RAY_IP value: "localhost" - name: RAY_TMP_ROOT value: *rayTmpRoot - # Shorter than the 30s default so a full cycle fits inside - # ttlSecondsAfterFinished above. + # Shorter than the 30s default so a cycle fits inside ttlSecondsAfterFinished. - name: RAY_COLLECTOR_POLL_INTERVAL value: "5s" - name: S3DISABLE_SSL diff --git a/historyserver/config/raycluster-azureblob.yaml b/historyserver/config/raycluster-azureblob.yaml index 11bab489c5c..be24c616a27 100644 --- a/historyserver/config/raycluster-azureblob.yaml +++ b/historyserver/config/raycluster-azureblob.yaml @@ -60,20 +60,14 @@ spec: value: raycluster-historyserver-head-svc.default.svc.cluster.local - name: RAY_TMP_ROOT value: *rayTmpRoot - # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same - # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard - # listens on a non-default port. Worker collectors do not use it. + # Optional; defaults to http://localhost:8265 (head only). # - name: RAY_DASHBOARD_ADDRESS # value: "http://localhost:9265" - # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray - # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # Optional; defaults to 30s. # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" - # The head collector always polls its built-in endpoints (Serve applications, - # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # is optional and adds more on top; uncomment to use it. Each comma-separated path - # must match what the dashboard frontend requests, query string included, because - # the storage key is derived from the request URI. + # 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" # reference: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite#connect-to-the-emulator-by-using-the-azure-storage-explorer diff --git a/historyserver/config/raycluster-gcs.yaml b/historyserver/config/raycluster-gcs.yaml index 4de79f1a242..d52dda7ee99 100644 --- a/historyserver/config/raycluster-gcs.yaml +++ b/historyserver/config/raycluster-gcs.yaml @@ -63,20 +63,14 @@ spec: value: *rayTmpRoot - name: GCS_BUCKET value: "${GCS_BUCKET}" - # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same - # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard - # listens on a non-default port. Worker collectors do not use it. + # Optional; defaults to http://localhost:8265 (head only). # - name: RAY_DASHBOARD_ADDRESS # value: "http://localhost:9265" - # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray - # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # Optional; defaults to 30s. # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" - # The head collector always polls its built-in endpoints (Serve applications, - # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # is optional and adds more on top; uncomment to use it. Each comma-separated path - # must match what the dashboard frontend requests, query string included, because - # the storage key is derived from the request URI. + # 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" command: diff --git a/historyserver/config/raycluster.yaml b/historyserver/config/raycluster.yaml index e76ac243ff4..1ebc70da5c7 100644 --- a/historyserver/config/raycluster.yaml +++ b/historyserver/config/raycluster.yaml @@ -66,20 +66,14 @@ spec: value: raycluster-historyserver-head-svc.default.svc.cluster.local - name: RAY_TMP_ROOT value: *rayTmpRoot - # RAY_DASHBOARD_ADDRESS points the head collector at the Ray Dashboard in the same - # pod. Optional; defaults to http://localhost:8265. Uncomment only if the dashboard - # listens on a non-default port. Worker collectors do not use it. + # Optional; defaults to http://localhost:8265 (head only). # - name: RAY_DASHBOARD_ADDRESS # value: "http://localhost:9265" - # RAY_COLLECTOR_POLL_INTERVAL sets how often the head collector polls the Ray - # Dashboard endpoints. Optional; defaults to 30s. Accepts Go duration format. + # Optional; defaults to 30s. # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" - # The head collector always polls its built-in endpoints (Serve applications, - # placement groups, and per-job Ray Data datasets). RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # is optional and adds more on top; uncomment to use it. Each comma-separated path - # must match what the dashboard frontend requests, query string included, because - # the storage key is derived from the request URI. + # 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" - name: S3DISABLE_SSL diff --git a/historyserver/config/rayservice.yaml b/historyserver/config/rayservice.yaml index ba2eae56cb0..13de62f1983 100644 --- a/historyserver/config/rayservice.yaml +++ b/historyserver/config/rayservice.yaml @@ -19,8 +19,7 @@ spec: ray_actor_options: num_cpus: 0.5 rayClusterConfig: - # Head-only on purpose: worker collectors need the head Service FQDN in FQ_RAY_IP, - # which cannot be written here because KubeRay generates the cluster name. + # Head-only: worker collectors would need the generated head Service FQDN in FQ_RAY_IP. headGroupSpec: rayStartParams: dashboard-host: 0.0.0.0 @@ -76,8 +75,7 @@ spec: valueFrom: fieldRef: fieldPath: status.podIP - # KubeRay generates the cluster name, so these are read back from the labels - # it stamps on the pod rather than hardcoded. + # KubeRay generates the cluster name; read it back from the pod label. - name: RAY_CLUSTER_NAME valueFrom: fieldRef: @@ -86,15 +84,12 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace - # Hardcoded, unlike the cluster name: KubeRay puts ray.io/originated-from-* - # on the RayCluster but not on the pod, so the downward API cannot read them. - # Must match metadata.name above. + # ray.io/originated-from-* labels are on the RayCluster, not the pod; must match metadata.name. - name: OWNER_KIND value: "RayService" - name: OWNER_NAME value: "rayservice-historyserver" - # Only used to look up this pod's Ray NodeID, and the dashboard is in this - # same pod. A worker collector would need the head Service FQDN instead. + # Only used to look up this pod's Ray NodeID; the dashboard is in this pod. - name: FQ_RAY_IP value: "localhost" - name: RAY_TMP_ROOT diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index 55e08bfbe86..9e1a8295974 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -86,15 +86,13 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { go r.WatchSessionLatestLoops() // Watch session_latest symlink changes go r.FetchAndStoreClusterMetadata() go r.FetchAndStoreTimezone() - go r.PollAdditionalEndpointsPeriodically() + go r.PollAdditionalEndpointsPeriodically(stop) } <-stop logrus.Info("Received stop signal, processing all logs...") - // The final endpoint poll races the Ray head's own shutdown: once the dashboard is - // gone the data is unrecoverable, while log files stay on local disk. Run it - // concurrently so it never queues behind a slow log upload. + // Endpoint data dies with the dashboard; log files stay on disk, so poll concurrently. var wg sync.WaitGroup if r.IsHead { wg.Go(r.processAdditionalEndpoints) @@ -102,7 +100,7 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { r.processSessionLatestLogs() wg.Wait() - // Only now, because pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. + // Only now: pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. close(r.ShutdownChan) return nil diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index 71b9b5bbe29..235a03d719d 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -17,36 +17,24 @@ import ( "github.com/ray-project/kuberay/historyserver/pkg/utils" ) -// These are polled periodically, unlike the one-shot startup endpoints in -// startup_endpoints.go. Each string must match the frontend's request URI, query -// string included: the history server derives the storage key from that URI, so a -// mismatch silently makes the stored object unreachable. +// Each string must match the frontend's request URI, query string included: the +// storage key is derived from it, so a mismatch makes the stored object unreachable. const ( - // Paths mirror the Ray Dashboard frontend: serve.ts, placementGroup.ts, data.ts. serveApplicationsEndpoint = "/api/serve/applications/" - - // detail=1 adds the bundle and stats fields PlacementGroupTable needs; - // limit=10000 matches the frontend default. - placementGroupsEndpoint = "/api/v0/placement_groups?detail=1&limit=10000" - - // Only used to discover job IDs. Its response is not stored: the history - // server rebuilds the job list from Ray events. + placementGroupsEndpoint = "/api/v0/placement_groups?detail=1&limit=10000" + // Only used to discover job IDs; its response is not stored. jobsEndpoint = "/api/jobs/" - - // Requested per job, where job_id is the Ray core job ID in hex (e.g. - // "01000000"), not the submission ID. + // Per job, where job_id is the hex core job ID (e.g. "01000000"), not the submission ID. dataDatasetsEndpointPrefix = "/api/data/datasets/" ) -// dataDatasetsEndpointPrefix is absent here: it needs a job ID, so pollDataDatasets -// handles it. +// dataDatasetsEndpointPrefix needs a job ID, so pollDataDatasets handles it. var staticPolledEndpoints = []string{ serveApplicationsEndpoint, placementGroupsEndpoint, } -// polledEndpoints deduplicates so that listing a built-in endpoint in -// RAY_COLLECTOR_ADDITIONAL_ENDPOINTS does not fetch and store it twice per cycle. +// polledEndpoints merges the built-in and configured endpoints, deduplicated. func (r *RayLogHandler) polledEndpoints() []string { endpoints := make([]string, 0, len(staticPolledEndpoints)+len(r.AdditionalEndpoints)) seen := make(map[string]struct{}, cap(endpoints)) @@ -62,8 +50,7 @@ func (r *RayLogHandler) polledEndpoints() []string { return endpoints } -// terminalJobStatuses are the /api/jobs/ statuses a job never leaves, so its Ray -// Data datasets only need to be stored once. +// Statuses a job never leaves, so its datasets only need to be stored once. // Ref: https://github.com/ray-project/ray/blob/ray-2.54.1/python/ray/dashboard/modules/job/common.py#L38-L50 var terminalJobStatuses = map[string]bool{ "SUCCEEDED": true, @@ -71,8 +58,7 @@ var terminalJobStatuses = map[string]bool{ "STOPPED": true, } -// pollOutcome distinguishes "nothing worth storing" from "could not store", which -// decides whether a terminal job is worth polling again. +// pollOutcome distinguishes "nothing worth storing" from "could not store". type pollOutcome int const ( @@ -81,16 +67,13 @@ const ( pollSkippedEmpty ) -// A job's stats can appear slightly after it reports terminal, so giving up on the -// first empty response would lose them permanently. +// Stats can appear slightly after a job reports terminal, so one empty response is not final. const terminalEmptyPollsBeforeGivingUp = 2 -// The final poll shares the pod's termination grace period (30s by default) with the -// log upload, so it gives up rather than risking a SIGKILL partway through. +// The final poll shares the pod's termination grace period, so it gives up rather than overrun it. const shutdownPollBudget = 10 * time.Second -// datasetPollState remembers across cycles which jobs no longer need their datasets -// fetched. The polling loop owns it exclusively, so it needs no lock. +// datasetPollState tracks jobs whose datasets no longer need fetching. Owned by one goroutine; no lock. type datasetPollState struct { done map[string]struct{} emptyRuns map[string]int @@ -103,13 +86,11 @@ func newDatasetPollState() *datasetPollState { } } -// PollAdditionalEndpointsPeriodically fetches the built-in endpoints, plus anything -// from RAY_COLLECTOR_ADDITIONAL_ENDPOINTS, on a timer until shutdown. Each response -// is stored at {ClusterDir}/{sessionName}/fetched_endpoints/{storageKey}, and each -// cycle overwrites the previous one. -func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { - // Blocking resolve is fine here but not in the loop below: on startup there is - // nothing to poll until session_latest exists. +// 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. +func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{}) { sessionName, err := r.resolveSessionName() if err != nil { logrus.Errorf("Failed to resolve session name for endpoint polling: %v", err) @@ -118,16 +99,14 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { logrus.Infof("Starting endpoint polling (interval=%v, endpoints=%v)", r.EndpointPollInterval, r.polledEndpoints()) state := newDatasetPollState() - - // Perform an initial poll immediately on startup. - r.pollAllEndpoints(context.Background(), sessionName, state) + r.pollAllEndpoints(context.Background(), sessionName, state, false) ticker := time.NewTicker(r.EndpointPollInterval) defer ticker.Stop() for { select { - case <-r.ShutdownChan: + case <-stop: logrus.Info("Shutdown signaled, stopping endpoint polling") return case <-ticker.C: @@ -136,12 +115,8 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically() { } } -// pollCycle runs one polling pass, re-resolving the session first. -// -// The Ray head container can restart on its own (an OOMKill restarts that container, not -// the pod), which starts a new session while this sidecar keeps running. Job IDs restart -// with it, so a stale state would both write into the dead session's directory and skip -// the new session's jobs as already captured. +// pollCycle runs one polling pass, re-resolving the session first: the Ray head container +// can restart alone, starting a new session (and new job IDs) while this sidecar lives on. func (r *RayLogHandler) pollCycle(ctx context.Context, sessionName string, state *datasetPollState) (string, *datasetPollState) { switch current, err := currentSessionName(); { case err != nil: @@ -151,12 +126,11 @@ func (r *RayLogHandler) pollCycle(ctx context.Context, sessionName string, state sessionName, state = current, newDatasetPollState() } - r.pollAllEndpoints(ctx, sessionName, state) + r.pollAllEndpoints(ctx, sessionName, state, false) return sessionName, state } -// currentSessionName resolves session_latest without retrying, for callers that must not -// block: the polling loop and the shutdown path. +// currentSessionName resolves session_latest without retrying, for callers that must not block. func currentSessionName() (string, error) { sessionRealDir, err := filepath.EvalSymlinks(utils.GetRaySessionLatestPath()) if err != nil { @@ -165,13 +139,7 @@ func currentSessionName() (string, error) { return filepath.Base(sessionRealDir), nil } -// processAdditionalEndpoints performs a final poll of all polled endpoints -// before shutdown. This mirrors processSessionLatestLogs as a shutdown cleanup step. -// -// Unlike PollAdditionalEndpointsPeriodically, this does NOT retry session name -// resolution because it runs during shutdown — if session_latest is gone (e.g., -// Ray head already exited), retrying would hang forever since ShutdownChan has -// not been closed yet. +// processAdditionalEndpoints performs one final poll before shutdown. func (r *RayLogHandler) processAdditionalEndpoints() { logrus.Info("Processing polled endpoints before shutdown") @@ -181,33 +149,30 @@ func (r *RayLogHandler) processAdditionalEndpoints() { return } - // One budget for the whole pass, not per request: the endpoints are fetched - // serially, so per-request timeouts would add up past the grace period. + // One budget for the whole pass: serial per-request timeouts would add up past the grace period. ctx, cancel := context.WithTimeout(context.Background(), shutdownPollBudget) defer cancel() - // Fresh state, so this final pass re-captures every job rather than trusting - // what the polling loop already stored. - r.pollAllEndpoints(ctx, sessionName, newDatasetPollState()) + // Fresh state, so this final pass re-captures every job. + r.pollAllEndpoints(ctx, sessionName, newDatasetPollState(), true) logrus.Info("Finished processing polled endpoints") } -func (r *RayLogHandler) pollAllEndpoints(ctx context.Context, sessionName string, state *datasetPollState) { +// finalPoll marks the shutdown pass, where an empty Serve response is distrusted. +func (r *RayLogHandler) pollAllEndpoints(ctx context.Context, sessionName string, state *datasetPollState, finalPoll bool) { for _, endpoint := range r.polledEndpoints() { if ctx.Err() != nil { logrus.Warnf("Stopped polling before %s: %v", endpoint, ctx.Err()) return } - r.pollSingleEndpoint(ctx, endpoint, sessionName) + r.pollSingleEndpoint(ctx, endpoint, sessionName, finalPoll) } - r.pollDataDatasets(ctx, sessionName, state) + r.pollDataDatasets(ctx, sessionName, state, finalPoll) } // pollDataDatasets stores one datasets object per job discovered via jobsEndpoint. -// Terminal jobs are dropped from future cycles once settled: without that the -// per-cycle cost would grow with the cluster's total job count forever, and each -// datasets request also makes the dashboard query Prometheus. -func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string, state *datasetPollState) { +// Settled terminal jobs are skipped so the per-cycle cost does not grow with job count. +func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string, state *datasetPollState, finalPoll bool) { body, err := r.fetchEndpoint(ctx, jobsEndpoint) if err != nil { logrus.Warnf("Failed to fetch %s for dataset polling: %v", jobsEndpoint, err) @@ -224,7 +189,6 @@ func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string } for i, job := range jobs { - // Bail out rather than letting every remaining job log its own failure. if ctx.Err() != nil { logrus.Warnf("Stopped dataset polling after %d/%d jobs: %v", i, len(jobs), ctx.Err()) return @@ -237,7 +201,7 @@ func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string continue } - outcome := r.pollSingleEndpoint(ctx, dataDatasetsEndpointPrefix+job.JobID, sessionName) + outcome := r.pollSingleEndpoint(ctx, dataDatasetsEndpointPrefix+job.JobID, sessionName, finalPoll) if !terminalJobStatuses[job.Status] { continue } @@ -250,19 +214,19 @@ func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string state.done[job.JobID] = struct{}{} } case pollFailed: - // Retry next cycle so a transient dashboard error does not lose datasets. + // Retried next cycle. } } } -func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessionName string) pollOutcome { +func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessionName string, finalPoll bool) pollOutcome { body, err := r.fetchEndpoint(ctx, endpoint) if err != nil { logrus.Warnf("Failed to poll endpoint %s: %v", endpoint, err) return pollFailed } - if isEmptyPayload(endpoint, body) { + if isEmptyPayload(endpoint, body, finalPoll) { logrus.Debugf("Skipping %s: nothing to store", endpoint) return pollSkippedEmpty } @@ -279,24 +243,20 @@ func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessio } // isEmptyPayload reports whether a response carries nothing worth storing. -// -// Overwriting a converged snapshot with an empty one is worse than not writing at all: a -// Ray head that is shutting down answers 200 with an empty body, and on replay that is -// indistinguishable from a cluster that never used the feature. The history server -// already synthesizes empty responses for these paths, so skipping the write costs -// nothing. -func isEmptyPayload(endpoint string, body []byte) bool { +// Datasets: empty can mean stats-actor eviction, so it never overwrites a snapshot. +// Serve: empty from a healthy cluster is the live truth and is stored, but on the final +// shutdown poll it usually means the Serve controller died before the dashboard. +func isEmptyPayload(endpoint string, body []byte, finalPoll bool) bool { switch { case strings.HasPrefix(endpoint, dataDatasetsEndpointPrefix): return !hasDatasets(body) - case endpoint == serveApplicationsEndpoint: + case finalPoll && endpoint == serveApplicationsEndpoint: return !hasServeApplications(body) default: return false } } -// hasServeApplications reports whether a Serve response lists at least one application. // Unparsable bodies count as non-empty so unexpected shapes are stored, not dropped. func hasServeApplications(body []byte) bool { var resp struct { @@ -308,7 +268,6 @@ func hasServeApplications(body []byte) bool { return len(resp.Applications) > 0 } -// hasDatasets reports whether a datasets response carries at least one dataset. // Unparsable bodies count as non-empty so unexpected shapes are stored, not dropped. func hasDatasets(body []byte) bool { var resp struct { @@ -320,8 +279,7 @@ func hasDatasets(body []byte) bool { return len(resp.Datasets) > 0 } -// fetchEndpoint performs a single GET against the Ray Dashboard and returns the -// response body. In-flight requests are canceled on shutdown. +// fetchEndpoint GETs one dashboard endpoint; in-flight requests are canceled on shutdown. func (r *RayLogHandler) fetchEndpoint(parent context.Context, endpoint string) ([]byte, error) { url := r.DashboardAddress + endpoint diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go index f5c7a4f9417..d696d580d03 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go @@ -11,14 +11,14 @@ import ( "strings" "sync" "testing" + "time" . "github.com/onsi/gomega" "github.com/ray-project/kuberay/historyserver/pkg/utils" ) -// fakeDashboard stands in for the Ray Dashboard, recording every path requested -// so tests can assert which endpoints the collector polled. +// fakeDashboard stands in for the Ray Dashboard, recording every requested path. type fakeDashboard struct { mu sync.Mutex requests []string @@ -93,9 +93,7 @@ func writtenKeys(writer *MockStorageWriter) []string { return keys } -// TestPollDataDatasetsFansOutPerJob verifies that job IDs are discovered from -// /api/jobs/, that blank IDs are skipped, and that each job with datasets gets -// its own storage object keyed by the frontend's request URI. +// TestPollDataDatasetsFansOutPerJob verifies per-job fan-out from /api/jobs/, skipping blank IDs. func TestPollDataDatasetsFansOutPerJob(t *testing.T) { g := NewWithT(t) @@ -113,7 +111,7 @@ func TestPollDataDatasetsFansOutPerJob(t *testing.T) { srv := dash.start(t) handler, writer := newPollTestHandler(t, srv.URL) - handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState()) + handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState(), false) // The blank job_id is skipped. g.Expect(dash.requestsFor(dataDatasetsEndpointPrefix)).To(ConsistOf( @@ -126,10 +124,8 @@ func TestPollDataDatasetsFansOutPerJob(t *testing.T) { })) } -// TestPollDataDatasetsSkipsEmptyResponse verifies that a job reporting no Ray -// Data datasets does not get an object written. Most jobs never use Ray Data, -// and storing the empty response would also let a stats-actor eviction replace -// datasets that were captured earlier. +// TestPollDataDatasetsSkipsEmptyResponse verifies an empty datasets response is never written, +// so a stats-actor eviction cannot replace datasets captured earlier. func TestPollDataDatasetsSkipsEmptyResponse(t *testing.T) { g := NewWithT(t) @@ -141,15 +137,14 @@ func TestPollDataDatasetsSkipsEmptyResponse(t *testing.T) { srv := dash.start(t) handler, writer := newPollTestHandler(t, srv.URL) - handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState()) + handler.pollDataDatasets(context.Background(), "session_1", newDatasetPollState(), false) g.Expect(dash.requestsFor(dataDatasetsEndpointPrefix)).To(HaveLen(1)) g.Expect(writtenKeys(writer)).To(BeEmpty()) } -// TestPollDataDatasetsStopsPollingTerminalJobs verifies that a terminal job is -// fetched once and then skipped, so the per-cycle cost does not grow with the -// cluster's total job count, while a running job keeps being refreshed. +// TestPollDataDatasetsStopsPollingTerminalJobs verifies a terminal job is fetched once +// while a running job keeps being refreshed. func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { g := NewWithT(t) @@ -167,9 +162,9 @@ func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { handler, _ := newPollTestHandler(t, srv.URL) state := newDatasetPollState() - handler.pollDataDatasets(context.Background(), "session_1", state) - handler.pollDataDatasets(context.Background(), "session_1", state) - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) + handler.pollDataDatasets(context.Background(), "session_1", state, false) + handler.pollDataDatasets(context.Background(), "session_1", state, false) g.Expect(state.done).To(HaveKey("01000000")) g.Expect(state.done).NotTo(HaveKey("02000000")) @@ -178,9 +173,8 @@ func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { g.Expect(dash.requestsFor("/api/data/datasets/02000000")).To(HaveLen(3)) } -// TestPollDataDatasetsRetriesFailedTerminalJob verifies that a terminal job -// whose fetch failed is retried on the next cycle rather than being marked -// captured, so a transient dashboard error does not lose its datasets. +// TestPollDataDatasetsRetriesFailedTerminalJob verifies a failed fetch is retried, so a +// transient dashboard error does not lose a terminal job's datasets. func TestPollDataDatasetsRetriesFailedTerminalJob(t *testing.T) { g := NewWithT(t) @@ -206,20 +200,19 @@ func TestPollDataDatasetsRetriesFailedTerminalJob(t *testing.T) { handler, writer := newPollTestHandler(t, srv.URL) state := newDatasetPollState() - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) g.Expect(state.done).To(BeEmpty()) g.Expect(writtenKeys(writer)).To(BeEmpty()) - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) g.Expect(state.done).To(HaveKey("01000000")) g.Expect(writtenKeys(writer)).To(Equal([]string{ "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", })) } -// TestPollDataDatasetsRetriesTerminalJobWithLateStats verifies a terminal job whose -// first datasets response is empty is polled again, because Ray Data registers its -// stats slightly after the job reports SUCCEEDED. +// TestPollDataDatasetsRetriesTerminalJobWithLateStats verifies an empty first response is +// polled again, because Ray Data registers stats slightly after the job reports SUCCEEDED. func TestPollDataDatasetsRetriesTerminalJobWithLateStats(t *testing.T) { g := NewWithT(t) @@ -245,19 +238,18 @@ func TestPollDataDatasetsRetriesTerminalJobWithLateStats(t *testing.T) { handler, writer := newPollTestHandler(t, srv.URL) state := newDatasetPollState() - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) g.Expect(state.done).To(BeEmpty()) g.Expect(writtenKeys(writer)).To(BeEmpty()) - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) g.Expect(state.done).To(HaveKey("01000000")) g.Expect(writtenKeys(writer)).To(Equal([]string{ "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", })) } -// TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob verifies the retry above is -// bounded, so jobs that never touch Ray Data stop costing a request every cycle. +// TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob verifies the retry is bounded. func TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob(t *testing.T) { g := NewWithT(t) @@ -271,7 +263,7 @@ func TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob(t *testing.T) { state := newDatasetPollState() for i := 0; i < 4; i++ { - handler.pollDataDatasets(context.Background(), "session_1", state) + handler.pollDataDatasets(context.Background(), "session_1", state, false) } g.Expect(state.done).To(HaveKey("01000000")) @@ -279,9 +271,8 @@ func TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob(t *testing.T) { g.Expect(writtenKeys(writer)).To(BeEmpty()) } -// TestPollAllEndpointsStoresStaticEndpoints verifies the built-in static -// endpoints are polled with the exact URIs the dashboard frontend requests, so -// the storage keys match what the history server looks up on replay. +// TestPollAllEndpointsStoresStaticEndpoints verifies the built-in endpoints are stored under +// the exact URIs the frontend requests. func TestPollAllEndpointsStoresStaticEndpoints(t *testing.T) { g := NewWithT(t) @@ -289,7 +280,7 @@ func TestPollAllEndpointsStoresStaticEndpoints(t *testing.T) { srv := dash.start(t) handler, writer := newPollTestHandler(t, srv.URL) - handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), false) g.Expect(dash.requestsFor("/api/serve/applications/")).To(HaveLen(1)) g.Expect(dash.requestsFor("/api/v0/placement_groups?detail=1&limit=10000")).To(HaveLen(1)) @@ -299,10 +290,8 @@ func TestPollAllEndpointsStoresStaticEndpoints(t *testing.T) { })) } -// TestPollCycleFollowsSessionChange verifies that a Ray head restart, which starts a new -// session without restarting this sidecar, moves polling to the new session and resets -// the dataset state. Job IDs restart with the session, so a stale state would write into -// the dead session's directory and skip the new session's jobs as already captured. +// TestPollCycleFollowsSessionChange verifies a Ray head restart moves polling to the new +// session and resets state, since job IDs restart with the session. func TestPollCycleFollowsSessionChange(t *testing.T) { g := NewWithT(t) @@ -340,9 +329,34 @@ func TestPollCycleFollowsSessionChange(t *testing.T) { )) } -// TestPollAllEndpointsStopsWhenContextExpires verifies the shutdown budget bounds the -// whole pass. Without it, an unresponsive dashboard would cost one request timeout per -// endpoint and per job on the way out, overrunning the pod's grace period. +// TestPeriodicPollingStopsOnShutdownSignal verifies the loop exits on the shutdown signal, +// not ShutdownChan, so no tick can overwrite the final shutdown snapshot. +func TestPeriodicPollingStopsOnShutdownSignal(t *testing.T) { + g := NewWithT(t) + + tmpRoot := t.TempDir() + t.Setenv("RAY_TMP_ROOT", tmpRoot) + g.Expect(os.MkdirAll(filepath.Join(tmpRoot, "session_1"), 0o755)).To(Succeed()) + g.Expect(os.Symlink(filepath.Join(tmpRoot, "session_1"), filepath.Join(tmpRoot, "session_latest"))).To(Succeed()) + + dash := &fakeDashboard{jobs: `[]`} + srv := dash.start(t) + handler, _ := newPollTestHandler(t, srv.URL) + handler.EndpointPollInterval = time.Hour + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + handler.PollAdditionalEndpointsPeriodically(stop) + close(done) + }() + + // ShutdownChan stays open; the stop signal alone must end the loop. + close(stop) + g.Eventually(done).Should(BeClosed()) +} + +// TestPollAllEndpointsStopsWhenContextExpires verifies the shutdown budget bounds the whole pass. func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { g := NewWithT(t) @@ -353,15 +367,14 @@ func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - handler.pollAllEndpoints(ctx, "session_1", newDatasetPollState()) + handler.pollAllEndpoints(ctx, "session_1", newDatasetPollState(), false) g.Expect(dash.requestsFor("/")).To(BeEmpty()) g.Expect(writtenKeys(writer)).To(BeEmpty()) } -// TestPolledEndpointsAppendsConfiguredOnes verifies RAY_COLLECTOR_ADDITIONAL_ENDPOINTS -// adds to the built-in set rather than replacing it, and that repeating a -// built-in endpoint does not make it fetched twice per cycle. +// TestPolledEndpointsAppendsConfiguredOnes verifies RAY_COLLECTOR_ADDITIONAL_ENDPOINTS adds +// to the built-in set, deduplicated. func TestPolledEndpointsAppendsConfiguredOnes(t *testing.T) { g := NewWithT(t) @@ -386,8 +399,7 @@ func TestPolledEndpointsAppendsConfiguredOnes(t *testing.T) { })) } -// TestPollAllEndpointsStoresConfiguredEndpoint verifies a configured endpoint is -// actually fetched and stored alongside the built-in ones. +// TestPollAllEndpointsStoresConfiguredEndpoint verifies a configured endpoint is stored too. func TestPollAllEndpointsStoresConfiguredEndpoint(t *testing.T) { g := NewWithT(t) @@ -396,7 +408,7 @@ func TestPollAllEndpointsStoresConfiguredEndpoint(t *testing.T) { handler, writer := newPollTestHandler(t, srv.URL) handler.AdditionalEndpoints = []string{"/nodes?view=summary"} - handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), false) g.Expect(dash.requestsFor("/nodes?view=summary")).To(HaveLen(1)) g.Expect(writtenKeys(writer)).To(ContainElement( @@ -404,15 +416,13 @@ func TestPollAllEndpointsStoresConfiguredEndpoint(t *testing.T) { g.Expect(writtenKeys(writer)).To(HaveLen(3)) } -// TestPollAllEndpointsKeepsConvergedServeSnapshot verifies an empty Serve response never -// replaces a converged one. A Ray head that is shutting down still answers 200 with no -// applications, and the shutdown pass writes to the same storage key, so without this the -// last thing written before deletion would wipe the snapshot the replay depends on. -func TestPollAllEndpointsKeepsConvergedServeSnapshot(t *testing.T) { +// TestServeSnapshotFollowsLiveButSurvivesShutdown verifies a periodic poll mirrors the live +// cluster (empty included) while the final shutdown poll cannot erase a converged snapshot. +func TestServeSnapshotFollowsLiveButSurvivesShutdown(t *testing.T) { g := NewWithT(t) var mu sync.Mutex - converged := true + serveBody := `{"applications": {"app": {"status": "RUNNING"}}}` srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == jobsEndpoint { _, _ = w.Write([]byte(`[]`)) @@ -420,37 +430,37 @@ func TestPollAllEndpointsKeepsConvergedServeSnapshot(t *testing.T) { } mu.Lock() defer mu.Unlock() - if converged { - _, _ = w.Write([]byte(`{"applications": {"app": {"status": "RUNNING"}}}`)) - return - } - _, _ = w.Write([]byte(`{"applications": {}}`)) + _, _ = w.Write([]byte(serveBody)) })) t.Cleanup(srv.Close) handler, writer := newPollTestHandler(t, srv.URL) serveKey := "cluster-dir/session_1/fetched_endpoints/" + utils.EndpointPathToStorageKey(serveApplicationsEndpoint) + storedServe := func() string { + writer.mu.Lock() + defer writer.mu.Unlock() + return writer.writtenFiles[serveKey] + } + setServeBody := func(body string) { + mu.Lock() + defer mu.Unlock() + serveBody = body + } - handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) - writer.mu.Lock() - stored := writer.writtenFiles[serveKey] - writer.mu.Unlock() - g.Expect(stored).To(ContainSubstring("RUNNING")) + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), false) + g.Expect(storedServe()).To(ContainSubstring("RUNNING")) - // The Serve controller stops before the dashboard does, so the next poll sees nothing. - mu.Lock() - converged = false - mu.Unlock() - handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState()) + // A final poll must not erase the converged snapshot with a dying dashboard's answer. + setServeBody(`{"applications": {}}`) + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), true) + g.Expect(storedServe()).To(ContainSubstring("RUNNING")) - writer.mu.Lock() - defer writer.mu.Unlock() - g.Expect(writer.writtenFiles[serveKey]).To(Equal(stored), "the converged snapshot must survive") + // A periodic poll mirrors the live cluster, empty included. + handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), false) + g.Expect(storedServe()).To(Equal(`{"applications": {}}`)) } -// TestHasServeApplications covers the guard that decides whether a Serve response is -// worth storing, including the deliberate choice to store unparsable bodies. func TestHasServeApplications(t *testing.T) { g := NewWithT(t) @@ -461,19 +471,19 @@ func TestHasServeApplications(t *testing.T) { g.Expect(hasServeApplications([]byte(`not json`))).To(BeTrue()) } -// TestIsEmptyPayloadOnlyGuardsKnownEndpoints verifies the guard never suppresses an -// endpoint it does not understand, which would silently stop storing it. +// TestIsEmptyPayloadOnlyGuardsKnownEndpoints verifies the guard never suppresses endpoints it +// does not understand. func TestIsEmptyPayloadOnlyGuardsKnownEndpoints(t *testing.T) { g := NewWithT(t) - g.Expect(isEmptyPayload(serveApplicationsEndpoint, []byte(`{"applications": {}}`))).To(BeTrue()) - g.Expect(isEmptyPayload(dataDatasetsEndpointPrefix+"01000000", []byte(`{"datasets": []}`))).To(BeTrue()) - g.Expect(isEmptyPayload(placementGroupsEndpoint, []byte(`{}`))).To(BeFalse()) - g.Expect(isEmptyPayload("/nodes?view=summary", []byte(`{}`))).To(BeFalse()) + // Serve is guarded only on the final shutdown poll; datasets always. + g.Expect(isEmptyPayload(serveApplicationsEndpoint, []byte(`{"applications": {}}`), true)).To(BeTrue()) + g.Expect(isEmptyPayload(serveApplicationsEndpoint, []byte(`{"applications": {}}`), false)).To(BeFalse()) + g.Expect(isEmptyPayload(dataDatasetsEndpointPrefix+"01000000", []byte(`{"datasets": []}`), false)).To(BeTrue()) + g.Expect(isEmptyPayload(placementGroupsEndpoint, []byte(`{}`), true)).To(BeFalse()) + g.Expect(isEmptyPayload("/nodes?view=summary", []byte(`{}`), true)).To(BeFalse()) } -// TestHasDatasets covers the guard that decides whether a datasets response is -// worth storing, including the deliberate choice to store unparsable bodies. func TestHasDatasets(t *testing.T) { g := NewWithT(t) @@ -484,8 +494,7 @@ func TestHasDatasets(t *testing.T) { g.Expect(hasDatasets([]byte(`not json`))).To(BeTrue()) } -// TestTerminalJobStatusesMatchRay guards the status strings against drift from -// Ray's JobStatus enum, which is what /api/jobs/ serializes. +// TestTerminalJobStatusesMatchRay guards against drift from Ray's JobStatus enum. func TestTerminalJobStatusesMatchRay(t *testing.T) { g := NewWithT(t) @@ -497,8 +506,7 @@ func TestTerminalJobStatusesMatchRay(t *testing.T) { } } -// TestFakeDashboardJobsShapeMatchesRay documents the /api/jobs/ fields the -// collector depends on, so a Ray-side rename shows up as a decode failure here. +// TestFakeDashboardJobsShapeMatchesRay documents the /api/jobs/ fields the collector depends on. func TestFakeDashboardJobsShapeMatchesRay(t *testing.T) { g := NewWithT(t) diff --git a/historyserver/pkg/historyserver/router.go b/historyserver/pkg/historyserver/router.go index 194dcf1d477..4797f6b2354 100644 --- a/historyserver/pkg/historyserver/router.go +++ b/historyserver/pkg/historyserver/router.go @@ -1174,8 +1174,7 @@ func emptyResponseForEndpoint(urlPath string) []byte { }) return data default: - // The collector stores nothing for a job that never used Ray Data, so replay - // must synthesize what a live dashboard returns instead of 404ing. + // Nothing is stored for a job that never used Ray Data; match the live dashboard. if strings.HasPrefix(trimmed, "/api/data/datasets/") { data, _ := json.Marshal(map[string]interface{}{ "datasets": []interface{}{}, diff --git a/historyserver/test/e2e/collector_test.go b/historyserver/test/e2e/collector_test.go index 9266647fb4a..2446666f33d 100644 --- a/historyserver/test/e2e/collector_test.go +++ b/historyserver/test/e2e/collector_test.go @@ -675,11 +675,8 @@ func readS3Object(g Gomega, s3Client *s3.S3, key string) []byte { return body } -// listFetchedEndpoints returns the keys of polled-endpoint objects whose storage key -// starts with storageKeyPrefix, anywhere under clusterPrefix. -// -// Listing beats constructing the key: the session name sits between the two and would -// otherwise have to be read from the head pod, which these fixtures delete on their own. +// listFetchedEndpoints lists polled-endpoint objects by storage-key prefix. Listing beats +// constructing the key: the session name would have to come from an already-deleted head pod. func listFetchedEndpoints(g Gomega, s3Client *s3.S3, clusterPrefix, storageKeyPrefix string) []string { marker := "/" + utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME + "/" @@ -701,10 +698,8 @@ func listFetchedEndpoints(g Gomega, s3Client *s3.S3, clusterPrefix, storageKeyPr return keys } -// enterClusterForOwner points the client's session cookie at a cluster owned by a RayJob -// or RayService. setClusterContext cannot be reused: it hardcodes the raycluster kind, -// and for the other two kinds enter_cluster takes the owner's name and resolves the -// generated cluster name itself. +// enterClusterForOwner sets the session cookie for a RayJob/RayService-owned cluster: +// enter_cluster takes the owner's name and resolves the generated cluster name itself. func enterClusterForOwner(test Test, g *WithT, client *http.Client, historyServerURL, namespace, ownerKind, ownerName, clusterName, session string) { enterURL := fmt.Sprintf("%s/enter_cluster/%s/%s/%s/%s", historyServerURL, namespace, ownerKind, ownerName, session) LogWithTimestamp(test.T(), "Setting cluster context: %s", enterURL) @@ -730,24 +725,10 @@ func getHistoryServerJSON(g Gomega, client *http.Client, url string) []byte { return body } -// testCollectorStoresServeApplications verifies that the Head collector polls -// /api/serve/applications/ and stores a snapshot in which the Serve app has actually -// converged, not merely one that parses. -// -// Waiting for the object to exist is not a sufficient assertion: the collector writes -// every cycle, so it will happily store a valid response taken while the app is still -// DEPLOYING. The assertion therefore requires application status RUNNING and deployment -// status HEALTHY inside the stored bytes. -// -// A RayService is used rather than a RayJob because Serve outlives the driver: the app -// must still be reported once nothing is actively submitting work. -// -// The second half replays the dead cluster through the history server. Storing and -// serving both derive the storage key with EndpointPathToStorageKey but from different -// inputs — a constant in the collector, the live request URI in the server — so only a -// round trip proves the two agree. A mismatch would not even surface as an error: -// emptyResponseForEndpoint answers /api/serve/applications with {"applications": {}} and -// HTTP 200, which the frontend renders as "Serve not started". +// testCollectorStoresServeApplications verifies the collector stores a converged Serve +// snapshot, then replays it through the history server after the RayService is deleted. +// The round trip matters: collector and server derive the storage key from different +// inputs, and a mismatch surfaces as a valid-but-empty 200, not an error. func testCollectorStoresServeApplications(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { rayService := ApplyRayServiceAndWaitForRunning(test, g, namespace) clusterName := rayService.Status.ActiveServiceStatus.RayClusterName @@ -782,9 +763,8 @@ func testCollectorStoresServeApplications(test Test, g *WithT, namespace *corev1 DeleteS3Bucket(test, g, s3Client) } -// assertServeAppConverged requires the snapshot to show the app actually running, not -// merely to parse: the collector writes every cycle, so a response captured mid-deploy -// is still valid JSON, and the history server's empty fallback is valid JSON too. +// assertServeAppConverged requires RUNNING/HEALTHY: a mid-deploy snapshot and the empty +// fallback are both valid JSON. func assertServeAppConverged(g Gomega, body []byte) { var response struct { Applications map[string]struct { @@ -805,17 +785,9 @@ func assertServeAppConverged(g Gomega, body []byte) { g.Expect(deployment.Status).To(Equal("HEALTHY"), "NoOp replica should be healthy") } -// testCollectorStoresDataDatasets verifies that the Head collector discovers jobs via -// /api/jobs/ and stores /api/data/datasets/{job_id} for the job that used Ray Data. -// -// The job ID is assigned by Ray, so the object is located by prefix rather than guessed. -// Expecting exactly one object doubles as coverage for the empty-response rule: the -// dashboard and agent register their own jobs, and jobs without datasets must not be -// stored at all. -// -// The fixture sets shutdownAfterJobFinishes, so this also covers a cluster that tears -// itself down: the stored objects have to survive the cluster that produced them, and -// the history server then has to serve them back for the URI the frontend requests. +// testCollectorStoresDataDatasets verifies per-job Ray Data snapshots: exactly one object +// (jobs without datasets are never stored), surviving a self-shutdown cluster, and served +// back by the history server for the URI the frontend requests. func testCollectorStoresDataDatasets(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { rayJob := ApplyRayDataJobAndWaitForCompletion(test, g, namespace) clusterName := rayJob.Status.RayClusterName @@ -863,8 +835,7 @@ func testCollectorStoresDataDatasets(test Test, g *WithT, namespace *corev1.Name DeleteS3Bucket(test, g, s3Client) } -// assertDatasetsNonEmpty requires real Ray Data stats: an empty response is never -// stored, so an empty one coming back means the object was not found. +// assertDatasetsNonEmpty requires real stats: empty coming back means the object was missing. func assertDatasetsNonEmpty(g Gomega, body []byte) { var response struct { Datasets []json.RawMessage `json:"datasets"` diff --git a/historyserver/test/support/rayjob.go b/historyserver/test/support/rayjob.go index 7653daf84d6..0ff13b9b8d1 100644 --- a/historyserver/test/support/rayjob.go +++ b/historyserver/test/support/rayjob.go @@ -11,8 +11,7 @@ import ( const ( rayJobManifestPath = "../../config/rayjob.yaml" - // rayDataManifestPath is self-contained: it brings up its own cluster, so the - // cluster name is only known from the RayJob status afterwards. + // Self-contained; the generated cluster name is only known from the RayJob status. rayDataManifestPath = "../../config/ray-data.yaml" ) @@ -39,9 +38,8 @@ func ApplyRayJobAndWaitForCompletion(test Test, g *WithT, namespace *corev1.Name return rayJob } -// ApplyRayDataJobAndWaitForCompletion applies the self-contained Ray Data RayJob and -// waits for it to succeed. The returned RayJob carries Status.RayClusterName, which is -// the only way to learn the cluster name KubeRay generated. +// ApplyRayDataJobAndWaitForCompletion applies the Ray Data RayJob and waits for success; +// the returned Status.RayClusterName is the only way to learn the generated cluster name. func ApplyRayDataJobAndWaitForCompletion(test Test, g *WithT, namespace *corev1.Namespace) *rayv1.RayJob { rayJobFromYaml := DeserializeRayJobYAML(test, rayDataManifestPath) rayJobFromYaml.Namespace = namespace.Name diff --git a/historyserver/test/support/rayservice.go b/historyserver/test/support/rayservice.go index 1a456e9489c..5adabb03e88 100644 --- a/historyserver/test/support/rayservice.go +++ b/historyserver/test/support/rayservice.go @@ -10,13 +10,10 @@ import ( . "github.com/ray-project/kuberay/ray-operator/test/support" ) -// rayServiceManifestPath is self-contained: it brings up its own cluster, so the -// cluster name is only known from the RayService status afterwards. +// Self-contained; the generated cluster name is only known from the RayService status. const rayServiceManifestPath = "../../config/rayservice.yaml" -// ApplyRayServiceAndWaitForRunning applies the RayService and waits until Serve reports -// it Running. The returned RayService carries Status.ActiveServiceStatus.RayClusterName, -// which is the only way to learn the cluster name KubeRay generated. +// ApplyRayServiceAndWaitForRunning applies the RayService and waits until it is Running. func ApplyRayServiceAndWaitForRunning(test Test, g *WithT, namespace *corev1.Namespace) *rayv1.RayService { rayServiceFromYaml := DeserializeRayServiceYAML(test, rayServiceManifestPath) rayServiceFromYaml.Namespace = namespace.Name @@ -40,8 +37,7 @@ func ApplyRayServiceAndWaitForRunning(test Test, g *WithT, namespace *corev1.Nam return rayService } -// DeleteRayServiceAndWait deletes a RayService and waits until its RayCluster is gone, -// which is what turns the session from live into a replayable one. +// DeleteRayServiceAndWait deletes a RayService and waits until its RayCluster is gone. func DeleteRayServiceAndWait(test Test, g *WithT, namespace, name, clusterName string) { err := test.Client().Ray().RayV1().RayServices(namespace).Delete(test.Ctx(), name, metav1.DeleteOptions{}) g.Expect(err).NotTo(HaveOccurred()) From df8f4bf7df9e28bc97ee7779ca8cdb5f3d226f1e Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 18:54:54 -0500 Subject: [PATCH 04/10] update Signed-off-by: Future-Outlier --- historyserver/config/ray-data.yaml | 6 ++ .../runtime/logcollector/collector.go | 9 +- .../logcollector/runtime/logcollector/poll.go | 35 +++++++- .../runtime/logcollector/poll_test.go | 37 ++++++++ historyserver/test/e2e/collector_test.go | 90 ++++++++----------- 5 files changed, 120 insertions(+), 57 deletions(-) diff --git a/historyserver/config/ray-data.yaml b/historyserver/config/ray-data.yaml index 877711e980b..6049cdd2bce 100644 --- a/historyserver/config/ray-data.yaml +++ b/historyserver/config/ray-data.yaml @@ -10,8 +10,14 @@ spec: entrypoint: | python -c " import ray + from ray.util.placement_group import placement_group ray.init() + # Detached so the PG outlives the job and shows beside Ray Data on the job page. + pg = placement_group([{'CPU': 0.1}], lifetime='detached', name='demo_pg') + ray.get(pg.ready()) + print(f'Placement group created: {pg.bundle_specs}') + # materialize() is required: an unexecuted Dataset produces no stats. ds = ray.data.range(100).map_batches(lambda batch: batch).materialize() print(f'Dataset rows: {ds.count()}') diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index 9e1a8295974..787f5f33978 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -82,11 +82,16 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { // uploads from previous runs are resumed. go r.WatchPrevLogsLoops() go r.PollActiveSessionChanges() + var periodicPollDone chan struct{} if r.IsHead { go r.WatchSessionLatestLoops() // Watch session_latest symlink changes go r.FetchAndStoreClusterMetadata() go r.FetchAndStoreTimezone() - go r.PollAdditionalEndpointsPeriodically(stop) + periodicPollDone = make(chan struct{}) + go func() { + defer close(periodicPollDone) + r.PollAdditionalEndpointsPeriodically(stop) + }() } <-stop @@ -95,6 +100,8 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { // Endpoint data dies with the dashboard; log files stay on disk, so poll concurrently. var wg sync.WaitGroup if r.IsHead { + // Join the periodic poller first so a half-finished cycle cannot outlive the final poll. + <-periodicPollDone wg.Go(r.processAdditionalEndpoints) } r.processSessionLatestLogs() diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index 235a03d719d..ad9e319d374 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -90,8 +90,20 @@ func newDatasetPollState() *datasetPollState { // 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. func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{}) { - sessionName, err := r.resolveSessionName() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + select { + case <-stop: + cancel() + case <-ctx.Done(): + } + }() + + sessionName, err := waitForSessionName(ctx) if err != nil { logrus.Errorf("Failed to resolve session name for endpoint polling: %v", err) return @@ -99,7 +111,7 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{} logrus.Infof("Starting endpoint polling (interval=%v, endpoints=%v)", r.EndpointPollInterval, r.polledEndpoints()) state := newDatasetPollState() - r.pollAllEndpoints(context.Background(), sessionName, state, false) + r.pollAllEndpoints(ctx, sessionName, state, false) ticker := time.NewTicker(r.EndpointPollInterval) defer ticker.Stop() @@ -110,7 +122,24 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{} logrus.Info("Shutdown signaled, stopping endpoint polling") return case <-ticker.C: - sessionName, state = r.pollCycle(context.Background(), sessionName, state) + sessionName, state = r.pollCycle(ctx, sessionName, state) + } + } +} + +// waitForSessionName resolves session_latest, retrying until ctx is canceled: +// at startup the symlink appears only once Ray has bootstrapped. +func waitForSessionName(ctx context.Context) (string, error) { + for { + name, err := currentSessionName() + if err == nil { + return name, nil + } + logrus.Warnf("session_latest symlink not ready: %v, retrying in %v", err, defaultRetryInterval) + select { + case <-ctx.Done(): + return "", ctx.Err() + case <-time.After(defaultRetryInterval): } } } diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go index d696d580d03..46d0732965e 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go @@ -356,6 +356,43 @@ func TestPeriodicPollingStopsOnShutdownSignal(t *testing.T) { g.Eventually(done).Should(BeClosed()) } +// TestPeriodicPollingCancelsInFlightRequestOnShutdown verifies the stop signal aborts a +// cycle mid-request: Run joins this goroutine before the final poll, so a blocked request +// must neither stall shutdown nor store anything after being canceled. +func TestPeriodicPollingCancelsInFlightRequestOnShutdown(t *testing.T) { + g := NewWithT(t) + + tmpRoot := t.TempDir() + t.Setenv("RAY_TMP_ROOT", tmpRoot) + g.Expect(os.MkdirAll(filepath.Join(tmpRoot, "session_1"), 0o755)).To(Succeed()) + g.Expect(os.Symlink(filepath.Join(tmpRoot, "session_1"), filepath.Join(tmpRoot, "session_latest"))).To(Succeed()) + + var entered sync.Once + enteredCh := make(chan struct{}) + release := make(chan struct{}) + defer close(release) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + entered.Do(func() { close(enteredCh) }) + <-release + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + handler.EndpointPollInterval = time.Hour + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + handler.PollAdditionalEndpointsPeriodically(stop) + close(done) + }() + + g.Eventually(enteredCh).Should(BeClosed()) + close(stop) + g.Eventually(done, "5s").Should(BeClosed()) + g.Expect(writtenKeys(writer)).To(BeEmpty()) +} + // TestPollAllEndpointsStopsWhenContextExpires verifies the shutdown budget bounds the whole pass. func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { g := NewWithT(t) diff --git a/historyserver/test/e2e/collector_test.go b/historyserver/test/e2e/collector_test.go index 2446666f33d..e0d2835bfae 100644 --- a/historyserver/test/e2e/collector_test.go +++ b/historyserver/test/e2e/collector_test.go @@ -442,77 +442,61 @@ func testCollectorStoresTimezone(test Test, g *WithT, namespace *corev1.Namespac DeleteS3Bucket(test, g, s3Client) } -// testCollectorStoresPlacementGroups verifies that the Head collector periodically polls -// /api/v0/placement_groups from the Ray Dashboard and stores the result in S3. -// -// The placement_groups endpoint is one of the collector's built-in polled endpoints -// (see staticPolledEndpoints in poll.go), polled by PollAdditionalEndpointsPeriodically. -// -// The test case follows these steps: -// 1. Prepare test environment by applying a Ray cluster with the collector -// 2. Submit a RayJob that creates a detached placement group (so the PG persists after the job) -// 3. Get the sessionID from the head pod to build the expected S3 key -// 4. Wait for the placement groups file to appear in S3 at {sessionName}/fetched_endpoints/restful__api__v0__placement_groups -// 5. Read the file and verify it contains valid JSON with a non-empty placement_groups list -// 6. Delete S3 bucket to ensure test isolation +// testCollectorStoresPlacementGroups verifies the collector stores the placement_groups +// snapshot, then replays it through the history server after the RayCluster is deleted. +// The RayJob creates a detached placement group so the PG outlives the job itself. func testCollectorStoresPlacementGroups(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { rayCluster := PrepareTestEnv(test, g, namespace, s3Client) - // Submit a RayJob that creates a detached placement group named "test_pg". - // The detached lifetime ensures the PG persists after the job exits, so the - // collector captures non-empty data when polling /api/v0/placement_groups. ApplyRayJobAndWaitForCompletion(test, g, namespace, rayCluster) sessionID := GetSessionIDFromHeadPod(test, g, rayCluster) - // The collector stores the endpoint with query params (matching the built-in - // placementGroupsEndpoint constant in the collector's poll.go). + // Matches placementGroupsEndpoint in the collector's poll.go, query params included. storageKey := utils.EndpointPathToStorageKey("/api/v0/placement_groups?detail=1&limit=10000") sessionDir := clusterlogs.SessionDir("log", "", "", rayCluster.Namespace, rayCluster.Name, sessionID) pgKey := fmt.Sprintf("%s/%s/%s", sessionDir, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) LogWithTimestamp(test.T(), "Waiting for placement groups data to appear at S3 key: %s", pgKey) - - var pgBody []byte g.Eventually(func(gg Gomega) { - result, err := s3Client.GetObject(&s3.GetObjectInput{ - Bucket: aws.String(S3BucketName), - Key: aws.String(pgKey), - }) - gg.Expect(err).NotTo(HaveOccurred()) - defer result.Body.Close() + assertPlacementGroupsNonEmpty(gg, readS3Object(gg, s3Client, pgKey)) + }, TestTimeoutMedium).Should(Succeed()) - body, err := io.ReadAll(result.Body) - gg.Expect(err).NotTo(HaveOccurred()) - gg.Expect(body).NotTo(BeEmpty(), "Placement groups file should not be empty") - - // Verify it is valid JSON with a non-empty placement groups list. - var response map[string]interface{} - err = json.Unmarshal(body, &response) - gg.Expect(err).NotTo(HaveOccurred(), "Placement groups response should be valid JSON") - - // The Ray State API v2 returns {"result": true, "msg": "", "data": {"result": {"total": N, "result": [...], ...}}}. - gg.Expect(response).To(HaveKey("result"), "Placement groups response should contain result field") - gg.Expect(response["result"]).To(BeTrue(), "result field should be true") - gg.Expect(response).To(HaveKey("data"), "Placement groups response should contain data field") - data, ok := response["data"].(map[string]interface{}) - gg.Expect(ok).To(BeTrue(), "data field should be a JSON object") - gg.Expect(data).To(HaveKey("result"), "data should contain result field") - resultObj, ok := data["result"].(map[string]interface{}) - gg.Expect(ok).To(BeTrue(), "data.result field should be a JSON object") - gg.Expect(resultObj).To(HaveKey("result"), "data.result should contain result field") + DeleteRayClusterAndWait(test, g, namespace.Name, rayCluster.Name) - pgList, ok := resultObj["result"].([]interface{}) - gg.Expect(ok).To(BeTrue(), "data.result.result should be a JSON array") - gg.Expect(pgList).NotTo(BeEmpty(), "placement groups list should not be empty (RayJob creates a detached PG)") + ApplyHistoryServer(test, g, namespace, "") + historyServerURL := GetHistoryServerURL(test, g, namespace) + clusterInfo := getClusterFromList(test, g, historyServerURL, rayCluster.Name, namespace.Name) + g.Expect(clusterInfo.SessionName).NotTo(Equal(LiveSessionName), "Cluster should be a dead session after deletion") - pgBody = body - }, TestTimeoutMedium).Should(Succeed()) + client := CreateHTTPClientWithCookieJar(g) + enterClusterForOwner(test, g, client, historyServerURL, namespace.Name, + utils.RayClusterKind, rayCluster.Name, rayCluster.Name, clusterInfo.SessionName) - LogWithTimestamp(test.T(), "Placement groups data stored successfully: %s", string(pgBody)) + LogWithTimestamp(test.T(), "Replaying /api/v0/placement_groups through the history server") + g.Eventually(func(gg Gomega) { + assertPlacementGroupsNonEmpty(gg, getHistoryServerJSON(gg, client, + historyServerURL+"/api/v0/placement_groups?detail=1&limit=10000")) + }, TestTimeoutShort).Should(Succeed()) DeleteS3Bucket(test, g, s3Client) } +// assertPlacementGroupsNonEmpty requires at least one PG in the State API envelope +// {"result": true, "data": {"result": {"result": [...]}}}; the RayJob creates a detached one. +func assertPlacementGroupsNonEmpty(g Gomega, body []byte) { + var response struct { + Result bool `json:"result"` + Data struct { + Result struct { + Result []json.RawMessage `json:"result"` + } `json:"result"` + } `json:"data"` + } + g.Expect(json.Unmarshal(body, &response)).To(Succeed(), "placement groups response should be valid JSON") + g.Expect(response.Result).To(BeTrue(), "state API result field should be true") + g.Expect(response.Data.Result.Result).NotTo(BeEmpty(), "placement groups list should not be empty") +} + // verifyS3SessionDirs verifies file contents in logs/, node_events/, and job_events/ directories under a session prefix in S3. // There are two phases of verification: // 1. Verify file contents in logs/ directory @@ -698,8 +682,8 @@ func listFetchedEndpoints(g Gomega, s3Client *s3.S3, clusterPrefix, storageKeyPr return keys } -// enterClusterForOwner sets the session cookie for a RayJob/RayService-owned cluster: -// enter_cluster takes the owner's name and resolves the generated cluster name itself. +// enterClusterForOwner sets the session cookie via enter_cluster, which takes the owner's +// name and resolves the generated cluster name itself; a plain RayCluster is its own owner. func enterClusterForOwner(test Test, g *WithT, client *http.Client, historyServerURL, namespace, ownerKind, ownerName, clusterName, session string) { enterURL := fmt.Sprintf("%s/enter_cluster/%s/%s/%s/%s", historyServerURL, namespace, ownerKind, ownerName, session) LogWithTimestamp(test.T(), "Setting cluster context: %s", enterURL) From 16f6b8a758e80a02a5204670a2ffa83bf81f2be7 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 18:59:31 -0500 Subject: [PATCH 05/10] upd Signed-off-by: Future-Outlier --- historyserver/config/ray-data.yaml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/historyserver/config/ray-data.yaml b/historyserver/config/ray-data.yaml index 6049cdd2bce..877711e980b 100644 --- a/historyserver/config/ray-data.yaml +++ b/historyserver/config/ray-data.yaml @@ -10,14 +10,8 @@ spec: entrypoint: | python -c " import ray - from ray.util.placement_group import placement_group ray.init() - # Detached so the PG outlives the job and shows beside Ray Data on the job page. - pg = placement_group([{'CPU': 0.1}], lifetime='detached', name='demo_pg') - ray.get(pg.ready()) - print(f'Placement group created: {pg.bundle_specs}') - # materialize() is required: an unexecuted Dataset produces no stats. ds = ray.data.range(100).map_batches(lambda batch: batch).materialize() print(f'Dataset rows: {ds.count()}') From bff31de7835b00878ff63cfa5f7718d72a601584 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Wed, 5 Aug 2026 19:39:33 -0500 Subject: [PATCH 06/10] [History Server] Bound the shutdown join and fix e2e port-forward leak - 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 --- .../runtime/logcollector/collector.go | 6 +++++- .../logcollector/runtime/logcollector/poll.go | 9 ++++++++ historyserver/test/e2e/collector_test.go | 21 ++----------------- historyserver/test/support/support.go | 14 +++++++------ 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index 787f5f33978..5651d2bf5f6 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -101,7 +101,11 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { var wg sync.WaitGroup if r.IsHead { // Join the periodic poller first so a half-finished cycle cannot outlive the final poll. - <-periodicPollDone + select { + case <-periodicPollDone: + case <-time.After(periodicPollJoinTimeout): + logrus.Warn("Periodic endpoint poller still busy, starting the final poll anyway") + } wg.Go(r.processAdditionalEndpoints) } r.processSessionLatestLogs() diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index ad9e319d374..b59df043143 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -73,6 +73,10 @@ const terminalEmptyPollsBeforeGivingUp = 2 // The final poll shares the pod's termination grace period, so it gives up rather than overrun it. const shutdownPollBudget = 10 * time.Second +// Caps the shutdown join on the periodic poller: a storage write in flight is not +// cancelable, and waiting it out could eat the grace period the final poll needs. +const periodicPollJoinTimeout = 5 * time.Second + // datasetPollState tracks jobs whose datasets no longer need fetching. Owned by one goroutine; no lock. type datasetPollState struct { done map[string]struct{} @@ -260,6 +264,11 @@ func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessio return pollSkippedEmpty } + // A canceled ctx means shutdown started: the final poll owns the store from here. + if ctx.Err() != nil { + return pollFailed + } + storageKey := utils.EndpointPathToStorageKey(endpoint) objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) if err := r.Writer.WriteFile(objectKey, bytes.NewReader(body)); err != nil { diff --git a/historyserver/test/e2e/collector_test.go b/historyserver/test/e2e/collector_test.go index e0d2835bfae..7d37b2f6054 100644 --- a/historyserver/test/e2e/collector_test.go +++ b/historyserver/test/e2e/collector_test.go @@ -443,8 +443,8 @@ func testCollectorStoresTimezone(test Test, g *WithT, namespace *corev1.Namespac } // testCollectorStoresPlacementGroups verifies the collector stores the placement_groups -// snapshot, then replays it through the history server after the RayCluster is deleted. -// The RayJob creates a detached placement group so the PG outlives the job itself. +// snapshot; the RayJob creates a detached placement group so the PG outlives the job. +// The history-server replay of this endpoint is covered by testDeadClusterPlacementGroups. func testCollectorStoresPlacementGroups(test Test, g *WithT, namespace *corev1.Namespace, s3Client *s3.S3) { rayCluster := PrepareTestEnv(test, g, namespace, s3Client) @@ -461,23 +461,6 @@ func testCollectorStoresPlacementGroups(test Test, g *WithT, namespace *corev1.N assertPlacementGroupsNonEmpty(gg, readS3Object(gg, s3Client, pgKey)) }, TestTimeoutMedium).Should(Succeed()) - DeleteRayClusterAndWait(test, g, namespace.Name, rayCluster.Name) - - ApplyHistoryServer(test, g, namespace, "") - historyServerURL := GetHistoryServerURL(test, g, namespace) - clusterInfo := getClusterFromList(test, g, historyServerURL, rayCluster.Name, namespace.Name) - g.Expect(clusterInfo.SessionName).NotTo(Equal(LiveSessionName), "Cluster should be a dead session after deletion") - - client := CreateHTTPClientWithCookieJar(g) - enterClusterForOwner(test, g, client, historyServerURL, namespace.Name, - utils.RayClusterKind, rayCluster.Name, rayCluster.Name, clusterInfo.SessionName) - - LogWithTimestamp(test.T(), "Replaying /api/v0/placement_groups through the history server") - g.Eventually(func(gg Gomega) { - assertPlacementGroupsNonEmpty(gg, getHistoryServerJSON(gg, client, - historyServerURL+"/api/v0/placement_groups?detail=1&limit=10000")) - }, TestTimeoutShort).Should(Succeed()) - DeleteS3Bucket(test, g, s3Client) } diff --git a/historyserver/test/support/support.go b/historyserver/test/support/support.go index f9b43b442a7..ba2f4fb8a0e 100644 --- a/historyserver/test/support/support.go +++ b/historyserver/test/support/support.go @@ -1,7 +1,6 @@ package support import ( - "context" "fmt" "net/http" "net/http/cookiejar" @@ -38,11 +37,7 @@ func GetContainerStatusByName(pod *corev1.Pod, containerName string) (*corev1.Co } func PortForwardService(test Test, g *WithT, namespace, serviceName string, port int) { - ctx, cancel := context.WithCancel(context.Background()) - test.T().Cleanup(cancel) - - kubectlCmd := exec.CommandContext( - ctx, + kubectlCmd := exec.Command( "kubectl", "-n", namespace, "port-forward", @@ -51,6 +46,13 @@ func PortForwardService(test Test, g *WithT, namespace, serviceName string, port ) err := kubectlCmd.Start() g.Expect(err).NotTo(HaveOccurred()) + + // Kill and reap on cleanup: a leaked forward keeps the port, so the next test's + // forward cannot bind and silently talks to this test's deleted namespace. + test.T().Cleanup(func() { + _ = kubectlCmd.Process.Kill() + _ = kubectlCmd.Wait() + }) } // InstallGrafanaAndPrometheus installs Grafana and Prometheus in the cluster for testing. From 17626bc6a48197d614645bfe42dfac17f9d9f15d Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 6 Aug 2026 12:16:56 -0500 Subject: [PATCH 07/10] [History Server] Address review comments on collector polling - 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 --- historyserver/cmd/collector/main.go | 11 +++----- .../runtime/logcollector/collector.go | 2 ++ .../logcollector/runtime/logcollector/poll.go | 27 +++++++++---------- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/historyserver/cmd/collector/main.go b/historyserver/cmd/collector/main.go index e106a2e0daf..5dfb5df3e4d 100644 --- a/historyserver/cmd/collector/main.go +++ b/historyserver/cmd/collector/main.go @@ -2,7 +2,6 @@ package main import ( "encoding/json" - "errors" "flag" "fmt" "os" @@ -177,14 +176,10 @@ func main() { // out of its Service endpoints. endpointPollInterval := 30 * time.Second if v := os.Getenv("RAY_COLLECTOR_POLL_INTERVAL"); v != "" { - parsed, err := time.ParseDuration(v) - if err == nil && parsed <= 0 { - err = errors.New("must be positive") - } - if err != nil { - logrus.Warnf("Invalid RAY_COLLECTOR_POLL_INTERVAL=%s (%v), using default %s", v, err, endpointPollInterval) - } else { + if parsed, err := time.ParseDuration(v); err == nil && parsed > 0 { endpointPollInterval = parsed + } else { + logrus.Warnf("Invalid RAY_COLLECTOR_POLL_INTERVAL=%s, using default %s", v, endpointPollInterval) } } diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index 5651d2bf5f6..d57f925d257 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -87,6 +87,8 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { go r.WatchSessionLatestLoops() // Watch session_latest symlink changes go r.FetchAndStoreClusterMetadata() go r.FetchAndStoreTimezone() + // Driven by stop rather than ShutdownChan, which closes only after the final + // poll below: a tick in between would overwrite that final snapshot. periodicPollDone = make(chan struct{}) go func() { defer close(periodicPollDone) diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index b59df043143..b8cc3447b7d 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -9,6 +9,7 @@ import ( "net/http" "path" "path/filepath" + "slices" "strings" "time" @@ -36,16 +37,15 @@ var staticPolledEndpoints = []string{ // polledEndpoints merges the built-in and configured endpoints, deduplicated. 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} { - for _, endpoint := range list { - if _, ok := seen[endpoint]; ok { - continue - } - seen[endpoint] = struct{}{} - endpoints = append(endpoints, endpoint) + all := slices.Concat(staticPolledEndpoints, r.AdditionalEndpoints) + endpoints := make([]string, 0, len(all)) + seen := make(map[string]struct{}, len(all)) + for _, endpoint := range all { + if _, ok := seen[endpoint]; ok { + continue } + seen[endpoint] = struct{}{} + endpoints = append(endpoints, endpoint) } return endpoints } @@ -92,10 +92,7 @@ func newDatasetPollState() *datasetPollState { // 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. +// It stops when stop closes and cancels any blocked resolve or in-flight request at that point. func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{}) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -191,7 +188,9 @@ func (r *RayLogHandler) processAdditionalEndpoints() { logrus.Info("Finished processing polled endpoints") } -// 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). func (r *RayLogHandler) pollAllEndpoints(ctx context.Context, sessionName string, state *datasetPollState, finalPoll bool) { for _, endpoint := range r.polledEndpoints() { if ctx.Err() != nil { From 697ed1caec86af0954406570bca5574ee0892038 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Thu, 6 Aug 2026 12:48:56 -0500 Subject: [PATCH 08/10] Trigger CI Signed-off-by: Future-Outlier From 1092d7088e7ecd07ed28427b114653e3c73372b6 Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Sat, 8 Aug 2026 12:24:39 -0500 Subject: [PATCH 09/10] Address endpoint polling review feedback Signed-off-by: Future-Outlier --- historyserver/config/ray-data.yaml | 8 +- .../config/raycluster-azureblob.yaml | 8 +- historyserver/config/raycluster-gcs.yaml | 8 +- historyserver/config/raycluster.yaml | 8 +- historyserver/config/rayservice.yaml | 2 + .../runtime/logcollector/collector.go | 21 +- .../logcollector/endpoint_fetch_once.go | 3 +- .../logcollector/runtime/logcollector/poll.go | 92 ++++-- .../runtime/logcollector/poll_test.go | 307 +++++++++++++++++- .../pkg/storage/clusterlogs/clusterlogs.go | 6 + .../storage/clusterlogs/clusterlogs_test.go | 5 + 11 files changed, 397 insertions(+), 71 deletions(-) diff --git a/historyserver/config/ray-data.yaml b/historyserver/config/ray-data.yaml index 877711e980b..eb79f423650 100644 --- a/historyserver/config/ray-data.yaml +++ b/historyserver/config/ray-data.yaml @@ -34,13 +34,9 @@ spec: value: "true" - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EVENTS_EXPORT_ADDR value: "http://localhost:8084/v1/events" - # in ray 2.52.0, we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES - # in ray 2.53.0 (noy yet done). we need to set RAY_DASHBOARD_AGGREGATOR_AGENT_PUBLISHER_HTTP_ENDPOINT_EXPOSABLE_EVENT_TYPES - name: RAY_DASHBOARD_AGGREGATOR_AGENT_EXPOSABLE_EVENT_TYPES - 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" - image: rayproject/ray:2.52.0 + value: "ALL" + image: rayproject/ray:2.56.0 imagePullPolicy: IfNotPresent name: ray-head securityContext: diff --git a/historyserver/config/raycluster-azureblob.yaml b/historyserver/config/raycluster-azureblob.yaml index be24c616a27..8d4adea3167 100644 --- a/historyserver/config/raycluster-azureblob.yaml +++ b/historyserver/config/raycluster-azureblob.yaml @@ -67,9 +67,13 @@ spec: # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" # Optional extras on top of the built-in endpoints (Serve, placement groups, Ray Data). - # Paths must match the frontend request URI, query string included. + # Paths must match the History Server replay request URI, query string included. + # For example, preserve Ray Train V2 run details for a future History Server view. + # Requires the Ray Train workload/driver (not the collector) to set + # RAY_TRAIN_V2_ENABLED=1 and RAY_TRAIN_ENABLE_STATE_TRACKING=1; + # verify this DeveloperAPI URI when changing Ray versions. # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # value: "/nodes?view=summary" + # value: "/api/train/v2/runs/v1" # reference: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite#connect-to-the-emulator-by-using-the-azure-storage-explorer - name: AZURE_STORAGE_CONNECTION_STRING value: "DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://azurite-service.azurite-dev.svc.cluster.local:10000/devstoreaccount1;" diff --git a/historyserver/config/raycluster-gcs.yaml b/historyserver/config/raycluster-gcs.yaml index d52dda7ee99..3224b1594e8 100644 --- a/historyserver/config/raycluster-gcs.yaml +++ b/historyserver/config/raycluster-gcs.yaml @@ -70,9 +70,13 @@ spec: # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" # Optional extras on top of the built-in endpoints (Serve, placement groups, Ray Data). - # Paths must match the frontend request URI, query string included. + # Paths must match the History Server replay request URI, query string included. + # For example, preserve Ray Train V2 run details for a future History Server view. + # Requires the Ray Train workload/driver (not the collector) to set + # RAY_TRAIN_V2_ENABLED=1 and RAY_TRAIN_ENABLE_STATE_TRACKING=1; + # verify this DeveloperAPI URI when changing Ray versions. # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # value: "/nodes?view=summary" + # value: "/api/train/v2/runs/v1" command: - collector - --role=Head diff --git a/historyserver/config/raycluster.yaml b/historyserver/config/raycluster.yaml index 1ebc70da5c7..4563b29caab 100644 --- a/historyserver/config/raycluster.yaml +++ b/historyserver/config/raycluster.yaml @@ -73,9 +73,13 @@ spec: # - name: RAY_COLLECTOR_POLL_INTERVAL # value: "1m" # Optional extras on top of the built-in endpoints (Serve, placement groups, Ray Data). - # Paths must match the frontend request URI, query string included. + # Paths must match the History Server replay request URI, query string included. + # For example, preserve Ray Train V2 run details for a future History Server view. + # Requires the Ray Train workload/driver (not the collector) to set + # RAY_TRAIN_V2_ENABLED=1 and RAY_TRAIN_ENABLE_STATE_TRACKING=1; + # verify this DeveloperAPI URI when changing Ray versions. # - name: RAY_COLLECTOR_ADDITIONAL_ENDPOINTS - # value: "/nodes?view=summary" + # value: "/api/train/v2/runs/v1" - name: S3DISABLE_SSL value: "true" - name: AWS_ACCESS_KEY_ID diff --git a/historyserver/config/rayservice.yaml b/historyserver/config/rayservice.yaml index 13de62f1983..a21e9ea0990 100644 --- a/historyserver/config/rayservice.yaml +++ b/historyserver/config/rayservice.yaml @@ -110,6 +110,8 @@ spec: value: "test" - name: S3FORCE_PATH_STYLE value: "true" + - name: RAY_COLLECTOR_POLL_INTERVAL + value: "5s" command: - collector - --role=Head diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go index d57f925d257..1a1183ac6ff 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/collector.go @@ -82,18 +82,14 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { // uploads from previous runs are resumed. go r.WatchPrevLogsLoops() go r.PollActiveSessionChanges() - var periodicPollDone chan struct{} + var periodicPollResults <-chan periodicPollResult if r.IsHead { go r.WatchSessionLatestLoops() // Watch session_latest symlink changes go r.FetchAndStoreClusterMetadata() go r.FetchAndStoreTimezone() // Driven by stop rather than ShutdownChan, which closes only after the final // poll below: a tick in between would overwrite that final snapshot. - periodicPollDone = make(chan struct{}) - go func() { - defer close(periodicPollDone) - r.PollAdditionalEndpointsPeriodically(stop) - }() + periodicPollResults = r.startPeriodicEndpointPolling(stop) } <-stop @@ -102,18 +98,19 @@ func (r *RayLogHandler) Run(stop <-chan struct{}) error { // Endpoint data dies with the dashboard; log files stay on disk, so poll concurrently. var wg sync.WaitGroup if r.IsHead { - // Join the periodic poller first so a half-finished cycle cannot outlive the final poll. - select { - case <-periodicPollDone: - case <-time.After(periodicPollJoinTimeout): + // Reuse the periodic poller's state only after it exits and transfers ownership. + periodicResult, joined := waitForPeriodicPollResult(periodicPollResults, periodicPollJoinTimeout) + if !joined { logrus.Warn("Periodic endpoint poller still busy, starting the final poll anyway") } - wg.Go(r.processAdditionalEndpoints) + wg.Go(func() { + r.processAdditionalEndpoints(periodicResult) + }) } r.processSessionLatestLogs() wg.Wait() - // Only now: pollSingleEndpoint uses ShutdownChan to cancel in-flight requests. + // Signal background goroutines after the synchronous shutdown flushes complete. close(r.ShutdownChan) return nil diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/endpoint_fetch_once.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/endpoint_fetch_once.go index 93d9c92c5f9..5737eb2fac3 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/endpoint_fetch_once.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/endpoint_fetch_once.go @@ -12,6 +12,7 @@ import ( "github.com/sirupsen/logrus" + "github.com/ray-project/kuberay/historyserver/pkg/storage/clusterlogs" "github.com/ray-project/kuberay/historyserver/pkg/utils" ) @@ -125,7 +126,7 @@ func (r *RayLogHandler) fetchAndStoreEndpoint(cfg endpointFetchConfig) { // Successfully fetched — store it under the session path storageKey := utils.EndpointPathToStorageKey(cfg.endpoint) - objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) + objectKey := path.Join(clusterlogs.FetchedEndpointsDir(r.ClusterDir, sessionName), storageKey) if err := r.Writer.WriteFile(objectKey, bytes.NewReader(body)); err != nil { logrus.Errorf("Failed to store %s at %s: %v", cfg.endpoint, objectKey, err) if !r.sleepOrShutdown(retryInterval) { diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go index b8cc3447b7d..34a78d616b9 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll.go @@ -15,6 +15,7 @@ import ( "github.com/sirupsen/logrus" + "github.com/ray-project/kuberay/historyserver/pkg/storage/clusterlogs" "github.com/ray-project/kuberay/historyserver/pkg/utils" ) @@ -77,23 +78,56 @@ const shutdownPollBudget = 10 * time.Second // cancelable, and waiting it out could eat the grace period the final poll needs. const periodicPollJoinTimeout = 5 * time.Second -// datasetPollState tracks jobs whose datasets no longer need fetching. Owned by one goroutine; no lock. +// datasetPollState is owned by one goroutine at a time; no lock is needed. type datasetPollState struct { - done map[string]struct{} + // terminalStored contains terminal jobs whose dataset response was successfully written. + terminalStored map[string]struct{} + // emptyRuns counts consecutive empty responses observed after a job becomes terminal. emptyRuns map[string]int } func newDatasetPollState() *datasetPollState { return &datasetPollState{ - done: make(map[string]struct{}), - emptyRuns: make(map[string]int), + terminalStored: make(map[string]struct{}), + emptyRuns: make(map[string]int), } } +// periodicPollResult transfers ownership of state after the periodic poller exits. +type periodicPollResult struct { + sessionName string + state *datasetPollState +} + // 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. func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{}) { + r.pollAdditionalEndpointsPeriodically(stop) +} + +func (r *RayLogHandler) startPeriodicEndpointPolling(stop <-chan struct{}) <-chan periodicPollResult { + // The core sends exactly once. A one-element buffer lets that send finish even if + // the shutdown join times out and no receiver remains. + results := make(chan periodicPollResult, 1) + go func() { + results <- r.pollAdditionalEndpointsPeriodically(stop) + }() + return results +} + +func waitForPeriodicPollResult(results <-chan periodicPollResult, timeout time.Duration) (periodicPollResult, bool) { + timer := time.NewTimer(timeout) + defer timer.Stop() + select { + case result := <-results: + return result, true + case <-timer.C: + return periodicPollResult{}, false + } +} + +func (r *RayLogHandler) pollAdditionalEndpointsPeriodically(stop <-chan struct{}) periodicPollResult { ctx, cancel := context.WithCancel(context.Background()) defer cancel() go func() { @@ -107,7 +141,7 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{} sessionName, err := waitForSessionName(ctx) if err != nil { logrus.Errorf("Failed to resolve session name for endpoint polling: %v", err) - return + return periodicPollResult{} } logrus.Infof("Starting endpoint polling (interval=%v, endpoints=%v)", r.EndpointPollInterval, r.polledEndpoints()) @@ -121,7 +155,7 @@ func (r *RayLogHandler) PollAdditionalEndpointsPeriodically(stop <-chan struct{} select { case <-stop: logrus.Info("Shutdown signaled, stopping endpoint polling") - return + return periodicPollResult{sessionName: sessionName, state: state} case <-ticker.C: sessionName, state = r.pollCycle(ctx, sessionName, state) } @@ -170,7 +204,7 @@ func currentSessionName() (string, error) { } // processAdditionalEndpoints performs one final poll before shutdown. -func (r *RayLogHandler) processAdditionalEndpoints() { +func (r *RayLogHandler) processAdditionalEndpoints(previous periodicPollResult) { logrus.Info("Processing polled endpoints before shutdown") sessionName, err := currentSessionName() @@ -179,12 +213,17 @@ func (r *RayLogHandler) processAdditionalEndpoints() { return } - // One budget for the whole pass: serial per-request timeouts would add up past the grace period. + state := newDatasetPollState() + if previous.state != nil && previous.sessionName == sessionName { + state = previous.state + } + + // The budget cancels HTTP fetches. StorageWriter.WriteFile does not accept a + // context, so a write already in progress may finish after the deadline. ctx, cancel := context.WithTimeout(context.Background(), shutdownPollBudget) defer cancel() - // Fresh state, so this final pass re-captures every job. - r.pollAllEndpoints(ctx, sessionName, newDatasetPollState(), true) + r.pollAllEndpoints(ctx, sessionName, state, true) logrus.Info("Finished processing polled endpoints") } @@ -229,7 +268,11 @@ func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string if job.JobID == "" { continue } - if _, ok := state.done[job.JobID]; ok { + if _, ok := state.terminalStored[job.JobID]; ok { + continue + } + if !finalPoll && terminalJobStatuses[job.Status] && + state.emptyRuns[job.JobID] >= terminalEmptyPollsBeforeGivingUp { continue } @@ -239,12 +282,10 @@ func (r *RayLogHandler) pollDataDatasets(ctx context.Context, sessionName string } switch outcome { case pollStored: - state.done[job.JobID] = struct{}{} + state.terminalStored[job.JobID] = struct{}{} + delete(state.emptyRuns, job.JobID) case pollSkippedEmpty: state.emptyRuns[job.JobID]++ - if state.emptyRuns[job.JobID] >= terminalEmptyPollsBeforeGivingUp { - state.done[job.JobID] = struct{}{} - } case pollFailed: // Retried next cycle. } @@ -258,18 +299,18 @@ func (r *RayLogHandler) pollSingleEndpoint(ctx context.Context, endpoint, sessio return pollFailed } + // Do not start a storage write after the parent context is canceled. + if ctx.Err() != nil { + return pollFailed + } + if isEmptyPayload(endpoint, body, finalPoll) { logrus.Debugf("Skipping %s: nothing to store", endpoint) return pollSkippedEmpty } - // A canceled ctx means shutdown started: the final poll owns the store from here. - if ctx.Err() != nil { - return pollFailed - } - storageKey := utils.EndpointPathToStorageKey(endpoint) - objectKey := path.Join(r.ClusterDir, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME, storageKey) + objectKey := path.Join(clusterlogs.FetchedEndpointsDir(r.ClusterDir, sessionName), storageKey) if err := r.Writer.WriteFile(objectKey, bytes.NewReader(body)); err != nil { logrus.Errorf("Failed to store endpoint %s at %s: %v", endpoint, objectKey, err) return pollFailed @@ -316,19 +357,12 @@ func hasDatasets(body []byte) bool { return len(resp.Datasets) > 0 } -// fetchEndpoint GETs one dashboard endpoint; in-flight requests are canceled on shutdown. +// fetchEndpoint GETs one dashboard endpoint with the parent and per-request deadlines. func (r *RayLogHandler) fetchEndpoint(parent context.Context, endpoint string) ([]byte, error) { url := r.DashboardAddress + endpoint ctx, cancel := context.WithTimeout(parent, defaultRequestTimeout) defer cancel() - go func() { - select { - case <-r.ShutdownChan: - cancel() - case <-ctx.Done(): - } - }() req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { diff --git a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go index 46d0732965e..92fb8ec3522 100644 --- a/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go +++ b/historyserver/pkg/collector/logcollector/runtime/logcollector/poll_test.go @@ -3,6 +3,8 @@ package logcollector import ( "context" "encoding/json" + "fmt" + "io" "net/http" "net/http/httptest" "os" @@ -18,6 +20,8 @@ import ( "github.com/ray-project/kuberay/historyserver/pkg/utils" ) +const trainRunsEndpoint = "/api/train/v2/runs/v1" + // fakeDashboard stands in for the Ray Dashboard, recording every requested path. type fakeDashboard struct { mu sync.Mutex @@ -93,6 +97,45 @@ func writtenKeys(writer *MockStorageWriter) []string { return keys } +type blockingStorageWriter struct { + entered chan struct{} + release chan struct{} + once sync.Once +} + +func (w *blockingStorageWriter) CreateDirectory(string) error { + return nil +} + +func (w *blockingStorageWriter) WriteFile(string, io.ReadSeeker) error { + w.once.Do(func() { close(w.entered) }) + <-w.release + return nil +} + +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +type cancelOnEOFBody struct { + io.Reader + cancel context.CancelFunc +} + +func (b *cancelOnEOFBody) Read(p []byte) (int, error) { + n, err := b.Reader.Read(p) + if err == io.EOF { + b.cancel() + } + return n, err +} + +func (b *cancelOnEOFBody) Close() error { + return nil +} + // TestPollDataDatasetsFansOutPerJob verifies per-job fan-out from /api/jobs/, skipping blank IDs. func TestPollDataDatasetsFansOutPerJob(t *testing.T) { g := NewWithT(t) @@ -143,6 +186,48 @@ func TestPollDataDatasetsSkipsEmptyResponse(t *testing.T) { g.Expect(writtenKeys(writer)).To(BeEmpty()) } +// TestPollDataDatasetsDoesNotCountRunningEmpties verifies the retry cap begins only after +// a job becomes terminal, so early empty responses cannot suppress its first terminal fetch. +func TestPollDataDatasetsDoesNotCountRunningEmpties(t *testing.T) { + g := NewWithT(t) + + var mu sync.Mutex + status := "RUNNING" + attempts := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + if r.URL.Path == jobsEndpoint { + _, _ = w.Write([]byte(fmt.Sprintf(`[{"job_id": "01000000", "status": %q}]`, status))) + return + } + attempts++ + if status == "RUNNING" { + _, _ = w.Write([]byte(`{"datasets": []}`)) + return + } + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "complete"}]}`)) + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + state := newDatasetPollState() + handler.pollDataDatasets(context.Background(), "session_1", state, false) + handler.pollDataDatasets(context.Background(), "session_1", state, false) + g.Expect(state.emptyRuns).NotTo(HaveKey("01000000")) + + mu.Lock() + status = "SUCCEEDED" + mu.Unlock() + handler.pollDataDatasets(context.Background(), "session_1", state, false) + + mu.Lock() + defer mu.Unlock() + g.Expect(attempts).To(Equal(3)) + g.Expect(state.terminalStored).To(HaveKey("01000000")) + g.Expect(writtenKeys(writer)).To(HaveLen(1)) +} + // TestPollDataDatasetsStopsPollingTerminalJobs verifies a terminal job is fetched once // while a running job keeps being refreshed. func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { @@ -166,8 +251,8 @@ func TestPollDataDatasetsStopsPollingTerminalJobs(t *testing.T) { handler.pollDataDatasets(context.Background(), "session_1", state, false) handler.pollDataDatasets(context.Background(), "session_1", state, false) - g.Expect(state.done).To(HaveKey("01000000")) - g.Expect(state.done).NotTo(HaveKey("02000000")) + g.Expect(state.terminalStored).To(HaveKey("01000000")) + g.Expect(state.terminalStored).NotTo(HaveKey("02000000")) // The terminal job is fetched only on the first cycle; the running one every cycle. g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(1)) g.Expect(dash.requestsFor("/api/data/datasets/02000000")).To(HaveLen(3)) @@ -201,11 +286,11 @@ func TestPollDataDatasetsRetriesFailedTerminalJob(t *testing.T) { state := newDatasetPollState() handler.pollDataDatasets(context.Background(), "session_1", state, false) - g.Expect(state.done).To(BeEmpty()) + g.Expect(state.terminalStored).To(BeEmpty()) g.Expect(writtenKeys(writer)).To(BeEmpty()) handler.pollDataDatasets(context.Background(), "session_1", state, false) - g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(state.terminalStored).To(HaveKey("01000000")) g.Expect(writtenKeys(writer)).To(Equal([]string{ "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", })) @@ -239,11 +324,11 @@ func TestPollDataDatasetsRetriesTerminalJobWithLateStats(t *testing.T) { state := newDatasetPollState() handler.pollDataDatasets(context.Background(), "session_1", state, false) - g.Expect(state.done).To(BeEmpty()) + g.Expect(state.terminalStored).To(BeEmpty()) g.Expect(writtenKeys(writer)).To(BeEmpty()) handler.pollDataDatasets(context.Background(), "session_1", state, false) - g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(state.terminalStored).To(HaveKey("01000000")) g.Expect(writtenKeys(writer)).To(Equal([]string{ "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", })) @@ -266,11 +351,85 @@ func TestPollDataDatasetsGivesUpOnRepeatedlyEmptyTerminalJob(t *testing.T) { handler.pollDataDatasets(context.Background(), "session_1", state, false) } - g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(state.terminalStored).NotTo(HaveKey("01000000")) + g.Expect(state.emptyRuns).To(HaveKeyWithValue("01000000", terminalEmptyPollsBeforeGivingUp)) g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(terminalEmptyPollsBeforeGivingUp)) g.Expect(writtenKeys(writer)).To(BeEmpty()) } +// TestFinalDatasetPollReusesPeriodicState verifies the final poll skips terminal jobs +// already stored, while retrying terminal jobs that were empty or failed periodically. +func TestFinalDatasetPollReusesPeriodicState(t *testing.T) { + g := NewWithT(t) + + var mu sync.Mutex + attempts := map[string]int{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == jobsEndpoint { + _, _ = w.Write([]byte(`[ + {"job_id": "01000000", "status": "SUCCEEDED"}, + {"job_id": "02000000", "status": "SUCCEEDED"}, + {"job_id": "03000000", "status": "SUCCEEDED"} + ]`)) + return + } + jobID := strings.TrimPrefix(r.URL.Path, dataDatasetsEndpointPrefix) + mu.Lock() + attempts[jobID]++ + attempt := attempts[jobID] + mu.Unlock() + + switch jobID { + case "01000000": + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "stored"}]}`)) + case "02000000": + if attempt <= terminalEmptyPollsBeforeGivingUp { + _, _ = w.Write([]byte(`{"datasets": []}`)) + return + } + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "late"}]}`)) + case "03000000": + if attempt <= 2 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte(`{"datasets": [{"dataset": "retried"}]}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(srv.Close) + + handler, writer := newPollTestHandler(t, srv.URL) + state := newDatasetPollState() + handler.pollDataDatasets(context.Background(), "session_1", state, false) + handler.pollDataDatasets(context.Background(), "session_1", state, false) + + g.Expect(state.terminalStored).To(HaveKey("01000000")) + g.Expect(state.terminalStored).NotTo(HaveKey("02000000")) + g.Expect(state.terminalStored).NotTo(HaveKey("03000000")) + g.Expect(state.emptyRuns).To(HaveKeyWithValue("02000000", terminalEmptyPollsBeforeGivingUp)) + + handler.pollDataDatasets(context.Background(), "session_1", state, true) + + mu.Lock() + defer mu.Unlock() + g.Expect(attempts).To(Equal(map[string]int{ + "01000000": 1, + "02000000": 3, + "03000000": 3, + })) + g.Expect(state.terminalStored).To(HaveKey("01000000")) + g.Expect(state.terminalStored).To(HaveKey("02000000")) + g.Expect(state.terminalStored).To(HaveKey("03000000")) + g.Expect(state.emptyRuns).NotTo(HaveKey("02000000")) + g.Expect(writtenKeys(writer)).To(ConsistOf( + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__01000000", + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__02000000", + "cluster-dir/session_1/fetched_endpoints/restful__api__data__datasets__03000000", + )) +} + // TestPollAllEndpointsStoresStaticEndpoints verifies the built-in endpoints are stored under // the exact URIs the frontend requests. func TestPollAllEndpointsStoresStaticEndpoints(t *testing.T) { @@ -314,12 +473,12 @@ func TestPollCycleFollowsSessionChange(t *testing.T) { pointAt("session_old") session, state := handler.pollCycle(context.Background(), "session_old", newDatasetPollState()) g.Expect(session).To(Equal("session_old")) - g.Expect(state.done).To(HaveKey("01000000")) + g.Expect(state.terminalStored).To(HaveKey("01000000")) pointAt("session_new") session, state = handler.pollCycle(context.Background(), session, state) g.Expect(session).To(Equal("session_new")) - g.Expect(state.done).To(HaveKey("01000000"), "the new session's job must be captured, not skipped") + g.Expect(state.terminalStored).To(HaveKey("01000000"), "the new session's job must be captured, not skipped") // The same job ID is stored once per session, not once overall. g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(2)) @@ -329,6 +488,49 @@ func TestPollCycleFollowsSessionChange(t *testing.T) { )) } +// TestFinalPollReusesStateOnlyForCurrentSession verifies a restarted Ray head cannot +// inherit terminal job IDs from the previous session. +func TestFinalPollReusesStateOnlyForCurrentSession(t *testing.T) { + for _, tc := range []struct { + name string + previousSession string + wantRequests int + }{ + {name: "same session", previousSession: "session_new", wantRequests: 0}, + {name: "changed session", previousSession: "session_old", wantRequests: 1}, + } { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + tmpRoot := t.TempDir() + t.Setenv("RAY_TMP_ROOT", tmpRoot) + g.Expect(os.MkdirAll(filepath.Join(tmpRoot, "session_new"), 0o755)).To(Succeed()) + g.Expect(os.Symlink(filepath.Join(tmpRoot, "session_new"), filepath.Join(tmpRoot, "session_latest"))).To(Succeed()) + + dash := &fakeDashboard{ + jobs: `[{"job_id": "01000000", "status": "SUCCEEDED"}]`, + datasets: map[string]string{"01000000": `{"datasets": [{"dataset": "new"}]}`}, + } + srv := dash.start(t) + handler, writer := newPollTestHandler(t, srv.URL) + state := newDatasetPollState() + state.terminalStored["01000000"] = struct{}{} + + handler.processAdditionalEndpoints(periodicPollResult{ + sessionName: tc.previousSession, + state: state, + }) + + g.Expect(dash.requestsFor("/api/data/datasets/01000000")).To(HaveLen(tc.wantRequests)) + dataKey := "cluster-dir/session_new/fetched_endpoints/restful__api__data__datasets__01000000" + if tc.wantRequests == 0 { + g.Expect(writtenKeys(writer)).NotTo(ContainElement(dataKey)) + } else { + g.Expect(writtenKeys(writer)).To(ContainElement(dataKey)) + } + }) + } +} + // TestPeriodicPollingStopsOnShutdownSignal verifies the loop exits on the shutdown signal, // not ShutdownChan, so no tick can overwrite the final shutdown snapshot. func TestPeriodicPollingStopsOnShutdownSignal(t *testing.T) { @@ -393,7 +595,43 @@ func TestPeriodicPollingCancelsInFlightRequestOnShutdown(t *testing.T) { g.Expect(writtenKeys(writer)).To(BeEmpty()) } -// TestPollAllEndpointsStopsWhenContextExpires verifies the shutdown budget bounds the whole pass. +// TestPeriodicPollTimeoutBuffersLateResult verifies a timed-out join does not share state +// and the periodic goroutine can still transfer its result and exit after a blocked write. +func TestPeriodicPollTimeoutBuffersLateResult(t *testing.T) { + g := NewWithT(t) + + tmpRoot := t.TempDir() + t.Setenv("RAY_TMP_ROOT", tmpRoot) + g.Expect(os.MkdirAll(filepath.Join(tmpRoot, "session_1"), 0o755)).To(Succeed()) + g.Expect(os.Symlink(filepath.Join(tmpRoot, "session_1"), filepath.Join(tmpRoot, "session_latest"))).To(Succeed()) + + dash := &fakeDashboard{jobs: `[]`} + srv := dash.start(t) + writer := &blockingStorageWriter{ + entered: make(chan struct{}), + release: make(chan struct{}), + } + handler, _ := newPollTestHandler(t, srv.URL) + handler.Writer = writer + handler.EndpointPollInterval = time.Hour + + stop := make(chan struct{}) + results := handler.startPeriodicEndpointPolling(stop) + g.Eventually(writer.entered).Should(BeClosed()) + close(stop) + + result, joined := waitForPeriodicPollResult(results, 0) + g.Expect(joined).To(BeFalse()) + g.Expect(result.state).To(BeNil()) + + close(writer.release) + g.Eventually(func() int { return len(results) }).Should(Equal(1)) + lateResult := <-results + g.Expect(lateResult.sessionName).To(Equal("session_1")) + g.Expect(lateResult.state).NotTo(BeNil()) +} + +// TestPollAllEndpointsStopsWhenContextExpires verifies a canceled context prevents new requests. func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { g := NewWithT(t) @@ -410,6 +648,41 @@ func TestPollAllEndpointsStopsWhenContextExpires(t *testing.T) { g.Expect(writtenKeys(writer)).To(BeEmpty()) } +// TestPollSingleEndpointDoesNotStoreAfterCancellation verifies a response fetched just as +// shutdown starts remains retryable instead of being recorded as durably stored. +func TestPollSingleEndpointDoesNotStoreAfterCancellation(t *testing.T) { + for _, tc := range []struct { + name string + body string + }{ + {name: "non-empty response", body: `{"datasets": [{"dataset": "complete"}]}`}, + {name: "empty response", body: `{"datasets": []}`}, + } { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + ctx, cancel := context.WithCancel(context.Background()) + handler, writer := newPollTestHandler(t, "http://dashboard") + handler.HttpClient = &http.Client{Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: &cancelOnEOFBody{ + Reader: strings.NewReader(tc.body), + cancel: cancel, + }, + Request: req, + }, nil + })} + + outcome := handler.pollSingleEndpoint(ctx, dataDatasetsEndpointPrefix+"01000000", "session_1", false) + + g.Expect(outcome).To(Equal(pollFailed)) + g.Expect(ctx.Err()).To(MatchError(context.Canceled)) + g.Expect(writtenKeys(writer)).To(BeEmpty()) + }) + } +} + // TestPolledEndpointsAppendsConfiguredOnes verifies RAY_COLLECTOR_ADDITIONAL_ENDPOINTS adds // to the built-in set, deduplicated. func TestPolledEndpointsAppendsConfiguredOnes(t *testing.T) { @@ -419,14 +692,14 @@ func TestPolledEndpointsAppendsConfiguredOnes(t *testing.T) { g.Expect(handler.polledEndpoints()).To(Equal(staticPolledEndpoints)) handler.AdditionalEndpoints = []string{ - "/nodes?view=summary", + trainRunsEndpoint, serveApplicationsEndpoint, // already built in - "/nodes?view=summary", // repeated by the user + trainRunsEndpoint, // repeated by the user } g.Expect(handler.polledEndpoints()).To(Equal([]string{ serveApplicationsEndpoint, placementGroupsEndpoint, - "/nodes?view=summary", + trainRunsEndpoint, })) // The built-in list itself must not be mutated by the append. @@ -443,13 +716,13 @@ func TestPollAllEndpointsStoresConfiguredEndpoint(t *testing.T) { dash := &fakeDashboard{jobs: `[]`} srv := dash.start(t) handler, writer := newPollTestHandler(t, srv.URL) - handler.AdditionalEndpoints = []string{"/nodes?view=summary"} + handler.AdditionalEndpoints = []string{trainRunsEndpoint} handler.pollAllEndpoints(context.Background(), "session_1", newDatasetPollState(), false) - g.Expect(dash.requestsFor("/nodes?view=summary")).To(HaveLen(1)) + g.Expect(dash.requestsFor(trainRunsEndpoint)).To(HaveLen(1)) g.Expect(writtenKeys(writer)).To(ContainElement( - "cluster-dir/session_1/fetched_endpoints/restful__nodes?view=summary")) + "cluster-dir/session_1/fetched_endpoints/restful__api__train__v2__runs__v1")) g.Expect(writtenKeys(writer)).To(HaveLen(3)) } @@ -518,7 +791,7 @@ func TestIsEmptyPayloadOnlyGuardsKnownEndpoints(t *testing.T) { g.Expect(isEmptyPayload(serveApplicationsEndpoint, []byte(`{"applications": {}}`), false)).To(BeFalse()) g.Expect(isEmptyPayload(dataDatasetsEndpointPrefix+"01000000", []byte(`{"datasets": []}`), false)).To(BeTrue()) g.Expect(isEmptyPayload(placementGroupsEndpoint, []byte(`{}`), true)).To(BeFalse()) - g.Expect(isEmptyPayload("/nodes?view=summary", []byte(`{}`), true)).To(BeFalse()) + g.Expect(isEmptyPayload(trainRunsEndpoint, []byte(`{}`), true)).To(BeFalse()) } func TestHasDatasets(t *testing.T) { diff --git a/historyserver/pkg/storage/clusterlogs/clusterlogs.go b/historyserver/pkg/storage/clusterlogs/clusterlogs.go index 907aee72c4a..36aabc479e1 100644 --- a/historyserver/pkg/storage/clusterlogs/clusterlogs.go +++ b/historyserver/pkg/storage/clusterlogs/clusterlogs.go @@ -43,6 +43,12 @@ func SessionDir(rootDir, ownerKind, ownerName, namespace, clusterName, sessionNa return path.Join(cp, sessionName) } +// FetchedEndpointsDir returns the directory containing dashboard endpoint snapshots: +// //fetched_endpoints +func FetchedEndpointsDir(prefix, sessionName string) string { + return path.Join(prefix, sessionName, utils.RAY_SESSIONDIR_FETCHED_ENDPOINTS_NAME) +} + // NodeDir returns the path to a node's directory under a session: // // func NodeDir(rootDir, ownerKind, ownerName, namespace, clusterName, sessionName, nodeName string) string { diff --git a/historyserver/pkg/storage/clusterlogs/clusterlogs_test.go b/historyserver/pkg/storage/clusterlogs/clusterlogs_test.go index 53b8a9deaf2..7d14e08613b 100644 --- a/historyserver/pkg/storage/clusterlogs/clusterlogs_test.go +++ b/historyserver/pkg/storage/clusterlogs/clusterlogs_test.go @@ -24,6 +24,11 @@ func TestClusterLogsPaths(t *testing.T) { t.Errorf("SessionDir() = %q, want %q", got, wantSession) } + wantFetchedEndpoints := wantSession + "/fetched_endpoints" + if got := FetchedEndpointsDir(wantPrefix, session); got != wantFetchedEndpoints { + t.Errorf("FetchedEndpointsDir() = %q, want %q", got, wantFetchedEndpoints) + } + wantNode := wantSession + "/node-1" if got := NodeDir(rootDir, ownerKind, ownerName, ns, cluster, session, node); got != wantNode { t.Errorf("NodeDir() = %q, want %q", got, wantNode) From f4f0c841f58c5e321b68246816327a5e887c4f4d Mon Sep 17 00:00:00 2001 From: Future-Outlier Date: Sat, 8 Aug 2026 14:57:05 -0500 Subject: [PATCH 10/10] Set RayService collector poll interval to 30s Signed-off-by: Future-Outlier --- historyserver/config/rayservice.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/historyserver/config/rayservice.yaml b/historyserver/config/rayservice.yaml index a21e9ea0990..79cb0a5fa7e 100644 --- a/historyserver/config/rayservice.yaml +++ b/historyserver/config/rayservice.yaml @@ -111,7 +111,7 @@ spec: - name: S3FORCE_PATH_STYLE value: "true" - name: RAY_COLLECTOR_POLL_INTERVAL - value: "5s" + value: "30s" command: - collector - --role=Head