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 syslog logging driver #11458

Merged
merged 1 commit into from Mar 23, 2015

Conversation

Projects
None yet
@ibuildthecloud
Contributor

ibuildthecloud commented Mar 18, 2015

This PR adds support for logging to syslog from Docker by using the go built in syslog package. This code uses the existing logging driver framework added to Docker 1.6. An example output to syslog looks like

Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:                 _._                                                  
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:            _.-``__ ''-._                                             
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:       _.-``    `.  `_.  ''-._           Redis 2.8.19 (00000000/0) 64 bit
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:   .-`` .-```.  ```\/    _.,_ ''-._                                   
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  (    '      ,       .-`  | `,    )     Running in stand alone mode
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |`-._`-...-` __...-.``-._|'` _.-'|     Port: 6379
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |    `-._   `._    /     _.-'    |     PID: 1
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:   `-._    `-._  `-./  _.-'    _.-'                                   
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |`-._`-._    `-.__.-'    _.-'_.-'|                                  
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |    `-._`-._        _.-'_.-'    |           http://redis.io        
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:   `-._    `-._`-.__.-'_.-'    _.-'                                   
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |`-._`-._    `-.__.-'    _.-'_.-'|                                  
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:  |    `-._`-._        _.-'_.-'    |                                  
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:   `-._    `-._`-.__.-'_.-'    _.-'                                   
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:       `-._    `-.__.-'    _.-'                                       
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:           `-._        _.-'                                           
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b:               `-.__.-'                                               
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b: 
Mar 18 00:36:04 inotmac docker[28485]: 94ce4ded774b: [1] 18 Mar 07:36:04.448 # Server started, Redis version 2.8.19

NOTE: Credit goes to @wlan0 for the development of this, I'm just submitting the PR

@GordonTheTurtle

This comment has been minimized.

Show comment
Hide comment
@GordonTheTurtle

GordonTheTurtle Mar 18, 2015

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "syslog" git@github.com:ibuildthecloud/docker.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

GordonTheTurtle commented Mar 18, 2015

Can you please sign your commits following these rules:

https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work

The easiest way to do this is to amend the last commit:

$ git clone -b "syslog" git@github.com:ibuildthecloud/docker.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

This will update the existing PR, so you do not need to open a new one.

Show outdated Hide outdated docs/sources/reference/run.md Outdated
@sirupsen

This comment has been minimized.

Show comment
Hide comment
@sirupsen

sirupsen Mar 18, 2015

Contributor

There's a bunch of other docs where you need to add it, especially as a CLI example. The initial PR points out those places. Also needs some tests. Other than that, this looks good.

Contributor

sirupsen commented Mar 18, 2015

There's a bunch of other docs where you need to add it, especially as a CLI example. The initial PR points out those places. Also needs some tests. Other than that, this looks good.

@@ -1377,6 +1378,12 @@ func (container *Container) startLogging() error {
return err
}
l = dl
case "syslog":
dl, err := syslog.New(container.ID[:12])

This comment has been minimized.

@sirupsen

sirupsen Mar 18, 2015

Contributor

Why only 12 bytes?

@sirupsen

sirupsen Mar 18, 2015

Contributor

Why only 12 bytes?

This comment has been minimized.

@wlan0

wlan0 Mar 18, 2015

Contributor

The container id is too long and takes up a large portion of the log line.

@wlan0

wlan0 Mar 18, 2015

Contributor

The container id is too long and takes up a large portion of the log line.

Show outdated Hide outdated daemon/logger/syslog/syslog.go Outdated
Show outdated Hide outdated daemon/logger/syslog/syslog.go Outdated
@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Mar 18, 2015

Contributor

One caveat with writing to syslog is that /dev/log (which syslog uses) can be a SOCK_STREAM (as opposed to SOCK_DGRAM) which blocks if the buffer fills up on the other end, such as if syslog stops reading (rsyslog is notorious for this). Once it blocks, and the buffer between the application inside the container and docker fills up, the application inside the container will block trying to write to STDOUT/STDERR.

If we want to handle this, the only solution to this is to drop log entries if writing to syslog blocks. I can't think of any way of doing this other than introducing a channel and starting up a goroutine which reads off the channel and calls writer.Err/writer.Info

Contributor

phemmer commented Mar 18, 2015

One caveat with writing to syslog is that /dev/log (which syslog uses) can be a SOCK_STREAM (as opposed to SOCK_DGRAM) which blocks if the buffer fills up on the other end, such as if syslog stops reading (rsyslog is notorious for this). Once it blocks, and the buffer between the application inside the container and docker fills up, the application inside the container will block trying to write to STDOUT/STDERR.

If we want to handle this, the only solution to this is to drop log entries if writing to syslog blocks. I can't think of any way of doing this other than introducing a channel and starting up a goroutine which reads off the channel and calls writer.Err/writer.Info

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 18, 2015

Contributor

@phemmer also there is bug in golang syslog package golang/go#5932 :( I solved it with copying std syslog and adding WriteDeadline for connection.

Contributor

LK4D4 commented Mar 18, 2015

@phemmer also there is bug in golang syslog package golang/go#5932 :( I solved it with copying std syslog and adding WriteDeadline for connection.

@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud Mar 18, 2015

Contributor

@LK4D4 @phemmer Do you think we need to address this syslog blocking issue in this PR?

Contributor

ibuildthecloud commented Mar 18, 2015

@LK4D4 @phemmer Do you think we need to address this syslog blocking issue in this PR?

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 18, 2015

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 18, 2015

Contributor

@ibuildthecloud I don't know actually how to do this, because Write is not low-level enough to interrupt it.
@phemmer Actually application won't be blocked. We use pre-read tactics for stdout/stderr pipes. So probably fine for now.

Contributor

LK4D4 commented Mar 18, 2015

@ibuildthecloud I don't know actually how to do this, because Write is not low-level enough to interrupt it.
@phemmer Actually application won't be blocked. We use pre-read tactics for stdout/stderr pipes. So probably fine for now.

@phemmer

This comment has been minimized.

Show comment
Hide comment
@phemmer

phemmer Mar 18, 2015

Contributor

Actually application won't be blocked. We use pre-read tactics for stdout/stderr pipes. So probably fine for now.

I don't follow. There's a buffer somewhere, that buffer is going to get full, and something is going to block (unless we're already discarding data when buffers fill up).

Contributor

phemmer commented Mar 18, 2015

Actually application won't be blocked. We use pre-read tactics for stdout/stderr pipes. So probably fine for now.

I don't follow. There's a buffer somewhere, that buffer is going to get full, and something is going to block (unless we're already discarding data when buffers fill up).

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 18, 2015

Contributor

@phemmer Buffer just will grow uncontrollable inside docker :) and after #10347 it'll became more smart.

Contributor

LK4D4 commented Mar 18, 2015

@phemmer Buffer just will grow uncontrollable inside docker :) and after #10347 it'll became more smart.

@wlan0

This comment has been minimized.

Show comment
Hide comment
@wlan0

wlan0 Mar 18, 2015

Contributor

@LK4D4 I updated the code based on the comments and added documentation following the logging drivers PR.

Contributor

wlan0 commented Mar 18, 2015

@LK4D4 I updated the code based on the comments and added documentation following the logging drivers PR.

Show outdated Hide outdated daemon/config.go Outdated
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Mar 19, 2015

Contributor

Nice, moving to code review

Contributor

crosbymichael commented Mar 19, 2015

Nice, moving to code review

}
func (s *Syslog) Name() string {
return "Syslog"

This comment has been minimized.

@crosbymichael

crosbymichael Mar 19, 2015

Contributor

Should this be lower case?

@crosbymichael

crosbymichael Mar 19, 2015

Contributor

Should this be lower case?

This comment has been minimized.

@wlan0

wlan0 Mar 19, 2015

Contributor

The name of json-file driver is 'JSONFile', I saw that and used 'Syslog' here.

@wlan0

wlan0 Mar 19, 2015

Contributor

The name of json-file driver is 'JSONFile', I saw that and used 'Syslog' here.

This comment has been minimized.

@crosbymichael

crosbymichael Mar 20, 2015

Contributor

ok

@crosbymichael

crosbymichael Mar 20, 2015

Contributor

ok

Show outdated Hide outdated daemon/logger/syslog/syslog.go Outdated

@jessfraz jessfraz added dco/yes and removed dco/no labels Mar 20, 2015

add syslog driver
Signed-off-by: wlan0 <sid@rancher.com>
@ibuildthecloud

This comment has been minimized.

Show comment
Hide comment
@ibuildthecloud

ibuildthecloud Mar 22, 2015

Contributor

I pulled in the latest commit from @wlan0, I think all changes have been addressed at this time.

Contributor

ibuildthecloud commented Mar 22, 2015

I pulled in the latest commit from @wlan0, I think all changes have been addressed at this time.

@@ -83,7 +83,7 @@ func (config *Config) InstallFlags() {
opts.LabelListVar(&config.Labels, []string{"-label"}, "Set key=value labels to the daemon")
config.Ulimits = make(map[string]*ulimit.Ulimit)
opts.UlimitMapVar(config.Ulimits, []string{"-default-ulimit"}, "Set default ulimits for containers")
flag.StringVar(&config.LogConfig.Type, []string{"-log-driver"}, "json-file", "Containers logging driver(json-file/none)")
flag.StringVar(&config.LogConfig.Type, []string{"-log-driver"}, "json-file", "Containers logging driver")

This comment has been minimized.

@sirupsen

sirupsen Mar 22, 2015

Contributor

Nitpick: containers => container's

@sirupsen

sirupsen Mar 22, 2015

Contributor

Nitpick: containers => container's

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 23, 2015

Contributor

cool it works! I do really like how simple the implementaion is too

Contributor

jessfraz commented Mar 23, 2015

cool it works! I do really like how simple the implementaion is too

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Mar 23, 2015

Contributor

LGTM

Contributor

jessfraz commented Mar 23, 2015

LGTM

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 23, 2015

Contributor

LGTM

Contributor

LK4D4 commented Mar 23, 2015

LGTM

LK4D4 added a commit that referenced this pull request Mar 23, 2015

@LK4D4 LK4D4 merged commit 65e21f5 into moby:master Mar 23, 2015

2 checks passed

janky Jenkins build Docker-PRs 3926 has succeeded
Details
windows Jenkins build Windows-PRs 934 has succeeded
Details
@neurogenesis

This comment has been minimized.

Show comment
Hide comment
@neurogenesis

neurogenesis Mar 23, 2015

👍 thanks to all for the syslog functionality.

neurogenesis commented Mar 23, 2015

👍 thanks to all for the syslog functionality.

type Syslog struct {
writer *syslog.Writer
tag string
mu sync.Mutex

This comment has been minimized.

@noxiouz

noxiouz Mar 23, 2015

Contributor

Sorry, but this mutex seems not to be anywhere

@noxiouz

noxiouz Mar 23, 2015

Contributor

Sorry, but this mutex seems not to be anywhere

This comment has been minimized.

@LK4D4

LK4D4 Mar 23, 2015

Contributor

Yup, it's task for docker birthday :)
#11659

@LK4D4

LK4D4 Mar 23, 2015

Contributor

Yup, it's task for docker birthday :)
#11659

}
func (s *Syslog) Log(msg *logger.Message) error {
logMessage := fmt.Sprintf("%s: %s", s.tag, string(msg.Line))

This comment has been minimized.

@noxiouz

noxiouz Mar 23, 2015

Contributor

If msg.Line is []byte, you needn't cast it to string explicitly.

@noxiouz

noxiouz Mar 23, 2015

Contributor

If msg.Line is []byte, you needn't cast it to string explicitly.

This comment has been minimized.

@LK4D4

LK4D4 Mar 23, 2015

Contributor

@noxiouz Nice catch, you can create issue with label exp/beginner for this or fix it yourself.

@LK4D4

LK4D4 Mar 23, 2015

Contributor

@noxiouz Nice catch, you can create issue with label exp/beginner for this or fix it yourself.

@jberryman

This comment has been minimized.

Show comment
Hide comment
@jberryman

jberryman Apr 20, 2015

We'd like to be able to specify the socket to use instead of /dev/log, so that we can run a simple socket server to process logs. Is that easily done? Should I open another issue?

jberryman commented Apr 20, 2015

We'd like to be able to specify the socket to use instead of /dev/log, so that we can run a simple socket server to process logs. Is that easily done? Should I open another issue?

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Apr 20, 2015

Contributor

@jberryman Yup, is is easy to add it when --log-opts PR will be merged.

Contributor

LK4D4 commented Apr 20, 2015

@jberryman Yup, is is easy to add it when --log-opts PR will be merged.

@jberryman

This comment has been minimized.

Show comment
Hide comment
@jberryman

jberryman Apr 20, 2015

@LK4D4 Oh that would be great. I think this is the PR you're referring to: #12422

jberryman commented Apr 20, 2015

@LK4D4 Oh that would be great. I think this is the PR you're referring to: #12422

@grayhemp

This comment has been minimized.

Show comment
Hide comment
@grayhemp

grayhemp Jun 3, 2015

Some new ideas around it #13726. Could you guys please take a look?

grayhemp commented Jun 3, 2015

Some new ideas around it #13726. Could you guys please take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment