add log opts flag to pass in logging options by wlan0 · Pull Request #12422 · moby/moby

wlan0 · 2015-04-15T23:51:31Z

As per the discussions on this -- https://github.com/docker/docker/pull/11485/files

I'm dividing #11485 into two Pull requests, one for log-opts and one for max-size and max-file options.

This is the PR for log-opts.

@ibuildthecloud
@tiborvass
@LK4D4

ibuildthecloud · 2015-04-15T23:55:30Z

Can you remove the commented lines?

@ibuildthecloud I wanted to enforce a validation step at this point for the log options. The commented lines have a pattern for validating log options.

Perhaps just a TODO? The commented code looks trivial

Yeah, TODO should be ok.

LK4D4 · 2015-04-16T00:20:30Z

Shouldn't we pass this to logging drivers?

LK4D4 · 2015-04-16T00:24:57Z

Maybe SplitN with 2?

That would allow one to specify an option like --map-opt key=value1=value2 I cant think of a use case for something like that. Are there any potential benefits of doing that?

Just checked runconfig/parse.go, and it does splitN with n=2. For consistency between docker run and docker -d, i'll make it splitN

LK4D4 · 2015-04-16T00:29:32Z

Also tests for MapOpts needed.

thaJeztah · 2015-04-16T06:47:04Z

Thanks for splitting it, @wlan0!

LK4D4 · 2015-04-16T19:06:01Z

makes sense to test 2 keys

LK4D4 · 2015-04-16T19:08:00Z

Why do you plan validation for daemon and not for run?

wlan0 · 2015-04-16T20:25:37Z

@LK4D4 Addressed all your comments I think.

Good catch btw, we need validation for docker daemon as well.

Now there are,

Tests for MapOpts
Validation for docker daemon
other minor code changes

LK4D4 · 2015-04-16T21:17:41Z

Is this basically strings.Contains check?

Then I think better to use strings.Contains :)

@LK4D4 This cannot be strings.Contains. strings.Contains will validate some-string=allowed-string, whereas we only want allowed-string=some-string to be valid.

tiborvass · 2015-04-17T15:41:46Z

@wlan0 what is this PR supposed to do? Just introduce log-opt and passing it to the logging driver and that's it? If that's the case I would remove references to max-size etc. in the docs.

wlan0 · 2015-04-22T20:49:32Z

@LK4D4 I've updated the code to initialize LogConfig.Config in docker/daemon.go:init()

LK4D4 · 2015-04-22T21:53:19Z

Code okay for me. I'm eager to see it's usage.
@tiborvass WDYT about adding it to syslog right here?

ibuildthecloud · 2015-04-23T05:18:06Z

FYI, Syslog implementation using --log-opts in #12391

tiborvass · 2015-04-23T20:29:13Z

@ibuildthecloud the PR you referenced doesn't use --log-opt or am I missing something?

@LK4D4 I think I'd prefer in a separate PR, so we can merge this, but I don't really mind.

tiborvass · 2015-04-23T20:34:18Z

@LK4D4 why not use opts.NewListOpts with opts.ValidateEnv like --label ?
Also there are already functions like convertKVStringsToMap that we can reuse.

Have no idea :/ Wanna carry this?

ibuildthecloud · 2015-04-24T02:32:55Z

Lol, wrong PR, too many late night drunken pull requests. Here's the right one #12668

Signed-off-by: wlan0 <[email protected]>

icecrime · 2015-05-05T22:34:41Z

Nit: extra pair of parenthesis.

icecrime · 2015-05-06T16:26:55Z

@wlan0 Can you please rebase?

icecrime · 2015-05-06T20:38:37Z

Thanks, code LGTM.

wlan0 · 2015-05-06T20:50:59Z

@icecrime rebased.
Oh, just saw your comment. :)

jessfraz · 2015-05-07T23:59:36Z

ah man so sorry needs rebase then you can ping me for review and merge thanks!

wlan0 · 2015-05-08T01:07:08Z

@jfrazelle Rebased :)

jessfraz · 2015-05-08T01:10:02Z

LGTM thanks!

add log opts flag to pass in logging options

wlan0 · 2015-05-08T01:47:53Z

Thanks all!

ahmetb · 2015-05-14T07:09:10Z

@LK4D4 @wlan0 so I can see that --log-opt is added to the daemon command. Does this PR also make --log-opts available on docker run because that's another place we configure logging drivers. (I can't see it documented in this PR.)

