Add ctr image prune command#7730
Conversation
|
Hi @djdongjin. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
|
Thanks for review @mikebrow. Updated based on your comments. |
dcantah
left a comment
dcantah
left a comment
There was a problem hiding this comment.
Change looks fine, curious on one aspect
| for image, digests := range removedImages { | ||
| log.G(ctx).Infof("untagged: %s\n", image) | ||
| for _, digest := range digests { | ||
| log.G(ctx).Infof("deleted: %s\n", digest) |
There was a problem hiding this comment.
I don't think this can be assumed and not something that is surfaced to the client. With ctr I think its best to error on accuracy or skip the functionality. The containerd logs would accurately show what got removed during GC. I don't think it is worth it here to check each layer to see if it still exists.
There was a problem hiding this comment.
Thanks for review @dmcgowan. I removed the layer check/log here.
| if err != nil { | ||
| log.G(ctx).WithError(err).Warnf("failed to enumerate rootfs of image %s", image.Name) | ||
| } | ||
| if err := imageStore.Delete(ctx, image.Name, delOpts...); err != nil { |
There was a problem hiding this comment.
If the delete could be deferred to the next loop, SynchronousDelete option could be reserved for the last run. Synchronous delete can also be run for a lease as well. It is not a good pattern to do synchronous deletes in a loop though.
There was a problem hiding this comment.
not sure if it helps .. but here is the code in CRI for removing an image:
There was a problem hiding this comment.
Thanks Derek and Mike :) Yes the code is helpful! I've updated the logic to match the CRI logic.
I used []string to store removed image names which assumes image.Name returned from imageStore.List(ctx) is unique. If it doesn't hold I can change to use map[string]interface{}.
Another thing is I notice the CRI code has the Update if Delete succeeds logic: https://github.com/containerd/containerd/blame/main/pkg/cri/server/image_remove.go#L58-L64
Do we need the same logic here? Thanks
There was a problem hiding this comment.
Do we need the same logic here? Thanks
There is no equivalent image store cache outside of CRI and we are working on removing that image store cache so that we don't have to do that sort of synchronization and operations outside of CRI are reflected better through CRI.
There was a problem hiding this comment.
Hi Derek, thanks for the explanation! I've updated the logic to using SynchronousDelete only for the last run (and ignore IsNotFound err).
djdongjin
force-pushed
the
ctr-prune
branch
3 times, most recently
from
08e4312 to
66595da
Compare
| if i == len(removedImages)-1 { | ||
| delOpts = []images.DeleteOpt{images.SynchronousDelete()} | ||
| } | ||
| if err := imageStore.Delete(ctx, imageName, delOpts...); err != nil { |
There was a problem hiding this comment.
errdefs.IsNotFound(err) should be ignored here... because async deletes.. though a simple warning probably doesn't hurt...
There was a problem hiding this comment.
thanks for the reminder Mike. Added errdefs.IsNotFound(err) check :)
Signed-off-by: Jin Dong <[email protected]>
6 participants
Fix #5844.
The implementation is based on
nerdctl image prune.It deletes unused images and logs deleted images and their digests.
Although
nerdctlis recommended for daily usage,ctr image prunemight still helpful for debugging to easily clean up leftover images.Signed-off-by: Jin Dong [email protected]