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

Adds support for oci manifests and manifestlists #2076

Merged
merged 21 commits into from Jul 20, 2018

Conversation

@mikebrow
Contributor

mikebrow commented Nov 21, 2016

Here is a draft of the oci manifest/manifest lists that addresses @stevvooe 's request in pr #2021 for oci manifest to be it's own schema.

Also fixes some test case failure issues.

Signed-off-by: Mike Brown brownwm@us.ibm.com

Show outdated Hide outdated registry/handlers/images.go Outdated
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 21, 2016

Codecov Report

Merging #2076 into master will decrease coverage by 9.25%.
The diff coverage is 71.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2076      +/-   ##
==========================================
- Coverage    60.8%   51.55%   -9.26%     
==========================================
  Files         129      132       +3     
  Lines       11901    12122     +221     
==========================================
- Hits         7236     6249     -987     
- Misses       3764     5092    +1328     
+ Partials      901      781     -120
Impacted Files Coverage Δ
blobs.go 62.5% <ø> (ø) ⬆️
manifest/schema2/manifest.go 87.8% <100%> (+6.22%) ⬆️
registry/storage/manifeststore.go 84.05% <100%> (+2.7%) ⬆️
registry/storage/registry.go 90.54% <100%> (+0.39%) ⬆️
registry/handlers/manifests.go 53.64% <50%> (-1.64%) ⬇️
registry/storage/ocimanifesthandler.go 67.79% <67.79%> (ø)
manifest/ocischema/builder.go 73.8% <73.8%> (ø)
manifest/manifestlist/manifestlist.go 76.71% <80%> (+1.71%) ⬆️
manifest/ocischema/manifest.go 80.48% <80.48%> (ø)
registry/storage/driver/gcs/gcs.go 0.39% <0%> (-68.67%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 749f6af...20aecf1. Read the comment docs.

codecov-io commented Nov 21, 2016

Codecov Report

Merging #2076 into master will decrease coverage by 9.25%.
The diff coverage is 71.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2076      +/-   ##
==========================================
- Coverage    60.8%   51.55%   -9.26%     
==========================================
  Files         129      132       +3     
  Lines       11901    12122     +221     
==========================================
- Hits         7236     6249     -987     
- Misses       3764     5092    +1328     
+ Partials      901      781     -120
Impacted Files Coverage Δ
blobs.go 62.5% <ø> (ø) ⬆️
manifest/schema2/manifest.go 87.8% <100%> (+6.22%) ⬆️
registry/storage/manifeststore.go 84.05% <100%> (+2.7%) ⬆️
registry/storage/registry.go 90.54% <100%> (+0.39%) ⬆️
registry/handlers/manifests.go 53.64% <50%> (-1.64%) ⬇️
registry/storage/ocimanifesthandler.go 67.79% <67.79%> (ø)
manifest/ocischema/builder.go 73.8% <73.8%> (ø)
manifest/manifestlist/manifestlist.go 76.71% <80%> (+1.71%) ⬆️
manifest/ocischema/manifest.go 80.48% <80.48%> (ø)
registry/storage/driver/gcs/gcs.go 0.39% <0%> (-68.67%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 749f6af...20aecf1. Read the comment docs.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 21, 2016

Contributor

@mikebrow This seems like a reasonable start.

Where does this sit vs #2021?

Contributor

stevvooe commented Nov 21, 2016

@mikebrow This seems like a reasonable start.

Where does this sit vs #2021?

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 21, 2016

Contributor

Where does this sit vs #2021?

For example, should we close it or just take it as a proposed change?

Contributor

stevvooe commented Nov 21, 2016

Where does this sit vs #2021?

For example, should we close it or just take it as a proposed change?

Show outdated Hide outdated manifest/ocischema/builder.go Outdated
@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Nov 21, 2016

Contributor

I recoded 2021 (starting mostly from scratch)... to fix some testing bugs in #2021, and more importantly to move it from the design in 2021 where the docker schema2 was overloaded to also have oci support in it, to a design with a new schema type. You didn't like the design in 2021 so I started it over :-) This can be merged on top of 2021 or we can just start from here.

Contributor

mikebrow commented Nov 21, 2016

I recoded 2021 (starting mostly from scratch)... to fix some testing bugs in #2021, and more importantly to move it from the design in 2021 where the docker schema2 was overloaded to also have oci support in it, to a design with a new schema type. You didn't like the design in 2021 so I started it over :-) This can be merged on top of 2021 or we can just start from here.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 21, 2016

Contributor

@mikebrow This approach looks like a good start.

Mostly, we would like to make this as simple as using distribution.UnmarshalManifest. I am seeing a few interface fixes we need to address to make this work in all cases. I wonder if we refactor the tests from there, if we can get a better gauge on the quality of the package structure.

Contributor

stevvooe commented Nov 21, 2016

@mikebrow This approach looks like a good start.

Mostly, we would like to make this as simple as using distribution.UnmarshalManifest. I am seeing a few interface fixes we need to address to make this work in all cases. I wonder if we refactor the tests from there, if we can get a better gauge on the quality of the package structure.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 22, 2016

Contributor

Thanks for doing this @mikebrow !

Contributor

vbatts commented Nov 22, 2016

Thanks for doing this @mikebrow !

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Nov 24, 2016

Contributor

@vbatts Should we close #2021?

Contributor

stevvooe commented Nov 24, 2016

@vbatts Should we close #2021?

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Nov 24, 2016

Contributor
Contributor

vbatts commented Nov 24, 2016

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 5, 2017

Contributor

What's next here?

Contributor

vbatts commented Apr 5, 2017

What's next here?

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Apr 5, 2017

Member

@vbatts v1.0 spec hopefully :)

Discussed this a little bit yesterday. Given the current timeline we should aim for having this in the 2.8 registry release. We can possibly time such a release around the release of v1.0 image spec as well. The 2.7 release should probably be done sooner and not wait for this change.

Would it be helpful if we milestoned this? Maybe we can consider merging this using the image spec release candidate after we cut a 2.7 release branch.

Member

dmcgowan commented Apr 5, 2017

@vbatts v1.0 spec hopefully :)

Discussed this a little bit yesterday. Given the current timeline we should aim for having this in the 2.8 registry release. We can possibly time such a release around the release of v1.0 image spec as well. The 2.7 release should probably be done sooner and not wait for this change.

Would it be helpful if we milestoned this? Maybe we can consider merging this using the image spec release candidate after we cut a 2.7 release branch.

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Apr 5, 2017

Contributor
Contributor

vbatts commented Apr 5, 2017

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jul 8, 2017

Contributor

@mikebrow ready to rebase this??? 🤓

Contributor

vbatts commented Jul 8, 2017

@mikebrow ready to rebase this??? 🤓

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Jul 10, 2017

Contributor

@mikebrow Pretty please? It is time... 💯 🥇

Contributor

stevvooe commented Jul 10, 2017

@mikebrow Pretty please? It is time... 💯 🥇

@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jul 10, 2017

Contributor

cool.. I'm on it..

Contributor

mikebrow commented Jul 10, 2017

cool.. I'm on it..

@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jul 10, 2017

Contributor

👾

Contributor

vbatts commented Jul 10, 2017

👾

@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jul 10, 2017

Contributor

PR rebased.. and nit comments addressed.

Contributor

mikebrow commented Jul 10, 2017

PR rebased.. and nit comments addressed.

@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jul 10, 2017

Contributor

The only comment I didn't address (I believe) is vendoring the image spec and refactoring this code to use it. Want to do that on a subsequent PR or this one? Oh yeah and it's missing annotations support.

Contributor

mikebrow commented Jul 10, 2017

The only comment I didn't address (I believe) is vendoring the image spec and refactoring this code to use it. Want to do that on a subsequent PR or this one? Oh yeah and it's missing annotations support.

@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jul 10, 2017

Contributor

I see two commits went into schema2 that we might want put into ocischema as well.. 8867e8f
9ab7b0e

Contributor

mikebrow commented Jul 10, 2017

I see two commits went into schema2 that we might want put into ocischema as well.. 8867e8f
9ab7b0e

@owtaylor

Not a thorough review, but some things I noticed when I tried taking the last version of the patch and rebasing it to master last week. I'm not very experienced in either the docker/distribution code or the OCI Image spec, so apologies for any misunderstandings!

Show outdated Hide outdated registry/storage/manifeststore.go Outdated
Show outdated Hide outdated manifest/ocischema/manifest.go Outdated
Show outdated Hide outdated manifest/ocischema/manifest.go Outdated
Show outdated Hide outdated manifest/ocischema/manifest.go Outdated

