diff --git a/pkg/kubelet/images/image_gc_manager.go b/pkg/kubelet/images/image_gc_manager.go index be6689943b3..f625cac20d7 100644 --- a/pkg/kubelet/images/image_gc_manager.go +++ b/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 } diff --git a/pkg/kubelet/images/image_gc_manager_test.go b/pkg/kubelet/images/image_gc_manager_test.go index 523baae94cc..c74e1185640 100644 --- a/pkg/kubelet/images/image_gc_manager_test.go +++ b/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 } diff --git a/pkg/kubelet/nodestatus/setters.go b/pkg/kubelet/nodestatus/setters.go index 5b94047ada0..ef535bc72bd 100644 --- a/pkg/kubelet/nodestatus/setters.go +++ b/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]