Skip to content

logfile: Check if log is closed on close error during rotate#40920

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
cpuguy83:log_rotate_error_handling
May 7, 2020
Merged

logfile: Check if log is closed on close error during rotate#40920
cpuguy83 merged 1 commit intomoby:masterfrom
cpuguy83:log_rotate_error_handling

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 commented May 6, 2020

This prevents getting into a situation where a container log cannot make
progress because we tried to rotate a file, got an error, and now the
file is closed. The next time we try to write a log entry it will try
and rotate again but error that the file is already closed.

I wonder if there is more we can do to beef up this rotation logic.
Found this issue while investigating missing logs with errors in the
docker daemon logs like:

Failed to log message for json-file: error closing file: close <file>:
file already closed

I'm not sure why the original rotation failed since the data was no
longer available.

This prevents getting into a situation where a container log cannot make
progress because we tried to rotate a file, got an error, and now the
file is closed. The next time we try to write a log entry it will try
and rotate again but error that the file is already closed.

I wonder if there is more we can do to beef up this rotation logic.
Found this issue while investigating missing logs with errors in the
docker daemon logs like:

```
Failed to log message for json-file: error closing file: close <file>:
file already closed
```

I'm not sure why the original rotation failed since the data was no
longer available.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the log_rotate_error_handling branch from 94aa126 to 3989f91 Compare May 7, 2020 18:37
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants