c8d/image: Simplify `presentImages` and better "platform not found" error by vvoland · Pull Request #48276 · moby/moby

vvoland · 2024-07-31T12:16:43Z

- What I did
A minor refactor, best to review commit by commit.
Summary:

Simplify presentImages to getBestPresentImageManifest - all usages were only using the first sorted result
Add errPlatformNotFound which produces a better error message in case where the image exists, but the requested platform is not present
Extract matchRequestedOrDefault function to refactor a common pattern of creating a platform matcher from *ocispec.Platform

- How to verify it

- Description for the changelog

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

laurazard · 2024-08-01T10:31:51Z


-		return &childManifests[0], nil
+	if best == nil {
+		return nil, &errPlatformNotFound{imageRef: imageFamiliarName(img)}


should we not set the wanted platform here?

Nevermind, saw that we're doing this in the caller. Wonder if we could move that.

We should, but we can't set a specific ocispec.Platform here because the input is a platform matcher (which can match multiple platforms).

The expectation is that the caller can fill this on its own, like:

moby/daemon/containerd/image_history.go

Lines 31 to 36 in 063ee50

im, err := i.getBestPresentImageManifest(ctx, img, platform)

if err != nil {

var e *errPlatformNotFound

if errors.As(err, &e) {

e.wanted = platforms.DefaultSpec()

}

I see. I don't love having to remember to do that, but oh well 🤷‍♀️ Maybe we could mention that in getBestPresentImageManifest's godoc?

Hmm I got a funny idea, let me try it out 🙈

So, I changed matchRequestedOrDefault to return a wrapper that also has the requested platform bundled in and use that to populate the error when it's created, so the caller doesn't need to do it.

WDYT?

Looks good to me, @thaJeztah?

thaJeztah · 2024-08-05T11:29:24Z

+
+func (e *errPlatformNotFound) NotFound() {}
+func (e *errPlatformNotFound) Error() string {
+	msg := "image with reference " + e.imageRef + " was found but does not match the specified platform"


Perhaps requested instead of specified would work for this? Thinking about situations where no explicit platform was requested, so the default was used (or don't we produce the error in any of those cases)?

Also considering if match -> provide (or support); which may be more accurate for multi-arch 🤔

It would! But that would be different from the graphdrivers error returned in the same case. So that could break potential error string matching.

IMO, until we provide a better way for consumers to check for this error, we should stick to the existing wording.

IMO, until we provide a better way for consumers to check for this error, we should stick to the existing wording.

Hmm.. right; you mean distinction between "image present, but not platform" vs "image not present"? FWIW; I was more concerned about the No such errors, because those prefixes were used in docker itself to match error-types; for the errors we're modifying here, we never had that as a contract, so I'm much more OK with changing the error format.

Yeah. We might not have had that as a contract explicitly, but unfortunately Go stringly-typed errors forced that upon us implicitly I think 😅

There's probably not much of consumers doing string matching for this specific error, but I found at least one instance: https://github.com/buildpacks/pack/blob/5d0687f636fc6ab0ac5aef4afe0274965943f574/pkg/image/fetcher.go#L125

but unfortunately Go stringly-typed errors forced that upon us implicitly I think 😅

For exported, sentinel errors, for sure (but also the reason we chose to use the "interface-matching" approach in errdefs). I think in this case, the error itself is never returned to the client as a go-error that can be matched, as the API is in between, so at most the client receives a typed error (errdefs type that is) with an error message that should be considered informational, but in itself not something to control flow.

but I found at least one instance:

:sad_panda: yeah, that's not great. At least it looks like that code already calls it out for being brittle and temporary, and it looks like the issue linked to that comment was closed / addressed buildpacks/pack#2079. That said, I agree we may have to look if the current logic of an image either being "there" or "not there" (errdefs.NotFound) is still sufficient to handle logic on the client side, or if we need more specific errors (that also make it accross the API).

I'm ok with keeping the error-message as-is for now, but we should indeed make sure that we don't paint ourself (again) into a corner where error-strings become an undocumented API, and we must preserve error-messages (like we had with the No such prefix).

Perhaps we should add a section to the API docs describing that error-message MUST be considered informational, and can change at any time, so string-matching being highly discouraged.

thaJeztah · 2024-08-05T11:33:39Z

+	if e.wanted.OS != "" {
+		msg += ": wanted " + platforms.Format(e.wanted)
+	}


Playing around a bit as I was wondering if putting this after a : delimiter may feel like it's a separate error that was wrapped. I guess we could included it between braces;

image with reference foo:bar was found but does not match the requested platform (linux/amd64)

Only downside of that would be if we'd have windows ( the requested platform (windows(10.0.17763)/amd64))

The wanted: part is also present in the graphdrivers error, see above comment.

thaJeztah · 2024-08-05T11:40:19Z

 	if len(presentImages) == 0 {
-		return nil, errdefs.NotFound(errors.New("no matching image found"))
+		return nil, &errPlatformNotFound{
+			imageRef: img.Name,
+		}


Is this situation is only if the manifest index does not provide a usable platform, or also if it does (but the content isn't present)?

Both for now.

To add to the above; the reason I was wondering is because I know we have logic on the client-side where we have special handling for not found errors; when doing a docker run (or docker create), and we get a 404, the CLI initiates a docker pull (image not found; let's try pulling it).

Very likely not for this PR, but something we need to start looking at, because with multi-arch there will be more permutations of that;

The image is missing as a whole; we'll try pulling the image (manifest-index) and the desired platform.

We pulled the manifest-index, and it has an image for the given platform, but it's not yet pulled. In this case, we can pull the missing platform (but keep the manifest-index at the same version)

We pulled the manifest-index, but it does not contain the given platform

The last one is the tricky one, at least with the current logic not being granular enough;

We can either bail out (we have the manifest-index, but unfortunately it doesn't have this platform)

We can try pulling the manifest-index to see if perhaps a current version of it does have the platform (maybe the image was updated since we pulled the manifest-index?)

Either of the above are "valid", but we don't have a lot of granularity for this currently. The last one also can be confusing, as pulling the manifest-index may mean that any image (for other platform) may now become "dangling" as they no longer match the digest of the newly pulled manifest-index.

thaJeztah · 2024-08-05T11:50:17Z

+
+// ReadConfig gets the image config and unmarshals it into the provided struct.
+// The provided struct should be a pointer to the config struct or its subset.
+func (m *ImageManifest) ReadConfig(ctx context.Context, outConfig any) error {


Adding any may require this file to have a go:build comment (#46941) (although I don't think this file would be imported by consumers)

Looks like this is only used internally; so we may not need to export it; more so because having to pass the type to read feels a bit "internal"; i.e., if it's encapsulated in the ImageManifest type, then somewhat expecting that to know what it returns; being able to unmarshal to something else is mostly an internal optimisation.

I can change it to interface{}.

This struct already has most of the methods marked as exported (to signify that they're not "internal" and should be used by the consumers of the ImageManifest).

I think we could just unexport the whole ImageManifest instead.. WDYT?

Perhaps "both"; we could indeed un-export the ImageManifest, but that would still keep the methods exported (i.e., matching it to an interface would still expose them); un-exporting the method as well makes it clear it's internal (at least currently).

Unexporting the method itself is more of an indicator that the method is "internal" to the ImageManifest itself though, which is not the case here as it's used outside of the struct itself by the ImageService.

Maybe. It's just that an interface could still be matched if it would ever be returned, so doesn't really remain internal; e.g. the errPlatformNotFound is not exported, but still satisfies the errdefs.NotFound interface.

It shouldn't be really problematic (just don't do that!), just thinking if un-exporting it makes it even more clear and it for sure can help looking at code, knowing that it cannot be used outside of the package, without having to go search if it possibly was exported to match some interface .. somewhere..

The `refOrId` parameter is only needed to construct an error in case where the doesn't have the matching platform. Move the responsibility of creating the error message to the caller. Signed-off-by: Paweł Gronowski <[email protected]>

Return a similar error as the graphdrivers implementation when an image was found, but the requested platform is not present locally or in the image. The message doesn't include the "actual" platform, as it doesn't make sense with the multi-platform images. With graphdrivers all images were single platform. Signed-off-by: Paweł Gronowski <[email protected]>

All two usages only care about the "first" result from the slice sorted according to the platform preference. Signed-off-by: Paweł Gronowski <[email protected]>

Only two fields from the whole image config are used, so only unmarshal these. Signed-off-by: Paweł Gronowski <[email protected]>

When `getBestPresentImageManifest` fails with `errPlatformNotFound` - fill the requested platform. Signed-off-by: Paweł Gronowski <[email protected]>

Refactor a pattern where a passed `*ocispec.Platform` was used to create a platform matcher that matches the passed platform if not nil and uses a default host platform otherwise into a separate function. Also add some basic unit tests for its behavior. Signed-off-by: Paweł Gronowski <[email protected]>

Allowing it to obtain a specific `ocispec.Platform` used to create this platform matcher. Signed-off-by: Paweł Gronowski <[email protected]>

thaJeztah

LGTM

vvoland added status/2-code-review kind/refactor PR's that refactor, or clean-up code containerd-integration Issues and PRs related to containerd integration labels Jul 31, 2024

vvoland added this to the 28.0.0 milestone Jul 31, 2024

vvoland self-assigned this Jul 31, 2024

vvoland force-pushed the c8d-image-refactor branch from 5338fd4 to ff766a2 Compare July 31, 2024 12:50

vvoland requested review from laurazard and thaJeztah July 31, 2024 16:33

vvoland force-pushed the c8d-image-refactor branch from a00af73 to b729268 Compare August 1, 2024 07:48

thaJeztah reviewed Aug 1, 2024

View reviewed changes

Comment thread daemon/containerd/image.go Outdated

laurazard reviewed Aug 1, 2024

View reviewed changes

laurazard approved these changes Aug 1, 2024

View reviewed changes

vvoland requested a review from thaJeztah August 5, 2024 10:10

thaJeztah reviewed Aug 5, 2024

View reviewed changes

vvoland added 7 commits August 5, 2024 14:30

daemon/c8d: Simplify presentImages signature

e73c2a0

The `refOrId` parameter is only needed to construct an error in case where the doesn't have the matching platform. Move the responsibility of creating the error message to the caller. Signed-off-by: Paweł Gronowski <[email protected]>

c8d: Simplify presentImages into getBestPresentImageManifest

b9d04c0

All two usages only care about the "first" result from the slice sorted according to the platform preference. Signed-off-by: Paweł Gronowski <[email protected]>

c8d/history: Unmarshal only a subset of ImageConfig

427b111

Only two fields from the whole image config are used, so only unmarshal these. Signed-off-by: Paweł Gronowski <[email protected]>

daemon/c8d: Fill wanted platform in GetImageManifest

6617cdc

When `getBestPresentImageManifest` fails with `errPlatformNotFound` - fill the requested platform. Signed-off-by: Paweł Gronowski <[email protected]>

c8d: Wrap matchRequestedOrDefault result with a platform

ee766ae

Allowing it to obtain a specific `ocispec.Platform` used to create this platform matcher. Signed-off-by: Paweł Gronowski <[email protected]>

vvoland force-pushed the c8d-image-refactor branch from f1e4051 to ee766ae Compare August 5, 2024 12:30

thaJeztah approved these changes Aug 6, 2024

View reviewed changes

thaJeztah merged commit a1bcba8 into moby:master Aug 6, 2024

	im, err := i.getBestPresentImageManifest(ctx, img, platform)
	if err != nil {
	var e *errPlatformNotFound
	if errors.As(err, &e) {
	e.wanted = platforms.DefaultSpec()
	}

Conversation

vvoland commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vvoland commented Jul 31, 2024 •

edited

Loading