-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
events: no duplicates when streaming during a log rotation #17771
Conversation
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.
minor nits, otherwise LGTM. The duplication bothers me but I can't find a sane way to refactor it.
Thanks for the review! |
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.
some comments
When streaming events, prevent returning duplicates after a log rotation by marking a beginning and an end for rotated events. Before starting to stream, get a timestamp while holding the event lock. The timestamp allows for detecting whether a rotation event happened while reading the log file and to skip all events between the begin and end rotation event. In an ideal scenario, we could detect rotated events by enforcing a chronological order when reading and skip those detected to not be more recent than the last read event. However, events are not always _written_ in chronological order. While this can be changed, existing event files could not be read correctly anymore. Fixes: containers#17665 Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@containers/podman-maintainers PTAL |
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
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
When streaming events, prevent returning duplicates after a log rotation by marking a beginning and an end for rotated events. Before starting to stream, get a timestamp while holding the event lock. The timestamp allows for detecting whether a rotation event happened while reading the log file and to skip all events between the begin and end rotation event.
In an ideal scenario, we could detect rotated events by enforcing a chronological order when reading and skip those detected to not be more recent than the last read event. However, events are not always written in chronological order. While this can be changed, existing event files could not be read correctly anymore.
Fixes: #17665
Does this PR introduce a user-facing change?
@Luap99 WDYT?