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.

Sign up for GitHub

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 CPU/Memory in both config and hostconfig #1069

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Set CPU/Memory in both config and hostconfig #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
48a4490
api: Set CPU/Memory in config or hostconfig
Nov 10, 2017
23af90f
Fix failing test and add new test
Nov 13, 2017
8aced92
Change CPUShares to CPUPercent on windows
Nov 15, 2017
8633978
Minor refactor about variable name
Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 are not enforced. [#1069](https://github.com/aws/amazon-ecs-agent/pull/1069)

## 1.15.0
* Feature - Support for provisioning tasks with ENIs.
Expand Down
61 changes: 53 additions & 8 deletions agent/api/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/ecscni"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
"github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume"
"github.com/aws/amazon-ecs-agent/agent/utils/ttime"
"github.com/aws/aws-sdk-go/aws/arn"
Expand Down Expand Up @@ -414,11 +415,11 @@ func (task *Task) getEarliestKnownTaskStatusForContainers() TaskStatus {

// DockerConfig converts the given container in this task to the format of
// GoDockerClient's 'Config' struct
func (task *Task) DockerConfig(container *Container) (*docker.Config, *DockerClientConfigError) {
return task.dockerConfig(container)
func (task *Task) DockerConfig(container *Container, apiVersion dockerclient.DockerVersion) (*docker.Config, *DockerClientConfigError) {
return task.dockerConfig(container, apiVersion)
}

func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerClientConfigError) {
func (task *Task) dockerConfig(container *Container, apiVersion dockerclient.DockerVersion) (*docker.Config, *DockerClientConfigError) {
dockerVolumes, err := task.dockerConfigVolumes(container)
if err != nil {
return nil, &DockerClientConfigError{err.Error()}
Expand Down Expand Up @@ -447,8 +448,11 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli
ExposedPorts: task.dockerExposedPorts(container),
Volumes: dockerVolumes,
Env: dockerEnv,
Memory: dockerMem,
CPUShares: task.dockerCPUShares(container.CPU),
}

err = task.SetConfigHostconfigBasedOnVersion(container, config, nil, apiVersion)
if err != nil {
return nil, &DockerClientConfigError{"setting docker config failed, err: " + err.Error()}
}

if container.DockerConfig.Config != nil {
Expand All @@ -464,6 +468,42 @@ func (task *Task) dockerConfig(container *Container) (*docker.Config, *DockerCli
return config, nil
}

// SetConfigHostconfigBasedOnVersion sets the fields in both Config and HostConfig based on api version for backward compatibility
func (task *Task) SetConfigHostconfigBasedOnVersion(container *Container, config *docker.Config, hc *docker.HostConfig, apiVersion dockerclient.DockerVersion) error {
// Convert MB to B
dockerMem := int64(container.Memory * 1024 * 1024)
if dockerMem != 0 && dockerMem < DockerContainerMinimumMemoryInBytes {
dockerMem = DockerContainerMinimumMemoryInBytes
}
cpuShare := task.dockerCPUShares(container.CPU)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to set CPU in both?

Copy link
Author

Choose a reason for hiding this comment

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

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


// Docker copied Memory and cpu field into hostconfig in 1.6 with api version(1.18)
// https://github.com/moby/moby/commit/837eec064d2d40a4d86acbc6f47fada8263e0d4c
dockerAPIVersion, err := docker.NewAPIVersion(string(apiVersion))
if err != nil {
seelog.Errorf("Creating docker api version failed, err: %v", err)
return err
}

dockerAPIVersion_1_18 := docker.APIVersion([]int{1, 18})
if dockerAPIVersion.GreaterThanOrEqualTo(dockerAPIVersion_1_18) {
// Set the memory and cpu in host config
if hc != nil {
hc.Memory = dockerMem
hc.CPUShares = cpuShare
}
return nil
}

// Set the memory and cpu in config
if config != nil {
config.Memory = dockerMem
config.CPUShares = cpuShare
}

return nil
}

// dockerCPUShares converts containerCPU shares if needed as per the logic stated below:
// Docker silently converts 0 to 1024 CPU shares, which is probably not what we
// want. Instead, we convert 0 to 2 to be closer to expected behavior. The
Expand Down Expand Up @@ -512,8 +552,8 @@ func (task *Task) dockerConfigVolumes(container *Container) (map[string]struct{}
}

// DockerHostConfig construct the configuration recognized by docker
func (task *Task) DockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer) (*docker.HostConfig, *HostConfigError) {
return task.dockerHostConfig(container, dockerContainerMap)
func (task *Task) DockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer, apiVersion dockerclient.DockerVersion) (*docker.HostConfig, *HostConfigError) {
return task.dockerHostConfig(container, dockerContainerMap, apiVersion)
}

// ApplyExecutionRoleLogsAuth will check whether the task has excecution role
Expand All @@ -538,7 +578,7 @@ func (task *Task) ApplyExecutionRoleLogsAuth(hostConfig *docker.HostConfig, cred
return nil
}

func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer) (*docker.HostConfig, *HostConfigError) {
func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[string]*DockerContainer, apiVersion dockerclient.DockerVersion) (*docker.HostConfig, *HostConfigError) {
dockerLinkArr, err := task.dockerLinks(container, dockerContainerMap)
if err != nil {
return nil, &HostConfigError{err.Error()}
Expand All @@ -564,6 +604,11 @@ func (task *Task) dockerHostConfig(container *Container, dockerContainerMap map[
VolumesFrom: volumesFrom,
}

err = task.SetConfigHostconfigBasedOnVersion(container, nil, hostConfig, apiVersion)
if err != nil {
return nil, &HostConfigError{err.Error()}
}

if container.DockerConfig.HostConfig != nil {
err := json.Unmarshal([]byte(*container.DockerConfig.HostConfig), hostConfig)
if err != nil {
Expand Down
80 changes: 59 additions & 21 deletions agent/api/task_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/credentials"
"github.com/aws/amazon-ecs-agent/agent/credentials/mocks"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
"github.com/aws/amazon-ecs-agent/agent/utils/ttime"
docker "github.com/fsouza/go-dockerclient"
"github.com/golang/mock/gomock"
Expand All @@ -31,6 +32,8 @@ import (

const dockerIDPrefix = "dockerid-"

var defaultDockerClientAPIVersion = dockerclient.Version_1_17

func strptr(s string) *string { return &s }

func dockerMap(task *Task) map[string]*DockerContainer {
Expand All @@ -51,7 +54,7 @@ func TestDockerConfigPortBinding(t *testing.T) {
},
}

config, err := testTask.DockerConfig(testTask.Containers[0])
config, err := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if err != nil {
t.Error(err)
}
Expand All @@ -76,7 +79,7 @@ func TestDockerConfigCPUShareZero(t *testing.T) {
},
}

config, err := testTask.DockerConfig(testTask.Containers[0])
config, err := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if err != nil {
t.Error(err)
}
Expand All @@ -96,7 +99,7 @@ func TestDockerConfigCPUShareMinimum(t *testing.T) {
},
}

config, err := testTask.DockerConfig(testTask.Containers[0])
config, err := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if err != nil {
t.Error(err)
}
Expand All @@ -116,7 +119,7 @@ func TestDockerConfigCPUShareUnchanged(t *testing.T) {
},
}

config, err := testTask.DockerConfig(testTask.Containers[0])
config, err := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if err != nil {
t.Error(err)
}
Expand All @@ -136,7 +139,7 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
},
}

config, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
config, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)

bindings, ok := config.PortBindings["10/tcp"]
Expand All @@ -163,7 +166,7 @@ func TestDockerHostConfigVolumesFrom(t *testing.T) {
},
}

config, err := testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask))
config, err := testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)

