remove non ecs containers and images changes with docker sdk client #1752

adnxn · 2018-12-21T02:00:05Z

Summary

updating #1736 with docker sdk client and merging to dev

Implementation details

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

adnxn · 2018-12-21T21:53:56Z

adding @haikuoliu, @yumex93 since reviewed #1736

yhlee-aws

We need integ and/or functional test coverage for this feature

yhlee-aws · 2018-12-24T17:31:06Z

agent/config/config_test.go

@@ -373,8 +374,8 @@ func TestInvalidFormatParseEnvVariableDuration(t *testing.T) {

 func TestValidForImagesCleanupExclusion(t *testing.T) {
 	defer setTestRegion()()
-	defer setTestEnv("ECS_IMAGE_CLEANUP_EXCLUDE", "amazonlinux:2,amazonlinux:3")()
-	imagesNotDelete := parseImageCleanupExclusionList("ECS_IMAGE_CLEANUP_EXCLUDE")
+	defer setTestEnv("ECS_NONECS_IMAGE_CLEANUP_EXCLUDE", "amazonlinux:2,amazonlinux:3")()


ECS_NONECS? I'm assuming first ECS is the prefix for internal/ECS env vars?
Howe about ECS_EXCLUDE_CLEANUP_NONECS_IMAGE

How about ECS_EXCLUDE_UNTRACKED_IMAGE? I'd like to get rid of "NONECS".

yikes, good points.

ECS_EXCLUDE_UNTRACKED_IMAGE

i like this, thanks @samuelkarp

yhlee-aws · 2018-12-24T17:34:13Z

agent/engine/docker_image_manager.go

+	}
+}
+
+func (imageManager *dockerImageManager) nonECSImagesNames(ctx context.Context) []string {


yhlee-aws · 2018-12-24T17:37:21Z

agent/dockerclient/dockerapi/docker_client.go

+		return ListImagesResponse{Error: err}
+	}
+	images, err := client.ImageList(ctx, types.ImageListOptions{
+		All: false,


can you add comment on what All flag signifies? with false, it's not clear what we are ignoring. From docker API:
"All: Show all images (default hides intermediate images)"
So I'm wondering if this is even required.

i think you're right, seems like we can leave this out.

yhlee-aws · 2018-12-24T17:39:03Z

agent/dockerclient/dockerapi/docker_client_test.go

@@ -731,7 +736,7 @@ func TestContainerEventsStreamError(t *testing.T) {

 	eventsChan := make(chan events.Message, dockerEventBufferSize)
 	errChan := make(chan error)
-	mockDockerSDK.EXPECT().Events(gomock.Any(), gomock.Any()).Return(eventsChan, errChan).Times(2)
+	mockDockerSDK.EXPECT().Events(gomock.Any(), gomock.Any()).Return(eventsChan, errChan).MinTimes(1)


for my curiosity, why did you loosen the required call count check?

so looking at this test TestContainerEventsStreamError we only pass up a single event to test this codepath. so we should we expecting at least one call to Events(...).

where as with the Times(2), we would sometimes hit the Events(...) call on the first iteration through this loop and would get test failures for missing calls to the mock from time to time. for example running with -count=100 you'll likely see test failure every time.

let me know if you think i've missed something - i think we added this test as part of the moby migration yea?

yhlee-aws · 2018-12-24T17:41:53Z

agent/engine/docker_image_manager.go

+	}
+}
+
+func (imageManager *dockerImageManager) nonECSContainersIDs(ctx context.Context) ([]string, error) {


haikuoliu · 2018-12-26T22:48:02Z

agent/dockerclient/dockerapi/docker_client_test.go

+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+	response := client.ListImages(ctx, dockerclient.ListImagesTimeout)
+	if response.Error != nil {


I think you can just use assert.NoError and assert.Equal, same for other tests

haikuoliu · 2018-12-26T22:59:08Z

agent/engine/docker_image_manager.go

+	}
+	var nonECSContainerRemoveAvailableIDs []string
+	for _, id := range nonECSContainersIDs {
+		response, _ := imageManager.client.InspectContainer(ctx, id, dockerclient.InspectContainerTimeout)


I think it would be better to check the error

added checking and logging

haikuoliu · 2018-12-26T23:23:07Z

agent/engine/docker_image_manager.go

+
+	var imageWithSizeList []ImageWithSize
+	for _, imageName := range nonECSImageNamesRemoveEligible {
+		resp, _ := imageManager.client.InspectImage(imageName)


check error

added checking and logging

haikuoliu · 2018-12-26T23:24:04Z

agent/engine/docker_image_manager.go

+	})
+
+	// we will remove the remaining nonECSImages in each performPeriodicImageCleanup call()
+	var numImagesAlreadyDelete = 0


minor: numImagesAlreadyDeleted

adnxn · 2018-12-27T18:18:36Z

ftest logs showing:

--- FAIL: TestOOMContainer (10.50s)
	utils_windows.go:87: Registry location test_path_1545881813823392500
	utils_windows.go:97: datadir C:\Users\ADMINI~1\AppData\Local\Temp\amazon-ecs-agent\ftest_temp\ecs_integ_testdata336277954\data
	utils_windows.go:105: Created directory C:\Users\ADMINI~1\AppData\Local\Temp\amazon-ecs-agent\ftest_temp\ecs_integ_testdata336277954 to store test data in
	utils.go:163: Found agent metadata: {Cluster:ecs-functional-tests ContainerInstanceArn:0xc0422f5520 Version:Amazon ECS Agent - v1.23.0 (*UNKNOWN)}
	utils.go:184: Task definition: ecsftest-oom-container-windows-601ba04ca0a9e1d8536dadd3f48ea215:1
	utils.go:204: Started task: arn:aws:ecs:us-west-2:535959970326:task/cc2c3719-f779-4d6d-bbb9-c8c3a9ce2176
	functionaltests_windows_test.go:214: 
			Error Trace:	functionaltests_windows_test.go:214
			Error:      	Received unexpected error:
			            	Task terminal; will never reach RUNNING
			            	github.com/aws/amazon-ecs-agent/agent/functional_tests/util.(*TestTask).waitStatus.func1
			            		C:/Users/ADMINI~1/AppData/Local/Temp/amazon-ecs-agent/go/src/github.com/aws/amazon-ecs-agent/agent/functional_tests/util/utils.go:526
			            	runtime.goexit
			            		C:/Go/src/runtime/asm_amd64.s:2337
			Test:       	TestOOMContainer
			Messages:   	Expect task to be running
	utils.go:171: Preserving test dir for failed test C:\Users\ADMINI~1\AppData\Local\Temp\amazon-ecs-agent\ftest_temp\ecs_integ_testdata336277954

agent logs showing:

2018-12-27T03:36:57Z [WARN] DockerGoClient: failed to pull image amazon/amazon-ecs-windows-python:make: Error response from daemon: repository amazon/amazon-ecs-windows-python not found: does not exist or no pull access
2018-12-27T03:36:58Z [WARN] DockerGoClient: failed to pull image amazon/amazon-ecs-windows-python:make: Error response from daemon: repository amazon/amazon-ecs-windows-python not found: does not exist or no pull access
2018-12-27T03:36:59Z [WARN] DockerGoClient: failed to pull image amazon/amazon-ecs-windows-python:make: Error response from daemon: repository amazon/amazon-ecs-windows-python not found: does not exist or no pull access
2018-12-27T03:37:01Z [WARN] DockerGoClient: failed to pull image amazon/amazon-ecs-windows-python:make: Error response from daemon: repository amazon/amazon-ecs-windows-python not found: does not exist or no pull access
2018-12-27T03:37:03Z [WARN] DockerGoClient: failed to pull image amazon/amazon-ecs-windows-python:make: Error response from daemon: repository amazon/amazon-ecs-windows-python not found: does not exist or no pull access
2018-12-27T03:37:03Z [ERROR] Error inspecting image amazon/amazon-ecs-windows-python:make: Error: No such image: amazon/amazon-ecs-windows-python:make
2018-12-27T03:37:03Z [ERROR] Task engine [arn:aws:ecs:us-west-2:535959970326:task/cc2c3719-f779-4d6d-bbb9-c8c3a9ce2176]: Unable to add container reference to image state: Error: No such image: amazon/amazon-ecs-windows-python:make

tracking here: #1763

haikuoliu · 2018-12-27T18:47:21Z

agent/engine/docker_image_manager.go

-		response, _ := imageManager.client.InspectContainer(ctx, id, dockerclient.InspectContainerTimeout)
+		response, icErr := imageManager.client.InspectContainer(ctx, id, dockerclient.InspectContainerTimeout)
+		if icErr != nil {
+			seelog.Errorf("Error inspecting non-ECS container id: %s - %v", id, icErr)


do we need to add a continue in this case since response will contain nothing? same below

you're correct on both accounts, will update.

haikuoliu · 2018-12-29T00:08:54Z

agent/config/config.go

@@ -126,6 +126,10 @@ const (

 	// DefaultTaskMetadataBurstRate is set to handle 60 burst requests at once
 	DefaultTaskMetadataBurstRate = 60
+
+	//Known cached image names


adnxn · 2018-12-31T17:25:18Z

ftest failure for 4cd4804 is same issue being tracked here #1763

cyastella and others added 3 commits December 21, 2018 13:34

remove non ecs containers and images

602e48e

dockerclient, engine: image removal adds docker sdk client changes

40e9849

dockerclient: set mock expectations count

e9c4be7

adnxn force-pushed the merge-remove-nonecs branch from f5b466b to e9c4be7 Compare December 21, 2018 21:36

adnxn changed the title ~~update remove non ecs containers and images changes with docker sdk client~~ remove non ecs containers and images changes with docker sdk client Dec 21, 2018

adnxn requested review from haikuoliu and yumex93 December 21, 2018 21:53

adnxn requested a review from a team December 22, 2018 02:06

yhlee-aws reviewed Dec 24, 2018

View reviewed changes

agent: naming changes for exclude non ecs images

e9f23ab

haikuoliu reviewed Dec 26, 2018

View reviewed changes

yhlee-aws approved these changes Dec 27, 2018

View reviewed changes

haikuoliu approved these changes Dec 27, 2018

View reviewed changes

dockerapi, engine: use assert and add inspect* error logging

ad95b99

adnxn force-pushed the merge-remove-nonecs branch from 3a24c35 to ad95b99 Compare December 27, 2018 21:41

config: exclude known cached images

4cd4804

haikuoliu approved these changes Dec 29, 2018

View reviewed changes

adnxn merged commit 4cd4804 into aws:dev Dec 31, 2018

adnxn added a commit that referenced this pull request Dec 31, 2018

Merge pull request #1752 from adnxn:merge-remove-nonecs

5cfcf77

adnxn mentioned this pull request Dec 31, 2018

docs: add nonECS image removal #1769

Merged

8 tasks

adnxn added this to the 1.24.0 milestone Jan 2, 2019

adnxn mentioned this pull request Jan 7, 2019

Automated cleanup of non-ECS docker images #1380

Closed

adnxn deleted the merge-remove-nonecs branch February 6, 2019 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove non ecs containers and images changes with docker sdk client #1752

remove non ecs containers and images changes with docker sdk client #1752

adnxn commented Dec 21, 2018 •

edited

Loading

adnxn commented Dec 21, 2018

yhlee-aws left a comment

yhlee-aws Dec 24, 2018

samuelkarp Dec 24, 2018

adnxn Dec 26, 2018

yhlee-aws Dec 24, 2018

yhlee-aws Dec 24, 2018

adnxn Dec 26, 2018

yhlee-aws Dec 24, 2018

adnxn Dec 26, 2018 •

edited

Loading

yhlee-aws Dec 24, 2018

haikuoliu Dec 26, 2018

haikuoliu Dec 26, 2018

adnxn Dec 27, 2018 •

edited

Loading

haikuoliu Dec 26, 2018

adnxn Dec 27, 2018

haikuoliu Dec 26, 2018

adnxn commented Dec 27, 2018 •

edited

Loading

haikuoliu Dec 27, 2018

adnxn Dec 27, 2018

haikuoliu Dec 29, 2018

adnxn commented Dec 31, 2018

remove non ecs containers and images changes with docker sdk client #1752

remove non ecs containers and images changes with docker sdk client #1752

Conversation

adnxn commented Dec 21, 2018 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

adnxn commented Dec 21, 2018

yhlee-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn commented Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnxn commented Dec 31, 2018

adnxn commented Dec 21, 2018 •

edited

Loading

adnxn Dec 26, 2018 •

edited

Loading

adnxn Dec 27, 2018 •

edited

Loading

adnxn commented Dec 27, 2018 •

edited

Loading