Skip to content
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.

Sign up for GitHub

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

Jump to bottom

set correct media types during manifest updates and conversions #563

Merged
merged 1 commit into from
Sep 16, 2019
Merged

set correct media types during manifest updates and conversions #563

merged 1 commit into from
Sep 16, 2019

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Jan 21, 2019 •
edited
Loading

When copying an image, record the compression in the BlobInfo and use
the information when updating the manifest's layer infos to set the
layers' media types correctly.

Fixes: github.com/containers/podman/issues/2013
Fixes: github.com/containers/buildah/issues/1589

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@vrothberg
Copy link
Member Author

Tested locally and maybe incomplete w.r.t. to real mime type of the destination layers.

@vrothberg vrothberg mentioned this pull request Jan 21, 2019
Closed
@vrothberg
Copy link
Member Author

@mtrmac @runcom can you have a look at this? Skopeo CI is passing but I need your pairs of eyes :) Note that I only changed directory destination to return the mediaType. There might be other cases where it needs to be updated as well.

mtrmac
mtrmac reviewed Jan 21, 2019
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK on the copy idea, though it will need generalizing.

I can’t see why the dir: part is necessary (or if it is, why none of the other transports need something similar).

copy/copy.go Outdated Show resolved Hide resolved
copy/copy.go Outdated Show resolved Hide resolved
directory/directory_dest.go Outdated Show resolved Hide resolved
directory/directory_dest.go Outdated Show resolved Hide resolved
@mtrmac
Copy link
Collaborator

mtrmac commented Jan 21, 2019

Also, Buildah has some code to set up / modify manifest MIME types, please check whether this c/image feature can replace it, or to what extent.

@vrothberg
Copy link
Member Author

Also, Buildah has some code to set up / modify manifest MIME types, please check whether this c/image feature can replace it, or to what extent.

Thanks for the pointer!

@vrothberg
Copy link
Member Author

Also, Buildah has some code to set up / modify manifest MIME types, please check whether this c/image feature can replace it, or to what extent.

I think that's the code in question, right?
https://github.com/containers/buildah/blob/4bcddb7cbec960f93f4fea229cc45d9192349b17/cmd/buildah/push.go#L149-L161

@mtrmac
Copy link
Collaborator

mtrmac commented Jan 22, 2019

Also, Buildah has some code to set up / modify manifest MIME types, please check whether this c/image feature can replace it, or to what extent.

I think that's the code in question, right?
https://github.com/containers/buildah/blob/4bcddb7cbec960f93f4fea229cc45d9192349b17/cmd/buildah/push.go#L149-L161

No (no mention of compression).

https://github.com/containers/buildah/blob/4bcddb7cbec960f93f4fea229cc45d9192349b17/image.go#L112 and callers. (My overall impression is that this should not be reused, but it should mostly go away; at least in the case of pushes to docker://, buildah should not be creating temporary files and compressing layers into them, for GetBlob to then stream the temporary files — it should just provide the underlying uncompressed data and let copy.Image etc. do the compression and manifest updates. OTOH I’m not sure this is 100% applicable to all the possible destination transports.)

@vrothberg
Copy link
Member Author

https://github.com/containers/buildah/blob/4bcddb7cbec960f93f4fea229cc45d9192349b17/image.go#L112 and callers. (My overall impression is that this should not be reused, but it should mostly go away; at least in the case of pushes to docker://, buildah should not be creating temporary files and compressing layers into them, for GetBlob to then stream the temporary files — it should just provide the underlying uncompressed data and let copy.Image etc. do the compression and manifest updates. OTOH I’m not sure this is 100% applicable to all the possible destination transports.)

@TomSweeneyRedHat @nalind @rhatdan FYI, we might need to revisit that code in buildah.

@rhatdan
Copy link
Member

rhatdan commented Jan 22, 2019

I think this is @nalind code. I am fine with changing it.

@rhatdan
Copy link
Member

rhatdan commented Mar 8, 2019

@vrothberg Any movement on this?

@vrothberg
Copy link
Member Author

@vrothberg Any movement on this?

No, but I need to get this done soon. Other things flew in, but my PR-pipeline is getting smaller.

@rhatdan
Copy link
Member

rhatdan commented Apr 25, 2019

@vrothberg Ping, looking to create a new release of containers image, any movement?

@vrothberg vrothberg mentioned this pull request Jul 29, 2019
Closed
@vrothberg vrothberg changed the title add uncompressed media type WIP - copy: set media types Jul 29, 2019
@vrothberg
Copy link
Member Author

@mtrmac, PTAL. The current approach works in local smoke tests but maybe I miss something.

