engine: move default image exclusions

This commit moves the logic adding default excluded images from the
cleanup list to the engine package, away from the config package, and in
doing so fixes two bugs:

1. Prior to this change, any value for ImageCleanupExclusionList
   provided by a config mechanism *other* than environmentConfig would
   be ignored.  This is because environmentConfig has the highest
   precedence, the defaults were added to environmentConfig by
   parseImageCleanupExclusionList, and config.Merge will only merge a
   new value when the left config's field is its zero value.  Since the
   default excluded images were populated, environmentConfig's
   ImageCleanupExclusionList field was never zero.
2. CachedImageNamePauseContainer hard-coded a name for the pause
   container image that was used to populate the exclusion list, but the
   actual name of the pause container is a value populated at link-time
   into the DefaultPauseContainerImageName and DefaultPauseContainerTag
   variables.  If the value was set to anything other than what was
   defined in CachedImageNamePauseContainer, the pause container image
   would not be correctly excluded from image cleanup.

Signed-off-by: Samuel Karp <[email protected]>

agent/config/config.go

@@ -132,7 +132,6 @@ const (
 	DefaultTaskMetadataBurstRate = 60
 
 	//Known cached image names
-	CachedImageNamePauseContainer = "amazon/amazon-ecs-pause:0.1.0"
 	CachedImageNameAgentContainer = "amazon/amazon-ecs-agent:latest"
 
 	// DefaultNvidiaRuntime is the name of the runtime to pass Nvidia GPUs to containers

agent/config/config_test.go

@@ -419,7 +419,7 @@ func TestValidForImagesCleanupExclusion(t *testing.T) {
 	defer setTestRegion()()
 	defer setTestEnv("ECS_EXCLUDE_UNTRACKED_IMAGE", "amazonlinux:2,amazonlinux:3")()
 	imagesNotDelete := parseImageCleanupExclusionList("ECS_EXCLUDE_UNTRACKED_IMAGE")
-	expectedImages := []string{"amazonlinux:2", "amazonlinux:3", CachedImageNameAgentContainer, CachedImageNamePauseContainer}
+	expectedImages := []string{"amazonlinux:2", "amazonlinux:3"}
 	assert.Equal(t, expectedImages, imagesNotDelete, "unexpected imageCleanupExclusionList")
 }
 

agent/config/parse.go

@@ -309,16 +309,11 @@ func parseImageCleanupExclusionList(envVar string) []string {
 	var imageCleanupExclusionList []string
 	if imageEnv == "" {
 		seelog.Debugf("Environment variable empty: %s", imageEnv)
+		return nil
 	} else {
 		imageCleanupExclusionList = strings.Split(imageEnv, ",")
 	}
 
-	// append known cached internal images to imageCleanupExclusionLis
-	imageCleanupExclusionList = append(imageCleanupExclusionList, CachedImageNameAgentContainer, CachedImageNamePauseContainer)
-
-	for _, image := range imageCleanupExclusionList {
-		seelog.Infof("Image excluded from cleanup: %s", image)
-	}
 	return imageCleanupExclusionList
 }
 

agent/engine/docker_image_manager.go

@@ -81,14 +81,27 @@ func NewImageManager(cfg *config.Config, client dockerapi.DockerClient, state do
 		numImagesToDelete:                  cfg.NumImagesToDeletePerCycle,
 		imageCleanupTimeInterval:           cfg.ImageCleanupInterval,
 		imagePullBehavior:                  cfg.ImagePullBehavior,
-		imageCleanupExclusionList:          cfg.ImageCleanupExclusionList,
+		imageCleanupExclusionList:          buildImageCleanupExclusionList(cfg),
 		deleteNonECSImagesEnabled:          cfg.DeleteNonECSImagesEnabled,
 		nonECSContainerCleanupWaitDuration: cfg.TaskCleanupWaitDuration,
 		numNonECSContainersToDelete:        cfg.NumNonECSContainersToDeletePerCycle,
 		nonECSMinimumAgeBeforeDeletion:     cfg.NonECSMinimumImageDeletionAge,
 	}
 }
 
+func buildImageCleanupExclusionList(cfg *config.Config) []string {
+	// append known cached internal images to imageCleanupExclusionList
+	excludedImages := append(cfg.ImageCleanupExclusionList,
+		cfg.PauseContainerImageName+":"+cfg.PauseContainerTag,
+		config.DefaultPauseContainerImageName+":"+config.DefaultPauseContainerTag,
+		config.CachedImageNameAgentContainer,
+	)
+	for _, image := range excludedImages {
+		seelog.Infof("Image excluded from cleanup: %s", image)
+	}
+	return excludedImages
+}
+
 func (imageManager *dockerImageManager) SetSaver(stateManager statemanager.Saver) {
 	imageManager.saver = stateManager
 }

agent/engine/docker_image_manager_test.go

@@ -44,6 +44,23 @@ func defaultTestConfig() *config.Config {
 	return cfg
 }
 
+func TestNewImageManagerExcludesCachedImages(t *testing.T) {
+	cfg := defaultTestConfig()
+	cfg.PauseContainerImageName = "pause-name"
+	cfg.PauseContainerTag = "pause-tag"
+	cfg.ImageCleanupExclusionList = []string{"excluded:1"}
+	expected := []string{
+		"excluded:1",
+		"pause-name:pause-tag",
+		config.DefaultPauseContainerImageName + ":" + config.DefaultPauseContainerTag,
+		config.CachedImageNameAgentContainer,
+	}
+	imageManager := NewImageManager(cfg, nil, nil)
+	dockerImageManager, ok := imageManager.(*dockerImageManager)
+	require.True(t, ok, "imageManager must be *dockerImageManager")
+	assert.ElementsMatch(t, expected, dockerImageManager.imageCleanupExclusionList)
+}
+
 // TestImagePullRemoveDeadlock tests if there's a deadlock when trying to
 // pull an image while image clean up is in progress
 func TestImagePullRemoveDeadlock(t *testing.T) {