-
Notifications
You must be signed in to change notification settings - Fork 632
Increase default max-buffer-size to 10m for non-blocking containers #4524
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3039,6 +3039,110 @@ func TestCreateContainerAwslogsLogDriver(t *testing.T) { | |
|
||
} | ||
|
||
func TestCreateContainerLogDriverBufferSize(t *testing.T) { | ||
testCases := []struct { | ||
name string | ||
logMode string | ||
initialBufferSize string | ||
expectedBufferSize string | ||
shouldHaveBufferSize bool | ||
}{ | ||
{ | ||
name: "non-blocking mode without buffer size should set default", | ||
logMode: "non-blocking", | ||
initialBufferSize: "", | ||
expectedBufferSize: "10m", | ||
shouldHaveBufferSize: true, | ||
}, | ||
{ | ||
name: "non-blocking mode with existing buffer size should not change", | ||
logMode: "non-blocking", | ||
initialBufferSize: "8m", | ||
expectedBufferSize: "8m", | ||
shouldHaveBufferSize: true, | ||
}, | ||
{ | ||
name: "blocking mode should not set buffer size", | ||
logMode: "blocking", | ||
initialBufferSize: "", | ||
expectedBufferSize: "", | ||
shouldHaveBufferSize: false, | ||
}, | ||
{ | ||
name: "blocking mode with errant buffer size should not change", | ||
logMode: "blocking", | ||
initialBufferSize: "7m", | ||
expectedBufferSize: "7m", | ||
shouldHaveBufferSize: true, | ||
}, | ||
{ | ||
name: "no mode specified should not set buffer size", | ||
logMode: "", | ||
initialBufferSize: "", | ||
expectedBufferSize: "", | ||
shouldHaveBufferSize: false, | ||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
ctx, cancel := context.WithCancel(context.TODO()) | ||
defer cancel() | ||
ctrl, client, _, taskEngine, _, _, _, _ := mocks(t, ctx, &defaultConfig) | ||
defer ctrl.Finish() | ||
|
||
logConfig := map[string]string{} | ||
if tc.logMode != "" { | ||
logConfig["mode"] = tc.logMode | ||
} | ||
if tc.initialBufferSize != "" { | ||
logConfig["max-buffer-size"] = tc.initialBufferSize | ||
} | ||
|
||
rawHostConfigInput := dockercontainer.HostConfig{ | ||
LogConfig: dockercontainer.LogConfig{ | ||
Type: "awslogs", | ||
Config: logConfig, | ||
}, | ||
} | ||
rawHostConfig, err := json.Marshal(&rawHostConfigInput) | ||
require.NoError(t, err) | ||
|
||
testTask := &apitask.Task{ | ||
Arn: "arn:aws:ecs:region:account-id:task/test-task-arn", | ||
Containers: []*apicontainer.Container{ | ||
{ | ||
Name: "test-container", | ||
DockerConfig: apicontainer.DockerConfig{ | ||
HostConfig: func() *string { | ||
s := string(rawHostConfig) | ||
return &s | ||
}(), | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
client.EXPECT().APIVersion().Return(defaultDockerClientAPIVersion, nil).AnyTimes() | ||
client.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do( | ||
Comment on lines
+3126
to
+3127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: These mocks and all the additional setup won't be needed if we create a helper function that takes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
func(ctx context.Context, | ||
config *dockercontainer.Config, | ||
hostConfig *dockercontainer.HostConfig, | ||
name string, | ||
timeout time.Duration) { | ||
bufferSize, exists := hostConfig.LogConfig.Config["max-buffer-size"] | ||
assert.Equal(t, tc.shouldHaveBufferSize, exists) | ||
if tc.shouldHaveBufferSize { | ||
assert.Equal(t, tc.expectedBufferSize, bufferSize) | ||
} | ||
}) | ||
|
||
ret := taskEngine.(*DockerTaskEngine).createContainer(testTask, testTask.Containers[0]) | ||
assert.NoError(t, ret.Error) | ||
}) | ||
} | ||
} | ||
|
||
// TestCreateContainerAddFirelensLogDriverConfig tests that in createContainer, when the | ||
// container is using firelens log driver, its logConfig is properly set. | ||
func TestCreateContainerAddFirelensLogDriverConfig(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will be the buffer size for blocking mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blocking mode does not have a buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So every call is actually blocking? That's interesting. I'd be curious about its performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.