If one of you can provide more context I can start working on plumbing LoggingOpts to the logdrivers factory using logging.Context.

ahmetb · 2015-05-14T07:34:11Z

@wlan0 isn't this supposed to be &config.LogConfig.Config (not sure, just reading the code)

@ahmetalpbalkan Config is a map, its already a reference type

GordonTheTurtle added status/0-triage dco/no and removed dco/yes labels Apr 15, 2015

wlan0 force-pushed the logopts branch from 72d703f to 978a9e6 Compare April 15, 2015 23:52

GordonTheTurtle added dco/yes and removed dco/no labels Apr 15, 2015

ibuildthecloud reviewed Apr 15, 2015
View reviewed changes

LK4D4 reviewed Apr 16, 2015
View reviewed changes

icecrime added status/1-design-review and removed status/0-triage labels Apr 16, 2015

wlan0 force-pushed the logopts branch from 978a9e6 to fd6de65 Compare April 16, 2015 19:01

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

LK4D4 reviewed Apr 16, 2015
View reviewed changes

Comment thread opts/opts_test.go Outdated

Copy link
Copy Markdown

Contributor

LK4D4 Apr 16, 2015

Choose a reason for hiding this comment

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

makes sense to test 2 keys

wlan0 force-pushed the logopts branch from fd6de65 to 838ee31 Compare April 16, 2015 19:07

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

wlan0 force-pushed the logopts branch from 838ee31 to 49bf4d9 Compare April 16, 2015 20:21

GordonTheTurtle added dco/yes and removed dco/no labels Apr 16, 2015

LK4D4 reviewed Apr 16, 2015
View reviewed changes

tiborvass mentioned this pull request Apr 17, 2015

Add rollover log driver, and --log-driver-opts flag #11485

Merged

icecrime removed the dco/yes label Apr 23, 2015

ibuildthecloud mentioned this pull request Apr 23, 2015

Add tag option to syslog #12668

Closed

tiborvass reviewed Apr 23, 2015
View reviewed changes

LK4D4 mentioned this pull request Apr 30, 2015

daemon: Logging drivers architectural refactoring #12226

Merged

Add log opts flag to pass in logging options

dca9e02

Signed-off-by: wlan0 <[email protected]>

icecrime reviewed May 5, 2015
View reviewed changes

Comment thread opts/opts.go

Copy link
Copy Markdown

Contributor

icecrime May 5, 2015

Choose a reason for hiding this comment

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

Nit: extra pair of parenthesis.

wlan0 force-pushed the logopts branch from ab2e329 to 915842a Compare May 6, 2015 20:15

GordonTheTurtle added the dco/no label May 6, 2015

wlan0 force-pushed the logopts branch from 915842a to bf6b716 Compare May 6, 2015 20:16

GordonTheTurtle removed the dco/no label May 6, 2015

wlan0 force-pushed the logopts branch from bf6b716 to dca9e02 Compare May 8, 2015 00:21

jessfraz pushed a commit that referenced this pull request May 8, 2015

Merge pull request #12422 from wlan0/logopts

fe3bece

add log opts flag to pass in logging options

jessfraz merged commit fe3bece into moby:master May 8, 2015

thaJeztah mentioned this pull request May 10, 2015

Add new Logging driver "fluentd" #12876

Merged

ahmetb reviewed May 14, 2015
View reviewed changes

ahmetb mentioned this pull request May 14, 2015

Logdriver options (--log-opts) not implemented for containers #13225

Closed

Conversation

wlan0 commented Apr 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 16, 2015

Uh oh!

thaJeztah commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Apr 16, 2015

Uh oh!

wlan0 commented Apr 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Apr 17, 2015

Uh oh!

wlan0 commented Apr 22, 2015

Uh oh!

LK4D4 commented Apr 22, 2015

Uh oh!

ibuildthecloud commented Apr 23, 2015

Uh oh!

tiborvass commented Apr 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuildthecloud commented Apr 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icecrime commented May 6, 2015

Uh oh!

icecrime commented May 6, 2015

Uh oh!

wlan0 commented May 6, 2015

Uh oh!

jessfraz commented May 7, 2015

Uh oh!

wlan0 commented May 8, 2015

Uh oh!

jessfraz commented May 8, 2015

Uh oh!

wlan0 commented May 8, 2015

Uh oh!

ahmetb commented May 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels