ignore images in used by running containers when GC #57020
Conversation
k8s-ci-robot
added
release-note
|
/unassign @sjpotter |
| if isImageUsed(image, imagesInUse) { | ||
| glog.V(5).Infof("Image ID %s is being used", image) | ||
| continue | ||
| } |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So IIUC, this behavior was previously enforced by the runtime, but now you're making the Kubelet enforce it?
There was a problem hiding this comment.
@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.
| @@ -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) | |||
There was a problem hiding this comment.
i dont see any new unit tests added for the case you just fixed ?
There was a problem hiding this comment.
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.
|
/retest |
|
ping @tallclair @vishh PTAL. Thanks. |
|
ping @yujuhong PTAL. Thanks. |
| @@ -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 { | |||
There was a problem hiding this comment.
Here we only need imageID. Also we only got imageID in ImageRecords.
Changing to imageID makes it more convenient for calling.
| if isImageUsed(image, imagesInUse) { | ||
| glog.V(5).Infof("Image ID %s is being used", image) | ||
| continue | ||
| } |
There was a problem hiding this comment.
So IIUC, this behavior was previously enforced by the runtime, but now you're making the Kubelet enforce it?
|
Nice fix. Thanks! cc/ @dashpole /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, dixudx Associated issue: #57006 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
/test all Tests are more than 96 hours old. Re-running tests. |
|
Automatic merge from submit-queue (batch tested with PRs 57823, 58091, 58093, 58096, 57020). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Let kubelet not attempt to remove images being used by running containers.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #57006
Special notes for your reviewer:
@kubernetes/sig-node-pr-reviews
Release note: