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.

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

Handle pull for "internal" containers #990

Merged
merged 3 commits into from
Sep 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 8 additions & 5 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
* Feature - Support for provisioning Tasks with ENIs
* Enhancement - Retry failed container image pull operations.
* Bug - Fixed a memory leak issue when submitting the task state change [#967](https://github.com/aws/amazon-ecs-agent/pull/967)
* Bug - Fix a race condition where a container can be created twice when agent restarts. [#939](https://github.com/aws/amazon-ecs-agent/pull/939)
* Bug - Fixed a race condition where a container can be created twice when agent restarts. [#939](https://github.com/aws/amazon-ecs-agent/pull/939)
* Bug - Fixed an issue where `microsoft/windowsservercore:latest` was not
pulled on Windows under certain conditions.
[#990](https://github.com/aws/amazon-ecs-agent/pull/990)

## 1.14.4
* Enhancement - Batch container state change events. [#867](https://github.com/aws/amazon-ecs-agent/pull/867)
Expand All @@ -14,14 +17,14 @@
* Enhancement - Allow instance attributes to be provided from config file
by [@ejholmes](https://github.com/ejholmes). [#908](https://github.com/aws/amazon-ecs-agent/pull/908)
* Enhancement - Reduce the disconnection period to the backend for idle connections. [#912](https://github.com/aws/amazon-ecs-agent/pull/912)
* Bug - Fix data race where a pointer was returned in Getter. [#889](https://github.com/aws/amazon-ecs-agent/pull/899)
* Bug - Fixed data race where a pointer was returned in Getter. [#889](https://github.com/aws/amazon-ecs-agent/pull/899)
* Bug - Reset agent state if the instance id changed on agent restart. [#892](https://github.com/aws/amazon-ecs-agent/pull/892)
* Bug - Fix a situation in which containers may be falsely reported as STOPPED
* Bug - Fixed a situation in which containers may be falsely reported as STOPPED
in the case of a Docker "stop" API failure. [#910](https://github.com/aws/amazon-ecs-agent/pull/910)
* Bug - Fix typo in log string by [@sharuzzaman](https://github.com/sharuzzaman). [#930](https://github.com/aws/amazon-ecs-agent/pull/930)
* Bug - Fixed typo in log string by [@sharuzzaman](https://github.com/sharuzzaman). [#930](https://github.com/aws/amazon-ecs-agent/pull/930)

## 1.14.3
* Bug - Fix a deadlock that was caused by the ImageCleanup and Image Pull. [#836](https://github.com/aws/amazon-ecs-agent/pull/836)
* Bug - Fixed a deadlock that was caused by the ImageCleanup and Image Pull. [#836](https://github.com/aws/amazon-ecs-agent/pull/836)

## 1.14.2
* Enhancement - Added introspection API for querying tasks by short docker ID, by [@aaronwalker](https://github.com/aaronwalker). [#813](https://github.com/aws/amazon-ecs-agent/pull/813)
Expand Down
2 changes: 1 addition & 1 deletion agent/api/task_windows_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const (
emptyVolumeContainerPath2 = `C:\my\empty-volume-2`
expectedEmptyVolumeGeneratedPath2 = `c:\ecs-empty-volume\` + emptyVolumeName2

expectedEmptyVolumeContainerImage = "microsoft/windowsservercore"
expectedEmptyVolumeContainerImage = "microsoft/nanoserver"
expectedEmptyVolumeContainerTag = "latest"
expectedEmptyVolumeContainerCmd = "not-applicable"

Expand Down
36 changes: 26 additions & 10 deletions agent/engine/docker_container_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ type DockerClient interface {
// PullImage pulls an image. authData should contain authentication data provided by the ECS backend.
PullImage(image string, authData *api.RegistryAuthenticationData) DockerContainerMetadata

// ImportLocalEmptyVolumeImage imports a locally-generated empty-volume image for supported platforms.
ImportLocalEmptyVolumeImage() DockerContainerMetadata

// CreateContainer creates a container with the provided docker.Config, docker.HostConfig, and name. A timeout value
// should be provided for the request.
CreateContainer(*docker.Config, *docker.HostConfig, string, time.Duration) DockerContainerMetadata
Expand Down Expand Up @@ -272,16 +275,6 @@ func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenti
return CannotGetDockerClientError{version: dg.version, err: err}
}

// Special case; this image is not one that should be pulled, but rather
// should be created locally if necessary
if image == fmt.Sprintf(imageNameFormat, emptyvolume.Image, emptyvolume.Tag) {
scratchErr := dg.createScratchImageIfNotExists()
if scratchErr != nil {
return CreateEmptyVolumeError{scratchErr}
}
return nil
}

authConfig, err := dg.getAuthdata(image, authData)
if err != nil {
return wrapPullErrorAsEngineError(err)
Expand Down Expand Up @@ -365,6 +358,27 @@ func (dg *dockerGoClient) pullImage(image string, authData *api.RegistryAuthenti
return nil
}

// ImportLocalEmptyVolumeImage imports a locally-generated empty-volume image for supported platforms.
func (dg *dockerGoClient) ImportLocalEmptyVolumeImage() DockerContainerMetadata {
timeout := dg.time().After(pullImageTimeout)

response := make(chan DockerContainerMetadata, 1)
go func() {
err := dg.createScratchImageIfNotExists()
var wrapped engineError
if err != nil {
wrapped = CreateEmptyVolumeError{err}
}
response <- DockerContainerMetadata{Error: wrapped}
}()
select {
case resp := <-response:
return resp
case <-timeout:
return DockerContainerMetadata{Error: &DockerTimeoutError{pullImageTimeout, "pulled"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the transition string be a const ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular reason why? I generally like constants if there's string reuse, string comparison, or if it aids clarity. I suppose reuse might apply here, but it's only used in two places right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was primarily concerned with reuse but it was more of a suggestion.

}
}

func (dg *dockerGoClient) createScratchImageIfNotExists() error {
client, err := dg.dockerClient()
if err != nil {
Expand All @@ -376,6 +390,7 @@ func (dg *dockerGoClient) createScratchImageIfNotExists() error {

_, err = client.InspectImage(emptyvolume.Image + ":" + emptyvolume.Tag)
if err == nil {
seelog.Debug("Empty volume image is already present, skipping import")
// Already exists; assume that it's okay to use it
return nil
}
Expand All @@ -388,6 +403,7 @@ func (dg *dockerGoClient) createScratchImageIfNotExists() error {
writer.Close()
}()

seelog.Debug("Importing empty volume image")
// Create it from an empty tarball
err = client.ImportImage(docker.ImportImageOptions{
Repository: emptyvolume.Image,
Expand Down
66 changes: 33 additions & 33 deletions agent/engine/docker_container_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,39 +189,6 @@ func TestPullImageDigest(t *testing.T) {
assert.NoError(t, metadata.Error, "Expected pull to succeed")
}

func TestPullEmptyvolumeImage(t *testing.T) {
mockDocker, client, testTime, done := dockerClientSetup(t)
defer done()

// The special emptyvolume image leads to a create, not pull
testTime.EXPECT().After(gomock.Any()).AnyTimes()
gomock.InOrder(
mockDocker.EXPECT().InspectImage(emptyvolume.Image+":"+emptyvolume.Tag).Return(nil, errors.New("Does not exist")),
mockDocker.EXPECT().ImportImage(gomock.Any()).Do(func(x interface{}) {
req := x.(docker.ImportImageOptions)
require.Equal(t, emptyvolume.Image, req.Repository, "expected empty volume repository")
require.Equal(t, emptyvolume.Tag, req.Tag, "expected empty volume tag")
}),
)

metadata := client.PullImage(emptyvolume.Image+":"+emptyvolume.Tag, nil)
assert.NoError(t, metadata.Error, "Expected pull to succeed")
}

func TestPullExistingEmptyvolumeImage(t *testing.T) {
mockDocker, client, testTime, done := dockerClientSetup(t)
defer done()

// The special emptyvolume image leads to a create only if it doesn't exist
testTime.EXPECT().After(gomock.Any()).AnyTimes()
gomock.InOrder(
mockDocker.EXPECT().InspectImage(emptyvolume.Image+":"+emptyvolume.Tag).Return(&docker.Image{}, nil),
)

metadata := client.PullImage(emptyvolume.Image+":"+emptyvolume.Tag, nil)
assert.NoError(t, metadata.Error, "Expected pull to succeed")
}

func TestPullImageECRSuccess(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -315,6 +282,39 @@ func TestPullImageECRAuthFail(t *testing.T) {
assert.Error(t, metadata.Error, "expected pull to fail")
}

func TestImportLocalEmptyVolumeImage(t *testing.T) {
mockDocker, client, testTime, done := dockerClientSetup(t)
defer done()

// The special emptyvolume image leads to a create, not pull
testTime.EXPECT().After(gomock.Any()).AnyTimes()
gomock.InOrder(
mockDocker.EXPECT().InspectImage(emptyvolume.Image+":"+emptyvolume.Tag).Return(nil, errors.New("Does not exist")),
mockDocker.EXPECT().ImportImage(gomock.Any()).Do(func(x interface{}) {
req := x.(docker.ImportImageOptions)
require.Equal(t, emptyvolume.Image, req.Repository, "expected empty volume repository")
require.Equal(t, emptyvolume.Tag, req.Tag, "expected empty volume tag")
}),
)

metadata := client.ImportLocalEmptyVolumeImage()
assert.NoError(t, metadata.Error, "Expected import to succeed")
}

func TestImportLocalEmptyVolumeImageExisting(t *testing.T) {
mockDocker, client, testTime, done := dockerClientSetup(t)
defer done()

// The special emptyvolume image leads to a create only if it doesn't exist
testTime.EXPECT().After(gomock.Any()).AnyTimes()
gomock.InOrder(
mockDocker.EXPECT().InspectImage(emptyvolume.Image+":"+emptyvolume.Tag).Return(&docker.Image{}, nil),
)

metadata := client.ImportLocalEmptyVolumeImage()
assert.NoError(t, metadata.Error, "Expected import to succeed")
}

func TestCreateContainerTimeout(t *testing.T) {
mockDocker, client, _, done := dockerClientSetup(t)
defer done()
Expand Down
11 changes: 11 additions & 0 deletions agent/engine/docker_task_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/engine/dependencygraph"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerclient"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume"
"github.com/aws/amazon-ecs-agent/agent/eventstream"
"github.com/aws/amazon-ecs-agent/agent/statechange"
"github.com/aws/amazon-ecs-agent/agent/statemanager"
Expand Down Expand Up @@ -508,6 +509,16 @@ func (engine *DockerTaskEngine) GetTaskByArn(arn string) (*api.Task, bool) {
}

func (engine *DockerTaskEngine) pullContainer(task *api.Task, container *api.Container) DockerContainerMetadata {
switch container.Type {
case api.ContainerCNIPause:
// ContainerCNIPause image are managed at startup
return DockerContainerMetadata{}
case api.ContainerEmptyHostVolume:
// ContainerEmptyHostVolume image is either local (must be imported) or remote (must be pulled)
if emptyvolume.LocalImage {
return engine.client.ImportLocalEmptyVolumeImage()
Copy link
Contributor

Choose a reason for hiding this comment

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

Did create better describe what was happening behind the scenes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Import is the verb that is used in the Docker API.

}
}
if engine.enableConcurrentPull {
seelog.Infof("Pulling container %v concurrently. Task: %v", container, task)
return engine.concurrentPull(task, container)
Expand Down
48 changes: 45 additions & 3 deletions agent/engine/docker_task_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/aws/amazon-ecs-agent/agent/credentials/mocks"
"github.com/aws/amazon-ecs-agent/agent/ecscni/mocks"
"github.com/aws/amazon-ecs-agent/agent/engine/dockerstate"
"github.com/aws/amazon-ecs-agent/agent/engine/image"
"github.com/aws/amazon-ecs-agent/agent/engine/testdata"
"github.com/aws/amazon-ecs-agent/agent/eventstream"
"github.com/aws/amazon-ecs-agent/agent/statemanager/mocks"
Expand Down Expand Up @@ -285,7 +286,6 @@ func TestTaskWithSteadyStateResourcesProvisioned(t *testing.T) {
client.EXPECT().PullImage(sleepContainer.Image, nil).Return(DockerContainerMetadata{})
imageManager.EXPECT().RecordContainerReference(sleepContainer).Return(nil)
imageManager.EXPECT().GetImageStateFromImageName(sleepContainer.Image).Return(nil)
client.EXPECT().PullImage(pauseContainer.Image, nil).Return(DockerContainerMetadata{})

gomock.InOrder(
// Ensure that the pause container is created first
Expand Down Expand Up @@ -1233,7 +1233,7 @@ func TestEngineDisableConcurrentPull(t *testing.T) {
"Task engine should not be able to perform concurrent pulling for version < 1.11.1")
}

func TestPauseContaienrHappyPath(t *testing.T) {
func TestPauseContainerHappyPath(t *testing.T) {
ctrl, dockerClient, mockTime, taskEngine, _, imageManager := mocks(t, &defaultConfig)
defer ctrl.Finish()

Expand Down Expand Up @@ -1261,7 +1261,6 @@ func TestPauseContaienrHappyPath(t *testing.T) {
pauseContainerID := "pauseContainerID"
// Pause container will be launched first
gomock.InOrder(
dockerClient.EXPECT().PullImage(gomock.Any(), nil).Return(DockerContainerMetadata{}),
dockerClient.EXPECT().CreateContainer(
gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Do(
func(config *docker.Config, x, y, z interface{}) {
Expand Down Expand Up @@ -1515,3 +1514,46 @@ func TestCreateContainerOnAgentRestart(t *testing.T) {
t.Error("Unexpected error", metadata.Error)
}
}

func TestPullCNIImage(t *testing.T) {
ctrl, _, _, privateTaskEngine, _, _ := mocks(t, &config.Config{})
defer ctrl.Finish()
taskEngine, _ := privateTaskEngine.(*DockerTaskEngine)

container := &api.Container{
Type: api.ContainerCNIPause,
}
task := &api.Task{
Containers: []*api.Container{container},
}
metadata := taskEngine.pullContainer(task, container)
assert.Equal(t, DockerContainerMetadata{}, metadata, "expected empty metadata")
}

func TestPullNormalImage(t *testing.T) {
ctrl, client, _, privateTaskEngine, _, imageManager := mocks(t, &config.Config{})
defer ctrl.Finish()
taskEngine, _ := privateTaskEngine.(*DockerTaskEngine)
saver := mock_statemanager.NewMockStateManager(ctrl)
taskEngine.SetSaver(saver)

imageName := "image"
container := &api.Container{
Type: api.ContainerNormal,
Image: imageName,
}
task := &api.Task{
Containers: []*api.Container{container},
}
imageState := &image.ImageState{
Image: &image.Image{ImageID: "id"},
}

client.EXPECT().PullImage(imageName, nil)
imageManager.EXPECT().RecordContainerReference(container)
imageManager.EXPECT().GetImageStateFromImageName(imageName).Return(imageState)
saver.EXPECT().Save()

metadata := taskEngine.pullContainer(task, container)
assert.Equal(t, DockerContainerMetadata{}, metadata, "expected empty metadata")
}
49 changes: 49 additions & 0 deletions agent/engine/docker_task_engine_unix_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// +build !windows,!integration

// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.
package engine

import (
"testing"

"github.com/aws/amazon-ecs-agent/agent/api"
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume"
"github.com/aws/amazon-ecs-agent/agent/statemanager/mocks"

"github.com/stretchr/testify/assert"
)

func TestPullEmptyVolumeImage(t *testing.T) {
ctrl, client, _, privateTaskEngine, _, _ := mocks(t, &config.Config{})
defer ctrl.Finish()
taskEngine, _ := privateTaskEngine.(*DockerTaskEngine)
saver := mock_statemanager.NewMockStateManager(ctrl)
taskEngine.SetSaver(saver)

imageName := "image"
container := &api.Container{
Type: api.ContainerEmptyHostVolume,
Image: imageName,
}
task := &api.Task{
Containers: []*api.Container{container},
}

assert.True(t, emptyvolume.LocalImage, "empty volume image is local")
client.EXPECT().ImportLocalEmptyVolumeImage()

metadata := taskEngine.pullContainer(task, container)
assert.Equal(t, DockerContainerMetadata{}, metadata, "expected empty metadata")
}
49 changes: 49 additions & 0 deletions agent/engine/docker_task_engine_windows_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// +build windows,!integration

// Copyright 2014-2017 Amazon.com, Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.
package engine

import (
"testing"

"github.com/aws/amazon-ecs-agent/agent/api"
"github.com/aws/amazon-ecs-agent/agent/config"
"github.com/aws/amazon-ecs-agent/agent/engine/emptyvolume"
"github.com/aws/amazon-ecs-agent/agent/statemanager/mocks"

"github.com/stretchr/testify/assert"
)

func TestPullEmptyVolumeImage(t *testing.T) {
ctrl, client, _, privateTaskEngine, _, _ := mocks(t, &config.Config{})
defer ctrl.Finish()
taskEngine, _ := privateTaskEngine.(*DockerTaskEngine)
saver := mock_statemanager.NewMockStateManager(ctrl)
taskEngine.SetSaver(saver)

imageName := "image"
container := &api.Container{
Type: api.ContainerEmptyHostVolume,
Image: imageName,
}
task := &api.Task{
Containers: []*api.Container{container},
}

assert.False(t, emptyvolume.LocalImage, "Windows empty volume image is not local")
client.EXPECT().PullImage(imageName, nil)

metadata := taskEngine.pullContainer(task, container)
assert.Equal(t, DockerContainerMetadata{}, metadata, "expected empty metadata")
}
Loading