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

Fix issue with log rotation and max-file=1 #38489

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@achntrl
Copy link

achntrl commented Jan 3, 2019

- What I did

Fixed a bug where docker logs -f would stop reading logs from a container when using the json logging driver with the option max-file=1. See #37646

- How I did it

When max-file <= 1, the log file is truncated and no rename nor remove happens so we never reach this path:

case fsnotify.Rename, fsnotify.Remove:

where handleRotate() is called to properly handle the file rotation.

Since I didn't find a way to subscribe to a truncate event, I allowed the creation of a new file that is deleted shortly after the rotation event has been published. I did not keep the file longer because users that specify max-size=<size> and max-file=1 would expect their logs to never go over <size>. Keeping the file would allow this size of their logs to go up to 2 * <size> (unless we delete it shortly, in that case should not be problematic)

I had to wait some time (1 second in the PR, that should be above the 200ms poll interval for Windows) before the deletion because of a synchronisation issue with waitRead. I would be interested in seeing a better way of handling this synchronisation.

- How to verify it

$ cid=$(docker run --rm --log-driver json-file \
  --log-opt max-size=10k \
  --log-opt max-file=1 \
  -d busybox \
  sh -c 'i=0; while true; do i=$((i+1)); echo $i hello world; sleep 0.1; done')

$ docker logs -f $cid
1 hello world
2 hello world
...
121 hello world # before it would have hung after this log
122 hello world
...

- Description for the changelog

Fix a bug where docker logs -f command would stop after a log rotation when max-file=1 option was set.

- A picture of a cute animal (not mandatory but encouraged)
image

@achntrl achntrl force-pushed the achntrl:37646-fix-log-rotate branch from df9336c to 3414b5c Jan 3, 2019

@yongtang yongtang requested a review from cpuguy83 Jan 4, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 8, 2019

(reserved for my derek commands)

@derek derek bot added the rebuild/* label Jan 8, 2019

@derek derek bot added the rebuild/z label Jan 9, 2019

@derek derek bot added the rebuild/windowsRS1 label Jan 9, 2019

@olljanat

This comment has been minimized.

Copy link
Contributor

olljanat commented Jan 9, 2019

@achntrl RS5 build looks to be on some weird state and I cannot re-trigger it. Can you do forced git push which should trigger new CI round?

Fix issue with log rotation and max-file=1
Closes #37646

Signed-off-by: Alexandre Chaintreuil <alexandre.chaintreuil@datadoghq.com>

@achntrl achntrl force-pushed the achntrl:37646-fix-log-rotate branch from 3414b5c to a9e4a4a Jan 9, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 9, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cb50188). Click here to learn what that means.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##             master   #38489   +/-   ##
=========================================
  Coverage          ?   36.92%           
=========================================
  Files             ?      608           
  Lines             ?    45200           
  Branches          ?        0           
=========================================
  Hits              ?    16691           
  Misses            ?    26204           
  Partials          ?     2305
@achntrl

This comment has been minimized.

Copy link

achntrl commented Jan 9, 2019

@olljanat It looks like my test doesn't pass on Windows, not sure why

@@ -191,6 +191,16 @@ func (w *LogFile) checkCapacityAndRotate() error {
w.currentSize = 0
w.notifyRotate.Publish(struct{}{})

if w.maxFiles <= 1 {
go func() {

This comment has been minimized.

@cpuguy83

cpuguy83 Jan 9, 2019

Contributor

Why the goroutine here?

I would think in rotate we can call remove instead of rename when maxFiles <= 1

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