Preserve image name on matching images

[saschagrunert]

If two image names (including their tags) match, then we usually remove the name completely. This means in the end that we have no chance to tell the user what the original image name was. We now strip the tag away and add a <none>, which could serve as generic indicator that no tag is available for that image.

I tested it with the latest CRI-O master and this applied patch, we now have the chance to preserve the image name as follows:

> sudo crictl images
IMAGE                 TAG                 IMAGE ID            SIZE
localhost:5000/test   <none>              9fbc3a97f1aec       2.07kB
localhost:5000/test   latest              776f147227fde       3.76kB

Before it was:

> sudo crictl images
IMAGE                 TAG                 IMAGE ID            SIZE
<none>                <none>              9fbc3a97f1aec       2.07kB
localhost:5000/test   latest              776f147227fde       3.76kB

[rhatdan]

Could you add a test for this?
@mtrmac @nalind PTAL

[rhatdan]

@giuseppe PTAL

[saschagrunert]

Could you add a test for this?

Yes sure, I will add unit and integration tests if we decide to implement it in the proposed way.

[vrothberg]

I am interested in the motivation behind the change. Can you elaborate a bit on that?

…

On Wed 12. Jun 2019 at 12:55, Sascha Grunert ***@***.***> wrote: Could you add a test for this? Yes sure, I will add unit and integration tests if we decide to implement it in the proposed way. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#366?email_source=notifications&email_token=ACZDRAYM2LBIFSFXURZ7Z2DP2DI2BA5CNFSM4HXHE3NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQAZAQ#issuecomment-501222530>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACZDRA2GBYFLUFMYWEPP4TDP2DI2BANCNFSM4HXHE3NA> .

[saschagrunert]

I am interested in the motivation behind the change. Can you elaborate a bit on that?

Yes sure: A user has mentioned that containerd seems to preserve that kind of information and is therefore able to provide more details about available images on a node (see cri-o/cri-o#2455 (comment)). So my main intention was to preserve the image name when a tag moved away to another image layer.

Beside this, there is a bug in crictl which does not allow to show the image name when pulling via digest, but this is another story and addressed in kubernetes-sigs/cri-tools#471.

In the end the whole story is a bit mixed up, I guess that's because of 'historical reasons'. Kubernetes expects (currently) always some sort of image name for the repo tag and digest, whereas I discovered the following possible combinations in the wild:

• <none>:<none>/<none>@<none>: No image information available beside the ID (originally from the moby project and now addressed in Fix missing image names on list cri-o/cri-o#2455)
• name:<none> No image tag available but the name (addressed within this PR)
• <none>:<none>/image@sha256:... Use image name from digest if no tag available (addressed in Fallback to repo digest name if repoTag is <none>:<none> kubernetes-sigs/cri-tools#471)
• image:tag Tag and image name available, works as before

[mtrmac]

No, this must be done some other way. (See also the full discussion at cri-o/cri-o#2455 (comment) ).

• We are not compatibility-constrained like CRI maybe is, so we shouldn’t add random untyped string special cases when the values can be represented using well-typed Go data.

• (As mentioned in details inside, using a <none> tag non-value would already break existing code.)

• This propagates the Docker mistake of confusing data with presentation, and doubles down on it. <none> is a UI presentation matter and should not exist in the raw data; the top-level UI can easily use such a format, but that does not mean that it is the right to store on disk. Yes, https://github.com/projectatomic/docker/blob/9ef7699f0121ad650777cec91ca1c1c0b6ada739/daemon/images.go#L156 does that, but only in that limited case; this PR would do it even in the case where Docker does not do it, and only creates the <none> string in the UI layer: https://github.com/projectatomic/docker/blob/9ef7699f0121ad650777cec91ca1c1c0b6ada739/daemon/images.go#L156 . That approach, storing the RepoDigests value, and teaching crictl images to show known-digest-unknown-tag images as repo:<none> in the UI, makes much more sense to me.

• Conceptually I don’t think c/storage is the right place for any of this, anyway; it does not impose any semantics on the image names, skopeo copy docker://busybox containers-storage:hello-i-am-not-a-busy-box-at-all works just fine so far.

  The RepoDigests values (assuming that’s what we’ll end up implementing) can either be set in c/image when given a tagged name as a destination, or possibly at an ever higher level directly in the buildah/podman/CRI-O pull path. Arguably c/image is similarly neutral and doesn’t care about the source-destination names relationship, OTOH having this only implemented once and consistently for all of the tools does seem important, and RepoDigests are not exactly the name anyway, so the storageImageDestination can write the value based on the user-chosen name justifiably enough.

  Also, we will need to figure out how to deal with recording manifest list digests; c/image/storage.imageDestination never sees the manifest list, so it doesn’t know the digest value, right now, and similarly the manifest returned by c/image/copy.Image is the individual manifest, not the manifest list, so not even the callers could record the manifest list digest.

[mtrmac]

We use c/image/docker/reference throughout (IIRC for somewhat historical reasons, the upstream package went through a painful API break at some point).

[mtrmac]

<none> is not a valid tag (as is already visible from the fact that this needs to use a Sprintf instead of reference.WithTag).

While it’s not, IIRC, documented explicitly, c/storage names must all be parseable using ParseNormalizedNamed (tools like podman just fail when they see a non-compliant name).

[mtrmac]

Doing a non-trivial edit like this in a function innocuously named stringSliceWithoutValue is horribly confusing and almost certain to trip some future caller up. If this needs to exist at all, rename it to be clear about what the function does.

[saschagrunert]

I see, I will check if this topic can be addressed in a different way. Thank you for the input.