@mtrmac mtrmac mentioned this pull request Jul 29, 2019
Merged
@vrothberg vrothberg force-pushed the copy-update-mime-types branch 3 times, most recently from 00c1d1f to 529f074 Compare August 1, 2019 16:09
vrothberg
vrothberg commented Aug 1, 2019
View reviewed changes
storage/storage_image.go Outdated Show resolved Hide resolved
@mtrmac mtrmac mentioned this pull request Aug 5, 2019
Closed
@vrothberg vrothberg mentioned this pull request Aug 21, 2019
Merged
mtrmac
mtrmac reviewed Aug 22, 2019
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(A quick pass, may not be quite comprehensive. Highlights: c/storage, and where should / shouldn’t BlobInfo.Compression* be available.)

types/types.go Outdated Show resolved Hide resolved
copy/copy.go Outdated Show resolved Hide resolved
copy/copy.go Show resolved Hide resolved
manifest/docker_schema2.go Outdated Show resolved Hide resolved
image/oci.go Show resolved Hide resolved
pkg/compression/compression.go Outdated Show resolved Hide resolved
pkg/compression/compression.go Outdated Show resolved Hide resolved
storage/storage_image.go Outdated Show resolved Hide resolved
types/types.go Show resolved Hide resolved
manifest/manifest.go Outdated Show resolved Hide resolved
@vrothberg vrothberg force-pushed the copy-update-mime-types branch 2 times, most recently from b93a495 to 01d0302 Compare August 26, 2019 08:31
@vrothberg vrothberg force-pushed the copy-update-mime-types branch 2 times, most recently from af7f9bc to 81ee6de Compare August 26, 2019 14:47
@vrothberg
Copy link
Member Author

@mtrmac, if you have some cycles, PTAL. Notable changes from today:

  • Added tests to image (image conversions) and manifest (media type changes)
  • Completed non-distributable media types, their treatment during manifest updates and during image conversions.
  • Some more documentation.
  • Addressed previous comments.

I'll do some more smoke tests tomorrow but a code-review would be great.

@vrothberg vrothberg changed the title WIP - copy: set media types copy: set media types Aug 27, 2019
@vrothberg vrothberg changed the title copy: set media types set correct media types during manifest updates and conversions Aug 27, 2019
@vrothberg
Copy link
Member Author

Smoke tests are looking good. CI is green. We're getting closer.

The last remaining issue is whether we should pretend that Schema 2 supported Zstd or not (and error out during conversion).

@estesp, once that's in and re-vendored, buildah/containerd will like each other again.

@vrothberg
Copy link
Member Author

@mtrmac, @nalind, PTAL.

@containers containers deleted a comment from vrothberg Aug 29, 2019
@vrothberg
Copy link
Member Author

Rebased to pull in the go module.

@mtrmac @nalind, can we get this in? Other PRs are depending on the changes and there are some warts when building containers letting containerd (validly) reject the images.

@rhatdan
Copy link
Member

rhatdan commented Sep 4, 2019

I think we should throw an error unless you are using OCI images.

@vrothberg
Copy link
Member Author

I think we should throw an error unless you are using OCI images.

@mtrmac @nalind @giuseppe , do you agree on limiting zstd support to OCI and error out for non-OCI ones?

@giuseppe
Copy link
Member

giuseppe commented Sep 6, 2019

@vrothberg that works for me

@vrothberg
Copy link
Member Author

@giuseppe @mtrmac @rhatdan, I updated the PR to limit zstd support to OCIv1. This changed the compat matrix during conversion, so I added some more aggressive checks for supported media types.

PTAL, I really want to get this PR off my table.

@vrothberg
media type checks
69aa1e8 
When copying an image, record the compression in the BlobInfo and use
the information when updating the manifest's layer infos to set the
layers' media types correctly.

Also check for supported media types when parsing a v2s2/OCI1 manifest.

Note that consumers of the containers/image library need to update
opencontainers/image-spec to commit 775207bd45b6cb8153ce218cc59351799217451f.

Fixes: github.com/containers/podman/issues/2013
Fixes: github.com/containers/buildah/issues/1589

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2019

LGTM
@mtrmac @giuseppe PTAL

@vrothberg
Copy link
Member Author

@nalind, mind taking a look?

We are receiving more and more requests to get those fixes finally in and I am getting impatient.

This was referenced Sep 11, 2019
Closed
Merged
Merged
@vrothberg
Copy link
Member Author

Folks, we need this in. Not only to unblock other PRs but also to ship these fixes.

@rhatdan @mtrmac @giuseppe @nalind PTAL

@giuseppe
Copy link
Member

LGTM

@giuseppe giuseppe merged commit b344f2f into containers:master Sep 16, 2019
@mtrmac mtrmac mentioned this pull request Oct 3, 2019
Closed
@vrothberg vrothberg deleted the copy-update-mime-types branch January 4, 2020 15:39
@thaJeztah thaJeztah mentioned this pull request Apr 20, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vrothberg @mtrmac @rhatdan @giuseppe