Skip to content

Commit

Permalink
image manager: cleanup dangling images
Browse files Browse the repository at this point in the history 
Use ImageID to cleanup images rather than tag name, to guarantee that we
are passing a legitimate identifier to InspectImage and RemoveImage.

closes #2017
  • Loading branch information
@sparrc
sparrc committed May 7, 2019
1 parent 9fb3226 commit d3652ba
Show file tree
Hide file tree
Showing 2 changed files with 182 additions and 52 deletions.
87 changes: 48 additions & 39 deletions agent/engine/docker_image_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,70 +388,78 @@ func (imageManager *dockerImageManager) getNonECSContainerIDs(ctx context.Contex
return nonECSContainersIDs, nil
}

type ImageWithSize struct {
ImageName string
Size int64
type ImageWithSizeID struct {
RepoTags []string
ImageID string
Size int64
}

func (imageManager *dockerImageManager) removeNonECSImages(ctx context.Context, nonECSImagesNumToDelete int) {
if nonECSImagesNumToDelete == 0 {
return
}
var nonECSImageNames = imageManager.getNonECSImageNames(ctx)
var nonECSImageNamesRemoveEligible []string
for _, nonECSImage := range nonECSImageNames {
if !isInExclusionList(nonECSImage, imageManager.imageCleanupExclusionList) {
nonECSImageNamesRemoveEligible = append(nonECSImageNamesRemoveEligible, nonECSImage)
}
}
nonECSImages := imageManager.getNonECSImages(ctx)

var imageWithSizeList []ImageWithSize
for _, imageName := range nonECSImageNamesRemoveEligible {
resp, iiErr := imageManager.client.InspectImage(imageName)
if iiErr != nil {
seelog.Errorf("Error inspecting non-ECS image name: %s - %v", imageName, iiErr)
continue
}
imageWithSizeList = append(imageWithSizeList, ImageWithSize{imageName, resp.Size})
}
// we want to sort images with size ascending
sort.Slice(imageWithSizeList, func(i, j int) bool {
return imageWithSizeList[i].Size < imageWithSizeList[j].Size
sort.Slice(nonECSImages, func(i, j int) bool {
return nonECSImages[i].Size < nonECSImages[j].Size
})

// we will remove the remaining nonECSImages in each performPeriodicImageCleanup call()
var numImagesAlreadyDeleted = 0
for _, kv := range imageWithSizeList {
for _, image := range nonECSImages {
if numImagesAlreadyDeleted == nonECSImagesNumToDelete {
break
}
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)
if err != nil {
seelog.Errorf("Error removing Image %s - %v", kv.ImageName, err)
seelog.Errorf("Error removing Image %s (Tags: %s) - %v", image.ImageID, image.RepoTags, err)
continue
} else {
seelog.Infof("Image removed: %s", kv.ImageName)
seelog.Infof("Image removed: %s (Tags: %s)", image.ImageID, image.RepoTags)
numImagesAlreadyDeleted++
}
}
}

func (imageManager *dockerImageManager) getNonECSImageNames(ctx context.Context) []string {
response := imageManager.client.ListImages(ctx, dockerclient.ListImagesTimeout)
var allImagesNames []string
for _, name := range response.RepoTags {
allImagesNames = append(allImagesNames, name)
}
var ecsImageNames []string
for _, imageState := range imageManager.getAllImageStates() {
for _, imageName := range imageState.Image.Names {
ecsImageNames = append(ecsImageNames, imageName)
// getNonECSImages returns type ImageWithSizeID with all fields populated.
func (imageManager *dockerImageManager) getNonECSImages(ctx context.Context) []ImageWithSizeID {
r := imageManager.client.ListImages(ctx, dockerclient.ListImagesTimeout)
var allImages []ImageWithSizeID
for _, imageID := range r.ImageIDs {
resp, err := imageManager.client.InspectImage(imageID)
if err != nil {
seelog.Errorf("Error inspecting non-ECS image: (ImageID: %s), %s", imageID, err)
continue
}
allImages = append(allImages,
ImageWithSizeID{
ImageID: imageID,
Size: resp.Size,
RepoTags: resp.RepoTags,
})
}
var ecsImageIDs []string
for _, imageState := range imageManager.getAllImageStates() {
ecsImageIDs = append(ecsImageIDs, imageState.Image.ImageID)
}

var nonECSImageNames = exclude(allImagesNames, ecsImageNames)
return nonECSImageNames
var nonECSImages []ImageWithSizeID
for _, image := range allImages {
if !isInExclusionList(image.ImageID, ecsImageIDs) {
var exclude bool
for _, repoTag := range image.RepoTags {
if isInExclusionList(repoTag, imageManager.imageCleanupExclusionList) {
exclude = true
}
}
if !exclude {
nonECSImages = append(nonECSImages, image)
}
}
}
return nonECSImages
}

func isInExclusionList(imageName string, imageExclusionList []string) bool {
Expand Down Expand Up @@ -486,8 +494,9 @@ func (imageManager *dockerImageManager) imagesConsiderForDeletion(allImageStates
for _, imageState := range allImageStates {
if imageManager.isExcludedFromCleanup(imageState) {
//imageState that we want to keep
seelog.Infof("Image excluded from deletion: [%s]", imageState.String())
seelog.Debugf("Image excluded from deletion: [%s]", imageState.String())
} else {
seelog.Debugf("Image going to be considered for deletion: [%s]", imageState.String())
imagesConsiderForDeletionMap[imageState.Image.ImageID] = imageState
}
}
Expand Down
147 changes: 134 additions & 13 deletions agent/engine/docker_image_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1022,8 +1022,8 @@ func TestNonECSImageAndContainersCleanupRemoveImage(t *testing.T) {
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), gomock.Any(), dockerclient.RemoveContainerTimeout).Return(nil).Times(1)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.RepoTags[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.RepoTags[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()
Expand All @@ -1035,7 +1035,128 @@ func TestNonECSImageAndContainersCleanupRemoveImage(t *testing.T) {
assert.Len(t, imageManager.imageStates, 0, "Error removing image state after the image is removed")
}

// Dead images should be cleaned up.
func TestNonECSImageAndContainersCleanupRemoveImage_DontDeleteExcludedImage(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mock_dockerapi.NewMockDockerClient(ctrl)
imageManager := &dockerImageManager{
client: client,
state: dockerstate.NewTaskEngineState(),
minimumAgeBeforeDeletion: config.DefaultImageDeletionAge,
numImagesToDelete: config.DefaultNumImagesToDeletePerCycle,
numNonECSContainersToDelete: 10,
imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval,
deleteNonECSImagesEnabled: true,
nonECSContainerCleanupWaitDuration: time.Hour * 3,
imageCleanupExclusionList: []string{"tester"},
}
imageManager.SetSaver(statemanager.NewNoopStateManager())

listContainersResponse := dockerapi.ListContainersResponse{
DockerIDs: []string{"1"},
}

inspectContainerResponse := &types.ContainerJSON{
ContainerJSONBase: &types.ContainerJSONBase{
ID: "1",
State: &types.ContainerState{
Status: "exited",
FinishedAt: time.Now().AddDate(0, -2, 0).Format(time.RFC3339Nano),
},
},
}

inspectImageResponse := &types.ImageInspect{
Size: 4096,
RepoTags: []string{"tester"},
}

listImagesResponse := dockerapi.ListImagesResponse{
ImageIDs: []string{"sha256:qwerty1"},
RepoTags: []string{"tester"},
}

client.EXPECT().ListContainers(gomock.Any(), gomock.Any(), dockerclient.ListImagesTimeout).Return(listContainersResponse).AnyTimes()
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), gomock.Any(), dockerclient.RemoveContainerTimeout).Return(nil).Times(1)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(0)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

imageManager.removeUnusedImages(ctx)

assert.Len(t, listContainersResponse.DockerIDs, 1, "error removing container IDs")
assert.Len(t, inspectContainerResponse.ID, 1, "error inspecting containers ids")
assert.Len(t, imageManager.imageStates, 0, "Error removing image state after the image is removed")
}

func TestNonECSImageAndContainersCleanupRemoveImage_DontDeleteECSImages(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
client := mock_dockerapi.NewMockDockerClient(ctrl)
imageManager := &dockerImageManager{
client: client,
state: dockerstate.NewTaskEngineState(),
minimumAgeBeforeDeletion: config.DefaultImageDeletionAge,
numImagesToDelete: config.DefaultNumImagesToDeletePerCycle,
numNonECSContainersToDelete: 10,
imageCleanupTimeInterval: config.DefaultImageCleanupTimeInterval,
deleteNonECSImagesEnabled: true,
nonECSContainerCleanupWaitDuration: time.Hour * 3,
}
imageState := &image.ImageState{
Image: &image.Image{ImageID: "sha256:qwerty1"},
PulledAt: time.Now(),
}
imageManager.addImageState(imageState)
imageManager.SetSaver(statemanager.NewNoopStateManager())

listContainersResponse := dockerapi.ListContainersResponse{
DockerIDs: []string{"1"},
}

inspectContainerResponse := &types.ContainerJSON{
ContainerJSONBase: &types.ContainerJSONBase{
ID: "1",
State: &types.ContainerState{
Status: "exited",
FinishedAt: time.Now().AddDate(0, -2, 0).Format(time.RFC3339Nano),
},
},
}

inspectImageResponse := &types.ImageInspect{
Size: 4096,
RepoTags: []string{"tester"},
ID: "sha256:qwerty1",
}

listImagesResponse := dockerapi.ListImagesResponse{
ImageIDs: []string{"sha256:qwerty1"},
RepoTags: []string{"tester"},
}

client.EXPECT().ListContainers(gomock.Any(), gomock.Any(), dockerclient.ListImagesTimeout).Return(listContainersResponse).AnyTimes()
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), gomock.Any(), dockerclient.RemoveContainerTimeout).Return(nil).Times(1)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(0)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

imageManager.removeUnusedImages(ctx)

assert.Len(t, listContainersResponse.DockerIDs, 1, "error removing container IDs")
assert.Len(t, inspectContainerResponse.ID, 1, "error inspecting containers ids")
assert.Len(t, imageManager.imageStates, 1, "there should still be an image state")
}

// Dead containers should be cleaned up.
func TestNonECSImageAndContainers_RemoveDeadContainer(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -1079,8 +1200,8 @@ func TestNonECSImageAndContainers_RemoveDeadContainer(t *testing.T) {
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), gomock.Any(), dockerclient.RemoveContainerTimeout).Return(nil).Times(1)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.RepoTags[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.RepoTags[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()
Expand All @@ -1092,7 +1213,7 @@ func TestNonECSImageAndContainers_RemoveDeadContainer(t *testing.T) {
assert.Len(t, imageManager.imageStates, 0, "Error removing image state after the image is removed")
}

// Old 'Created' Images should be cleaned up
// Old 'Created' cotainers should be cleaned up
func TestNonECSImageAndContainersCleanup_RemoveOldCreatedContainer(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -1136,8 +1257,8 @@ func TestNonECSImageAndContainersCleanup_RemoveOldCreatedContainer(t *testing.T)
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), "1", gomock.Any()).Return(nil).Times(1)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.RepoTags[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.RepoTags[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()
Expand Down Expand Up @@ -1192,8 +1313,8 @@ func TestNonECSImageAndContainersCleanup_DontRemoveContainerWithInvalidFinishedT
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), "1", gomock.Any()).Return(nil).Times(0)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.RepoTags[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.RepoTags[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()
Expand All @@ -1205,7 +1326,7 @@ func TestNonECSImageAndContainersCleanup_DontRemoveContainerWithInvalidFinishedT
assert.Len(t, imageManager.imageStates, 0, "Error removing image state after the image is removed")
}

// New 'Created' Images should NOT be cleaned up.
// New 'Created' containers should NOT be cleaned up.
func TestNonECSImageAndContainersCleanup_DoNotRemoveNewlyCreatedContainer(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()
Expand Down Expand Up @@ -1249,8 +1370,8 @@ func TestNonECSImageAndContainersCleanup_DoNotRemoveNewlyCreatedContainer(t *tes
client.EXPECT().InspectContainer(gomock.Any(), gomock.Any(), dockerclient.InspectContainerTimeout).Return(inspectContainerResponse, nil).AnyTimes()
client.EXPECT().RemoveContainer(gomock.Any(), gomock.Any(), dockerclient.RemoveContainerTimeout).Return(nil).Times(0)
client.EXPECT().ListImages(gomock.Any(), dockerclient.ListImagesTimeout).Return(listImagesResponse).AnyTimes()
client.EXPECT().InspectImage(listImagesResponse.RepoTags[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.RepoTags[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)
client.EXPECT().InspectImage(listImagesResponse.ImageIDs[0]).Return(inspectImageResponse, nil).Times(1)
client.EXPECT().RemoveImage(gomock.Any(), listImagesResponse.ImageIDs[0], dockerclient.RemoveImageTimeout).Return(nil).Times(1)

ctx, cancel := context.WithCancel(context.TODO())
defer cancel()
Expand Down

0 comments on commit d3652ba

Please sign in to comment.