Fluentd logger fluent-buffer-limit option semantic change in 19.03

[IRCody]

Description

When the fluent-logger-golang version was updated to v1.4.0 here I believe a semantic change was inadvertently introduced which changes the meaning of the fluent-buffer-limit option.

As part of the v1.40 changes fluent-logger-golang the BufferLimit option was changed to be used as the buffer limit for a channel instead of bytes here.

This results in allocation of 8x the memory usage on startup when this option is set along with the async option due to allocating pointers instead of bytes.

Steps to reproduce the issue:

1. Have dockerd 19.03
2. Run a command like below. The container image used doesn't matter.

docker run --rm  --log-driver=fluentd --log-opt fluentd-async-connect=true --log-opt fluentd-buffer-limit=1g alpine /bin/sh

It's easier to see the effect with larger buffer sizes but the behavior is the same regardless of buffer size afaict.

Describe the results you received:
See that memory usage has increased by ~8g instead of 1g (or if this puts you oom it will crash dockerd).

Describe the results you expected:
I am not 100% sure here. The semantics changed but the previous usage was under-documented. The documentation here says for the fluentd-buffer-limit option:

The amount of data to buffer before flushing to disk. Defaults to the amount of RAM available to the container.

It's not explicity stated that it is meant to be in bytes but the function here definitely makes it seem like that was the intent. The default also appears to be 1MB and not the amount of ram available in the container.

Additional Question:
I also had a question about the expected memory usage of this option. Is it expected to remain used even after the container exits?

The behavior I am seeing here is that if I run this command once, the memory usage will increase by ~8g. It does not go down when the container exits.

docker run --rm  --log-driver=fluentd --log-opt fluentd-async-connect=true --log-opt fluentd-buffer-limit=1g alpine /bin/sh

If I run this a second time, because my machine has ~16g of memory it will crash (presumably because it tries to allocate an additional 8g. Is this expected?

Output of docker version:

Docker version 19.03.6-ce, build 369ce74

Output of docker info:

Client:
 Debug Mode: false

Server:
 Containers: 18
  Running: 0
  Paused: 0
  Stopped: 18
 Images: 13
 Server Version: 19.03.6-ce
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: ff48f57fc83a8c44cf4ad5d672424a98ba37ded6
 runc version: dc9208a3303feef5b3839f4323d9beb36df0a9dd
 init version: fec3683
 Security Options:
  seccomp
   Profile: default
 Kernel Version: 4.14.173-137.229.amzn2.x86_64
 Operating System: Amazon Linux 2
 OSType: linux
 Architecture: x86_64
 CPUs: 4
 Total Memory: 15.66GiB
 Name: <>
 ID: <>
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false

Additional environment details (AWS, VirtualBox, physical, etc.):

My testing was done on an m4.xlarge on AWS with Amazon linux 2.

[thaJeztah]

Have you also reported this upstream in the https://github.com/fluent/fluent-logger-golang/ issue tracker? (looks like the change you mentioned was part of fluent/fluent-logger-golang#63)

@tagomoris PTAL

[IRCody]

@thaJeztah: I hadn't but I just created fluent/fluent-logger-golang#88.

[IRCody]

@thaJeztah: In the linked issue on fluent-logger-golang I think we found the root commit where this was changed. I'd be interested in what you think the best update here is. I think changing it is a little tricky because changing the fluentd-buffer-limit to match the current meaning from fluent-logger-golang (which seems like the right move) is not a backwards compatible change since people would no longer be able to pass values like 1M. We could leave that I suppose but it's unintuitive.

[Kern--]

Hi, I'd like to re-raise this issue. I cut a new issue to fluent-logger-golang (fluent/fluent-logger-golang#128) to try to get a new option to limit the number of bytes buffered without changing the current semantics again.