Increase default max-buffer-size to 10m for non-blocking containers #4524
Conversation
| // unset, then default to 10m. | ||
| if mode, ok := hostConfig.LogConfig.Config[logDriverMode]; ok { | ||
| _, hasBufferSize := hostConfig.LogConfig.Config[logDriverBufferSize] | ||
| if mode == string(dockercontainer.LogModeNonBlock) && !hasBufferSize { |
There was a problem hiding this comment.
What will be the buffer size for blocking mode?
There was a problem hiding this comment.
In other words, what does the logging driver use as buffer size if no buffer size is provided?
There was a problem hiding this comment.
blocking mode does not have a buffer
There was a problem hiding this comment.
So every call is actually blocking? That's interesting. I'd be curious about its performance.
There was a problem hiding this comment.
yes, if the log driver has issues sending logs to the destination, the application faces backpressure: https://aws.amazon.com/blogs/containers/choosing-container-logging-options-to-avoid-backpressure/
There was a problem hiding this comment.
There's no configuration parameter for "blocking" mode, but I believe the log driver uses a buffered channel of a few hundred items, where each item is a separate log line.
agent/engine/docker_task_engine.go
Outdated
| @@ -1924,6 +1927,15 @@ func (engine *DockerTaskEngine) createContainer(task *apitask.Task, container *a | |||
| } | |||
| } | |||
|
|
|||
| // If the container has mode=non-blocking set, and the max-buffer-size field is | |||
| // unset, then default to 10m. | |||
There was a problem hiding this comment.
nit: maybe we should not say 10m here so that the possibility of someone changing the constant in the future and forgetting to update this comment is removed. We can add a comment for the constant definition?
| client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil).AnyTimes() | ||
| client.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( |
There was a problem hiding this comment.
nit: These mocks and all the additional setup won't be needed if we create a helper function that takes hostConfig as a parameter and sets the buffer size if needed and just unit test that function.
There was a problem hiding this comment.
True that would be cleaner...the amount of special cases already handled in here would make it quite a large refactor though so I'll have to save it for another day
sparrc
changed the title
Summary
This changes the default max-buffer-size setting in the LogConfiguration struct of the task definition from 1m to 10m.
This change brings the ECS agent default value closer to our recommended value of 25m. ECS recommends 25m in docs and blog posts.
While 10m is obviously lower than the recommended value of 25m, it still largely mitigates the risk of log loss due to high through-put (see blog post linked earlier for benchmarking details). 10m was chosen to avoid too drastic of a change in the default value and memory usage while still mitigating most risk of log loss due to high through-put with non-blocking mode.
Testing
New tests cover the changes: yes
Manual tests were run to verify that blocking containers do not set this value, non-blocking containers with a max-buffer-size in the task def do not set this value, and non-blocking containers without max-buffer-size in the task def do have the value set to 10m.
Description for the changelog
Enhancement: Increase LogConfig default max-buffer-size from 1m to 10m.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.