Add syslog-address log-opt by runcom · Pull Request #13392 · moby/moby

runcom · 2015-05-21T20:24:47Z

Didn't add tests and docs yet, just want to agree on what I did here

This adds --log-opt syslog-address=tcp://127.0.0.1:514 to be able to specify a remote syslog server to log thinghies (suggestions regarding the name I've choosen are welcome if you don't like it)

I removed ValidateLogOpts because basically I think driver's specific opts shouldn't be checked in opts/opts.go otherwise it will be cluttered in no time and it's driver's business to handle its own opts.
But I still think it could be useful for drivers shared options like tag where docker could let people chose which field to use for the tag (like container or name for instance EDIT on this tag more info here: #12876 (comment))

ping @LK4D4 @duglin @thaJeztah

resolves #13362

Signed-off-by: Antonio Murdaca [email protected]

LK4D4 · 2015-05-21T20:28:07Z

Maybe just address?

other drivers may have their own parsing routine to decide if address is correct

thaJeztah · 2015-05-21T20:29:41Z

think driver's specific opts shouldn't be checked in opts/opts.go otherwise it will be cluttered in no time and it's driver business handling its own opts.

+1 makes sense

LK4D4 · 2015-05-21T20:30:59Z

Isn't it working without this?

mmm I tried this morning and golang syslog.Dial was complaining about missing :514, will check again now gimme a minute

yup isn't working w/o: panic: dial tcp: missing port in address localhost

address + ":514"
Also see net.SplitHostPort, I think this would be better.

@cpuguy83 better to check if port isn't defined?

Well, you are already doing that, but SplitHostPort is a bit more sophisticated and works with ipv6.

Yeah, I think it's better, too.

updated thanks

LK4D4 · 2015-05-21T21:09:08Z

Seems like it can return three types of error and your case only for one :)

@LK4D4 thanks updated

cpuguy83 · 2015-05-22T00:02:33Z

You should use + to concatenate strings instead of fmt.

done both, overlooked your last comments sorry

LK4D4 · 2015-05-22T21:08:14Z

ping @docker/core-maintainers
It's superimportant feature.

runcom · 2015-05-26T19:41:32Z

ping @cpuguy83 @duglin @calavera

tiborvass · 2015-05-26T19:47:44Z

@runcom can you verify this will not relax IsURL checks for other callers?

sure will do

@tiborvass this will def relax IsURL so I'm gonna do that simple Prefix checks ~~here~~ in logger

cpuguy83 · 2015-05-26T19:56:44Z

Adding to 1.7 milestone b/c I think this is super-important as well.

cpuguy83 · 2015-05-26T19:57:52Z

Also definitely +1 here.

thaJeztah · 2015-05-27T23:08:37Z

thaJeztah · 2015-05-27T23:11:58Z

Not completely happy with the "The following logging options are supported for this logging driver: [none]" sentences, but for now it should suffice 😄

LGTM

do we need another look at #13392 (comment)?

runcom · 2015-05-27T23:15:02Z

@thaJeztah I borrowed that sentence from the previous docs but I'm sure we can change it later during a refactor (other pr are pulling in log driver options doc as well)

wrt that comment, I prefer to have a final go from them

jberryman · 2015-05-28T07:10:53Z

Having a bit of trouble following all the --log-driver discussions and have a couple questions:

Does this support specifying an alternative socket file to /dev/log? (see my comment on the code above). This is something I need and have implemented in a fork.
Can this be set per-container when doing run? (this would be nice for me)

runcom · 2015-05-28T07:14:41Z

No, but make sense to have Unix sockets opt as well since syslog has imuxsock module http://www.rsyslog.com/doc/v8-stable/configuration/modules/imuxsock.html
Yes

moxiegirl · 2015-05-28T13:22:53Z

tics around syslod and syslog in text.

syslod is a typo, it should be syslogd.

Yup noticed

moxiegirl · 2015-05-28T13:24:11Z

With formatting changes, LGTM. Thank you for the contribution.

runcom · 2015-05-28T22:23:56Z

@jberryman updated this pr to include custom unix socket log options

ping @tiborvass @LK4D4 @cpuguy83 to have another last look on this since I updated code to include specifing a custom unix socket path

icecrime · 2015-05-28T22:25:45Z

Moving back to code review then.

runcom · 2015-05-28T22:26:34Z

thx @icecrime

LK4D4 · 2015-05-28T22:27:12Z

@runcom lol. need rebase :)

tiborvass · 2015-05-28T22:28:06Z

i would split this in two for tcp|udp and unix, because of the host:port|path part

tiborvass · 2015-05-28T22:28:34Z

no need for extra variable

isn't cleaner not to use url.Path twice? or is that bad? or is ok, I'll change it :)

it's a compiled language :)

yeah and I'm too used to dynamic ones doh

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2015-05-28T22:42:54Z

@tiborvass took care of your comments

calavera · 2015-05-29T17:21:53Z

LGTM. Merging since it looks like it already went through docs review.

Add syslog-address log-opt

runcom · 2015-05-29T17:24:19Z

thanks guys 🎉

runcom · 2015-05-29T17:26:00Z

@jberryman now you could test the latest master code with this log-opt for unix socket :)

jessfraz · 2015-05-29T17:28:55Z

cherry-picked \o/ thanks for assigning me

GordonTheTurtle added the status/0-triage label May 21, 2015

LK4D4 reviewed May 21, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch from 4920fa6 to 8ad17fd Compare May 21, 2015 20:30

LK4D4 reviewed May 21, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch from 8ad17fd to 261875e Compare May 21, 2015 20:45

runcom added status/1-design-review and removed status/0-triage labels May 21, 2015

runcom force-pushed the syslog-connection-url-log-opt branch from 261875e to 9baa64b Compare May 21, 2015 20:53

LK4D4 reviewed May 21, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch 3 times, most recently from 59ecada to 7cc8d05 Compare May 21, 2015 21:42

cpuguy83 reviewed May 22, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch 3 times, most recently from 6048313 to 23ae19d Compare May 22, 2015 18:32

runcom changed the title ~~Add -log-opt for syslog connection url~~ Add syslog-address log-opt May 22, 2015

runcom force-pushed the syslog-connection-url-log-opt branch 2 times, most recently from ef6916c to 5aec59d Compare May 25, 2015 19:40

pebo mentioned this pull request May 26, 2015

Add UDP and TCP support to syslog log adapter #13362

Closed

tiborvass added status/2-code-review and removed status/1-design-review labels May 26, 2015

tiborvass reviewed May 26, 2015
View reviewed changes

cpuguy83 added this to the 1.7.0 milestone May 26, 2015

thaJeztah reviewed May 27, 2015
View reviewed changes

Comment thread docs/sources/reference/commandline/cli.md

Copy link
Copy Markdown

Member

thaJeztah May 27, 2015

Choose a reason for hiding this comment

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

🎉 :)

moxiegirl reviewed May 28, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch 2 times, most recently from 4fa10c1 to 1052f9c Compare May 28, 2015 22:22

icecrime added status/2-code-review and removed status/3-docs-review labels May 28, 2015

tiborvass reviewed May 28, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch from 1052f9c to ffd02f4 Compare May 28, 2015 22:28

tiborvass reviewed May 28, 2015
View reviewed changes

runcom force-pushed the syslog-connection-url-log-opt branch from ffd02f4 to 40fd8b1 Compare May 28, 2015 22:30

Add syslog-address log-opt

e8c88d2

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the syslog-connection-url-log-opt branch from 40fd8b1 to e8c88d2 Compare May 28, 2015 22:42

calavera added a commit that referenced this pull request May 29, 2015

Merge pull request #13392 from runcom/syslog-connection-url-log-opt

f1fed87

Add syslog-address log-opt

calavera merged commit f1fed87 into moby:master May 29, 2015

calavera assigned jessfraz May 29, 2015

runcom deleted the syslog-connection-url-log-opt branch May 29, 2015 17:24

Conversation

runcom commented May 21, 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!

thaJeztah commented May 21, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented May 22, 2015

Uh oh!

runcom commented May 26, 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!

cpuguy83 commented May 26, 2015

Uh oh!

cpuguy83 commented May 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 27, 2015

Uh oh!

runcom commented May 27, 2015

Uh oh!

jberryman commented May 28, 2015

Uh oh!

runcom commented May 28, 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!

moxiegirl commented May 28, 2015

Uh oh!

runcom commented May 28, 2015

Uh oh!

icecrime commented May 28, 2015

Uh oh!

runcom commented May 28, 2015

Uh oh!

LK4D4 commented May 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment