Merge pull request #29388 from ronnielai/image-gc-check

Automatic merge from submit-queue

Avoiding trying to gc images with no tags which are still in use

#29325

pkg/kubelet/container/helpers.go

@@ -98,6 +98,7 @@ func ConvertPodStatusToRunningPod(podStatus *PodStatus) Pod {
 			ID:      containerStatus.ID,
 			Name:    containerStatus.Name,
 			Image:   containerStatus.Image,
+			ImageID: containerStatus.ImageID,
 			Hash:    containerStatus.Hash,
 			State:   containerStatus.State,
 		}

pkg/kubelet/container/runtime.go

@@ -245,6 +245,8 @@ type Container struct {
 	// The image name of the container, this also includes the tag of the image,
 	// the expected form is "NAME:TAG".
 	Image string
+	// The id of the image used by the container.
+	ImageID string
 	// Hash of the container, used for comparison. Optional for containers
 	// not managed by kubelet.
 	Hash uint64

pkg/kubelet/dockertools/convert.go

@@ -59,6 +59,7 @@ func toRuntimeContainer(c *dockertypes.Container) (*kubecontainer.Container, err
 		ID:      kubecontainer.DockerID(c.ID).ContainerID(),
 		Name:    dockerName.ContainerName,
 		Image:   c.Image,
+		ImageID: c.ImageID,
 		Hash:    hash,
 		// (random-liu) docker uses status to indicate whether a container is running or exited.
 		// However, in kubernetes we usually use state to indicate whether a container is running or exited,

pkg/kubelet/image_manager.go

@@ -170,8 +170,8 @@ func (im *realImageManager) detectImages(detectTime time.Time) error {
 	imagesInUse := sets.NewString()
 	for _, pod := range pods {
 		for _, container := range pod.Containers {
-			glog.V(5).Infof("Pod %s/%s, container %s uses image %s", pod.Namespace, pod.Name, container.Name, container.Image)
+			glog.V(5).Infof("Pod %s/%s, container %s uses image %s(%s)", pod.Namespace, pod.Name, container.Name, container.Image, container.ImageID)
-			imagesInUse.Insert(container.Image)
+			imagesInUse.Insert(container.ImageID)
 		}
 	}
 
@@ -341,14 +341,9 @@ func (ev byLastUsedAndDetected) Less(i, j int) bool {
 }
 
 func isImageUsed(image container.Image, imagesInUse sets.String) bool {
-	// Check the image ID and all the RepoTags and RepoDigests.
+	// Check the image ID.
 	if _, ok := imagesInUse[image.ID]; ok {
 		return true
 	}
-	for _, tag := range append(image.RepoTags, image.RepoDigests...) {
-		if _, ok := imagesInUse[tag]; ok {
-			return true
-		}
-	}
 	return false
 }

pkg/kubelet/image_manager_test.go

@@ -59,15 +59,20 @@ func (im *realImageManager) getImageRecord(name string) (*imageRecord, bool) {
 	return &vCopy, ok
 }
 
+// Returns the id of the image with the given ID.
+func imageID(id int) string {
+	return fmt.Sprintf("image-%d", id)
+}
+
 // Returns the name of the image with the given ID.
 func imageName(id int) string {
-	return fmt.Sprintf("image-%d", id)
+	return imageID(id) + "-name"
 }
 
 // Make an image with the specified ID.
 func makeImage(id int, size int64) container.Image {
 	return container.Image{
-		ID:   imageName(id),
+		ID:   imageID(id),
 		Size: size,
 	}
 }
@@ -77,6 +82,7 @@ func makeContainer(id int) *container.Container {
 	return &container.Container{
 		ID:      container.ContainerID{Type: "test", ID: fmt.Sprintf("container-%d", id)},
 		Image:   imageName(id),
+		ImageID: imageID(id),
 	}
 }
 
@@ -85,11 +91,21 @@ func TestDetectImagesInitialDetect(t *testing.T) {
 	fakeRuntime.ImageList = []container.Image{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
+		makeImage(2, 2048),
 	}
 	fakeRuntime.AllPodList = []*containertest.FakePod{
 		{Pod: &container.Pod{
 			Containers: []*container.Container{
-				makeContainer(1),
+				{
+					ID:      container.ContainerID{Type: "test", ID: fmt.Sprintf("container-%d", 1)},
+					ImageID: imageID(1),
+					// The image filed is not set to simulate a no-name image
+				},
+				{
+					ID:      container.ContainerID{Type: "test", ID: fmt.Sprintf("container-%d", 2)},
+					Image:   imageName(2),
+					ImageID: imageID(2),
+				},
 			},
 		}},
 	}
@@ -98,12 +114,16 @@ func TestDetectImagesInitialDetect(t *testing.T) {
 	err := manager.detectImages(zero)
 	assert := assert.New(t)
 	require.NoError(t, err)
-	assert.Equal(manager.imageRecordsLen(), 2)
+	assert.Equal(manager.imageRecordsLen(), 3)
-	noContainer, ok := manager.getImageRecord(imageName(0))
+	noContainer, ok := manager.getImageRecord(imageID(0))
 	require.True(t, ok)
 	assert.Equal(zero, noContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
-	withContainer, ok := manager.getImageRecord(imageName(1))
+	withContainerUsingNoNameImage, ok := manager.getImageRecord(imageID(1))
+	require.True(t, ok)
+	assert.Equal(zero, withContainerUsingNoNameImage.firstDetected)
+	assert.True(withContainerUsingNoNameImage.lastUsed.After(startTime))
+	withContainer, ok := manager.getImageRecord(imageID(2))
 	require.True(t, ok)
 	assert.Equal(zero, withContainer.firstDetected)
 	assert.True(withContainer.lastUsed.After(startTime))
@@ -141,15 +161,15 @@ func TestDetectImagesWithNewImage(t *testing.T) {
 	err = manager.detectImages(detectedTime)
 	require.NoError(t, err)
 	assert.Equal(manager.imageRecordsLen(), 3)
-	noContainer, ok := manager.getImageRecord(imageName(0))
+	noContainer, ok := manager.getImageRecord(imageID(0))
 	require.True(t, ok)
 	assert.Equal(zero, noContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
-	withContainer, ok := manager.getImageRecord(imageName(1))
+	withContainer, ok := manager.getImageRecord(imageID(1))
 	require.True(t, ok)
 	assert.Equal(zero, withContainer.firstDetected)
 	assert.True(withContainer.lastUsed.After(startTime))
-	newContainer, ok := manager.getImageRecord(imageName(2))
+	newContainer, ok := manager.getImageRecord(imageID(2))
 	require.True(t, ok)
 	assert.Equal(detectedTime, newContainer.firstDetected)
 	assert.Equal(zero, noContainer.lastUsed)
@@ -173,7 +193,7 @@ func TestDetectImagesContainerStopped(t *testing.T) {
 	assert := assert.New(t)
 	require.NoError(t, err)
 	assert.Equal(manager.imageRecordsLen(), 2)
-	withContainer, ok := manager.getImageRecord(imageName(1))
+	withContainer, ok := manager.getImageRecord(imageID(1))
 	require.True(t, ok)
 
 	// Simulate container being stopped.
@@ -181,11 +201,11 @@ func TestDetectImagesContainerStopped(t *testing.T) {
 	err = manager.detectImages(time.Now())
 	require.NoError(t, err)
 	assert.Equal(manager.imageRecordsLen(), 2)
-	container1, ok := manager.getImageRecord(imageName(0))
+	container1, ok := manager.getImageRecord(imageID(0))
 	require.True(t, ok)
 	assert.Equal(zero, container1.firstDetected)
 	assert.Equal(zero, container1.lastUsed)
-	container2, ok := manager.getImageRecord(imageName(1))
+	container2, ok := manager.getImageRecord(imageID(1))
 	require.True(t, ok)
 	assert.Equal(zero, container2.firstDetected)
 	assert.True(container2.lastUsed.Equal(withContainer.lastUsed))
@@ -331,62 +351,6 @@ func TestFreeSpaceTiesBrokenByDetectedTime(t *testing.T) {
 	assert.Len(fakeRuntime.ImageList, 1)
 }
 
-func TestFreeSpaceImagesAlsoDoesLookupByRepoTags(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
-		makeImage(0, 1024),
-		{
-			ID:       "5678",
-			RepoTags: []string{"potato", "salad"},
-			Size:     2048,
-		},
-	}
-	fakeRuntime.AllPodList = []*containertest.FakePod{
-		{Pod: &container.Pod{
-			Containers: []*container.Container{
-				{
-					ID:    container.ContainerID{Type: "test", ID: "c5678"},
-					Image: "salad",
-				},
-			},
-		}},
-	}
-
-	spaceFreed, err := manager.freeSpace(1024, time.Now())
-	assert := assert.New(t)
-	require.NoError(t, err)
-	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
-}
-
-func TestFreeSpaceImagesAlsoDoesLookupByRepoDigests(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
-		makeImage(0, 1024),
-		{
-			ID:          "5678",
-			RepoDigests: []string{"potato", "salad"},
-			Size:        2048,
-		},
-	}
-	fakeRuntime.AllPodList = []*containertest.FakePod{
-		{Pod: &container.Pod{
-			Containers: []*container.Container{
-				{
-					ID:    container.ContainerID{Type: "test", ID: "c5678"},
-					Image: "salad",
-				},
-			},
-		}},
-	}
-
-	spaceFreed, err := manager.freeSpace(1024, time.Now())
-	assert := assert.New(t)
-	require.NoError(t, err)
-	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
-}
-
 func TestGarbageCollectBelowLowThreshold(t *testing.T) {
 	policy := ImageGCPolicy{
 		HighThresholdPercent: 90,

pkg/kubelet/rkt/rkt.go

@@ -1529,6 +1529,7 @@ func (r *Runtime) convertRktPod(rktpod *rktapi.Pod) (*kubecontainer.Pod, error)
 			Name: app.Name,
 			// By default, the version returned by rkt API service will be "latest" if not specified.
 			Image:   fmt.Sprintf("%s:%s", app.Image.Name, app.Image.Version),
+			ImageID: app.Image.Id,
 			Hash:    containerHash,
 			State:   appStateToContainerState(app.State),
 		})

pkg/kubelet/rkt/rkt_test.go

@@ -377,6 +377,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4002:app-1"),
 							Name:    "app-1",
 							Image:   "img-name-1:latest",
+							ImageID: "img-id-1",
 							Hash:    1001,
 							State:   "running",
 						},
@@ -384,6 +385,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4002:app-2"),
 							Name:    "app-2",
 							Image:   "img-name-2:latest",
+							ImageID: "img-id-2",
 							Hash:    1002,
 							State:   "exited",
 						},
@@ -438,6 +440,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4002:app-1"),
 							Name:    "app-1",
 							Image:   "img-name-1:latest",
+							ImageID: "img-id-1",
 							Hash:    1001,
 							State:   "running",
 						},
@@ -445,6 +448,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4002:app-2"),
 							Name:    "app-2",
 							Image:   "img-name-2:latest",
+							ImageID: "img-id-2",
 							Hash:    1002,
 							State:   "exited",
 						},
@@ -459,6 +463,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4003:app-11"),
 							Name:    "app-11",
 							Image:   "img-name-11:latest",
+							ImageID: "img-id-11",
 							Hash:    10011,
 							State:   "exited",
 						},
@@ -466,6 +471,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4003:app-22"),
 							Name:    "app-22",
 							Image:   "img-name-22:latest",
+							ImageID: "img-id-22",
 							Hash:    10022,
 							State:   "exited",
 						},
@@ -473,6 +479,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4004:app-11"),
 							Name:    "app-11",
 							Image:   "img-name-11:latest",
+							ImageID: "img-id-11",
 							Hash:    10011,
 							State:   "running",
 						},
@@ -480,6 +487,7 @@ func TestGetPods(t *testing.T) {
 							ID:      kubecontainer.BuildContainerID("rkt", "uuid-4004:app-22"),
 							Name:    "app-22",
 							Image:   "img-name-22:latest",
+							ImageID: "img-id-22",
 							Hash:    10022,
 							State:   "running",
 						},