mikebrow and others added some commits Sep 1, 2017

folow commit 9c88801
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
register oci image index
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
Check media types when unmarshalling manifests
When unmarshalling manifests from JSON, check that the MediaType field
corresponds to the type that we are unmarshalling as. This makes sure
that when we retrieve a manifest from the manifest store, it will have
the same type as it was handled as before storing it in the manifest
store.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
Handle OCI manifests and image indexes without a media type
In the OCI image specification, the MediaType field is reserved
and otherwise undefined; assume that manifests without a media
in storage are OCI images or image indexes, and determine which
by looking at what fields are in the JSON. We do keep a check
that when unmarshalling an OCI image or image index, if it has
a MediaType field, it must match that media type of the upload.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
Test storing OCI image manifests and indexes with/without a media type
OCI Image manifests and indexes are supported both with and without
an embeded MediaType (the field is reserved according to the spec).
Test storing and retrieving both types from the manifest store.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>

@owtaylor owtaylor referenced this pull request Jun 19, 2018

Closed

Add Taylor #2614

@GordonTheTurtle

This comment has been minimized.

Show comment
Hide comment
@GordonTheTurtle

GordonTheTurtle Jun 19, 2018

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ocitype" git@github.com:mikebrow/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354379640
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

GordonTheTurtle commented Jun 19, 2018

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "ocitype" git@github.com:mikebrow/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354379640
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Merge pull request #1 from owtaylor/oci-media-types
Handle OCI manifests and image indexes without a media type

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

@GordonTheTurtle GordonTheTurtle removed the dco/no label Jun 19, 2018

@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jun 20, 2018

Contributor

@dmcgowan @crosbymichael FYI.. this is the PR for OCI support in the registry that we discussed at the dockercon containerd maintainer face to face.

Contributor

mikebrow commented Jun 20, 2018

@dmcgowan @crosbymichael FYI.. this is the PR for OCI support in the registry that we discussed at the dockercon containerd maintainer face to face.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jun 25, 2018

Member

Thanks for the ping @mikebrow

Member

crosbymichael commented Jun 25, 2018

Thanks for the ping @mikebrow

@sargun sargun referenced this pull request Jun 25, 2018

Closed

Please tag new release #2623

Show outdated Hide outdated manifest/ocischema/builder.go Outdated
address lint and gofmt issues
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jul 19, 2018

Member

I was testing this with containerd and pushes work but on a pull i get an error

registry logs:

time="2018-07-19T20:10:24.236940177Z" level=error msg="response completed with error" err.code=unknown err.detail="unrecognized manifest schema version 0" err.message="unknown error" go.version=go1.10.3 http.request.host="127.0.0.1:5000" http.request.id=3886b765-a36d-48a5-973c-bc5fc60c6bf9 http.request.method=HEAD http.request.remoteaddr="127.0.0.1:40058" http.request.uri="/v2/redis/manifests/latest" http.request.useragent="Go-http-client/1.1" http.response.contenttype="application/json; charset=utf-8" http.response.duration=6.019726ms http.response.status=500 http.response.written=70 vars.name=redis vars.reference=latest

ctr

sudo ctr images pull --plain-http 127.0.0.1:5000/redis:latest
ctr: failed to resolve reference "127.0.0.1:5000/redis:latest": unexpected status code http://127.0.0.1:5000/v2/redis/manifests/latest: 500 Internal Server Error
Member

crosbymichael commented Jul 19, 2018

I was testing this with containerd and pushes work but on a pull i get an error

registry logs:

time="2018-07-19T20:10:24.236940177Z" level=error msg="response completed with error" err.code=unknown err.detail="unrecognized manifest schema version 0" err.message="unknown error" go.version=go1.10.3 http.request.host="127.0.0.1:5000" http.request.id=3886b765-a36d-48a5-973c-bc5fc60c6bf9 http.request.method=HEAD http.request.remoteaddr="127.0.0.1:40058" http.request.uri="/v2/redis/manifests/latest" http.request.useragent="Go-http-client/1.1" http.response.contenttype="application/json; charset=utf-8" http.response.duration=6.019726ms http.response.status=500 http.response.written=70 vars.name=redis vars.reference=latest

ctr

