fix(deployer): read since as an absolute epoch on Kubernetes - #30
Merged
Merged
Conversation
The abstract deployer declares `since` as an absolute epoch in seconds, and Docker and Podman implement it that way, but the Kubernetes implementation hands the value straight to sinceSeconds, which the API reads as a duration counted back from now. An epoch arrives as roughly 54 years, so the filter silently matches everything: a caller resuming a log stream from a cursor gets the whole log back instead, with no error to notice it by. Convert inside the deployer, rounding up and adding a second so the requested instant lands inside the window rather than just outside it, and keep the result positive -- a cursor ahead of the node's clock would otherwise produce a duration the API ignores. The same log paths also fail on their own declared default. `tail` defaults to None, and the two Kubernetes paths and both endoscopic ones compare it against zero before the API sees it, so a caller relying on the default gets a TypeError rather than every line. Fetching a workload's logs without a tail is exactly how a benchmark snapshot reads them. Task 2 of large-serving-logs. Signed-off-by: yxf <xf.ye@gpustack.ai>
There was a problem hiding this comment.
Code Review
This pull request standardizes and corrects how log retrieval options, specifically since (as an absolute epoch in seconds) and tail, are handled across the Docker, Podman, and Kubernetes deployers. It introduces a to_since_seconds helper in the Kubernetes deployer to properly convert absolute epochs into positive relative durations as required by the Kubernetes API, and ensures that tail handles None values gracefully without causing comparison errors. Additionally, a comprehensive test suite has been added to verify these behaviors. There are no review comments to address, and the changes look solid.
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.
The abstract deployer declares
sinceas an absolute epoch in seconds, and Docker and Podman implement it that way, but the Kubernetes implementation hands the value straight tosinceSeconds, which the API reads as a duration counted back from now. An epoch arrives as roughly 54 years, so the filter silently matches everything: a caller resuming a log stream from a cursor gets the whole log back instead, with no error to notice it by. Reproduced on minikube v1.34.0 —since_seconds=1789884554returned all 29 lines, byte for byte what not filtering returns.to_since_secondsconverts inside the deployer, because the installed Python client exposes onlysinceSecondsand has nosince_timeto pass an instant through. It rounds up and adds a second so the requested instant lands inside the window rather than just outside it, and the caller sees a couple of seconds of older lines rather than a gap — measured at two extra lines, never a missing one. It also floors the result at 1: a cursor ahead of the node's clock would otherwise produce a zero or negative duration, which the API ignores, which is the same "returns everything" failure by another route.The facade docstrings said "since a given time (in seconds)", which reads either way and is how the two implementations came to disagree in the first place. They now say the epoch outright, and the Kubernetes parameter doc that claimed a relative duration follows the facade rather than contradicting it.
The same log paths also fail on their own declared default.
taildefaults toNone, and both Kubernetes paths plus the Docker and Podman endoscopic ones compare it against zero before the API sees it, so a caller relying on the default gets aTypeErrorrather than every line. Docker's and Podman's workload paths passtailthrough untouched and were never affected, which is most of why this has gone unnoticed; on Kubernetes the reachable caller is a benchmark log snapshot, where the exception is swallowed into a log line and shows up only as a snapshot that came back empty.Verified with 20 cases in
tests/gpustack_runtime/deployer/test_logs_options.py: Docker and Podman forward the epoch untouched, both Kubernetes paths convert it,since=Nonestays unset on all four, the declaredtaildefault reaches the API, andto_since_secondsnever returns a non-positive duration. Full suite 671 passed, 20 skipped.This unblocks a timestamp-cursor log resume on the GPUStack side, which needs
sinceto mean the same thing on every deployer. ref gpustack/gpustack#6181