Skip to content

Commit

Permalink
Fix failing test and add new test
Browse files Browse the repository at this point in the history
  • Loading branch information
Peng Yin committed Nov 16, 2017
1 parent ce18f83 commit 4d085de
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 39 deletions.
81 changes: 58 additions & 23 deletions agent/api/task_test.go
View file
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 Down Expand Up @@ -207,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 @@ -251,15 +254,13 @@ 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{
Privileged: true,
SecurityOpt: []string{"foo", "bar"},
VolumesFrom: []string{"dockername-c2"},
Memory: 100 * 1024 * 1024,
CPUShares: 50,
MemorySwappiness: memorySwappinessDefault,
}

Expand Down Expand Up @@ -288,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 @@ -310,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 @@ -337,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 @@ -371,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 @@ -402,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 @@ -439,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 @@ -469,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 @@ -1175,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 @@ -1219,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 @@ -1263,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
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)
}
2 changes: 1 addition & 1 deletion agent/api/task_windows_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func TestWindowsMemorySwappinessOption(t *testing.T) {
},
}

config, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask))
config, configErr := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion)
if configErr != nil {
t.Fatal(configErr)
}
Expand Down
Loading

0 comments on commit 4d085de

Please sign in to comment.