Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore images in used by running containers when GC #57020

Merged
merged 1 commit into from
Jan 10, 2018
Merged

ignore images in used by running containers when GC #57020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions pkg/kubelet/images/image_gc_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func (im *realImageGCManager) Start() {
if im.initialized {
ts = time.Now()
}
err := im.detectImages(ts)
_, err := im.detectImages(ts)
if err != nil {
glog.Warningf("[imageGCManager] Failed to monitor images: %v", err)
} else {
Expand All @@ -194,18 +194,19 @@ func (im *realImageGCManager) GetImageList() ([]kubecontainer.Image, error) {
return im.imageCache.get(), nil
}

func (im *realImageGCManager) detectImages(detectTime time.Time) error {
func (im *realImageGCManager) detectImages(detectTime time.Time) (sets.String, error) {
imagesInUse := sets.NewString()

images, err := im.runtime.ListImages()
if err != nil {
return err
return imagesInUse, err
}
pods, err := im.runtime.GetPods(true)
if err != nil {
return err
return imagesInUse, err
}

// Make a set of images in use by containers.
imagesInUse := sets.NewString()
for _, pod := range pods {
for _, container := range pod.Containers {
glog.V(5).Infof("Pod %s/%s, container %s uses image %s(%s)", pod.Namespace, pod.Name, container.Name, container.Image, container.ImageID)
Expand All @@ -231,7 +232,7 @@ func (im *realImageGCManager) detectImages(detectTime time.Time) error {
}

// Set last used time to now if the image is being used.
if isImageUsed(image, imagesInUse) {
if isImageUsed(image.ID, imagesInUse) {
glog.V(5).Infof("Setting Image ID %s lastUsed to %v", image.ID, now)
im.imageRecords[image.ID].lastUsed = now
}
Expand All @@ -248,7 +249,7 @@ func (im *realImageGCManager) detectImages(detectTime time.Time) error {
}
}

return nil
return imagesInUse, nil
}

func (im *realImageGCManager) GarbageCollect() error {
Expand Down Expand Up @@ -309,7 +310,7 @@ func (im *realImageGCManager) DeleteUnusedImages() (int64, error) {
// Note that error may be nil and the number of bytes free may be less
// than bytesToFree.
func (im *realImageGCManager) freeSpace(bytesToFree int64, freeTime time.Time) (int64, error) {
err := im.detectImages(freeTime)
imagesInUse, err := im.detectImages(freeTime)
if err != nil {
return 0, err
}
Expand All @@ -320,6 +321,10 @@ func (im *realImageGCManager) freeSpace(bytesToFree int64, freeTime time.Time) (
// Get all images in eviction order.
images := make([]evictionInfo, 0, len(im.imageRecords))
for image, record := range im.imageRecords {
if isImageUsed(image, imagesInUse) {
glog.V(5).Infof("Image ID %s is being used", image)
continue
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the impact of this earlier for a running container ? Did this cause the container to be killed if its image was GC'd ? If yes , why was it not detected until now ? Is the kubelet GC feature not enabled by default ?

Copy link
Member Author

@dixudx dixudx Dec 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this cause the container to be killed if its image was GC'd? If yes , why was it not detected until now ?

This function is to delete unused images to free some space. No containers will be killed since deleting a image that is referred by a running container is forbid. The containers won't become orphanage.

Is the kubelet GC feature not enabled by default ?

Image GC is enabled by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC, this behavior was previously enforced by the runtime, but now you're making the Kubelet enforce it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tallclair When kubelet is trying to free space by deleting docker images, images that are used by running containers are not skipped.

Even if we let runtime to handle this, these images should not be deleted.

What I am trying to do here is to skip these in-use images to avoid unnecessary overheads and spam log messages as well.

images = append(images, evictionInfo{
id: image,
imageRecord: *record,
Expand Down Expand Up @@ -385,9 +390,9 @@ func (ev byLastUsedAndDetected) Less(i, j int) bool {
}
}

func isImageUsed(image container.Image, imagesInUse sets.String) bool {
func isImageUsed(imageID string, imagesInUse sets.String) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we only need imageID. Also we only got imageID in ImageRecords.

Changing to imageID makes it more convenient for calling.

// Check the image ID.
if _, ok := imagesInUse[image.ID]; ok {
if _, ok := imagesInUse[imageID]; ok {
return true
}
return false
Expand Down
35 changes: 21 additions & 14 deletions pkg/kubelet/images/image_gc_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func TestDetectImagesInitialDetect(t *testing.T) {
}

startTime := time.Now().Add(-time.Millisecond)
err := manager.detectImages(zero)
_, err := manager.detectImages(zero)
assert := assert.New(t)
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 3)
Expand Down Expand Up @@ -145,7 +145,7 @@ func TestDetectImagesWithNewImage(t *testing.T) {
}},
}

err := manager.detectImages(zero)
_, err := manager.detectImages(zero)
assert := assert.New(t)
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 2)
Expand All @@ -159,7 +159,7 @@ func TestDetectImagesWithNewImage(t *testing.T) {

detectedTime := zero.Add(time.Second)
startTime := time.Now().Add(-time.Millisecond)
err = manager.detectImages(detectedTime)
_, err = manager.detectImages(detectedTime)
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 3)
noContainer, ok := manager.getImageRecord(imageID(0))
Expand Down Expand Up @@ -190,7 +190,7 @@ func TestDetectImagesContainerStopped(t *testing.T) {
}},
}

err := manager.detectImages(zero)
_, err := manager.detectImages(zero)
assert := assert.New(t)
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 2)
Expand All @@ -199,7 +199,7 @@ func TestDetectImagesContainerStopped(t *testing.T) {

// Simulate container being stopped.
fakeRuntime.AllPodList = []*containertest.FakePod{}
err = manager.detectImages(time.Now())
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 2)
container1, ok := manager.getImageRecord(imageID(0))
Expand All @@ -226,14 +226,14 @@ func TestDetectImagesWithRemovedImages(t *testing.T) {
}},
}

err := manager.detectImages(zero)
_, err := manager.detectImages(zero)
assert := assert.New(t)
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 2)

// Simulate both images being removed.
fakeRuntime.ImageList = []container.Image{}
err = manager.detectImages(time.Now())
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
assert.Equal(manager.imageRecordsLen(), 0)
}
Expand Down Expand Up @@ -297,21 +297,24 @@ func TestFreeSpaceRemoveByLeastRecentlyUsed(t *testing.T) {
}

// Make 1 be more recently used than 0.
require.NoError(t, manager.detectImages(zero))
_, err := manager.detectImages(zero)
require.NoError(t, err)
fakeRuntime.AllPodList = []*containertest.FakePod{
{Pod: &container.Pod{
Containers: []*container.Container{
makeContainer(1),
},
}},
}
require.NoError(t, manager.detectImages(time.Now()))
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
fakeRuntime.AllPodList = []*containertest.FakePod{
{Pod: &container.Pod{
Containers: []*container.Container{},
}},
}
require.NoError(t, manager.detectImages(time.Now()))
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
require.Equal(t, manager.imageRecordsLen(), 2)

spaceFreed, err := manager.freeSpace(1024, time.Now())
Expand All @@ -335,14 +338,17 @@ func TestFreeSpaceTiesBrokenByDetectedTime(t *testing.T) {
}

// Make 1 more recently detected but used at the same time as 0.
require.NoError(t, manager.detectImages(zero))
_, err := manager.detectImages(zero)
require.NoError(t, err)
fakeRuntime.ImageList = []container.Image{
makeImage(0, 1024),
makeImage(1, 2048),
}
require.NoError(t, manager.detectImages(time.Now()))
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
fakeRuntime.AllPodList = []*containertest.FakePod{}
require.NoError(t, manager.detectImages(time.Now()))
_, err = manager.detectImages(time.Now())
require.NoError(t, err)
require.Equal(t, manager.imageRecordsLen(), 2)

spaceFreed, err := manager.freeSpace(1024, time.Now())
Expand Down Expand Up @@ -448,7 +454,8 @@ func TestGarbageCollectImageNotOldEnough(t *testing.T) {

fakeClock := clock.NewFakeClock(time.Now())
t.Log(fakeClock.Now())
require.NoError(t, manager.detectImages(fakeClock.Now()))
_, err := manager.detectImages(fakeClock.Now())
require.NoError(t, err)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see any new unit tests added for the case you just fixed ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more tests are needed, since we've already got TestFreeSpaceImagesInUseContainersAreIgnored. This test does test the function.

What this pr does is to not delete images used by running containers and prevent spam event messages like image is being used by running container.

require.Equal(t, manager.imageRecordsLen(), 2)
// no space freed since one image is in used, and another one is not old enough
spaceFreed, err := manager.freeSpace(1024, fakeClock.Now())
Expand Down