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

Implement plugins for logging drivers #28403

Merged
merged 1 commit into from Apr 10, 2017

Conversation

@cpuguy83
Contributor

cpuguy83 commented Nov 14, 2016

This is intended as PoC code that as of yet untested other than ensuring compilation.

Logging plugins use the same HTTP interface as other plugins for basic
command operations meanwhile actual logging operations are handled (on
Unix) via a fifo. I believe this is much cleaner than dealing with go1.8 style plugins.

The plugin interface looks like so:

type loggingPlugin interface {
  StartLogging(fifoPath string, loggingContext Context) error
  StopLogging(fifoPath)
}

This means a plugin must implement LoggingDriver.StartLogging and
LoggingDriver.StopLogging endpoints and be able to consume the passed
in fifo.

Logs are sent via stream encoder to the fifo encoded with json.
Ideally this would be protobuf, but I did not implement this yet.

ping @tiborvass @anusha-ragunathan

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 15, 2016

Contributor

The other alternative here would be to, instead of using fifo's, either hijack the HTTP stream (getting rid of the HTTP overhead), or potentially force an upgrade to HTTP2 (which is likely simpler for implementers than hijack).

Contributor

cpuguy83 commented Nov 15, 2016

The other alternative here would be to, instead of using fifo's, either hijack the HTTP stream (getting rid of the HTTP overhead), or potentially force an upgrade to HTTP2 (which is likely simpler for implementers than hijack).

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Nov 17, 2016

Member

Just to note for those looking for something in the interim, Docker does currently support GELF (which is a standardized protocol for logging), so anything which can consume that (and/or translate that, like logstash input/output plugins) can be connected currently that way while a proper plugin interface is designed/developed. 👍

Member

tianon commented Nov 17, 2016

Just to note for those looking for something in the interim, Docker does currently support GELF (which is a standardized protocol for logging), so anything which can consume that (and/or translate that, like logstash input/output plugins) can be connected currently that way while a proper plugin interface is designed/developed. 👍

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Nov 21, 2016

Member

Thank you for working on this 👍

Some questions about FIFO, from my previous similar PR (#23198)

  • What happens if the plugin doesn't read(2) from the FIFO? Probably the container gets stuck because its write(2) to stdout blocks?
  • What happens if the plugin crashes?
Member

AkihiroSuda commented Nov 21, 2016

Thank you for working on this 👍

Some questions about FIFO, from my previous similar PR (#23198)

  • What happens if the plugin doesn't read(2) from the FIFO? Probably the container gets stuck because its write(2) to stdout blocks?
  • What happens if the plugin crashes?
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 22, 2016

Contributor

What happens if the plugin doesn't read(2) from the FIFO? Probably the container gets stuck because its write(2) to stdout blocks?

Correct, write will be blocked, which is the behavior for all existing drivers except UDP based ones.

What happens if the plugin crashes?

writes will be blocked until the plugin comes back

Contributor

cpuguy83 commented Nov 22, 2016

What happens if the plugin doesn't read(2) from the FIFO? Probably the container gets stuck because its write(2) to stdout blocks?

Correct, write will be blocked, which is the behavior for all existing drivers except UDP based ones.

What happens if the plugin crashes?

writes will be blocked until the plugin comes back

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 23, 2016

Contributor

@AkihiroSuda For non-blocking behavior, I implemented a ring buffer that can be used with any driver in #28762

Contributor

cpuguy83 commented Nov 23, 2016

@AkihiroSuda For non-blocking behavior, I implemented a ring buffer that can be used with any driver in #28762

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 14, 2016

Contributor

Since we are already going to be communicating over a unix socket, I'm going to ditch the fifo. These offer similar performance, but with the unix socket I don't have to setup anything else.

Contributor

cpuguy83 commented Dec 14, 2016

Since we are already going to be communicating over a unix socket, I'm going to ditch the fifo. These offer similar performance, but with the unix socket I don't have to setup anything else.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 2, 2017

Contributor

Just an update on this:

  • writing logs works
  • Kept usage of fifo since it is actually much simpler than dealing with the http connection, can support named pipes on Windows later when there is platform support for this
  • Reading logs is supported if the log driver reports that it supports reading

To think about (need input from others):

  1. Should we just use a single stream for all log messages (from all containers) to the log driver? This would require encoding the log context with each message (easy enough). This would likely simplify plugin implementations, but make the docker side a bit more complex.
  2. Do we need some cleanup call for log drivers? I'm thinking likely not but throwing it out there. As is I have a plugin which wraps the jsonfile driver which would need a way cleanup logs for removed containers... but I think this is just because of the nature of the jsonfile logger, a real log driver wouldn't just remove logs in this manner.

Here's my plugin implementation: https://github.com/cpuguy83/docker-log-driver-test
I do have a (simple) integration test that uses this driver to both write and read back container logs.

Contributor

cpuguy83 commented Jan 2, 2017

Just an update on this:

  • writing logs works
  • Kept usage of fifo since it is actually much simpler than dealing with the http connection, can support named pipes on Windows later when there is platform support for this
  • Reading logs is supported if the log driver reports that it supports reading

To think about (need input from others):

  1. Should we just use a single stream for all log messages (from all containers) to the log driver? This would require encoding the log context with each message (easy enough). This would likely simplify plugin implementations, but make the docker side a bit more complex.
  2. Do we need some cleanup call for log drivers? I'm thinking likely not but throwing it out there. As is I have a plugin which wraps the jsonfile driver which would need a way cleanup logs for removed containers... but I think this is just because of the nature of the jsonfile logger, a real log driver wouldn't just remove logs in this manner.

Here's my plugin implementation: https://github.com/cpuguy83/docker-log-driver-test
I do have a (simple) integration test that uses this driver to both write and read back container logs.

@cpuguy83 cpuguy83 changed the title from [WIP] Implement plugins for logging drivers to Implement plugins for logging drivers Jan 3, 2017

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 26, 2017

Contributor

I'm ok with design
ping @docker/core-maintainers for moving to code-review.

Contributor

LK4D4 commented Jan 26, 2017

I'm ok with design
ping @docker/core-maintainers for moving to code-review.

Show outdated Hide outdated daemon/logger/adapter.go Outdated
Show outdated Hide outdated daemon/logger/plugin.go Outdated
**Request**:
```json
{
"File": "/path/to/file/stream",

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

Might be worth mentioning that the pipe is a 64bit random filename always created in the plugin's rootfs under /run/docker/logging.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

Might be worth mentioning that the pipe is a 64bit random filename always created in the plugin's rootfs under /run/docker/logging.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

I really consider this to be an implementation detail, especially for Linux. On windows it would most likely be a named pipe.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

I really consider this to be an implementation detail, especially for Linux. On windows it would most likely be a named pipe.

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

It's non-obvious how the fifo path is visible to the plugin, until looking at the code. Something that plugin authors might wonder.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

It's non-obvious how the fifo path is visible to the plugin, until looking at the code. Something that plugin authors might wonder.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

👍 Ok makes sense.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

👍 Ok makes sense.

Config ReadConfig
}
func (pp *logPluginProxy) ReadLogs(info Info, config ReadConfig) (stream io.ReadCloser, err error) {

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

dont need named returns here.

@anusha-ragunathan

anusha-ragunathan Apr 8, 2017

Contributor

dont need named returns here.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

This is generated code.

@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

This is generated code.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 8, 2017

Contributor

This should be good to go.

Contributor

cpuguy83 commented Apr 8, 2017

This should be good to go.

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

docs/extend/config.md has the list of drivers currently supported. It needs to be updated.

Contributor

anusha-ragunathan commented Apr 10, 2017

docs/extend/config.md has the list of drivers currently supported. It needs to be updated.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Added.

Contributor

cpuguy83 commented Apr 10, 2017

Added.

}
func (d *logEntryDecoder) Decode(l *LogEntry) error {
_, err := io.ReadFull(d.r, d.lenBuf)

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

Is this call to io.ReadFull necessary, given the real read at L 83?

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

Is this call to io.ReadFull necessary, given the real read at L 83?

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Yes, this gets the size of the message.
83 gets the actual message, which we need the size of to know where it ends.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Yes, this gets the size of the message.
83 gets the actual message, which we need the size of to know where it ends.

if total > len(e.buf) {
e.buf = make([]byte, total)
}
binary.BigEndian.PutUint32(e.buf, uint32(n))

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

shouldnt uint32(n) -> uint32(total) ?

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

shouldnt uint32(n) -> uint32(total) ?

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Here we are putting writing the size of the log entry to the buffer.
But the buffer needs to be the total size otherwise we'll be 4-bytes short.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Here we are putting writing the size of the log entry to the buffer.
But the buffer needs to be the total size otherwise we'll be 4-bytes short.

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

Thanks, I just looked at the logEntry's Size() implementation. Looks good.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

Thanks, I just looked at the logEntry's Size() implementation. Looks good.

if m.TimeNano != 0 {
dAtA[i] = 0x10
i++
i = encodeVarintEntry(dAtA, i, uint64(m.TimeNano))

This comment has been minimized.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

TimeNano is not copied into the dAtA marshal output. Looks incorrect.

@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

TimeNano is not copied into the dAtA marshal output. Looks incorrect.

This comment has been minimized.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

I think this is an protobuf optimization since TimeNano is an int64 already there is nothing extra to copy.

@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

I think this is an protobuf optimization since TimeNano is an int64 already there is nothing extra to copy.

Implement plugins for logging drivers
Logging plugins use the same HTTP interface as other plugins for basic
command operations meanwhile actual logging operations are handled (on
Unix) via a fifo.

The plugin interface looks like so:

```go
type loggingPlugin interface {
  StartLogging(fifoPath string, loggingContext Context) error
  StopLogging(fifoPath)
```

This means a plugin must implement `LoggingDriver.StartLogging` and
`LoggingDriver.StopLogging` endpoints and be able to consume the passed
in fifo.

Logs are sent via stream encoder to the fifo encoded with protobuf.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

@cpuguy83: I'm okay with a follow up PR to address

Contributor

anusha-ragunathan commented Apr 10, 2017

@cpuguy83: I'm okay with a follow up PR to address

@anusha-ragunathan

This comment has been minimized.

Show comment
Hide comment
@anusha-ragunathan

anusha-ragunathan Apr 10, 2017

Contributor

LGTM

Contributor

anusha-ragunathan commented Apr 10, 2017

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 10, 2017

Contributor

Cool, going to move to docs-review then, thanks!

Contributor

cpuguy83 commented Apr 10, 2017

Cool, going to move to docs-review then, thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 10, 2017

Member

Discussed with @cpuguy83 and we'll do a follow up for docs due to code-freeze today

Member

thaJeztah commented Apr 10, 2017

Discussed with @cpuguy83 and we'll do a follow up for docs due to code-freeze today

@thaJeztah thaJeztah merged commit 28334c1 into moby:master Apr 10, 2017

7 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32780 has succeeded
Details
janky Jenkins build Docker-PRs 41390 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 1578 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 3145 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 12517 has succeeded
Details
z Jenkins build Docker-PRs-s390x 1409 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 17.05.0 milestone Apr 10, 2017

@cpuguy83 cpuguy83 deleted the cpuguy83:logging_plugins branch Apr 10, 2017

@cpuguy83 cpuguy83 referenced this pull request Apr 10, 2017

Closed

Logging driver plugins #18604

@subfuzion subfuzion referenced this pull request Aug 7, 2017

Open

logs #7

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