if !reflect.DeepEqual(config.VolumesFrom, []string{"dockername-c1"}) {
Expand All @@ -179,6 +182,7 @@ func TestDockerHostConfigRawConfig(t *testing.T) {
DNSSearch: []string{"dns.search"},
ExtraHosts: []string{"extra:hosts"},
SecurityOpt: []string{"foo", "bar"},
CPUShares: 2,
LogConfig: docker.LogConfig{
Type: "foo",
Config: map[string]string{"foo": "bar"},
Expand Down Expand Up @@ -206,7 +210,7 @@ func TestDockerHostConfigRawConfig(t *testing.T) {
},
}

config, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
config, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, configErr)

expectedOutput := rawHostConfigInput
Expand Down Expand Up @@ -250,7 +254,7 @@ func TestDockerHostConfigRawConfigMerging(t *testing.T) {
},
}

hostConfig, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
hostConfig, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, configErr)

expected := docker.HostConfig{
Expand Down Expand Up @@ -285,18 +289,18 @@ func TestDockerHostConfigPauseContainer(t *testing.T) {

// Verify that the network mode is set to "container:<pause-container-docker-id>"
// for a non empty volume, non pause container
config, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
config, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)
assert.Equal(t, "container:"+dockerIDPrefix+PauseContainerName, config.NetworkMode)

// Verify that the network mode is not set to "none" for the
// empty volume container
config, err = testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask))
config, err = testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)
assert.Equal(t, networkModeNone, config.NetworkMode)

// Verify that the network mode is set to "none" for the pause container
config, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask))
config, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)
assert.Equal(t, networkModeNone, config.NetworkMode)

Expand All @@ -307,13 +311,13 @@ func TestDockerHostConfigPauseContainer(t *testing.T) {

// DNS overrides are only applied to the pause container. Verify that the non-pause
// container contains no overrides
config, err = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
config, err = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)
assert.Equal(t, 0, len(config.DNS))
assert.Equal(t, 0, len(config.DNSSearch))

// Verify DNS settings are overridden for the pause container
config, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask))
config, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)
assert.Equal(t, []string{"169.254.169.253"}, config.DNS)
assert.Equal(t, []string{"us-west-2.compute.internal"}, config.DNSSearch)
Expand All @@ -334,7 +338,7 @@ func TestBadDockerHostConfigRawConfig(t *testing.T) {
},
},
}
_, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(&testTask))
_, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(&testTask), defaultDockerClientAPIVersion)
assert.Error(t, err)
}
}
Expand Down Expand Up @@ -368,7 +372,7 @@ func TestDockerConfigRawConfig(t *testing.T) {
},
}

config, configErr := testTask.DockerConfig(testTask.Containers[0])
config, configErr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if configErr != nil {
t.Fatal(configErr)
}
Expand Down Expand Up @@ -399,7 +403,7 @@ func TestDockerConfigRawConfigNilLabel(t *testing.T) {
},
}

_, configErr := testTask.DockerConfig(testTask.Containers[0])
_, configErr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if configErr != nil {
t.Fatal(configErr)
}
Expand Down Expand Up @@ -436,7 +440,7 @@ func TestDockerConfigRawConfigMerging(t *testing.T) {
},
}

config, configErr := testTask.DockerConfig(testTask.Containers[0])
config, configErr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if configErr != nil {
t.Fatal(configErr)
}
Expand Down Expand Up @@ -466,7 +470,7 @@ func TestBadDockerConfigRawConfig(t *testing.T) {
},
},
}
_, err := testTask.DockerConfig(testTask.Containers[0])
_, err := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
if err == nil {
t.Fatal("Expected error, was none for: " + badConfig)
}
Expand Down Expand Up @@ -1172,7 +1176,7 @@ func TestApplyExecutionRoleLogsAuthSet(t *testing.T) {
credentialsManager.EXPECT().GetTaskCredentials(credentialsIDInTask).Return(taskCredentials, true)
task.initializeCredentialsEndpoint(credentialsManager)

config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task))
config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task), defaultDockerClientAPIVersion)
assert.Nil(t, err)

err = task.ApplyExecutionRoleLogsAuth(config, credentialsManager)
Expand Down Expand Up @@ -1216,7 +1220,7 @@ func TestApplyExecutionRoleLogsAuthFailEmptyCredentialsID(t *testing.T) {

task.initializeCredentialsEndpoint(credentialsManager)

config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task))
config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task), defaultDockerClientAPIVersion)
assert.Nil(t, err)

err = task.ApplyExecutionRoleLogsAuth(config, credentialsManager)
Expand Down Expand Up @@ -1260,9 +1264,43 @@ func TestApplyExecutionRoleLogsAuthFailNoCredentialsForTask(t *testing.T) {
credentialsManager.EXPECT().GetTaskCredentials(credentialsIDInTask).Return(credentials.TaskIAMRoleCredentials{}, false)
task.initializeCredentialsEndpoint(credentialsManager)

config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task))
config, err := task.DockerHostConfig(task.Containers[0], dockerMap(task), defaultDockerClientAPIVersion)
assert.Error(t, err)

err = task.ApplyExecutionRoleLogsAuth(config, credentialsManager)
assert.Error(t, err)
}

// TestSetConfigHostconfigBasedOnAPIVersion tests the docker hostconfig was correctly set// based on the docker client version
func TestSetConfigHostconfigBasedOnAPIVersion(t *testing.T) {
testTask := &Task{
Containers: []*Container{
{
Name: "c1",
CPU: uint(100),
Memory: uint(50),
},
},
}

hostconfig, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
assert.Nil(t, err)

config, cerr := testTask.DockerConfig(testTask.Containers[0], defaultDockerClientAPIVersion)
assert.Nil(t, cerr)

assert.Equal(t, int64(50*1024*1024), config.Memory)
assert.Equal(t, int64(100), config.CPUShares)
assert.Empty(t, hostconfig.CPUShares)
assert.Empty(t, hostconfig.Memory)

hostconfig, err = testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), dockerclient.Version_1_18)
assert.Nil(t, err)

config, cerr = testTask.DockerConfig(testTask.Containers[0], dockerclient.Version_1_18)
assert.Nil(t, err)
assert.Equal(t, int64(50*1024*1024), hostconfig.Memory)
assert.Equal(t, int64(100), hostconfig.CPUShares)
assert.Empty(t, config.CPUShares)
assert.Empty(t, config.Memory)
}
2 changes: 1 addition & 1 deletion agent/api/task_unix_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ func TestPlatformHostConfigOverrideErrorPath(t *testing.T) {
},
}

dockerHostConfig, err := task.DockerHostConfig(task.Containers[0], dockerMap(task))
dockerHostConfig, err := task.DockerHostConfig(task.Containers[0], dockerMap(task), defaultDockerClientAPIVersion)
assert.Error(t, err)
assert.Empty(t, dockerHostConfig)
}
4 changes: 4 additions & 0 deletions agent/api/task_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package api

import (
"path/filepath"
"runtime"
"strings"

"github.com/aws/amazon-ecs-agent/agent/config"
Expand All @@ -28,6 +29,8 @@ const (
memorySwappinessDefault = -1
)

var cpuShareScaleFactor = runtime.NumCPU() * 1024

// adjustForPlatform makes Windows-specific changes to the task after unmarshal
func (task *Task) adjustForPlatform(cfg *config.Config) {
task.downcaseAllVolumePaths()
Expand Down Expand Up @@ -59,6 +62,7 @@ func getCanonicalPath(path string) string {
// passed to Docker API.
func (task *Task) platformHostConfigOverride(hostConfig *docker.HostConfig) error {
task.overrideDefaultMemorySwappiness(hostConfig)
hostConfig.CPUPercent = hostConfig.CPUShares / int64(cpuShareScaleFactor)
return nil
}

Expand Down
Loading