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

Set firelens mem_buf_limit by default #4405

Merged
merged 2 commits into from
Nov 14, 2024
Merged

Conversation

yinyic
Copy link
Contributor

@yinyic yinyic commented Oct 22, 2024

Summary

awsfirelens logging driver uses a Fluentbit-based image. Fluentbit supports mem_buf_limit configuration parameter which controls the amount of buffer memory that can be used by a log input. This prevents excessive memory usage of the buffer which may lead to container and task OOM kill. (see https://docs.fluentbit.io/manual/administration/backpressure)

mem_buf_limit is not enabled by Fluentbit by default, making buffer unbounded. In this PR we let ECS Agent sets a default mem_buf_limit when generating Fluentbit configuration.

Implementation details

The value of the mem_buf_limit is determined as follows:

  1. if firelens container memoryReservation is specified, mem_buf_limit = memoryReservation / 2. We resolve memoryReservation from container host config property (a json blob).
  2. else if firelens container memory is specified, mem_buf_limit = memory / 2. We resolve memory from container property directly.
    (memoryReservation and memory are optional parameters indicating container soft and hard memory limit respectively. The goal here is to scale the mem_buf_limit in accordance with the memory allocation for the firelens container. The scaling factor is derived from this recommendation)
  3. if neither memoryReservation nor memory is specified, we set mem_buf_limit to a fixed amount of 25MB (rationale)

Main changes are:

  • Updated NewFirelensResource method to take in a new containerMemoryLimit argument, and updated caller to pass in this argument after resolving the value from container memory and memoryReservation
  • Added container helper method GetMemoryReservationFromHostConfig to retrieve container memoryReservation
  • Updated firelens generateConfig method to write Mem_Buf_Limit config for the default input, after resolving the value from resolveMemBufLimit helper method

Testing

New tests cover the changes: yes

Description for the changelog

Set firelens mem_buf_limit by default

Additional Information

Does this PR include breaking model changes? If so, Have you added transformation functions?

N/A

Does this PR include the addition of new environment variables in the README?

N/A

Licensing

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

@yinyic yinyic requested a review from a team as a code owner October 22, 2024 17:38
@yinyic yinyic force-pushed the firelens-mem-buf-limit branch 10 times, most recently from f9662dc to dc92608 Compare October 23, 2024 21:45
@yinyic yinyic changed the title [WIP - DO NOT REVIEW] set firelens mem_buf_limit [Draft] set firelens mem_buf_limit Oct 23, 2024
@yinyic yinyic changed the title [Draft] set firelens mem_buf_limit Set firelens mem_buf_limit by default Oct 24, 2024
@yinyic yinyic changed the title Set firelens mem_buf_limit by default Set firelens mem_buf_limit by default Oct 24, 2024
@yinyic yinyic changed the title Set firelens mem_buf_limit by default Set firelens mem_buf_limit by default Oct 24, 2024
@yinyic yinyic force-pushed the firelens-mem-buf-limit branch from dc92608 to 82b864b Compare October 24, 2024 16:29
@yinyic yinyic assigned yinyic and unassigned yinyic Oct 24, 2024
@sparrc sparrc force-pushed the firelens-mem-buf-limit branch from 82b864b to db454d3 Compare November 13, 2024 18:16
@sparrc sparrc merged commit 4843cd0 into aws:dev Nov 14, 2024
40 checks passed
JoseVillalta pushed a commit to JoseVillalta/amazon-ecs-agent that referenced this pull request Nov 21, 2024
Co-authored-by: Cameron Sparr <sparrc@users.noreply.github.com>
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.

4 participants