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

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 19, 2015
Collaborator

This probably needs to be protected by a mutex, no?

This comment has been minimized.

@ibuildthecloud

ibuildthecloud Mar 19, 2015
Contributor

The impression I got was that calls to Log() are serialized. @LK4D4 can you confirm that?

This comment has been minimized.

@LK4D4

LK4D4 Mar 19, 2015
Contributor

Uhm, serialized?

This comment has been minimized.

@wlan0

wlan0 Mar 19, 2015
Author Contributor

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.

This comment has been minimized.

@LK4D4

LK4D4 Mar 19, 2015
Contributor

They are. You need to make logger threadsafe, because for now copier writes stdout and stderr concurrently.
Look #11472 :)

@LK4D4
Copy link
Contributor

@LK4D4 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 wlan0 force-pushed the wlan0:rollover_log branch from 2b79f22 to fefdc26 Mar 19, 2015
@wlan0
Copy link
Contributor Author

@wlan0 wlan0 commented Mar 19, 2015

@LK4D4 fixed the gofmt issue.

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Mar 19, 2015

@wlan0 Haha, tests don't compile :)

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

@wlan0 wlan0 commented Mar 19, 2015

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

@tiborvass
Copy link
Collaborator

@tiborvass tiborvass commented Mar 19, 2015

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

@cpuguy83 cpuguy83 commented Mar 19, 2015

+1 I like @tiborvass's suggestion.

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Mar 19, 2015

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

@ibuildthecloud
Copy link
Contributor

@ibuildthecloud ibuildthecloud commented Mar 19, 2015

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

@tiborvass tiborvass commented Mar 19, 2015

@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

@ibuildthecloud ibuildthecloud commented Mar 19, 2015

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
Collaborator

@tiborvass tiborvass commented Mar 19, 2015

@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 wlan0:rollover_log branch 5 times, most recently from aa8e6a6 to 7042451 Mar 19, 2015
@wlan0
Copy link
Contributor Author

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

@tiborvass tiborvass commented Mar 20, 2015

@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 pires commented Mar 21, 2015

Great!

@mahnunchik
Copy link

@mahnunchik mahnunchik commented Mar 21, 2015

👍

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 15, 2015

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

This comment has been minimized.

@thaJeztah

thaJeztah Jul 15, 2015
Member

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 wlan0:rollover_log branch 2 times, most recently from 75de868 to f1e2313 Jul 15, 2015
@wlan0
Copy link
Contributor Author

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

This comment has been minimized.

@thaJeztah

thaJeztah Jul 15, 2015
Member

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.

This comment has been minimized.

@thaJeztah

thaJeztah Jul 15, 2015
Member

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?

This comment has been minimized.

@wlan0

wlan0 Jul 15, 2015
Author Contributor

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.

@@ -25,6 +25,16 @@ container's logging driver. The following options are supported:

The `docker logs`command is available only for the `json-file` logging driver.

### The json-file options

The following logging options are supported for the `json-file` logging driver:

This comment has been minimized.

@thaJeztah

thaJeztah Jul 15, 2015
Member

Can you change this line to the example that @moxiegirl gave in #11485 (comment) ?

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

@wlan0 wlan0 commented Jul 16, 2015

@draghuram
Copy link
Contributor

@draghuram draghuram commented Jul 16, 2015

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

@moxiegirl moxiegirl commented Jul 16, 2015

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

@wlan0 wlan0 force-pushed the wlan0:rollover_log branch from 51b7e5a to 9b782d3 Jul 16, 2015

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

This comment has been minimized.

@draghuram

draghuram Jul 17, 2015
Contributor

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.

This comment has been minimized.

@draghuram

draghuram Jul 17, 2015
Contributor

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

This comment has been minimized.

@moxiegirl

moxiegirl Jul 17, 2015
Contributor

@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

@draghuram draghuram commented Jul 17, 2015

@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

@moxiegirl moxiegirl commented Jul 17, 2015

LGTM thanks @wlan0

@moxiegirl
Copy link
Contributor

@moxiegirl moxiegirl commented Jul 17, 2015

@thaJeztah for the merge.

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 17, 2015

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
4 checks passed
4 checks passed
@GordonTheTurtle
docker/dco-signed All commits signed
Details
@GordonTheTurtle
experimental Jenkins build Docker-PRs-experimental 2946 has succeeded
Details
@GordonTheTurtle
janky Jenkins build Docker-PRs 11505 has succeeded
Details
@GordonTheTurtle
windows Jenkins build Windows-PRs 8126 has succeeded
Details
@wlan0
Copy link
Contributor Author

@wlan0 wlan0 commented Jul 17, 2015

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

@cdancy
Copy link

@cdancy cdancy commented Jul 17, 2015

+1!!!

@henfri
Copy link

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

@thaJeztah thaJeztah commented Feb 17, 2016

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

Successfully merging this pull request may close these issues.