Skip to content
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

Merged
merged 1 commit into from Jul 17, 2015

Conversation

wlan0
Copy link
Contributor

@wlan0 wlan0 commented Mar 19, 2015

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 this

docker run --logging-driver rollover --log-driver-opts max_size=1k --log-driver-opts file_count=10

return err
}

func writeLog(l *RolloverLogger) (int64, error) {
Copy link
Member

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?

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Uhm, serialized?

Copy link
Contributor Author

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.

Copy link
Contributor

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 :)

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 19, 2015

@wlan0 Gofmt check failed.
We need design review on new flag, but I sorta like it, not tried to use though.
ping @crosbymichael @tiborvass @jfrazelle

@wlan0
Copy link
Contributor Author

wlan0 commented Mar 19, 2015

@LK4D4 fixed the gofmt issue.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 19, 2015

@wlan0 Haha, tests don't compile :)

@wlan0 wlan0 force-pushed the rollover_log branch 2 times, most recently from 616b6fc to 878416e Compare March 19, 2015 06:15
@wlan0
Copy link
Contributor Author

wlan0 commented Mar 19, 2015

@LK4D4 All tests pass now.. There were some unused packages earlier

@tiborvass
Copy link
Contributor

@wlan0 @LK4D4 I'd much rather have --log-opt like we have --storage-opt.

Also, I think we should not make it a new driver, but instead make the default json-file driver have options with this new flag. So rollover would be done like this: docker run --log-opt max_file=10 --log-opt max_size=1k.

Proposed defaults:

  • max_size = 0 (no limit)
  • max_file = 1

What do you guys think of this? If we can agree, we could move on to code review.

@cpuguy83
Copy link
Member

+1 I like @tiborvass's suggestion.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 19, 2015

@tiborvass I'm okay with moving that logic to default driver.

@ibuildthecloud
Copy link
Contributor

@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.

@tiborvass
Copy link
Contributor

@ibuildthecloud except this PR would introduce log options, and that's a whole new world :)

EDIT: https://www.youtube.com/watch?v=-kl4hJ4j48s

@ibuildthecloud
Copy link
Contributor

Okay. To sum up.

  • Change --log-driver-opts to --log-opts
  • Remove "rollover" driver and put logic in json-file
  • max_size = 0 should mean unlimited
  • Defaults should be max_size=0, max_file=1

Question:

  • Should it be max-size or max_size, which is more consistent with existing Docker?
  • docker logs should read from all historical logs or just the latest?

@tiborvass
Copy link
Contributor

@ibuildthecloud

flag would be singular: --log-opt.

Let's make it max-size to make it consistent with -restart on-failure[:max-retry]. Also I generally prefer - rather than _.

If docker logs outputs only the latest file, the default value of --tail would need to change to latest or something like that. I'm not sure what's the best solution for that, I'm still thinking.

@wlan0 wlan0 force-pushed the rollover_log branch 5 times, most recently from aa8e6a6 to 7042451 Compare March 19, 2015 21:54
@wlan0
Copy link
Contributor Author

wlan0 commented Mar 19, 2015

@LK4D4 @tiborvass updated the code according to comments.

  • Now, the flag is --log-opt
  • the options use - instead of _, so max-file and max-size are the options
  • the options have been added to the json-file log driver
  • the options have default values max-file=1 and max-size=0 (infinite)
  • updated docs

example

docker run --log-driver json-file --log-opt max-size=1k --log-opt max-file=10 -d redis

@tiborvass
Copy link
Contributor

@wlan0 in your example --log-driver json-file is not necessary since it's the default.

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 docker logs... :S @LK4D4 @jfrazelle @icecrime

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 ?

@pires
Copy link

pires commented Mar 21, 2015

Great!

@mahnunchik
Copy link

👍

@moxiegirl
Copy link
Contributor

@wlan0 Thanks for the contribution. Please check your docs output with a make docs from the docs dir.

@@ -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. |
Copy link
Member

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.

@wlan0 wlan0 force-pushed the rollover_log branch 2 times, most recently from 75de868 to f1e2313 Compare July 15, 2015 22:48
@wlan0
Copy link
Contributor Author

wlan0 commented Jul 15, 2015

@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
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

@wlan0 wlan0 force-pushed the rollover_log branch 3 times, most recently from f15e712 to 51b7e5a Compare July 16, 2015 04:20
@wlan0
Copy link
Contributor Author

wlan0 commented Jul 16, 2015

@thaJeztah @moxiegirl Updated.

@draghuram
Copy link
Contributor

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.

  • docs/reference/logging/index.md

    • As @thaJeztah mentioned, I think it is worthwhile to clearly document that max-file is meaningless without max-size. At the same time, we should also mention the default value for max-size (which is unlimited).
    • There is a missing space in "The docker logscommand" though it is not this PR's doing.
  • docs/reference/commandline/logs.md

    • It is not clear to me why log-opt option is added to docker logs command. What exactly can I pass with this option? I may be missing something here but the options max-size and max-file make sense only for docker run command.
    • If I run docker logs without any options, does it combine all the log files or just return the latest? I ask because the command seems to default to "latest" when an invalid value is passed to --tail option. Similarly, does --since option apply across all the log files?
  • docs/reference/commandline/run.md

    It would be beneficial if an example is provided in this file covering the new log options.

@moxiegirl
Copy link
Contributor

@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.

@wlan0
Copy link
Contributor Author

wlan0 commented Jul 16, 2015

@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 -

  1. --log-opt doesn't make sense in logs command. Good catch. Removed this from the logs.md file in the docs.
  2. If you run docker logs without options, it only provides the latest logs. Added this to the docs.

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.
Copy link
Contributor

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.

Copy link
Contributor

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.".

Copy link
Contributor

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.

@draghuram
Copy link
Contributor

@wlan0 Thanks for the changes. In addition to my comment above, I would really like to see an example in run.md but I am ok with the PR being merged without one.

@moxiegirl
Copy link
Contributor

LGTM thanks @wlan0

@moxiegirl
Copy link
Contributor

@thaJeztah for the merge.

@thaJeztah
Copy link
Member

LGTM! Thanks so much for being so patient and cooperative, @wlan0. Apologies that it took so long :-)

thaJeztah added a commit that referenced this pull request Jul 17, 2015
Add rollover log driver, and --log-driver-opts flag
@thaJeztah thaJeztah merged commit 415f744 into moby:master Jul 17, 2015
@wlan0
Copy link
Contributor Author

wlan0 commented Jul 17, 2015

yaay finally!!! thanks all... :)

@cdancy
Copy link

cdancy commented Jul 17, 2015

+1!!!

@henfri
Copy link

henfri commented Feb 17, 2016

Hello,

thanks for this feature!
Is it possible to use it with docker-compose?
How do I specify/call it in the compose.yml

Greetings,
Hendrik

@thaJeztah
Copy link
Member

@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;

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.

[Proposal] Limit or rotate docker container logs