fix: read the logs once more before a goodput measurement completes - #63
Merged
Conversation
Collaborator
Author
|
/ok to test |
handleSucceeded and handleFailed both build from the stored status and never read logs again, so anything the workload wrote in the last sampling window was dropped. That window is sampleInterval, 60 seconds by default. Seen on hardware: a script that logged up to iteration 100 was recorded as reaching step 90. The workaround at the time was to make the workload sleep before exiting, which is not something a user should have to know. It matters most on failure, where the last lines before a crash are the interesting ones. The bandwidth controller already does this. On JobSucceeded and JobFailed it calls handleRunning for one more parse before going terminal (bandwidthmeasurement_controller.go:152-167). finalSample gives goodput the same shape. One difference worth noting: the throttle in handleRunning exists so that a status update does not re-trigger a read, and the final read lands moments after the previous sample, so it would normally be skipped. finalSample clears the recorded sample time first. Tests cover both halves: the throttle holds on a plain handleRunning call, and finalSample clears it. Best effort, like the bandwidth version: a failure here leaves the previous status in place, which is what would have happened anyway. Signed-off-by: Davanum Srinivas <dsrinivas@nvidia.com>
dims
force-pushed
the
worktree-goodput-final
branch
from
August 8, 2026 01:10
dfeb601 to
92cd1e5
Compare
Follows pkg/catalog/gpu_defaults_test.go, as pkg/controller does throughout. Two cases under testdata/goodput-final-sample. They are a behavioural pin rather than a value transform, so the recorded output is the two facts that matter: whether the throttle was cleared, and whether the call asked for a requeue instead of reading. Together they show why the fix needs the clear. A plain handleRunning call one second after a sample is throttled and requeues. finalSample clears the sample time first, so the last read actually happens. Signed-off-by: Davanum Srinivas <dsrinivas@nvidia.com>
lalitadithya
approved these changes
Aug 8, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What was wrong
handleSucceededandhandleFailedboth build from the stored status viabuildCumulativeFromStatusand never read logs again. Anything the workload wrote in the last sampling window was dropped —sampleInterval, 60 seconds by default.Seen on hardware (test-plan P3.1): a script that logged up to iteration 100 was recorded as reaching
currentStep: 90. The workaround at the time was to make the workload sleep before exiting, which is not something a user should have to know about.It matters most on failure, where the last lines before a crash are the interesting ones.
The shape already exists
The bandwidth controller does exactly this. On
JobSucceededandJobFailedit callshandleRunningfor one more parse before going terminal:bandwidthmeasurement_controller.go:152-167.finalSamplegives goodput the same shape.The detail that makes it work
handleRunningthrottles reads so a status update does not re-trigger one:The final read lands moments after the previous sample, so it would normally be skipped — the throttle would silently defeat the fix.
finalSampleclears the recorded sample time first.Two tests cover both halves, so the mechanism is pinned rather than assumed:
TestHandleRunningIsThrottledWithoutFinalSample— a plain call is throttled and asks for a requeueTestFinalSampleBypassesTheThrottle—finalSampleclears the sample timeScope
Best effort, matching the bandwidth version: a failure in the final read logs and leaves the previous status in place, which is what would have happened anyway.
Verification
make lint0 issues,make buildclean,make testall packages pass. No golden file changes — the existing goodput cases read the same fixture logs, so one extra read produces the same result.