add support for image inspect with containerd-integration by thaJeztah · Pull Request #43818 · moby/moby

thaJeztah · 2022-07-15T12:53:49Z

depends on containerd integration: imageservice: add contexts to various methods (step 1) #43939
closes [draft] containerd integration: add support for image inspect #44621

Upstreaming:

These patches were selected by looking at the history for https://github.com/rumpl/moby/commits/dd-4.15.0/daemon/containerd/image.go, and only the relevant parts for this feature were included.

Quite a few of those are touching-up previous commits, so we should consider squashing (some of) them, but keeping them separate for now to somewhat help review.

p 85d6399836 add support for image inspect
p fa430d6686 Implement run using the containerd snapshotter
p d37b797d34 introduce GetImageOpts to manage image inspect data in backend
p 4543499f86 Introduce support for docker commit
p f1213b08bd Fix linting issues
p 2840f63ee6 GetImage to return image tags with details
p 75df7b7a05 list images matching digest to discover all tags
p d9b76c6682 c8d/prune: Handle filters, don't delete used
p a930139556 Add ExposedPorts and Volumes to the image returned
p 8df0a88265 Refactor resolving/getting images
p d58184802b Return the image ID on inspect
p 61ea993343 consider digest and ignore tag when both are set
p eaa810ab98 docker run --platform
p ab6918735d c8d/builder: store untagged images
p 960cff2c71 c8d/commit: Save as dangling name
p f7962b37ef Remove the call to resolveImage
p 9ac5d768c7 Handle multi-platform images when listing them

Also included rumpl#122

p a5a01b5cf7 daemon/containerd: fix compat for "docker run" with image that's not present

This is a squashed version of various PRs (or related code-changes)
to implement image inspect with the containerd-integration;

add support for image inspect
introduce GetImageOpts to manage image inspect data in backend
GetImage to return image tags with details
list images matching digest to discover all tags
Add ExposedPorts and Volumes to the image returned
Refactor resolving/getting images
Return the image ID on inspect
consider digest and ignore tag when both are set
docker run --platform

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

AkihiroSuda · 2022-07-16T13:46:41Z

Would it be possible to split the "ctx" changes into a separate commit?

thaJeztah · 2022-07-18T08:52:58Z

moved it to #43828

thaJeztah · 2022-12-09T23:23:53Z

Moving this temporarily back to draft, as I'll probably update this one with changes from #44621 (or use that one instead)

corhere

Unfortunately, there's a(nother) problem with the "image not found" error returns.

Aside: have you seen how nerdctl resolves images? It's quite elegant. There might be some inspiration to take from it.

corhere · 2022-12-20T17:37:08Z

daemon/containerd/image.go

+		// This requires the error to be prefixed with "No such image:".
+		// Without this prefix, the CLI prints the error message, and exits.
+		//
+		// FIXME(thaJeztah): we need to fix this; if needed, gated by API version (API < X add the prefix)


Tracking issue?

@thaJeztah I removed the comment since I'm not returning images.ErrImageDoesNotExist but you might want to create an issue for this

daemon/containerd/image.go

corhere · 2022-12-21T17:28:11Z

daemon/containerd/image.go

+
+// resolveImage searches for an image based on the given
+// reference or identifier. Returns the descriptor of
+// the image, could be manifest list, manifest, or config.


Suggested change

// the image, could be manifest list, manifest, or config.

// the image, which could be a manifest list, manifest, or config.

corhere · 2022-12-21T17:57:27Z

daemon/containerd/image.go

+	// If the identifier could be a short ID, attempt to match
+	if shortID.MatchString(refOrID) {


"short ID" is an unfortunate choice of name as it could be confused with a long-deprecated reference syntax. Call it a "truncated ID" or "ID prefix" instead?

corhere · 2022-12-21T19:37:25Z

daemon/containerd/image.go

+	if ok {
+		imgs, err := is.List(ctx, fmt.Sprintf("target.digest==%s", digested.Digest()))
+		if err != nil {
+			return containerdimages.Image{}, errors.Wrap(err, "failed to lookup digest")


Does err need to be translated to (wrapped in) an errdefs value?

corhere · 2022-12-21T20:21:09Z

daemon/containerd/image.go

+			fmt.Sprintf("name==%q", ref),
+			fmt.Sprintf(`target.digest~=/sha256:%s[0-9a-fA-F]{%d}/`, refOrID, 64-len(refOrID)),


Suggested change

fmt.Sprintf("name==%q", ref),

fmt.Sprintf(`target.digest~=/sha256:%s[0-9a-fA-F]{%d}/`, refOrID, 64-len(refOrID)),

"target.digest~=" + strconv.Quote(fmt.Sprintf(`sha256:^%s[0-9a-fA-F]{%d}$`, regexp.QuoteMeta(refOrID), 64-len(refOrID))),

What can I say, I'm a little paranoid about properly escaping user input.

Also, the digest regexp filters should probably be anchored as nerdctl does.

corhere · 2022-12-21T21:10:05Z

daemon/containerd/image.go

+	// If the identifier could be a short ID, attempt to match
+	if shortID.MatchString(refOrID) {
+		filters := []string{
+			fmt.Sprintf("name==%q", ref),


It took me way too long to figure out what this filter is needed for.

Suggested change

fmt.Sprintf("name==%q", ref),

fmt.Sprintf("name==%q", ref), // Or it could just look like one.

corhere · 2022-12-21T22:21:56Z

daemon/containerd/image.go

+			return nil, err
+		}
+
+		tagged, err := i.client.ImageService().List(ctx, fmt.Sprintf("target.digest==%s", containerdImage.Target().Digest.String()))


Nit: @thaJeztah has been on a rampage lately subbing trivial fmt.Sprintf calls with string concatenation. Let's be nice and not leave more work for him.

Suggested change

tagged, err := i.client.ImageService().List(ctx, fmt.Sprintf("target.digest==%s", containerdImage.Target().Digest.String()))

tagged, err := i.client.ImageService().List(ctx, "target.digest=="+containerdImage.Target().Digest.String()))

corhere · 2022-12-21T22:25:39Z

daemon/containerd/image.go

+	var ociimage ocispec.Image
+	imageConfigBytes, err := content.ReadBlob(ctx, containerdImage.ContentStore(), conf)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	if err := json.Unmarshal(imageConfigBytes, &ociimage); err != nil {
+		return nil, nil, err
+	}


Suggested change

var ociimage ocispec.Image

imageConfigBytes, err := content.ReadBlob(ctx, containerdImage.ContentStore(), conf)

if err != nil {

return nil, nil, err

}

if err := json.Unmarshal(imageConfigBytes, &ociimage); err != nil {

return nil, nil, err

}

imageConfigBytes, err := content.ReadBlob(ctx, containerdImage.ContentStore(), conf)

if err != nil {

return nil, nil, err

}

var ociimage ocispec.Image

if err := json.Unmarshal(imageConfigBytes, &ociimage); err != nil {

return nil, nil, err

}

corhere · 2022-12-21T22:29:13Z

daemon/containerd/image.go

+				return img, nil
+			}
+		}
+		return img, errdefs.NotFound(errors.Errorf("platform %s not supported", platforms.Format(*platform)))


Suggested change

return img, errdefs.NotFound(errors.Errorf("platform %s not supported", platforms.Format(*platform)))

return img, errdefs.NotFound(errors.Errorf("%s: platform %s not supported by image", refOrID, platforms.Format(*platform)))

vvoland · 2022-12-30T15:48:03Z

daemon/containerd/image.go

+			}
+
+			if len(digests) > 1 {
+				return containerdimages.Image{}, errdefs.NotFound(errors.New("ambiguous reference"))


Maybe errdefs.Conflict would be more suitable?

Conflict (409) is mostly intended if there's a conflict with another request (e.g. update a container that is in the process of being deleted); that said; "it's hard", as those status codes weren't really designed with APIs in mind. Also see the discussion on #44685

This page has some flow-charts that have been quite useful; https://www.codetinkerer.com/2015/12/04/choosing-an-http-status-code.html

Maybe a 422 would make sense, but it's not about the body (although that may be semantics)

thaJeztah · 2023-01-01T12:24:00Z

daemon/containerd/image.go

 	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
 	"github.com/docker/go-connections/nat"
+	"github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb"


I'm still not comfortable with having our internal types defined by another project; see the discussion on rumpl#121 (comment)

If that type is implementing the image "v1" spec defined in this repository (https://github.com/moby/moby/tree/master/image/spec), and we don't have a type for that in this repository, we should define it here

I removed the commit

thaJeztah · 2023-01-02T11:37:55Z

daemon/containerd/image.go

+		exposedPorts[nat.Port(k)] = v
+	}
+
+	img := image.NewImage(image.IDFromDigest(ctrdimg.Target.Digest))


Just recalled #44426 - we should update this to

Suggested change

img := image.NewImage(image.IDFromDigest(ctrdimg.Target.Digest))

img := image.NewImage(image.ID(ctrdimg.Target.Digest))

vvoland · 2023-01-03T08:53:34Z

daemon/containerd/image.go

+	if platform != nil {
+		cs := i.client.ContentStore()
+		imgPlatforms, err := containerdimages.Platforms(ctx, cs, img.Target)
+		if err != nil {
+			return img.Target, err
+		}
+
+		comparer := platforms.Only(*platform)
+		for _, p := range imgPlatforms {
+			if comparer.Match(p) {
+				return img.Target, nil
+			}
+		}
+		return img.Target, errdefs.NotFound(errors.Errorf("%s: platform %s not supported by image", refOrID, platforms.Format(*platform)))
+	}


I think this should be extracted from resolveDescriptor to GetImage. Accepting platform as an argument to this function gives an impression that the returned descriptor will be platform specific and it doesn't.
The platform is relevant only in the GetImage context, which uses it to pick the correct image config.

👍 I moved the code

vvoland · 2023-01-03T09:37:58Z

daemon/containerd/image.go

+
+	cs := i.client.ContentStore()
+
+	platform := platforms.Default()


Hm, this won't work, if we have image for the non-default platform only (like if we only do docker pull --platform linux/arm64 alpine on non-arm64 host).
Is this expected?

I think it makes sense that when no --platform flag is given then we consider this as the user asking for the current platform

That bugs me - if the user doesn't set the --platform then we don't know what he's asking for, consider this:

$ docker pull --platform linux/arm64 alpine $ docker inspect alpine alpine: platform linux/amd64 not supported by image

seems a bit harsh, especially for doing an inspect which should allow me to get information about an image. The user might not remember which platform the image was pulled for and currently there's no way for him to find out that.

What should we show in this case then?

$ docker pull --platform linux/arm64 alpine $ docker pull --platform linux/s390x alpine $ docker inspect alpine

The "old" behavior would be to pick the first platform that's found (but of course that would always be the only platform). Ideally we would show all platforms for the image, but that brings us to the discussion in #44582. If we must show a single architecture, but multiple architectures are present, it could follow the same method of selecting as a docker pull would do (basically, use a platform-matcher?)

Consider the API currently shows whatever image is currently there regardless of platform.

We could error out with like ambiguous image reference: multiple platforms are available, you must specify a platform: available platforms <platform1>,<platform2>. It would be nice, though, if the API's json error could list those platforms so clients can parse it nicely (without having to parse the error string) or pick the first one... I'm inclined to error in case of ambiguity here, though.

Alternatively... we could update the API to return a list of objects
The docker CLI at least already outputs a json array.
There wouldn't be much too change to support the updated API.

... or change it to return the image index (with the appropriate media type on the response headers).
This does mean larger changes would be needed on the client.

On the last point... we could gate this by accept header.

Talking with @thaJeztah and we decided to go with a AllPlatformsWithPreference that returns the preferred platform first and then the others in order that they are defined by containerd.

^^ That approach is the "first step" for this PR, to keep the behavior (mostly) consistent with what we have pre-containers, and to get us going, but we should indeed continue the discussion on UX (which could be return multiple platforms if there's multiple variations present)

Definitely, the discussion is only getting started. Way back when we talked about snapshotters we decided that the way to go was to make everything as close as possible to the existing and after that we can shape the things in different ways

vvoland · 2023-01-03T09:40:25Z

daemon/containerd/image.go

+
+	platform := platforms.Default()
+	if desc.Platform != nil {
+		platform = platforms.OnlyStrict(*desc.Platform)


We use OnlyStrict here, but Only when doing the previous check which will make some cases pass the check but fail on getting the config. Maybe we could get rid of the previous platform check completely, and rely on containerdimages.Config returning an error when we don't have that platform present?

cpuguy83 · 2023-01-03T18:24:25Z

Outside of the discussion around multiple platforms being available, the rest of the code LGTM.

vvoland

LGTM

thaJeztah

LGTM

thaJeztah · 2023-01-05T15:14:17Z

daemon/images/image.go

 type ErrImageDoesNotExist struct {
-	ref reference.Reference
+	Ref reference.Reference
 }


For a follow-up; we need to check if we really need this error-type, as it already implements errdefs.NotFound, perhaps we don't need it (with the possible exception of the "special" No such ... prefix, which may be needed for older CLIs).

Yeah this one might not be needed, the only difference is that this error type formats the message with "No such image: %s", ref whereas the errdefs.NotFound doesn't have that extra data

daemon/containerd/image.go

This is a squashed version of various PRs (or related code-changes) to implement image inspect with the containerd-integration; - add support for image inspect - introduce GetImageOpts to manage image inspect data in backend - GetImage to return image tags with details - list images matching digest to discover all tags - Add ExposedPorts and Volumes to the image returned - Refactor resolving/getting images - Return the image ID on inspect - consider digest and ignore tag when both are set - docker run --platform Signed-off-by: Djordje Lukic <[email protected]> Signed-off-by: Nicolas De Loof <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah

still LGTM

thaJeztah · 2023-01-06T16:40:07Z

Let me bring this one in 👍

corhere · 2023-01-06T16:48:20Z

pkg/platforms/platforms.go

+}
+
+func (c allPlatformsWithPreferenceMatcher) Less(p1, p2 ocispec.Platform) bool {
+	return c.preferred.Less(p1, p2)


If this is to live in a pkg package, we should make sure it's robust for any preferred matcher, including ones whose Less implementation may not necessarily sort platforms which Match less than platforms it would not.

Suggested change

return c.preferred.Less(p1, p2)

m1, m2 := c.preferred.Match(p1), c.preferred.Match(p2)

if m1 && m2 {

return c.preferred.Less(p1, p2)

}

return m1 // Not totally-ordered

thaJeztah added status/2-code-review area/images Image Distribution labels Jul 15, 2022

thaJeztah added this to the v-next milestone Jul 15, 2022

thaJeztah requested a review from tonistiigi as a code owner July 15, 2022 12:53

This was referenced Jul 15, 2022

containerd integration: Implement ImageDelete for containerd #43820

Merged

containerd integration: add support for image tag #43821

Closed

containerd integration: Implement load/save #43822

Merged

This comment was marked as outdated.

Sign in to view

thaJeztah force-pushed the image_inspect branch 2 times, most recently from fe465a1 to 7e7249c Compare July 15, 2022 16:11

thaJeztah force-pushed the image_inspect branch from 7e7249c to fe9e6c7 Compare July 18, 2022 08:52

thaJeztah marked this pull request as draft July 18, 2022 08:53

thaJeztah added the containerd-integration Issues and PRs related to containerd integration label Jul 21, 2022

thaJeztah force-pushed the image_inspect branch 2 times, most recently from cfa4765 to 157e515 Compare August 4, 2022 12:36

thaJeztah mentioned this pull request Aug 4, 2022

emptyfs: Use newer image layout #43912

Merged

thaJeztah force-pushed the image_inspect branch from 157e515 to 1184eb5 Compare August 9, 2022 14:10

thaJeztah mentioned this pull request Aug 9, 2022

containerd integration: imageservice: add contexts to various methods (step 1) #43939

Merged

thaJeztah mentioned this pull request Sep 1, 2022

manage image inspect data in backend #44080

Closed

thaJeztah force-pushed the image_inspect branch from 1184eb5 to ee27455 Compare September 7, 2022 17:46

thaJeztah marked this pull request as ready for review September 7, 2022 20:29

rumpl mentioned this pull request Sep 12, 2022

Refactor resolving/getting images rumpl/moby#88

Merged

thaJeztah force-pushed the image_inspect branch from ee27455 to f4f6760 Compare December 9, 2022 12:30

This was referenced Dec 9, 2022

[draft] containerd integration: add support for image inspect #44621

Closed

manage image inspect data in backend #44622

Merged

thaJeztah marked this pull request as draft December 9, 2022 23:23

thaJeztah force-pushed the image_inspect branch from f4f6760 to 92f1d26 Compare December 20, 2022 16:02

corhere requested changes Dec 21, 2022

View reviewed changes

rumpl force-pushed the image_inspect branch 3 times, most recently from 805e2c6 to e35edac Compare December 26, 2022 15:01

vvoland reviewed Dec 30, 2022

View reviewed changes

corhere approved these changes Dec 30, 2022

View reviewed changes

thaJeztah commented Jan 1, 2023

View reviewed changes

thaJeztah commented Jan 2, 2023

View reviewed changes

rumpl force-pushed the image_inspect branch 2 times, most recently from 6228c7a to 1d1988c Compare January 2, 2023 16:41

rumpl requested review from corhere and vvoland January 2, 2023 16:49

vvoland reviewed Jan 3, 2023

View reviewed changes

rumpl force-pushed the image_inspect branch from 1d1988c to c9cdf3f Compare January 3, 2023 09:27

vvoland reviewed Jan 3, 2023

View reviewed changes

rumpl force-pushed the image_inspect branch from c9cdf3f to 1042682 Compare January 3, 2023 09:46

rumpl force-pushed the image_inspect branch 2 times, most recently from 40f184f to 82430d6 Compare January 5, 2023 14:18

vvoland approved these changes Jan 5, 2023

View reviewed changes

thaJeztah commented Jan 5, 2023

View reviewed changes

rumpl force-pushed the image_inspect branch from 82430d6 to 1616a09 Compare January 5, 2023 15:27

thaJeztah commented Jan 6, 2023

View reviewed changes

cpuguy83 approved these changes Jan 6, 2023

View reviewed changes

thaJeztah merged commit 9374912 into moby:master Jan 6, 2023

thaJeztah deleted the image_inspect branch January 6, 2023 16:40

corhere requested changes Jan 6, 2023

View reviewed changes

thaJeztah mentioned this pull request Jan 20, 2024

pkg/platforms: internalize in daemon/containerd #47141

Merged

	// the image, could be manifest list, manifest, or config.
	// the image, which could be a manifest list, manifest, or config.

		// If the identifier could be a short ID, attempt to match
		if shortID.MatchString(refOrID) {

		fmt.Sprintf("name==%q", ref),
		fmt.Sprintf(`target.digest~=/sha256:%s[0-9a-fA-F]{%d}/`, refOrID, 64-len(refOrID)),

	fmt.Sprintf("name==%q", ref),
	fmt.Sprintf(`target.digest~=/sha256:%s[0-9a-fA-F]{%d}/`, refOrID, 64-len(refOrID)),
	"target.digest~=" + strconv.Quote(fmt.Sprintf(`sha256:^%s[0-9a-fA-F]{%d}$`, regexp.QuoteMeta(refOrID), 64-len(refOrID))),

	fmt.Sprintf("name==%q", ref),
	fmt.Sprintf("name==%q", ref), // Or it could just look like one.

	tagged, err := i.client.ImageService().List(ctx, fmt.Sprintf("target.digest==%s", containerdImage.Target().Digest.String()))
	tagged, err := i.client.ImageService().List(ctx, "target.digest=="+containerdImage.Target().Digest.String()))

	return img, errdefs.NotFound(errors.Errorf("platform %s not supported", platforms.Format(*platform)))
	return img, errdefs.NotFound(errors.Errorf("%s: platform %s not supported by image", refOrID, platforms.Format(*platform)))

	img := image.NewImage(image.IDFromDigest(ctrdimg.Target.Digest))
	img := image.NewImage(image.ID(ctrdimg.Target.Digest))


		cs := i.client.ContentStore()

		platform := platforms.Default()

-	return c.preferred.Less(p1, p2)
+	m1, m2 := c.preferred.Match(p1), c.preferred.Match(p2)
+	if m1 && m2 {
+		return c.preferred.Less(p1, p2)
+	}
+	return m1 // Not totally-ordered

Comments

Conversation

thaJeztah commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

AkihiroSuda commented Jul 16, 2022

Uh oh!

thaJeztah commented Jul 18, 2022

Uh oh!

thaJeztah commented Dec 9, 2022

Uh oh!

corhere left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 1, 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!

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!

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!

rumpl Jan 5, 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!

cpuguy83 commented Jan 3, 2023

Uh oh!

thaJeztah commented Jul 15, 2022 •

edited

Loading

thaJeztah Jan 1, 2023 •

edited

Loading

rumpl Jan 5, 2023 •

edited

Loading