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 rollover log driver, and --log-driver-opts flag #11485
Conversation
return err | ||
} | ||
|
||
func writeLog(l *RolloverLogger) (int64, error) { |
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.
This probably needs to be protected by a mutex, no?
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.
The impression I got was that calls to Log() are serialized. @LK4D4 can you confirm 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.
Uhm, serialized?
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.
I think he meant to ask if the calls to Log are being made in parallel. They are not, right
? My understanding was that each container has a copier that calls Log(). Therefore, we shouldn't worry about race conditions.
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.
They are. You need to make logger threadsafe, because for now copier writes stdout
and stderr
concurrently.
Look #11472 :)
@wlan0 Gofmt check failed. |
@LK4D4 fixed the gofmt issue. |
@wlan0 Haha, tests don't compile :) |
616b6fc
to
878416e
Compare
@LK4D4 All tests pass now.. There were some unused packages earlier |
@wlan0 @LK4D4 I'd much rather have Also, I think we should not make it a new driver, but instead make the default Proposed defaults:
What do you guys think of this? If we can agree, we could move on to code review. |
+1 I like @tiborvass's suggestion. |
@tiborvass I'm okay with moving that logic to default driver. |
@tiborvass I also agree it should be in the default, we did it as a separate only because we didn't know if you guys would want us screwing with the default logging driver. For example, just keep the default stupid simple because you know it works. |
@ibuildthecloud except this PR would introduce log options, and that's a whole new world :) |
Okay. To sum up.
Question:
|
flag would be singular: Let's make it If |
aa8e6a6
to
7042451
Compare
@LK4D4 @tiborvass updated the code according to comments.
example
|
@wlan0 in your example I agree with everything you pointed out. Don't forget to update the API as well. What's left to decide is what to do with EDIT: also, what happens when it reaches maximum? How are the log files named? Do you go back to "file1" or do you remove file1, and write to file11 ? |
Great! |
👍 |
@wlan0 Thanks for the contribution. Please check your docs output with a |
@@ -17,7 +17,7 @@ container's logging driver. The following options are supported: | |||
|
|||
| `none` | Disables any logging for the container. `docker logs` won't be available with this driver. | | |||
|-------------|-------------------------------------------------------------------------------------------------------------------------------| | |||
| `json-file` | Default logging driver for Docker. Writes JSON messages to file. No logging options are supported for this driver. | | |||
| `json-file` | Default logging driver for Docker. Writes JSON messages to file. Two logging options are supported for this driver. | |
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.
I think you can remove "Two logging options are supported for this driver.". We don't mention this for the other drivers and it is explained below, so doesn't add much.
75de868
to
f1e2313
Compare
@moxiegirl @thaJeztah I've updated the docs based on your comments. |
The following logging options are supported for the `json-file` logging driver: | ||
|
||
--log-opt max-size=(unlimited) | ||
--log-opt max-file=1 |
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.
This isn't rendered as code-block. Perhaps the indentation is 3 in stead of 4 spaces? You can test the docs by running make docs
, which will build the and serve the docs in a container so that you can preview them in your webbrowser.
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.
Thinking of the max-size
option; if the default is "unlimited", then specifying max-files
without specifying max-size
is a no-op? I.e., the max-size is never reached, so there will never be a roll-over?
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.
yes. That is correct. max-file
is meaningless without max-size. It won't be honored even if you set it. If you want me to describe it differently, please let me know.
f15e712
to
51b7e5a
Compare
@thaJeztah @moxiegirl Updated. |
Hi, Thanks for the PR as I have no doubt it adds a very useful feature to Docker. Here are some comments about the doc changes.
|
@draghuram makes very good points. @wlan0 can you clarify in the docs these points please? Thank you for being so cooperative on making changes. User would appreciate this extra clarification. |
@moxiegirl @draghuram Thanks! Absolutely not a problem. I would like to be as clear as possible while describing these options as well. Firstly, I've updated the docs keeping in mind all of your comments. Secondly, to answer your questions -
I've updated the docs according to the rest of your comments. |
|
||
`max-file` specifies the maximum number of files that a log is rolled over before being discarded. eg `--log-opt max-file=100`. If `max-size` is not set, then `max-file` is not honored. | ||
|
||
If `max-size` and `max-file` are set, `docker logs` only returns the log lines from the newest log file. |
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.
I think this information really belongs in logs.md
. In fact, I would like logs.md
to provide some description of rollover feature and how that impacts the behavior of options. For example, --tail
description says that when an invalid value is passed, the value is set to latest
. However, you can't tell what latest
means just by looking at the man page.
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.
Just to be clear, my comment above only applies to the line "If max-size
and max-file
are set, docker logs
only returns the log lines from the newest log file.".
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.
@draghuram I think you have a point about the logs.md file but it is beyond the scope of this PR. I created an issue here: #14711 and assigned it to myself.
@wlan0 Thanks for the changes. In addition to my comment above, I would really like to see an example in |
LGTM thanks @wlan0 |
@thaJeztah for the merge. |
LGTM! Thanks so much for being so patient and cooperative, @wlan0. Apologies that it took so long :-) |
Add rollover log driver, and --log-driver-opts flag
yaay finally!!! thanks all... :) |
+1!!! |
Hello, thanks for this feature! Greetings, |
@henfri please don't comment on closed/merged PR's for support questions; the issue tracker is meant for tracking bugs, and reviewing pull requests; your question is better asked;
|
Closes #8911
This adds log rollover capability to docker. I have added a new driver called
rollover
for this.There are two options that can be specified along with this option. In order to specify these options, I added a new flag called
--log-driver-opts
. That would allow one to specify options like thisdocker run --logging-driver rollover --log-driver-opts max_size=1k --log-driver-opts file_count=10