parsing ref consistently for refCache, including adding `docker.io/library` prefix by pacoxu · Pull Request #7728 · containerd/containerd

pacoxu · 2022-11-28T09:00:12Z

For the refCache below, the ref needs to be consistent.

There is a test case in #7698, I can quickly reproduce it with the second method there.

Before restarting and importing image, the refCahe is using ref: import-2022-11-28@sha256:c8a01d8610c642f0e6b83af5f6185b3eed34e322354be5a515da1a21453ac966" and crictl cannot remove it correctly.

After restarting containerd, the cache became ref: docker.io/library/import-2022-11-28@sha256:c8a01d8610c642f0e6b83af5f6185b3eed34e322354be5a515da1a21453ac966" and crictl can remove it correctly.

I add another cleanup in services/images/local.go without docker.io/library/ prefix.

see parsing ref consistently for refCache, including adding docker.io/library prefix #7728 (comment)

k8s-ci-robot · 2022-11-28T09:00:22Z

Hi @pacoxu. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

pacoxu · 2022-11-28T11:24:34Z

/cc @pmengelbert
for the issue

/cc @mxpv @dmcgowan
for you are recent contributors pkg/cri/store/image/image.go

k8s-ci-robot · 2022-11-28T11:24:37Z

@pacoxu: GitHub didn't allow me to request PR reviews from the following users: pmengelbert.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @pmengelbert
for the issue

/cc @mxpv @dmcgowan
for you are recent contributors pkg/cri/store/image/image.go

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.

pacoxu · 2022-11-29T04:06:11Z

The failure seems to be unrelated.

https://github.com/containerd/containerd/pull/7728/checks?check_run_id=9742873492

sozercan · 2022-12-01T18:52:44Z

/cc @cpuguy83

ruiwen-zhao · 2022-12-02T23:39:40Z

Mostly for my own education, can you help me understand why restarting containerd changed the ref in the cache? Is another path of code triggered upon containerd start?

pacoxu · 2022-12-05T05:37:58Z

Mostly for my own education, can you help me understand why restarting containerd changed the ref in the cache? Is another path of code triggered upon containerd start?

As I described in #7728 (comment), when we first import it with ctr, ref cache add cache with prefix.

Or we can fix it during import only?

ruiwen-zhao · 2022-12-06T06:40:30Z

Mostly for my own education, can you help me understand why restarting containerd changed the ref in the cache? Is another path of code triggered upon containerd start?

As I described in #7728 (comment), when we first import it with ctr, ref cache add cache with prefix.

Or we can fix it during import only?

Sorry maybe I am misunderstanding the issue here. According to #7698 and section "II. Manually load an image with --digests=true", the issue seems to be that crictl only removes repoTags but fails to remove the actual image, which seems to be problematic because cri attempts to remove all refereneces [1]. Is my understanding correct? If so then we should try to fix cri's image removal, no?

[1] https://github.com/containerd/containerd/blob/main/pkg/cri/server/image_remove.go#L49

pacoxu · 2022-12-14T11:57:43Z

I did some tests with some more logs.

The first crictl rmi $imgeid shows all ref removing are triggered. But import image is not deleted indeed.

12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.735311390+08:00" level=debug msg="image remove i 0,ref docker.io/library/hello:world"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.735862343+08:00" level=debug msg="image remove Delete err <nil>"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.735974214+08:00" level=debug msg="image remove i 1,ref docker.io/library/import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.736032431+08:00" level=debug msg="image remove Delete err image \"docker.io/library/import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59\": not found"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.736090162+08:00" level=debug msg="image remove i 2,ref docker.io/library/import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.736120975+08:00" level=debug msg="image remove Delete err image \"docker.io/library/import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59\": not found"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.736145294+08:00" level=debug msg="image remove i 3,ref sha256:c55b4d09bd7bbc82fec32fe98a363c99f476cc6c5eaed89968aaadc1f84e443e"
12月 14 17:46:15 paco-centos-9 containerd[4192410]: time="2022-12-14T17:46:15.744530837+08:00" level=debug msg="image remove Delete err <nil>"

After restarting containerd, the removing is done when runnging crictl rmi $imgeid.

12月 14 17:34:03 paco-centos-9 containerd[4192410]: time="2022-12-14T17:34:03.709711741+08:00" level=debug msg="Loaded image \"import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59\""
12月 14 17:34:16 paco-centos-9 containerd[4192410]: time="2022-12-14T17:34:16.171106366+08:00" level=debug msg="image remove i 0,ref import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59"
12月 14 17:34:16 paco-centos-9 containerd[4192410]: time="2022-12-14T17:34:16.171214731+08:00" level=debug msg="delete image" name="import-2022-12-14@sha256:a0010348fb08fdacb9c3fbe798e18d20c19e98aeb3c3bf7c471279f63ab1ee59"

pacoxu · 2022-12-14T11:59:30Z

It seems that the local image removing is without docker.io/library/ prefix.

BTW,

nerdctl inspect shows no docker.io/library/ prefix.
crictl inspect shows docker.io/library/ prefix.
ctr i ls shows no docker.io/library/ prefix.

pacoxu · 2022-12-14T12:17:43Z

So I add another cleanup in services/images/local.go without docker.io/library/ prefix.

kzys · 2022-12-23T18:32:37Z


 	if err := l.store.Delete(ctx, req.Name); err != nil {
-		return nil, errdefs.ToGRPC(err)
+		if strings.HasPrefix(req.Name, "docker.io/library/") {


I'm unsure about this change. This service doesn't do any caching. Does it have to worry about the prefix?

The reason is related to

nerdctl inspect shows no docker.io/library/ prefix.

crictl inspect shows docker.io/library/ prefix.

ctr i ls shows no docker.io/library/ prefix.

When the user removes an image thought crictl, it adds a prefix for it by default.

So I have to double-check to delete the image without the prefix here as well.

If docker.io/library/xxx:yyy equals xxx:yyy, this logic change will have no side effect.

I'm tempted to say: don't strip it (unless this is causing severe issues). Stripping the docker.io/library (and docker.io/) prefixes mostly originates from remaining the UX on docker, which (when there was only 1 registry: the docker index (later "docker hub")), so images had no prefix. ctr probably strips the prefix to have a closer match with docker commands, but for most cases (and APIs), this should probably be considered part of the presentation, not what APIs returns, and make sure the API returns the canonical (full) image reference.

Adding to the above; the docker.io prefix as a whole is causing a lot of headaches (e.g., docker.io is not actually the domain of the registry (which currently is at registry-1.docker.com). We're actively exploring what options we have to get out of this confusing situation (but it's complicated especially to remain backward compatible with existing container runtimes/tools). From that perspective, it'd be good to not introduce more places where special handling is performed (unless there's a strong reason it's needed).

/cc @ingshtrom (FYI)

(for others: @ingshtrom is one of my colleagues at docker exploring options around this 😅)

nerdctl inspect shows no docker.io/library/ prefix.

crictl inspect shows docker.io/library/ prefix.

ctr i ls shows no docker.io/library/ prefix.

Another way to fix the issue mentioned is to make crictl not adding the prefix. I need to look into the code if this is doable.

nerdctl is designed to provide a UX that matches docker. It should probably be looked at where that UX should be applied. Containerd in itself is (should be) less opinionated, which may mean: always assume canonical references. Some amount of compatibility is probably OK (which may mean convert to (normalise to) a canonical reference on the way in, and leave it to products on top of containerd to decide on the UX they want to provide (which may be "remove the prefix" when presenting).

We should really be careful not to leak docker's UX into containerd.

I saved the original change in https://github.com/containerd/containerd/compare/main...pacoxu:image-clean-original?expand=1.

I reverted the docker.io/library trim in services/images/local.go. And I will look into crictl code to see if it can be optimized.

pacoxu · 2023-01-15T14:49:50Z

TestContainerPids failed. It seems unrelated.
https://github.com/containerd/containerd/pull/7728/checks?check_run_id=10658354511

thaJeztah · 2023-01-15T14:59:10Z

 // the image does not exist in containerd.
 func (s *Store) update(ref string, img *Image) error {
+	// parsing ref consistently for refCache, including adding `docker.io/library` prefix
+	if !strings.HasPrefix(ref, "sha256:") {


Note that technically the OCI also allows other digest formats (sha512, sha384, and optionally others);

https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests

https://github.com/opencontainers/distribution-spec/blob/f72e5010d8441a94de8b35ebbe505b827d05fee2/spec.md?plain=1#L663

https://github.com/opencontainers/go-digest/blob/b9e02e015be61903bbee58e3fd349114fa28e0b4/sha.go#L7-L17

So possibly this should check if it's a digested reference

ruiwen-zhao · 2023-02-24T22:55:28Z

 // the image does not exist in containerd.
 func (s *Store) update(ref string, img *Image) error {
+	refName := strings.Split(ref, ":")[0]
+	switch refName {


Will ref be something like alpine:3.9@sha256:414e0518bb9228d35e4cd5165567fb91d26c6a214e9c95899e1e056fcd349011?

I searched the code a bit and looks like ref here will only be tag or digest or image ID, so we should be fine.

This can be tag or digest or image ID here if I understand correctly.

ruiwen-zhao · 2023-02-24T22:55:59Z

Thanks Paco! LGTM except for the two minor comments.

ruiwen-zhao · 2023-03-03T22:13:18Z

/lgtm

…brary` prefix Signed-off-by: Paco Xu <[email protected]>

Signed-off-by: Paco Xu <[email protected]>

k8s-ci-robot · 2024-02-03T14:00:09Z

PR needs rebase.

Details

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.

github-actions · 2024-05-07T00:09:08Z

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

github-actions · 2024-05-14T00:09:27Z

This PR was closed because it has been stalled for 7 days with no activity.

k8s-ci-robot added the needs-ok-to-test label Nov 28, 2022

pacoxu changed the title ~~crictl remove images: clean refCache during removing~~ [wip]crictl remove images: clean refCache during removing Nov 28, 2022

pacoxu force-pushed the image-clean branch from 55a2703 to 14bc396 Compare November 28, 2022 11:17

pacoxu changed the title ~~[wip]crictl remove images: clean refCache during removing~~ parsing ref consistently for refCache, including adding docker.io/library prefix Nov 28, 2022

pacoxu force-pushed the image-clean branch from 14bc396 to 51e4403 Compare November 28, 2022 11:19

k8s-ci-robot requested review from dmcgowan and mxpv November 28, 2022 11:24

k8s-ci-robot requested a review from cpuguy83 December 1, 2022 18:52

ruiwen-zhao reviewed Dec 2, 2022

View reviewed changes

Comment thread pkg/cri/store/image/image.go Outdated

pacoxu force-pushed the image-clean branch from 51e4403 to c323e32 Compare December 14, 2022 12:16

pacoxu requested review from ruiwen-zhao and removed request for cpuguy83, dmcgowan and mxpv December 14, 2022 12:18

pmengelbert mentioned this pull request Dec 14, 2022

Tracking issue for containerd sideloaded images eraser-dev/eraser#483

Open

kzys reviewed Dec 23, 2022

View reviewed changes

kzys mentioned this pull request Dec 23, 2022

To remove certain images using CRI, containerd requires a restart #7698

Closed

pacoxu force-pushed the image-clean branch 2 times, most recently from baf6ee8 to a843906 Compare December 26, 2022 02:58

pacoxu requested review from kzys and removed request for ruiwen-zhao December 26, 2022 03:02

pacoxu force-pushed the image-clean branch from 180aad7 to eeca27e Compare January 15, 2023 14:13

pacoxu mentioned this pull request Jan 15, 2023

TestContainerPids failed. It seems unrelated. #7965

Closed

thaJeztah reviewed Jan 15, 2023

View reviewed changes

pacoxu force-pushed the image-clean branch 3 times, most recently from 089fe40 to 97253bc Compare January 16, 2023 06:23

ruiwen-zhao reviewed Feb 24, 2023

View reviewed changes

Comment thread pkg/cri/store/image/image.go Outdated

ruiwen-zhao reviewed Feb 24, 2023

View reviewed changes

ruiwen-zhao reviewed Mar 3, 2023

View reviewed changes

Comment thread reference/docker/reference_test.go Outdated

pacoxu added 2 commits March 8, 2023 17:26

parsing ref consistently for refCache, including adding `docker.io/li…

e4228cd

…brary` prefix Signed-off-by: Paco Xu <[email protected]>

parsing ref consistently for refCache

df801ba

Signed-off-by: Paco Xu <[email protected]>

pacoxu force-pushed the image-clean branch 2 times, most recently from 542734b to c99fbfb Compare March 8, 2023 09:46

pacoxu marked this pull request as draft March 8, 2023 10:22

save current workspace temp

e212aeb

Signed-off-by: Paco Xu <[email protected]>

pacoxu force-pushed the image-clean branch from c99fbfb to e212aeb Compare March 9, 2023 04:34

pacoxu marked this pull request as ready for review March 9, 2023 04:35

pmengelbert mentioned this pull request May 5, 2023

ci: Fix flaking e2e test (imagelist_alias) eraser-dev/eraser#735

Merged

k8s-ci-robot added the needs-rebase label Feb 3, 2024

github-actions Bot added the Stale label May 7, 2024

github-actions Bot closed this May 14, 2024

Conversation

pacoxu commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Nov 28, 2022

Uh oh!

pacoxu commented Nov 28, 2022

Uh oh!

k8s-ci-robot commented Nov 28, 2022

Uh oh!

pacoxu commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sozercan commented Dec 1, 2022

Uh oh!

Uh oh!

ruiwen-zhao commented Dec 2, 2022

Uh oh!

pacoxu commented Dec 5, 2022

Uh oh!

ruiwen-zhao commented Dec 6, 2022

Uh oh!

pacoxu commented Dec 14, 2022

Uh oh!

pacoxu commented Dec 14, 2022

Uh oh!

pacoxu commented Dec 14, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacoxu Jan 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacoxu commented Jan 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruiwen-zhao commented Feb 24, 2023

Uh oh!

ruiwen-zhao commented Mar 3, 2023

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 3, 2024

Uh oh!

github-actions Bot commented May 7, 2024

Uh oh!

github-actions Bot commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pacoxu commented Nov 28, 2022 •

edited

Loading

pacoxu commented Nov 29, 2022 •

edited

Loading

pacoxu Jan 15, 2023 •

edited

Loading