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

Jump to bottom

non-ECS image cleanup: cleanup dangling images #2023

Merged
merged 1 commit into from
May 23, 2019
Merged

non-ECS image cleanup: cleanup dangling images #2023

merged 1 commit into from
May 23, 2019

Conversation

sparrc
Copy link
Contributor

@sparrc sparrc commented May 7, 2019

Use ImageID to cleanup images rather than tag name, to guarantee that we
are passing a legitimate identifier to InspectImage and RemoveImage.

closes #2017

Licensing

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

adnxn
adnxn approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@adnxn adnxn left a comment

Choose a reason for hiding this comment

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

changes lgtm. i wonder if we can do something to make that exclusion list handling logic a little clearer, but yea i know it was already before your changes.

agent/engine/docker_image_manager.go Show resolved Hide resolved
@adnxn
Copy link
Contributor

adnxn commented May 14, 2019

oh also this This branch is out-of-date with the base branch

shubham2892
shubham2892 approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@shubham2892 shubham2892 left a comment

Choose a reason for hiding this comment

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

LGTM. Left couple of NIT. 🚢

agent/engine/docker_image_manager_test.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager_test.go Show resolved Hide resolved
agent/engine/docker_image_manager_test.go Outdated Show resolved Hide resolved
@sharanyad
Copy link
Contributor

just a general question:
is the image ID used to clean up dangling images or all ECS/other non ECS images?

sharanyad
sharanyad reviewed May 14, 2019
View reviewed changes
agent/engine/docker_image_manager.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager.go Outdated
seelog.Infof("Removing non-ECS Image: %s", kv.ImageName)
err := imageManager.client.RemoveImage(ctx, kv.ImageName, dockerclient.RemoveImageTimeout)
seelog.Debugf("Removing non-ECS Image: %s (Tags: %s)", image.ImageID, image.RepoTags)
err := imageManager.client.RemoveImage(ctx, image.ImageID, dockerclient.RemoveImageTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

has the case been tested where there can be two images with same image ID, but different names?
Does this remove both the images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should both be removed, though I haven't tested this case explicitly. I can add a unit test for this.

Copy link
Contributor

@sharanyad sharanyad May 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

please test manually once. on a quick testing, it does not remove the image

$ docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
ubuntu              latest              d131e0fa2585        2 weeks ago         102MB
ubuntu              mirror              d131e0fa2585        2 weeks ago         102MB
$ docker rmi d131e0fa2585
Error response from daemon: conflict: unable to delete d131e0fa2585 (must be forced) - image is referenced in multiple repositories
$ docker rmi ubuntu:mirror
Untagged: ubuntu:mirror
$ docker rmi d131e0fa2585
Untagged: ubuntu:latest
Untagged: ubuntu@sha256:70fc21e832af32eeec9b0161a805c08f6dddf64d341748379de9a527c01b6ca1
Deleted: sha256:d131e0fa2585a7efbfb187f70d648aa50e251d9d3b7031edf4730ca6154e221e
Deleted: sha256:c59a62c2bba8db73ca8b8847baa7fa77e4d573f38e57d69592a9716f9fa075c4
Deleted: sha256:a4eb6208f601c2f32e043972b9fa8e813767aef2e60676e91796cabf8a0afdc4
Deleted: sha256:a14c708b62677e8acfb75ac873147e71dd26aa7bf75a8b63b3408e0826b174dc
Deleted: sha256:604cbde1a4c8fee1b102f8b64d4f41e62d770b5f8a6b06fb809cfd873a2643c3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I've added a change now to detect if an ImageID has multiple tags. If there are multiple tags on an ImageID, the tags will be removed first before removing the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, do we have unit tests for this case as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is tested here: https://github.com/aws/amazon-ecs-agent/pull/2023/files#diff-47eca467a5b10ea4396074d535ccea4cR1038

@sparrc
Copy link
Contributor Author

sparrc commented May 15, 2019

just a general question:
is the image ID used to clean up dangling images or all ECS/other non ECS images?

With this change, the ImageID will always be used for both cleanup and inspection of images.

@sparrc sparrc force-pushed the sparrc-2017 branch 2 times, most recently from 2da5c60 to a21a69c Compare May 15, 2019 10:08
@sparrc sparrc force-pushed the sparrc-2017 branch 2 times, most recently from 2a0795e to 43c488e Compare May 21, 2019 20:58
sharanyad
sharanyad reviewed May 21, 2019
View reviewed changes
agent/engine/docker_image_manager.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager.go Outdated
seelog.Infof("Removing non-ECS Image: %s", kv.ImageName)
err := imageManager.client.RemoveImage(ctx, kv.ImageName, dockerclient.RemoveImageTimeout)
seelog.Debugf("Removing non-ECS Image: %s (Tags: %s)", image.ImageID, image.RepoTags)
err := imageManager.client.RemoveImage(ctx, image.ImageID, dockerclient.RemoveImageTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, do we have unit tests for this case as well?

@sparrc sparrc changed the title image manager: cleanup dangling images non-ECS image cleanup: cleanup dangling images May 21, 2019
@sparrc sparrc force-pushed the sparrc-2017 branch 2 times, most recently from 7dfcc8d to 0d0c548 Compare May 22, 2019 16:40
sharanyad
sharanyad reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@sharanyad sharanyad left a comment

Choose a reason for hiding this comment

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

Could you please edit the commit message as well to mention this change is only for non-ECS images?

agent/engine/docker_image_manager.go Show resolved Hide resolved
agent/engine/docker_image_manager.go Show resolved Hide resolved
@sparrc
Copy link
Contributor Author

sparrc commented May 22, 2019

Could you please edit the commit message as well to mention this change is only for non-ECS images?

yes, done 👍

sharanyad
sharanyad reviewed May 22, 2019
View reviewed changes
agent/engine/docker_image_manager.go Outdated Show resolved Hide resolved
agent/engine/docker_image_manager.go Outdated Show resolved Hide resolved
sharanyad
sharanyad approved these changes May 22, 2019
View reviewed changes
Copy link
Contributor

@sharanyad sharanyad left a comment

Choose a reason for hiding this comment

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

lgtm

agent/engine/docker_image_manager.go Show resolved Hide resolved
@sparrc
non-ECS image cleanup: cleanup dangling images
22fa19a 
Use ImageID to cleanup images rather than tag name, to guarantee that we
are passing a legitimate identifier to InspectImage and RemoveImage.

closes aws#2017
@sparrc sparrc merged commit 22fa19a into aws:dev May 23, 2019
@sparrc sparrc deleted the sparrc-2017 branch May 23, 2019 19:15
@sparrc sparrc mentioned this pull request May 23, 2019
Closed
@sparrc sparrc added this to the 1.29.0 milestone May 23, 2019
@fenxiong fenxiong modified the milestones: 1.29.0, 1.28.1 May 31, 2019
@fenxiong fenxiong mentioned this pull request May 31, 2019
Merged
@sparrc sparrc mentioned this pull request Jun 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yumex93 yumex93 yumex93 left review comments

@sharanyad sharanyad sharanyad approved these changes

@shubham2892 shubham2892 shubham2892 approved these changes

@adnxn adnxn adnxn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.28.1
Development

Successfully merging this pull request may close these issues.
8 participants
@sparrc @adnxn @sharanyad @shubham2892 @yumex93 @amazon-ecs-bot @yhlee-aws @fenxiong