sudo ctr images pull --plain-http 127.0.0.1:5000/redis:latest
ctr: failed to resolve reference "127.0.0.1:5000/redis:latest": unexpected status code http://127.0.0.1:5000/v2/redis/manifests/latest: 500 Internal Server Error
}
)
func init() {

This comment has been minimized.

@crosbymichael

crosbymichael Jul 19, 2018

Member

is this how distribution handles things or can this be moved out of an init?

@crosbymichael

crosbymichael Jul 19, 2018

Member

is this how distribution handles things or can this be moved out of an init?

This comment has been minimized.

@mikebrow

mikebrow Jul 19, 2018

Contributor

it's how distribution registers the manifest media types (schema types), see:
https://github.com/docker/distribution/blob/master/manifest/schema2/manifest.go#L56
for the example that registers docker's schema2 manifest.

In this case we're setting up to handle "application/vnd.oci.image.manifest.v1+json"

@mikebrow

mikebrow Jul 19, 2018

Contributor

it's how distribution registers the manifest media types (schema types), see:
https://github.com/docker/distribution/blob/master/manifest/schema2/manifest.go#L56
for the example that registers docker's schema2 manifest.

In this case we're setting up to handle "application/vnd.oci.image.manifest.v1+json"

address review comment regarding panic use
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
@mikebrow

This comment has been minimized.

Show comment
Hide comment
@mikebrow

mikebrow Jul 19, 2018

Contributor

I was testing this with containerd and pushes work but on a pull i get an error
registry logs:

..."unrecognized manifest schema version 0" err.message="

@crosbymichael

Initially I was thinking it made sense, given oci schema's near identical alignment with docker schema2 manifests to go with version 2 as the initial oci schema manifest version number.

WDYT make it 0, 1, or stick with 2?

Contributor

mikebrow commented Jul 19, 2018

I was testing this with containerd and pushes work but on a pull i get an error
registry logs:

..."unrecognized manifest schema version 0" err.message="

@crosbymichael

Initially I was thinking it made sense, given oci schema's near identical alignment with docker schema2 manifests to go with version 2 as the initial oci schema manifest version number.

WDYT make it 0, 1, or stick with 2?

@jonjohnsonjr

This comment has been minimized.

Show comment
Hide comment
@jonjohnsonjr

jonjohnsonjr Jul 19, 2018

Contributor

@mikebrow

This REQUIRED property specifies the image manifest schema version. For this version of the specification, this MUST be 2 to ensure backward compatibility with older versions of Docker. The value of this field will not change. This field MAY be removed in a future version of the specification.

https://github.com/opencontainers/image-spec/blob/master/manifest.md#image-manifest-property-descriptions

Contributor

jonjohnsonjr commented Jul 19, 2018

@mikebrow

This REQUIRED property specifies the image manifest schema version. For this version of the specification, this MUST be 2 to ensure backward compatibility with older versions of Docker. The value of this field will not change. This field MAY be removed in a future version of the specification.

https://github.com/opencontainers/image-spec/blob/master/manifest.md#image-manifest-property-descriptions

added test for initial oci schema version
Signed-off-by: Mike Brown <brownwm@us.ibm.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael
Member

crosbymichael commented Jul 20, 2018

LGTM

@dmcgowan

This comment has been minimized.

Show comment
Hide comment
@dmcgowan

dmcgowan Jul 20, 2018

Member

LGTM

Thanks everyone for testing this and your patience. Note to use the OCI manifests with Docker you will need to update to a newer version which has support for these media types. Support is being backported to all supported version of Docker released in the last year.

Member

dmcgowan commented Jul 20, 2018

LGTM

Thanks everyone for testing this and your patience. Note to use the OCI manifests with Docker you will need to update to a newer version which has support for these media types. Support is being backported to all supported version of Docker released in the last year.

@dmcgowan dmcgowan merged commit 0dae095 into docker:master Jul 20, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 71.48% of diff hit (target 60.8%)
Details
codecov/project Absolute coverage decreased by -9.25% but relative coverage increased by +10.68% compared to 749f6af
Details
janky Jenkins build Distribution-PRs 2289 has succeeded
Details
@vbatts

This comment has been minimized.

Show comment
Hide comment
@vbatts

vbatts Jul 20, 2018

Contributor
Contributor

vbatts commented Jul 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment