daemon/c8d: Implement `docker tag` by vvoland · Pull Request #44781 · moby/moby

vvoland · 2023-01-10T13:51:43Z

Upstreams: add support for image tag rumpl/moby#13

- What I did
Implement docker tag for containerd image store.

- How I did it
See individual commits.

- How to verify it

$ docker pull ubuntu
$ docker tag ubuntu test
$ docker pull alpine
$ docker tag alpine test

Tag replace

$ docker pull alpine
Using default tag: latest
f271e74b17ce: Download complete
41d876d4e443: Download complete
04eeaa5f8c35: Download complete
a9eaa45ef418: Download complete
docker.io/library/alpine:latest
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED                  SIZE
alpine       latest    f271e74b17ce   Less than a second ago   3.26MB
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED         SIZE
alpine       latest    f271e74b17ce   6 seconds ago   3.26MB
$ docker pull busybox
Using default tag: latest
7b3ccabffc97: Download complete
5e42fbc46b17: Download complete
abaa813f94fd: Download complete
f78e6840ded1: Download complete
docker.io/library/busybox:latest
$ docker tag busybox myimage
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED         SIZE
alpine       latest    f271e74b17ce   8 seconds ago   3.26MB
busybox      latest    7b3ccabffc97   5 seconds ago   2.01MB
myimage      latest    7b3ccabffc97   1 second ago    2.01MB
$ docker rmi busybox
Untagged: busybox
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
alpine       latest    f271e74b17ce   13 seconds ago   3.26MB
myimage      latest    7b3ccabffc97   6 seconds ago    2.01MB
$ docker tag alpine myimage
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
<none>       <none>    7b3ccabffc97   2 seconds ago    2.01MB
alpine       latest    f271e74b17ce   24 seconds ago   3.26MB
myimage      latest    f271e74b17ce   2 seconds ago    3.26MB
$ docker tag 7b3ccabffc97 busybox # this needs https://github.com/moby/moby/pull/44842
$ docker image ls
REPOSITORY   TAG       IMAGE ID       CREATED              SIZE
alpine       latest    f271e74b17ce   About a minute ago   3.26MB
busybox      latest    7b3ccabffc97   2 seconds ago        2.01MB
myimage      latest    f271e74b17ce   52 seconds ago       3.26MB

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

rumpl · 2023-01-10T14:13:59Z

