Don't remove pinned images on image prune #1360

sondavidb · 2024-02-22T23:13:36Z

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?

crictl rmi will not remove pinned images on --prune

linux-foundation-easycla · 2024-02-22T23:13:39Z

The committers listed above are authorized under a signed CLA.

✅ login: sondavidb / name: David Son (af4707c)

k8s-ci-robot · 2024-02-22T23:13:44Z

Welcome @sondavidb!

It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

kwilczynski · 2024-02-23T09:42:57Z

cmd/crictl/image.go

+				// 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
+				}


What about something like this?

Suggested change

// 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.

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

This fix also stops crictl from erreneously removing running pause containers. Signed-off-by: David Son <[email protected]>

kwilczynski · 2024-02-24T07:32:16Z

@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.

kwilczynski · 2024-02-24T07:32:37Z

/approved
/lgtm

k8s-ci-robot · 2024-02-24T07:32:40Z

@kwilczynski: changing LGTM is restricted to collaborators

In response to this:

/approved
/lgtm

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 · 2024-02-26T08:07:58Z

[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:

~~OWNERS~~ [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kwilczynski · 2024-02-26T09:27:12Z

/release-note-edit crictl rmi will not remove pinned images on --prune

kwilczynski · 2024-02-28T08:26:23Z

/release-note-edit crictl rmi will not remove pinned images on --prune

@saschagrunert, this is probably something only you could do. Unless it's too late already.

saschagrunert · 2024-02-28T08:31:53Z

/release-note-edit crictl rmi will not remove pinned images on --prune

@saschagrunert, this is probably something only you could do. Unless it's too late already.

Done 👍

wangyuan0916 · 2024-10-23T08:40:33Z

Is there any plan to cherry pick this fix?/

kwilczynski · 2024-10-23T08:42:57Z

Is there any plan to cherry pick this fix?/

@wangyuan0916, cherry-pick to which version?

You should be able to pick up latest version of the CLI without any issues, it should work with older versions of CRI-O and such, given that we implement the specification and such.

What issues are you having?

wangyuan0916 · 2024-10-23T08:48:46Z

Hi @kwilczynski , thank you for your reply. We are in kubernetes 1.28.x. I check the cri-tools README about the Compatibility matrix and find that the kubernetes and cri-tool minor versions should be the same. So can I use the latest cri-tool with kubernetes 1.28.x? Or is it better to get this fix backported?

kwilczynski · 2024-10-23T09:10:31Z

[...]

So can I use the latest cri-tool with kubernetes 1.28.x? Or is it better to get this fix backported?

@wangyuan0916, try. 😄

If things work, then you get the latest version with all the bug fixes and new features. If there are issues, then we can think of a backport.

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 22, 2024

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 22, 2024

k8s-ci-robot requested review from feiskyer and mrunalp February 22, 2024 23:13

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 22, 2024

kwilczynski reviewed Feb 23, 2024

View reviewed changes

sondavidb force-pushed the dont-remove-pinned-images-on-prune branch from b25828e to 0413082 Compare February 23, 2024 18:07

Don't remove pinned images on image prune

af4707c

This fix also stops crictl from erreneously removing running pause containers. Signed-off-by: David Son <[email protected]>

sondavidb force-pushed the dont-remove-pinned-images-on-prune branch from 0413082 to af4707c Compare February 23, 2024 23:38

saschagrunert approved these changes Feb 26, 2024

View reviewed changes

k8s-ci-robot assigned saschagrunert Feb 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2024

k8s-ci-robot merged commit f638383 into kubernetes-sigs:master Feb 26, 2024
23 checks passed

sondavidb deleted the dont-remove-pinned-images-on-prune branch February 27, 2024 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't remove pinned images on image prune #1360

Don't remove pinned images on image prune #1360

sondavidb commented Feb 22, 2024 •

edited by saschagrunert

Loading

linux-foundation-easycla bot commented Feb 22, 2024 •

edited

Loading

k8s-ci-robot commented Feb 22, 2024

kwilczynski Feb 23, 2024

sondavidb Feb 23, 2024 •

edited

Loading

kwilczynski commented Feb 24, 2024

kwilczynski commented Feb 24, 2024

k8s-ci-robot commented Feb 24, 2024

k8s-ci-robot commented Feb 26, 2024

kwilczynski commented Feb 26, 2024

kwilczynski commented Feb 28, 2024

saschagrunert commented Feb 28, 2024

wangyuan0916 commented Oct 23, 2024

kwilczynski commented Oct 23, 2024

wangyuan0916 commented Oct 23, 2024

kwilczynski commented Oct 23, 2024

Don't remove pinned images on image prune #1360

Don't remove pinned images on image prune #1360

Conversation

sondavidb commented Feb 22, 2024 • edited by saschagrunert Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Feb 22, 2024 • edited Loading

k8s-ci-robot commented Feb 22, 2024

kwilczynski Feb 23, 2024

Choose a reason for hiding this comment

sondavidb Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

kwilczynski commented Feb 24, 2024

kwilczynski commented Feb 24, 2024

k8s-ci-robot commented Feb 24, 2024

k8s-ci-robot commented Feb 26, 2024

kwilczynski commented Feb 26, 2024

kwilczynski commented Feb 28, 2024

saschagrunert commented Feb 28, 2024

wangyuan0916 commented Oct 23, 2024

kwilczynski commented Oct 23, 2024

wangyuan0916 commented Oct 23, 2024

kwilczynski commented Oct 23, 2024

sondavidb commented Feb 22, 2024 •

edited by saschagrunert

Loading

linux-foundation-easycla bot commented Feb 22, 2024 •

edited

Loading

sondavidb Feb 23, 2024 •

edited

Loading