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

logger: copy to log driver's bufsize, fixes #34887 #34888

Merged
merged 1 commit into from Oct 30, 2017

Conversation

@jahkeup
Contributor

jahkeup commented Sep 18, 2017

Log drivers may have an internal buffer size that can be accommodated
by the copier as it is more effective to buffer and send fewer though
larger messages that the log driver can consume.

This eliminates the need for Partial handling for drivers that do not
support the concept (ie: awslogs, which can only have events up to
service limits).

- What I did

I added an interface to the logger package that allows drivers to specify a buffer size for the copier to use for buffering reads.

- How I did it

I added a single function interface that allows drivers to indicate what the size is that should be used, and called them SizedLogger's.

- How to verify it

go test includes a test case to verify that when this interface is implemented that the driver's will receive larger message buffers. See #34887 for use case and steps to reproduce.

- Description for the changelog

log drivers can now specify buffer size for buffering container log output

@samuelkarp

This comment has been minimized.

Show comment
Hide comment
@samuelkarp

samuelkarp Sep 18, 2017

Contributor

daemon/logger/logger.go:84:6⚠️ exported type SizedLogger should have comment or be unexported (golint)

Can you fix the golint failure?

Contributor

samuelkarp commented Sep 18, 2017

daemon/logger/logger.go:84:6⚠️ exported type SizedLogger should have comment or be unexported (golint)

Can you fix the golint failure?

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 20, 2017

I suggest to implement a more generic approach: #34913

More context in #34855, in short -- the ability to set the log size for all log drivers is important and is a superset of the functionality of this PR. Are you fine with that?

crassirostris commented Sep 20, 2017

I suggest to implement a more generic approach: #34913

More context in #34855, in short -- the ability to set the log size for all log drivers is important and is a superset of the functionality of this PR. Are you fine with that?

@samuelkarp

This comment has been minimized.

Show comment
Hide comment
@samuelkarp

samuelkarp Sep 20, 2017

Contributor

@crassirostris It doesn't make sense to have the max size be user-configurable for the awslogs driver. The max size here is driven by the CloudWatch Logs API and is not expected to change for different users.

Contributor

samuelkarp commented Sep 20, 2017

@crassirostris It doesn't make sense to have the max size be user-configurable for the awslogs driver. The max size here is driven by the CloudWatch Logs API and is not expected to change for different users.

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 20, 2017

@samuelkarp You can specify this limit together with specifying the logging driver type

But I see the argument. Can we make these two efforts work together then? E.g. user-defined limit > plugin-defined limit > default limit

Edit: main ask is to move the buffer size parameter to the field, initialized in the constructor.

crassirostris commented Sep 20, 2017

@samuelkarp You can specify this limit together with specifying the logging driver type

But I see the argument. Can we make these two efforts work together then? E.g. user-defined limit > plugin-defined limit > default limit

Edit: main ask is to move the buffer size parameter to the field, initialized in the constructor.

@samuelkarp

This comment has been minimized.

Show comment
Hide comment
@samuelkarp

samuelkarp Sep 20, 2017

Contributor

You can specify this limit together with specifying the logging driver type

Yes, but you shouldn't have to.

Can we make these two efforts work together then? E.g. user-defined limit > plugin-defined limit > default limit

I think that would make sense, though I'd argue that a user-defined limit > driver-defined limit should be an error.

Contributor

samuelkarp commented Sep 20, 2017

You can specify this limit together with specifying the logging driver type

Yes, but you shouldn't have to.

Can we make these two efforts work together then? E.g. user-defined limit > plugin-defined limit > default limit

I think that would make sense, though I'd argue that a user-defined limit > driver-defined limit should be an error.

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 20, 2017

I think that would make sense, though I'd argue that a user-defined limit > driver-defined limit should be an error.

Makes sense to me also

crassirostris commented Sep 20, 2017

I think that would make sense, though I'd argue that a user-defined limit > driver-defined limit should be an error.

Makes sense to me also

@cpuguy83

This seems reasonable. At least allows the log driver to dictate the buffer size since it'll be buffering anyway.

What about if people want to specify a custom buffer size?

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 27, 2017

What about if people want to specify a custom buffer size?