+		err := is.Delete(ctx, img.Name)
+
+		if err != nil {
+			logger.WithError(err).Error("failed to delete old image")


The other logs are Debug these could be too no?

My reasoning was that the first one can be Debug, because it mostly should be just an user error (non existing id) and it's kind of expected to happen normally.

But if the image couldn't be created and it's not an "already exists" error, then probably it's something that's not expected to happen under normal circumstances s and it's worth having logging without debug enabled.

But if the image couldn't be created and it's not an "already exists" error, then probably it's something that's not expected to happen under normal circumstances s and it's worth having logging without debug enabled.

That one should probably be an Error(), as it's an unexpected error (and we currently return a errdefs.System() (internal server error).

Really we shouldn't need to log and return an error.

The log has additional information about the context (imageID, tag) - which might be useful when debugging failures. The only caveat is that these will only appear in the daemon log.
Should we drop the logging and just include these in the returned error?

I think it makes sense to include it in the error.

Removed the extra logrus logs and included details directly in returned errors.

vvoland · 2023-01-10T14:44:00Z

CI failures are related, will fix those.

thaJeztah · 2023-01-17T08:58:08Z

+	if err != nil {
+		if !cerrdefs.IsAlreadyExists(err) {
+			logger.WithError(err).Debug("failed to create image")
+			return errdefs.System(err)


Do we know what errors this can return, and if they should always be considered an internal server error? (does is.Create() return typed errors from containerd, and can any of those be a "user error" (4xx)?

(Perhaps a 5xx is okay in all cases, but might be worth checking and/or a comment to outline that irregardless of the error type, we consider it an internal server error).

Yeah definitely there are some errors which shouldn't be considered a server error, like: missing name, labels too big. Those are the minority of all errors that could be returned though. The rest look system related (grpc, internal storage failures).
I think we should keep this as System, but validate the image ourselves before calling is.Create() and return an user error.

Actually, in this case already have a valid reference which is a valid image name. So it's not possible for this one to fail with user user. Will change the Debug to Error log then.

cpuguy83 · 2023-01-26T20:13:27Z

+		// Error out in case we couldn't persist the old image.
+		// If it already exists, then just continue.
+		if err != nil && !cerrdefs.IsAlreadyExists(err) {
+			logger.WithError(err).Error("failed to create a dangling image for the image being replaced")


Same as above, we shouldn't need to log and return an error.

neersighted

Would like to see the following changes:

Amending commits squashed, TODOs between commits minimized
Don't override ctx
TagImage instead of TagImageWithReference would be nice

I feel like you may have preferred TagImageWithReference over TagImage as it makes the need to create the correct reference first clear. It looks like there's common that does this shared between the tag API and the commit code.

Can we consider DRYing that up? It seems possible we could preserve TagImage as a fully encapsulated version, though on the other hand, the need for TagImageWithReference would be decreased. In any case, I'd like the logic of creating the reference to be de-duplicated and contained in the tagging code, if possible.

vvoland

Squashed, removed the ctx overriding and changed TagImageWithReference into TagImage.

I still kept the reference parsing logic outside of tagging code - I don't see how it belongs here, many operations should make sure the reference is valid, before attempting the tag, so they already have a parsed reference. Also it feel weird to make ImageService responsible for parsing that.
I moved the reference parsing from commit to the caller code and made it accept a NamedParsed reference instead. The parsing logic is extracted to httputils as for now only http handlers parse the reference, but ideally this should probably belong to the reference package,

vvoland · 2023-01-31T09:32:57Z

+		err := is.Delete(ctx, img.Name)
+
+		if err != nil {
+			logger.WithError(err).Error("failed to delete old image")


The log has additional information about the context (imageID, tag) - which might be useful when debugging failures. The only caveat is that these will only appear in the daemon log.
Should we drop the logging and just include these in the returned error?

neersighted

Updating my review: LGTM with the exception of dropping the unused method causing a linter error.

neersighted · 2023-02-03T13:07:24Z

I'd like to see @cpuguy83's logging concerns addressed (looks like he didn't mark them as changes requested). Looks ready to merge once he gives his sign-off.

neersighted · 2023-02-06T17:09:56Z

Let's

rumpl

👍👍👍

thaJeztah

LGTM, thanks!

thaJeztah · 2023-02-07T14:42:31Z

arf.. running into the "dubious permissions" issue; @vvoland can you try rebasing?

Signed-off-by: Nicolas De Loof <[email protected]> Signed-off-by: Paweł Gronowski <[email protected]>

TagImage is just a wrapper for TagImageWithReference which parses the repo and tag into a reference. Change TagImageWithReference into TagImage and move the responsibility of reference parsing to caller. Signed-off-by: Paweł Gronowski <[email protected]>

Implements image tagging under containerd image store. If an image with this tag already exists, and there's no other image with the same target, change its name. The name will have a special format `moby-dangling@<digest>` which isn't a valid canonical reference and doesn't resolve to any repository. Signed-off-by: Paweł Gronowski <[email protected]>

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland · 2023-02-07T14:45:18Z

@thaJeztah Rebased

thaJeztah · 2023-02-07T14:48:21Z

Thanks!

vvoland added area/api API area/images Image Service area/daemon Core Engine containerd-integration Issues and PRs related to containerd integration labels Jan 10, 2023

rumpl reviewed Jan 10, 2023

View reviewed changes

vvoland force-pushed the c8d-tag-upstream branch 2 times, most recently from 0bfe16f to 070f6a0 Compare January 10, 2023 14:55

rumpl approved these changes Jan 12, 2023

View reviewed changes

rumpl mentioned this pull request Jan 15, 2023

Fix tagging when image already exists rumpl/moby#127

Merged

rumpl reviewed Jan 15, 2023

View reviewed changes

Comment thread daemon/containerd/image_tag.go Outdated

thaJeztah reviewed Jan 17, 2023

View reviewed changes

vvoland force-pushed the c8d-tag-upstream branch from 070f6a0 to c8b96ac Compare January 17, 2023 09:22

vvoland mentioned this pull request Jan 17, 2023

images: Pass context to commit related operations #44818

Merged

rumpl reviewed Jan 17, 2023

View reviewed changes

Comment thread daemon/containerd/image_tag.go Outdated

vvoland force-pushed the c8d-tag-upstream branch from c8b96ac to 03a38c9 Compare January 17, 2023 09:39

vvoland commented Jan 17, 2023

View reviewed changes

Comment thread daemon/containerd/soft_delete.go Outdated

vvoland commented Jan 18, 2023

View reviewed changes

Comment thread daemon/containerd/image_tag.go Outdated

vvoland force-pushed the c8d-tag-upstream branch from 03a38c9 to eaf7acb Compare January 19, 2023 12:21

thaJeztah reviewed Jan 23, 2023

View reviewed changes

vvoland mentioned this pull request Jan 23, 2023

c8d/list: Fix Repo(Digests|Tags) for untagged images #44840

Merged

vvoland force-pushed the c8d-tag-upstream branch from eaf7acb to 47569ba Compare January 23, 2023 16:05

tianon reviewed Jan 24, 2023

View reviewed changes

Comment thread api/server/router/image/image_routes.go Outdated

tianon reviewed Jan 24, 2023

View reviewed changes

Comment thread daemon/containerd/soft_delete.go Outdated

vvoland force-pushed the c8d-tag-upstream branch 2 times, most recently from d29316b to 0e41b77 Compare January 25, 2023 10:20

tianon approved these changes Jan 25, 2023

View reviewed changes

cpuguy83 reviewed Jan 26, 2023

View reviewed changes

neersighted requested changes Jan 27, 2023

View reviewed changes

Comment thread daemon/containerd/image_tag.go

Comment thread daemon/containerd/image_tag.go Outdated

vvoland commented Jan 31, 2023

View reviewed changes

vvoland force-pushed the c8d-tag-upstream branch 2 times, most recently from 1eb14ec to b2d47f2 Compare January 31, 2023 14:59

neersighted requested review from cpuguy83, neersighted and thaJeztah January 31, 2023 15:09

vvoland force-pushed the c8d-tag-upstream branch from b2d47f2 to f539043 Compare January 31, 2023 17:24

neersighted requested changes Feb 2, 2023

View reviewed changes

vvoland force-pushed the c8d-tag-upstream branch from f539043 to c364411 Compare February 2, 2023 20:59

neersighted approved these changes Feb 3, 2023

View reviewed changes

vvoland force-pushed the c8d-tag-upstream branch 2 times, most recently from b8f4126 to 12efc2d Compare February 6, 2023 15:25

cpuguy83 approved these changes Feb 6, 2023

View reviewed changes

cpuguy83 added the status/4-merge label Feb 6, 2023

rumpl approved these changes Feb 7, 2023

View reviewed changes

thaJeztah approved these changes Feb 7, 2023

View reviewed changes

ndeloof and others added 6 commits February 7, 2023 15:43

daemon: Pass ctx to image tagging operations

7b6f71d

Signed-off-by: Nicolas De Loof <[email protected]> Signed-off-by: Paweł Gronowski <[email protected]>

api/tag: Reject digested target references

b3a8b6b

Signed-off-by: Paweł Gronowski <[email protected]>

c8d/import: Use danglingImageName instead of hardcoded

afc6e3f

Signed-off-by: Paweł Gronowski <[email protected]>

api: Extract parsing reference from repo and tag

62be425

Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-tag-upstream branch from 12efc2d to 62be425 Compare February 7, 2023 14:44

neersighted merged commit 5f369ff into moby:master Feb 7, 2023

vvoland mentioned this pull request Feb 22, 2023

c8d/tag: Don't create a separate error variable #45055

Merged

thaJeztah added this to the v-next milestone Feb 22, 2023

Conversation

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

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!

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!

vvoland commented Jan 10, 2023

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

neersighted left a comment

Choose a reason for hiding this comment

Uh oh!

neersighted commented Feb 3, 2023

Uh oh!

neersighted commented Feb 6, 2023

Uh oh!

rumpl left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 7, 2023

Uh oh!

vvoland commented Feb 7, 2023

Uh oh!

thaJeztah commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vvoland commented Jan 10, 2023 •

edited

Loading