Don't remove pinned images on image prune #1360
Conversation
k8s-ci-robot
added
the
kind/feature
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @sondavidb! |
k8s-ci-robot
added
size/XS
cmd/crictl/image.go
Outdated
| // Pinned images should not be removed on prune. | ||
| if all || (prune && !img.Pinned) { | ||
| logrus.Debugf("Adding image to be removed: %v", img.GetId()) | ||
| ids[img.GetId()] = true | ||
| } |
There was a problem hiding this comment.
What about something like this?
| // Pinned images should not be removed on prune. | |
| if all || (prune && !img.Pinned) { | |
| logrus.Debugf("Adding image to be removed: %v", img.GetId()) | |
| ids[img.GetId()] = true | |
| } | |
| // Pinned images should not be removed on prune. | |
| if prune && img.Pinned { | |
| logrus.Debugf("Excluding pinned container image: %v", img.GetId()) | |
| continue | |
| } | |
| logrus.Debugf("Adding container image to be removed: %v", img.GetId()) | |
| ids[img.GetId()] = true |
I updated the wording to match other log messages.
There was a problem hiding this comment.
Makes sense, I think it would be helpful in case users are confused why certain images are not being removed. Updated PR to reflect these changes.
EDIT: Properly edited the PR to reflect these changes, sorry about that
sondavidb
force-pushed
the
dont-remove-pinned-images-on-prune
branch
from
b25828e
to
0413082
Compare
This fix also stops crictl from erreneously removing running pause containers. Signed-off-by: David Son <davbson@amazon.com>
sondavidb
force-pushed
the
dont-remove-pinned-images-on-prune
branch
from
0413082
to
af4707c
Compare
|
@sondavidb, looks good! For testing, have a look at the testes that were recently added as part of some other Pull Request. Perhaps it will give you an idea how to do it. This will cover unit tests, for end-to-end testes, it would be a bit more involved to do, albeit not impossible. However, manual testing is also fine. |
|
/approved |
|
@kwilczynski: changing LGTM is restricted to collaborators In response to this:
Instructions 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. |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: saschagrunert, sondavidb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/release-note-edit |
@saschagrunert, this is probably something only you could do. Unless it's too late already. |
Done 👍 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
On
cricrl rmi --prune, crictl ignores the pinned images label when removing images. Not only is this unexpected behavior, it also removes the pause container, because the container runtime endpoint doesn't return the pause container in its list of running containers.This change honors the pinned label on
cricrl rmi --prune, which also stops the pause container from being removed, as it is pinned in every workflow.Which issue(s) this PR fixes:
Fixes #1356
Special notes for your reviewer:
I'd like to add testing for this, but I haven't been able to figure out how to make it work. I just ran a simple workflow of pulling an image, pinning it, and then running
crictl rmi --prune.Does this PR introduce a user-facing change?