Set CPU/Memory in both config and hostconfig #1069
Conversation
richardpen
requested review from
jamsajones,
nmeyerhans,
samuelkarp,
aaithal and
tehshwann
agent/api/task.go
Outdated
| @@ -464,6 +464,25 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli | |||
| return config, nil | |||
| } | |||
|
|
|||
| // SetConfigHostConfigCommonFileds sets the same fields in both Config and HostConfig for backward compatibility | |||
| func (task *Task) SetConfigHostConfigCommonFileds(container *Container, config *docker.Config, hc *docker.HostConfig) { | |||
There was a problem hiding this comment.
nit: typo s/SetConfigHostConfigCommonFileds/SetConfigHostConfigCommonFields
agent/api/task.go
Outdated
| // SetConfigHostConfigCommonFileds sets the same fields in both Config and HostConfig for backward compatibility | ||
| func (task *Task) SetConfigHostConfigCommonFileds(container *Container, config *docker.Config, hc *docker.HostConfig) { | ||
| // Convert MB to B | ||
| dockerMem := int64(container.Memory * 1024 * 1024) |
There was a problem hiding this comment.
I think we have a const (bytesPerMegabyte) for this multiplier in task_unix.go.
Could be moved and reused if necessary.
There was a problem hiding this comment.
Have you tested this and verified that the memory limit is now properly enforced on Windows?
Also: it seems weird to just blindly set memory in both places. It might make more sense to trigger this behavior in an API version adapter layer so that we move the field to the right place when we're using an API version that requires it.
CHANGELOG.md
Outdated
| @@ -5,6 +5,7 @@ | |||
| * Feature - Enable support for task level CPU and memory constraints. | |||
| * Bug - Fixed a bug where a task can be blocked in creating state. [#1048](https://github.com/aws/amazon-ecs-agent/pull/1048) | |||
| * Bug - Fixed dynamic HostPort in container metadata. [#1052](https://github.com/aws/amazon-ecs-agent/pull/1052) | |||
| * Bug - Fixed bug on windows where container memory limits isn't enforced. [#1069](https://github.com/aws/amazon-ecs-agent/pull/1069) | |||
There was a problem hiding this comment.
s/windows/Windows/
s/isn't/are not/
| if dockerMem != 0 && dockerMem < DockerContainerMinimumMemoryInBytes { | ||
| dockerMem = DockerContainerMinimumMemoryInBytes | ||
| } | ||
| cpuShare := task.dockerCPUShares(container.CPU) |
There was a problem hiding this comment.
Do we really want to set CPU in both?
There was a problem hiding this comment.
Based on this commit, it looks like the memory and cpu are both moved to HostConfig, I will modify this only be set when the api version is <1.18
richardpen
force-pushed
the
windows
branch
7 times, most recently
from
f519d40
to
d6511e5
Compare
agent/api/task_windows.go
Outdated
| @@ -28,6 +29,8 @@ const ( | |||
| memorySwappinessDefault = -1 | |||
| ) | |||
|
|
|||
| var cpus = runtime.NumCPU() * 1024 | |||
There was a problem hiding this comment.
How about "cpuShareScaleFactor"?
agent/api/task.go
Outdated
| return err | ||
| } | ||
|
|
||
| dockerAPIVersion_1_18, err := docker.NewAPIVersion("1.18") |
There was a problem hiding this comment.
APIVersion is just []int. I think you could do this:
var dockerAPIVersion_1_18 := APIVersion([]int{1,18})That gets rid of your err.
agent/api/task.go
Outdated
| return err | ||
| } | ||
|
|
||
| if dockerAPIVersion.GreaterThanOrEqualTo(dockerAPIVersion_1_18) { |
There was a problem hiding this comment.
should we log in an else case?
| @@ -106,6 +110,18 @@ func (f *factory) FindKnownAPIVersions() []DockerVersion { | |||
| return knownVersions | |||
| } | |||
|
|
|||
| // FindClientAPIVersion returns the version of the client from the map | |||
| // TODO we should let go docker client return this version information | |||
There was a problem hiding this comment.
the TODO seems like a better option, whats stopping us from doing that?
There was a problem hiding this comment.
go dockerclient doesn't support this today, we need contribute and update.
Docker moved the Memory from Config into HostConfig where on windows the memory information in Config wasn't respected. This commit sets the memory in both Config and HostConfig to make it compatiable.
Summary
Fix #616
Implementation details
Set the CPU and memory in both Docker Config and HostConfig
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes:
Description for the changelog
Licensing
This contribution is under the terms of the Apache 2.0 License: