let image cache do sort on write instead of on read to avoid data

race and improve efficienty

Author: roycaihw

pkg/kubelet/images/image_gc_manager.go

@@ -113,21 +113,26 @@ type imageCache struct {
 	images []container.Image
 }
 
-// set updates image cache.
+// set sorts the input list and updates image cache.
+// 'i' takes ownership of the list, you should not reference the list again
+// after calling this function.
 func (i *imageCache) set(images []container.Image) {
 	i.Lock()
 	defer i.Unlock()
+	// The image list needs to be sorted when it gets read and used in
+	// setNodeStatusImages. We sort the list on write instead of on read,
+	// because the image cache is more often read than written
+	sort.Sort(sliceutils.ByImageSize(images))
 	i.images = images
 }
 
-// get gets a sorted (by image size) image list from image cache.
-// There is a potentical data race in this function. See PR #60448
-// Because there is deepcopy function available currently, move sort
-// function inside this function
+// get gets image list from image cache.
+// NOTE: The caller of get() should not do mutating operations on the
+// returned list that could cause data race against other readers (e.g.
+// in-place sorting the returned list)
 func (i *imageCache) get() []container.Image {
 	i.Lock()
 	defer i.Unlock()
-	sort.Sort(sliceutils.ByImageSize(i.images))
 	return i.images
 }
 

pkg/kubelet/images/image_gc_manager_test.go

@@ -548,16 +548,6 @@ func TestValidateImageGCPolicy(t *testing.T) {
 	}
 }
 
-func TestImageCacheReturnCopiedList(t *testing.T) {
-	cache := &imageCache{}
-	testList := []container.Image{{ID: "1"}, {ID: "2"}}
-	cache.set(testList)
-	list := cache.get()
-	assert.Len(t, list, 2)
-	list[0].ID = "3"
-	assert.Equal(t, cache.get(), testList)
-}
-
 func uint64Ptr(i uint64) *uint64 {
 	return &i
 }

pkg/kubelet/nodestatus/setters.go

@@ -446,7 +446,9 @@ func Images(nodeStatusMaxImages int32,
 		}
 
 		for _, image := range containerImages {
-			names := append(image.RepoDigests, image.RepoTags...)
+			// make a copy to avoid modifying slice members of the image items in the list
+			names := append([]string{}, image.RepoDigests...)
+			names = append(names, image.RepoTags...)
 			// Report up to MaxNamesPerImageInNodeStatus names per image.
 			if len(names) > MaxNamesPerImageInNodeStatus {
 				names = names[0:MaxNamesPerImageInNodeStatus]