-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a job log length limit #2192
Conversation
81df96b
to
9f8dee9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ 😁 ✨
36c6dcd
to
6aa2978
Compare
ls.logger.Warn("The job log has reached %s in size, which has "+ | ||
"exceeded the maximum size (%s). Further logs may be dropped "+ | ||
"by the server, and a future version of the agent will stop "+ | ||
"sending logs at this point.", | ||
humanize.Bytes(ls.bytes), humanize.Bytes(ls.conf.MaxSizeBytes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewborden suggested this should just be a warning for now. Here's a cut of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Since removing the need to hold the whole log in memory, there have been a few cases of long-running jobs logging lots of logs. This adds a (server-specified) limit, with a default of 1GiB.
Why not just limit it server side? Because the job itself should get some kind of pushback once it approaches or hits the limit. Returning errors when writing to the buffer feels like the right place.