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

Jump to bottom

Fix duplicate image entries in k8s.io namespaces #3744

Merged
merged 1 commit into from
Jan 5, 2025
Merged

Fix duplicate image entries in k8s.io namespaces #3744

merged 1 commit into from
Jan 5, 2025

Conversation

fengwei0328
Copy link
Contributor

@fengwei0328 fengwei0328 commented Dec 10, 2024 •
edited
Loading

AkihiroSuda
AkihiroSuda requested changes Dec 10, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

This should be opt-in via a config entry like --kube-hide-dupe:
https://github.com/containerd/nerdctl/blob/main/docs/config.md

@fengwei0328
Copy link
Contributor Author

This should be opt-in via a config entry like --kube-hide-dupe: https://github.com/containerd/nerdctl/blob/main/docs/config.md

Okay, I'll try to implement it. Initially, I wanted to implement it separately, but this is a preliminary version for the sake of testing and discussion.

AkihiroSuda
AkihiroSuda reviewed Dec 10, 2024
View reviewed changes
pkg/cmd/image/list.go Outdated Show resolved Hide resolved
@fengwei0328 fengwei0328 force-pushed the dev1 branch 7 times, most recently from bf3dfaa to 9851575 Compare December 17, 2024 13:17
AkihiroSuda
AkihiroSuda reviewed Dec 17, 2024
View reviewed changes
pkg/cmd/image/list.go Outdated Show resolved Hide resolved
AkihiroSuda
AkihiroSuda reviewed Dec 17, 2024
View reviewed changes
pkg/cmd/image/list.go Outdated Show resolved Hide resolved
@fengwei0328 fengwei0328 force-pushed the dev1 branch 2 times, most recently from d5c7e2f to 086f530 Compare December 18, 2024 03:48
AkihiroSuda
AkihiroSuda reviewed Dec 18, 2024
View reviewed changes
docs/config.md Outdated Show resolved Hide resolved
@fengwei0328 fengwei0328 force-pushed the dev1 branch 2 times, most recently from 7adb2e8 to ef8e002 Compare December 18, 2024 08:32
@AkihiroSuda AkihiroSuda mentioned this pull request Dec 18, 2024
Open
@fengwei0328 fengwei0328 force-pushed the dev1 branch 3 times, most recently from 3c93355 to 57b1f61 Compare December 18, 2024 09:28
@fengwei0328 fengwei0328 force-pushed the dev1 branch 2 times, most recently from 63d6499 to 8da215c Compare December 19, 2024 09:06
apostasie
apostasie suggested changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@apostasie apostasie left a comment

Choose a reason for hiding this comment

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

Thanks a lot @fengwei0328

I left a few comments.

@fengwei0328 fengwei0328 force-pushed the dev1 branch 6 times, most recently from 7173cb6 to 191cf70 Compare December 23, 2024 10:47
@apostasie
Copy link
Contributor

apostasie commented Dec 23, 2024 •
edited
Loading

@fengwei0328

Maybe we could have an extra test for the following case?

nerdctl pull busybox
nerdctl tag busybox foo
id="$(nerdctl images busybox -q)"

Then

nerdctl rmi -f "$id"

Versus

nerdctl --kube-hide-dupe rmi -f "$id"

And confirm that in both cases, all images with that id are correctly removed?

And the same test, this time with id=$(nerdctl images debian -q --no-trunc)

And the same test, this time with id=$(nerdctl images busybox -q --no-trunc)

wdyt?

@fengwei0328
Copy link
Contributor Author

fengwei0328 commented Dec 24, 2024 •
edited
Loading

Maybe we could have an extra test for the following case?

Ok, that's also I do manually every time I'm locally
There is only fluent/fluentd:v1.17.0-debian-1.0 here. I don't think it's using this，is this to verify the deletion of a non-existent image?

@apostasie
Copy link
Contributor

debian

Sorry, I definitely meant busybox, not debian.

The point is to verify that force-deleting by short id, and digest, is still working as expected, and will delete images with multiple tags.

@fengwei0328
Copy link
Contributor Author

debian

Sorry, I definitely meant busybox, not debian.

The point is to verify that force-deleting by short id, and digest, is still working as expected, and will delete images with multiple tags.

short id and digest tests added

@apostasie
Copy link
Contributor

Thanks a lot @fengwei0328

One last comment on making sure the root test is NoParallel, LGTM otherwise.

Then, to maintainers.

@fengwei0328 fengwei0328 force-pushed the dev1 branch 3 times, most recently from 803ce63 to cc1f568 Compare December 24, 2024 08:47
@fengwei0328
Fix duplicate image entries in k8s.io namespaces
eaa8df0 
The same imageId underk8s.io is showing multiple results: repo:tag, repo:digest, configID.
We expect to display only repo:tag, consistent with other namespaces and CRI.
		e.g.
		nerdctl -n k8s.io images
		REPOSITORY    TAG       IMAGE ID        CREATED        PLATFORM       SIZE         BLOB SIZE
		centos        7         be65f488b776    3 hours ago    linux/amd64    211.5 MiB    72.6 MiB
		centos        <none>    be65f488b776    3 hours ago    linux/amd64    211.5 MiB    72.6 MiB
		<none>        <none>    be65f488b776    3 hours ago    linux/amd64    211.5 MiB    72.6 MiB

		expect:
		nerdctl --kube-hide-dupe -n k8s.io images
		REPOSITORY    TAG       IMAGE ID        CREATED        PLATFORM       SIZE         BLOB SIZE
		centos        7         be65f488b776    3 hours ago    linux/amd64    211.5 MiB    72.6 MiB
Of course, even after deduplicating the images displayed, there are still issues with deleting the images.
It is necessary to distinguish between repo:tag and configId, as well as repoDigest. Considering the situation with tags,
we need to ensure that all repo:tags under the same imageId are cleaned up before proceeding to clean up the configId and repoDigest.

see: containerd#3702

Signed-off-by: fengwei0328 <feng.wei8@zte.com.cn>
@fengwei0328
Copy link
Contributor Author

Thanks a lot @fengwei0328

One last comment on making sure the root test is NoParallel, LGTM otherwise.

Then, to maintainers.

Thanks a lot @apostasie

@AkihiroSuda AkihiroSuda added this to the v2.0.3 milestone Dec 25, 2024
AkihiroSuda
AkihiroSuda approved these changes Jan 5, 2025
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 84fbcf8 into containerd:main Jan 5, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apostasie apostasie apostasie approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

Assignees
No one assigned
Labels
area/kubernetes
Projects
None yet
Milestone
v2.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@fengwei0328 @apostasie @AkihiroSuda