Skip to content
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

Support buffer limit option in FireLens #2958

Merged
merged 1 commit into from
Jul 27, 2021

Conversation

zhonghui12
Copy link
Contributor

@zhonghui12 zhonghui12 commented Jul 20, 2021

Summary

Provide an entry to configurefluentd-buffer-limit in FireLens.
This is being enabled to resolve the following request: aws/containers-roadmap#964

Implementation details

Help to configure buffer limit size in fluentD Log Driver

Testing

  • Updated two unit tests and manually tested the feature with make test
  • The code also passed the end-to-end test on my instance.
    Here is my task definition file:
"logConfiguration": {
        "logDriver": "awsfirelens",
        "secretOptions": null,
        "options": {
          "log_group_name": "/aws/ecs/containerinsights/$(ecs_cluster)/application",
          "log-driver-buffer-limit": "123456",
          "auto_create_group": "true",
          "log_key": "log",
          "log_stream_prefix": "log_stream_name",
          "region": "us-west-2",
          "Name": "cloudwatch"
        }

Result for docker inspect:

"HostConfig": {
            "Binds": [],
            "ContainerIDFile": "",
            "LogConfig": {
                "Type": "fluentd",
                "Config": {
                    "fluentd-address": "unix:///var/lib/ecs/data/firelens/677a7f0ded6c47709e26665f714e4c76/socket/fluent.sock",
                    "fluentd-async-connect": "true",
                    "fluentd-buffer-limit": "123456",
                    "fluentd-sub-second-precision": "true",
                    "tag": "app-firelens-677a7f0ded6c47709e26665f714e4c76"
                }
            },

Also, tested it with no log-driver-buffer-limit option:

"HostConfig": {
            "Binds": [],
            "ContainerIDFile": "",
            "LogConfig": {
                "Type": "fluentd",
                "Config": {
                    "fluentd-address": "unix:///var/lib/ecs/data/firelens/65b06118b2014ccd9ef92abf5727577d/socket/fluent.sock",
                    "fluentd-async-connect": "true",
                    "fluentd-sub-second-precision": "true",
                    "tag": "app-firelens-65b06118b2014ccd9ef92abf5727577d" 
                }
            },

Description for the changelog

Feature FluentD: Support for buffer limit size configuration.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -1285,6 +1286,7 @@ func getFirelensLogConfig(task *apitask.Task, container *apicontainer.Container,
logConfig.Config[logDriverFluentdAddress] = fluentd
logConfig.Config[logDriverAsyncConnect] = strconv.FormatBool(true)
logConfig.Config[logDriverSubSecondPrecision] = strconv.FormatBool(true)
logConfig.Config[logDriverBufferLimit] = apitask.LogDriverBufferLimit
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused, this suggested its a field on the task structure, but then in the other file I don't see it added as a field in that struct...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a field on the task structure, it is a field in Config. And we need to get the value from the task package

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, but our requirement is that each container can have its own log driver buffer setting. So I think it needs to be stored somewhere which is both per-task and per-container. Not some global setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this needs to be saved anywhere. instead just do the following should work: (1) in collectFirelensLogOptions, skip adding the option to containerToLogOptions if the key equals firelensLogDriverBufferLimitOption; (2) here, get the actual value for "fluentd-buffer-limit" from the container's log config before we reset it at the previous line logConfig.Config = make(map[string]string), and then save it to the config here.

@zhonghui12
Copy link
Contributor Author

@PettitWesley @fenxiong updated the code and already tested it in my local by make test

logConfig.Type = logDriverTypeFluentd
logConfig.Config = make(map[string]string)
logConfig.Config[logDriverTag] = tag
logConfig.Config[logDriverFluentdAddress] = fluentd
logConfig.Config[logDriverAsyncConnect] = strconv.FormatBool(true)
logConfig.Config[logDriverSubSecondPrecision] = strconv.FormatBool(true)
logConfig.Config[logDriverBufferLimit] = bufferLimit
Copy link
Contributor

Choose a reason for hiding this comment

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

this should only be added when bufferLimit is not empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see here has a check for empty string so I didn't add it: https://github.com/moby/moby/blob/master/daemon/logger/fluentd/fluentd.go#L174. But yeah, we should handle this case here. Thanks for the suggestion.

agent/api/task/task.go Outdated Show resolved Hide resolved
@zhonghui12
Copy link
Contributor Author

Update the PR based on the comments. Will squash all commits into one if PR is approved

@fenxiong fenxiong changed the base branch from master to dev July 21, 2021 22:38
@fenxiong
Copy link
Contributor

static check is failing for other reason and has been fixed in 997c975. please rebase on latest dev branch to get the tests passed

@fenxiong fenxiong added this to the 1.55.0 milestone Jul 22, 2021
@fenxiong fenxiong requested a review from a team July 22, 2021 23:14
@fenxiong fenxiong removed this from the 1.55.0 milestone Jul 23, 2021
@chienhanlin chienhanlin merged commit 0df0593 into aws:dev Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants