-
Notifications
You must be signed in to change notification settings - Fork 18.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
c8d/list: Ignore attestation manifests #45124
Conversation
TBH, it feels wrong to actually work around this on our side, and I think attestation manifest should use a different media type. |
2e9adef
to
4e97d90
Compare
Unrelated CI failure on Windows |
4e97d90
to
aae50bd
Compare
One thing worth mentioning is that we need to look at this / revisit this as part of the UX discussion in; |
}).Warn("checking availability of platform content failed") | ||
return nil, nil | ||
} | ||
if !available || len(missing) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly a "note to self" (and others?) to check in what cases missing > 0
, and to verify if there's cases in the containerd-integration;
- are there cases where this would skip image(s) where we still want to show them?
- cases where we must NOT ignore partials (thinking of
delete
image,prune
image etc)
TBH, the containerd docs could use some improvement; https://github.com/containerd/containerd/blob/584d13d5cb350b48e64eb7c7b0e3e935b941e0d1/images/image.go#L294-L304
"missing will have the components that are part of required but not available in the provider."
- what are
components
? - what
components
are required ? - the whole signature is confusing (a
bool
, 3(!) slices, and anerror
) - ^^ lots of ambiguity there
Looks like it was originally added in containerd/containerd@c555df5, and that commit message contains an example of some of this information;
images: support checking status of image content
The
Check
function returns information about an image's content components
over a content provider. From this information, one can tell which content is
required, present or missing to run an image.The utility can be demonstrated with the
check
command:$ ctr images check REF TYPE DIGEST STATUS SIZE docker.io/library/alpine:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:f006ecbb824d87947d0b51ab8488634bf69fe4094959d935c0c103f4820a417d incomplete (1/2) 1.5 KiB/1.9 MiB docker.io/library/postgres:latest application/vnd.docker.distribution.manifest.v2+json sha256:2f8080b9910a8b4f38ff5a55a82e77cb43d88bdbb16d723c71d18493590832e9 complete (13/13) 99.3 MiB/99.3 MiB docker.io/library/redis:alpine application/vnd.docker.distribution.manifest.v2+json sha256:e633cded055a94202e4ccccb8125b7f383cd6ee56527ab890db643383a2647dd incomplete (6/7) 8.1 MiB/10.0 MiB docker.io/library/ubuntu:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:60f835698ea19e8d9d3a59e68fb96fb35bc43e745941cb2ea9eaf4ba3029ed8a unavailable (0/?) 0.0 B/? docker.io/trollin/busybox:latest application/vnd.docker.distribution.manifest.list.v2+json sha256:54a6424f7a2d5f4f27b3d69e5f9f2bc25fe9087f0449d3cb4215db349f77feae complete (2/2) 699.9 KiB/699.9 KiBThe above shows us that we have two incomplete images and one that is
unavailable. The incomplete images are those that we know the complete
size of all content but some are missing. "Unavailable" means that the
check could not get enough information about the image to get its full
size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, I hit the missing > 0
case mostly after playing for some time with building images (building, removing, cancelling build) which possibly could leave some blobs in the content store (as a cache), and lead to a situation where the manifest for some platform is present, but its rootfs is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks like for this case it's the right thing to do. The function on containerd is just quite confusing, and we likely do have to take some of these into account when pruning (not sure if that uses this function from the image-service though, but something we may have to check).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but;
- left a suggestion for unmarshaling (let me know if you think that would make sense)
- we should look at some test-cases for this (can be a follow-up); looks like it would be difficult to unit-test this 🤔
daemon/containerd/image_list.go
Outdated
@@ -303,3 +332,47 @@ func computeSharedSize(chainIDs []digest.Digest, layers map[digest.Digest]int, s | |||
} | |||
return sharedSize, nil | |||
} | |||
|
|||
func getManifestPlatform(ctx context.Context, store content.Store, desc v1.Descriptor) (v1.Platform, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we could (at some point?) use from
moby/vendor/github.com/containerd/containerd/image.go
Lines 432 to 444 in 1855a55
func (i *image) getManifestPlatform(ctx context.Context, manifest ocispec.Manifest) (ocispec.Platform, error) { | |
cs := i.ContentStore() | |
p, err := content.ReadBlob(ctx, cs, manifest.Config) | |
if err != nil { | |
return ocispec.Platform{}, err | |
} | |
var image ocispec.Image | |
if err := json.Unmarshal(p, &image); err != nil { | |
return ocispec.Platform{}, err | |
} | |
return platforms.Normalize(ocispec.Platform{OS: image.OS, Architecture: image.Architecture}), nil | |
} |
Or are there improvements from readConfig()
that we should contribute to containerd? (looks like our code has some more error-handling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c8d version needs to be passed an already unmarshaled manifest object and our version takes care of it for us.
If we have more code using this kind of helper, we may think about contributing them directly to the c8d, but I wouldn't do that before we have some visible patterns in using it.
aae50bd
to
0a468ab
Compare
0a468ab
to
2e011ad
Compare
It's unfortunate that the attestation manifest needs to use the image manifest mediaType for compatibility. The attestation doc says that the manifest's |
2e011ad
to
bb3dd25
Compare
Do we need a tracking issue to "un-skip" these once we get to the tree-views as discussed in #44582 (and assuming these will still be included as "image")? Thinking if we want to show these in the tree (if they're there), or if we should continue hiding them from the user. |
I like the idea - added a layer check. Still left the explicit Annotation check though. It's cheap as it doesn't involve reading and deserializing the manifest json. |
I think they should be shown, but in a different way than an actual image. (Btw, in last force push I fixed a typo in comment, which made the linter sad) |
bb3dd25
to
f3a3c03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Attestation manifests have an OCI image media type, which makes them being listed like they were a separate platform supported by the image. Don't use `images.Platforms` and walk the manifest list ourselves looking for all manifests that are an actual image manifest. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
f3a3c03
to
92e38b6
Compare
Unrelated buildkit test failure:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Related to:
Attestation manifests have an OCI image media type, which makes them being listed like they were a separate platform supported by the image:
- What I did
Fix
docker images
listing a separate image for attestation manifest.- How I did it
Don't use
images.Platforms
and walk the manifest list ourselves looking for all manifests that are an actual image manifest.- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)