[Test] [historyserver] [collector] Enhance log file coverage test for both head and worker pods #4351
+249
−87
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Jia-Wei Jiang <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
JiangJiaWei1103
commented
Jan 12, 2026
Contributor
Author
JiangJiaWei1103
left a comment
JiangJiaWei1103
left a comment
There was a problem hiding this comment.
Since this PR is based on #4343, we may need to wait for that PR to be merged before merging this one.
| // For a Ray cluster with one head node and one worker node, there are two log directories to verify: | ||
| // - logs/<headNodeID>/ | ||
| // - logs/<workerNodeID>/ | ||
| func assertNonEmptyFileExist(test Test, g *WithT, s3Client *s3.S3, nodeLogDirPrefix string, fileName string) { |
Contributor
Author
There was a problem hiding this comment.
We add this helper to verify specific files exist and have content under certain log directories.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: Future-Outlier <[email protected]>
Future-Outlier
changed the title
4 tasks
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Signed-off-by: JiangJiaWei1103 <[email protected]>
Contributor
Author
|
Hi @Future-Outlier, Please do a final pass. After the event type coverage e2e is merged, we can solve conflicts here, then merge this one. Thanks! |
Future-Outlier
changed the title
Signed-off-by: JiangJiaWei1103 <[email protected]>
Future-Outlier
moved this from to review
to can be merged
in @Future-Outlier's kuberay project
…274/e2e-test-head-worker-logs Signed-off-by: JiangJiaWei1103 <[email protected]>
| LogWithTimestamp(test.T(), "Loaded %d events from job_events/%s", len(jobEvents), jobDir) | ||
| } | ||
|
|
||
| assertAllEventTypesCovered(test, g, uploadedEvents) |
There was a problem hiding this comment.
Event verification lacks retry logic unlike log verification
Medium Severity
The event verification in verifyS3SessionDirs calls loadRayEventsFromS3, listS3Directories, and assertAllEventTypesCovered without retry logic, while the log file verification correctly uses g.Eventually via assertFileExist. Since S3 uploads are asynchronous, events may not be immediately available after cluster deletion, causing flaky test failures. This inconsistency means event checks fail immediately instead of retrying until the timeout.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Enhance log file coverage test for both head and worker pods
The current collector e2e test only checks that at least one non-empty log file exists for the head node. This can miss cases where important logs are missing or where worker logs aren't persisted correctly.
This PR strengthens the test by validating the presence of key log files for both the head and the worker nodes.
NOTE: The head node contains additional logs (e.g.,
gcs_server.out,dashboard.log) that don't exist on worker ndoes.Key Changes
raylet.out,gcs_server.out, andmonitor.outfor the head noderaylet.outfor the worker nodeRelated issue number
N/A
Related PRs
#4330 makes sure the Ray cluster has at least one worker pod.
Test Result
Checks