There are three options:

  1. Driver takes precedence over user-defined limit
  2. User-defined limit takes precedence over driver
  3. They're mutually exclusive

I think 3. is the most robust approach: it's not possible to break a driver by
setting a limit manually as in 1. and at the same time user will have a clear
expectation that their user-defined limit will not be actually applied as in 2.

crassirostris commented Sep 27, 2017

What about if people want to specify a custom buffer size?

There are three options:

  1. Driver takes precedence over user-defined limit
  2. User-defined limit takes precedence over driver
  3. They're mutually exclusive

I think 3. is the most robust approach: it's not possible to break a driver by
setting a limit manually as in 1. and at the same time user will have a clear
expectation that their user-defined limit will not be actually applied as in 2.

@jahkeup

This comment has been minimized.

Show comment
Hide comment
@jahkeup

jahkeup Sep 27, 2017

Contributor

@cpuguy83 thanks for taking a look.

What about if people want to specify a custom buffer size?

I'm not sold on the thought that end users should be able to specify the internal buffer size instead of configuring their driver. The internal buffer size should be based on the implementation and dependencies of the log drivers themselves. However, I would like to see more appropriate buffers sizes used for Messages and I believe this should be handled by the drivers themselves as need to allow drivers to decide how best (and when) to allow user defined size limits.

Option 3, where users may also specify a buffer size, would prohibit users from specifying a custom buffer size at all if SizedLogger is widely adopted. Instead, it makes more sense for drivers to handle this user defined value themselves and surface the user's value when appropriate (eg: json-file can pass on the user's custom buffer size when it returns the BufSize or return a driver defined default size). If the drivers choose to allow this then they can select a value that makes sense in their respective contexts. In the context of the awslogs driver, the buffer size doesn't make sense to be larger, or smaller for that matter - thus users should not be allowed to configure the value at all.

Contributor

jahkeup commented Sep 27, 2017

@cpuguy83 thanks for taking a look.

What about if people want to specify a custom buffer size?

I'm not sold on the thought that end users should be able to specify the internal buffer size instead of configuring their driver. The internal buffer size should be based on the implementation and dependencies of the log drivers themselves. However, I would like to see more appropriate buffers sizes used for Messages and I believe this should be handled by the drivers themselves as need to allow drivers to decide how best (and when) to allow user defined size limits.

Option 3, where users may also specify a buffer size, would prohibit users from specifying a custom buffer size at all if SizedLogger is widely adopted. Instead, it makes more sense for drivers to handle this user defined value themselves and surface the user's value when appropriate (eg: json-file can pass on the user's custom buffer size when it returns the BufSize or return a driver defined default size). If the drivers choose to allow this then they can select a value that makes sense in their respective contexts. In the context of the awslogs driver, the buffer size doesn't make sense to be larger, or smaller for that matter - thus users should not be allowed to configure the value at all.

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 27, 2017

@jahkeup

I'm not sold on the thought that end users should be able to specify the internal buffer size instead of configuring their driver.

Example: logging agent (e.g. fluentd) consuming json-file, journald or syslog. The actual destination of the logging agent may have different limits in different deployments with the same driver. While it's possible to setup in in a per-driver fashion, it results in duplication and is not logical: in aws case the driver itself dictates the max line length, but in the setup with a logging agent those requirements come from the outside of Docker ecosystem in general and from outside of log driver in particular.

If the drivers choose to allow this then they can select a value that makes sense in their respective contexts

Problem is, some drivers do not have this context and can work with any line. As I mentioned earlier, requirements come from the setup, not from the driver

users should not be allowed to configure the value at all

And that is why I agree that those options should be mutually exclusive

crassirostris commented Sep 27, 2017

@jahkeup

I'm not sold on the thought that end users should be able to specify the internal buffer size instead of configuring their driver.

Example: logging agent (e.g. fluentd) consuming json-file, journald or syslog. The actual destination of the logging agent may have different limits in different deployments with the same driver. While it's possible to setup in in a per-driver fashion, it results in duplication and is not logical: in aws case the driver itself dictates the max line length, but in the setup with a logging agent those requirements come from the outside of Docker ecosystem in general and from outside of log driver in particular.

If the drivers choose to allow this then they can select a value that makes sense in their respective contexts

Problem is, some drivers do not have this context and can work with any line. As I mentioned earlier, requirements come from the setup, not from the driver

users should not be allowed to configure the value at all

And that is why I agree that those options should be mutually exclusive

@jahkeup

This comment has been minimized.

Show comment
Hide comment
@jahkeup

jahkeup Sep 27, 2017

Contributor

@crassirostris , in a way we are speaking of the same goal - users should be able to influence their logging drivers and this ultimately will directly or indirectly configure this internal buffer. As for how that is handled: I don't think that users should be able to directly configure the internal buffer's size. I would prefer to see the --log-opt, the one you've proposed, handled by individual drivers once BufSize is adopted to enable validation and interpretation of the provided user value in the context of each driver rather than at a global level. If it remains on the global level we would have to err on the side of caution for every driver.

Option 3, enumerated above, is the most sensible in my opinion. However, the topic should be revisited if/when drivers begin to require different handling of user specified size. Until then, I agree that mutually exclusive user-provided-size/BufSize is sufficient to support the majority of the drivers today.

Contributor

jahkeup commented Sep 27, 2017

@crassirostris , in a way we are speaking of the same goal - users should be able to influence their logging drivers and this ultimately will directly or indirectly configure this internal buffer. As for how that is handled: I don't think that users should be able to directly configure the internal buffer's size. I would prefer to see the --log-opt, the one you've proposed, handled by individual drivers once BufSize is adopted to enable validation and interpretation of the provided user value in the context of each driver rather than at a global level. If it remains on the global level we would have to err on the side of caution for every driver.

Option 3, enumerated above, is the most sensible in my opinion. However, the topic should be revisited if/when drivers begin to require different handling of user specified size. Until then, I agree that mutually exclusive user-provided-size/BufSize is sufficient to support the majority of the drivers today.

@crassirostris

This comment has been minimized.

Show comment
Hide comment
@crassirostris

crassirostris Sep 28, 2017

@jahkeup

However, the topic should be revisited if/when drivers begin to require different handling of user specified size.

Fully agree

crassirostris commented Sep 28, 2017

@jahkeup

However, the topic should be revisited if/when drivers begin to require different handling of user specified size.

Fully agree

@cpuguy83

Design OK for me.
Would love to hear from another maintainer.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 4, 2017

Member

ping @vieux could you have a look?

Member

thaJeztah commented Oct 4, 2017

ping @vieux could you have a look?

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 12, 2017

Contributor

I personally prefer this approach to #34913

Contributor

mlaventure commented Oct 12, 2017

I personally prefer this approach to #34913

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 12, 2017

Contributor

Let's move this to code review.
Thanks!

Contributor

cpuguy83 commented Oct 12, 2017

Let's move this to code review.
Thanks!

Show outdated Hide outdated daemon/logger/copier_test.go Outdated
Show outdated Hide outdated daemon/logger/copier_test.go Outdated
Show outdated Hide outdated daemon/logger/copier_test.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 25, 2017

Member

@cpuguy83 PR was updated, PTAL

Member

thaJeztah commented Oct 25, 2017

@cpuguy83 PR was updated, PTAL

Show outdated Hide outdated daemon/logger/copier_test.go Outdated
for {
var msg Message
if err := dec.Decode(&msg); err != nil {
if err == io.EOF {

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 27, 2017

Contributor

If there's nothing in the buffer this will just EOF but could be that the implementation is broken.
Maybe a check to make sure we got the messages we expected.

@cpuguy83

cpuguy83 Oct 27, 2017

Contributor

If there's nothing in the buffer this will just EOF but could be that the implementation is broken.
Maybe a check to make sure we got the messages we expected.

This comment has been minimized.

@jahkeup

jahkeup Oct 27, 2017

Contributor

Agreed, I've added a check for the number of messages that are expected and received.

@jahkeup

jahkeup Oct 27, 2017

Contributor

Agreed, I've added a check for the number of messages that are expected and received.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 30, 2017

Member

@jahkeup looks like CI (golint) is failing;

21:58:12 daemon/logger/copier_test.go:254:3:warning: should replace recvdMsgs += 1 with recvdMsgs++ (golint)
21:58:12 Build step 'Execute shell' marked build as failure
Member

thaJeztah commented Oct 30, 2017

@jahkeup looks like CI (golint) is failing;

21:58:12 daemon/logger/copier_test.go:254:3:warning: should replace recvdMsgs += 1 with recvdMsgs++ (golint)
21:58:12 Build step 'Execute shell' marked build as failure
Show outdated Hide outdated daemon/logger/copier.go Outdated
@cpuguy83

cpuguy83 approved these changes Oct 30, 2017 edited

LGTM with lint fixes (assuming CI passes)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 30, 2017

Member

looks like #34888 (comment) needs a minor update to make CI pass; @jahkeup could you update that? I think plans are to cut off 17.11-rc1 today

Member

thaJeztah commented Oct 30, 2017

looks like #34888 (comment) needs a minor update to make CI pass; @jahkeup could you update that? I think plans are to cut off 17.11-rc1 today

@jahkeup

This comment has been minimized.

Show comment
Hide comment
@jahkeup

jahkeup Oct 30, 2017

Contributor

@theJeztah I'll get changes made and pushed ASAP.

Contributor

jahkeup commented Oct 30, 2017

@theJeztah I'll get changes made and pushed ASAP.

logger: copy to log driver's bufsize
Log drivers may have an internal buffer size that can be accommodated
by the copier as it is more effective to buffer and send fewer though
larger messages that the log driver can consume.

This eliminates the need for Partial handling for drivers that do not
support the concept (ie: awslogs, which can only have events up to
service limits).

Signed-off-by: Jacob Vallejo <jakeev@amazon.com>
@thaJeztah

LGTM if green, thanks!

@yongtang yongtang merged commit dfc2d62 into moby:master Oct 30, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37567 has succeeded
Details
janky Jenkins build Docker-PRs 46264 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6672 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17841 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6469 has succeeded
Details
@jahkeup

This comment has been minimized.

Show comment
Hide comment
@jahkeup

jahkeup Oct 30, 2017

Contributor

Hooray, thanks all!

Contributor

jahkeup commented Oct 30, 2017

Hooray, thanks all!

@bitsofinfo

This comment has been minimized.

Show comment
Hide comment
@bitsofinfo

bitsofinfo Mar 25, 2018

Is this available now in any docker release?

bitsofinfo commented Mar 25, 2018

Is this available now in any docker release?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 25, 2018

Member

This was merged 5 months ago, so likely in docker 17.10, or 17.11 and up

Member

thaJeztah commented Mar 25, 2018

This was merged 5 months ago, so likely in docker 17.10, or 17.11 and up

@bitsofinfo

This comment has been minimized.

Show comment
Hide comment
@bitsofinfo

bitsofinfo Apr 3, 2018

@thaJeztah @crassirostris @cpuguy83

So we are using json-file but still having this issue

I'm a bit lost as to where to go to get some help w/ this 16k issue and how it affects the json-file driver or where to fix the problem or what is the status of this with regards to a roadmap for support on this being added?

We are ingesting (and failing) ModSecurity audit logs from many containers which are often > 16k

elastic/beats#6605

bitsofinfo commented Apr 3, 2018

@thaJeztah @crassirostris @cpuguy83

So we are using json-file but still having this issue

I'm a bit lost as to where to go to get some help w/ this 16k issue and how it affects the json-file driver or where to fix the problem or what is the status of this with regards to a roadmap for support on this being added?

We are ingesting (and failing) ModSecurity audit logs from many containers which are often > 16k

elastic/beats#6605

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Apr 3, 2018

Member

@bitsofinfo first of all, note that the files generated by the json-file logging driver are not intended for external consumption. Having said the above; would it help if the json-file logging driver would set the "Partial" field in the generated files? Also see #35831

If that would be a solution for your use-case, perhaps you could open a separate issue for that

Member

thaJeztah commented Apr 3, 2018

@bitsofinfo first of all, note that the files generated by the json-file logging driver are not intended for external consumption. Having said the above; would it help if the json-file logging driver would set the "Partial" field in the generated files? Also see #35831

If that would be a solution for your use-case, perhaps you could open a separate issue for that

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Apr 4, 2018

@thaJeztah New issue is here #36777 for further discussion. See my comment there.

ruflin commented Apr 4, 2018

@thaJeztah New issue is here #36777 for further discussion. See my comment there.

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