content: change Writer/ReaderAt to take OCI descriptor by AkihiroSuda · Pull Request #2135 · containerd/containerd

AkihiroSuda · 2018-02-15T10:21:53Z

This change allows implementations to resolve the location of the actual
data using OCI descriptor fields such as MediaType.

No OCI descriptor field is written to the store.

No change on gRPC API.

~~contrib/registrycontentstore is added as an example usecase of this.~~ (EDIT: will be a separate PR)

Signed-off-by: Akihiro Suda [email protected]

Close #2129

@tonistiigi @dmcgowan

AkihiroSuda · 2018-02-15T11:08:15Z

This PR has no impact on gRPC API but changes this behavior.
I believe the previous behavior was bug.

Does size need to be updated in this case then? The section reader uses the reader at interface which may cause a seek to be performed beyond the size.

https://github.com/containerd/containerd/pull/2135/files#diff-b0e6ee8a9e6cf0f8ef1a8a4d2817b674R93 fails when the blob is < 512B

split this fix as #2221

codecov-io · 2018-02-15T11:34:13Z

Codecov Report

Merging #2135 into master will decrease coverage by 3.9%.
The diff coverage is 46.83%.

@@            Coverage Diff            @@
##           master   #2135      +/-   ##
=========================================
- Coverage   45.11%   41.2%   -3.91%     
=========================================
  Files          92      66      -26     
  Lines        9358    7809    -1549     
=========================================
- Hits         4222    3218    -1004     
+ Misses       4457    4086     -371     
+ Partials      679     505     -174

Flag	Coverage Δ
#linux	`?`
#windows	`41.2% <46.83%> (-0.21%)`	⬇️

Impacted Files	Coverage Δ
content/helpers.go	`22.64% <0%> (ø)`	⬆️
oci/spec_opts_windows.go	`0% <0%> (ø)`	⬆️
images/oci/importer.go	`5.3% <0%> (ø)`	⬆️
images/oci/exporter.go	`0% <0%> (ø)`	⬆️
content/content.go	`0% <0%> (ø)`	⬆️
content/local/store.go	`51.96% <53.84%> (-0.16%)`	⬇️
metadata/content.go	`39.53% <66.66%> (+0.11%)`	⬆️
snapshots/native/native.go	`1.8% <0%> (-42.09%)`	⬇️
oci/spec.go	`0% <0%> (-40%)`	⬇️
archive/tar.go	`16.85% <0%> (-26.2%)`	⬇️
... and 36 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 e4ad710...d88de4a. Read the comment docs.

dmcgowan · 2018-02-15T18:59:28Z

Can we split out the registry test and contib into a separate PR and focus on the interface change. Feel free to point at that work for justification.

AkihiroSuda · 2018-02-16T03:06:18Z

@dmcgowan
updated. (bak: https://github.com/AkihiroSuda/containerd/releases/tag/oci-content-store.20180216)

tonistiigi · 2018-02-16T19:06:30Z

This should take the descriptor as a parameter instead. All cases that I found where this was called it already took a digest from a descriptor before calling.

tonistiigi · 2018-02-16T19:11:02Z

This does not look like a correct assumption as a reader is supposed to track previous reads. Likely doesn't appear atm because tests use small blobs.

AkihiroSuda · 2018-02-19T05:27:01Z

@tonistiigi updated. I changed WriteBlob as well accordingly.

stevvooe · 2018-02-26T18:32:45Z

Please, no oci descriptor on write.

stevvooe · 2018-02-26T18:34:02Z

See this is wrong: the Descriptor is not for passing optional arguments.

stevvooe · 2018-02-26T18:36:44Z

In general, this is a problematic change. The content store is supposed to be purely content addressed with no understanding of types. The resolver/fetcher layer is the system to use for handling type aware content operations.

tonistiigi · 2018-02-26T19:19:17Z

The content store is supposed to be purely content addressed with no understanding of types.

That does not change. It doesn't change the store but the interface. Types are not stored and are not part of the interface guarantees. It is just a hint that an implementation can take. Data written with a specific digest will always return back for the same digest. It would be problematic if Provider returns back a type but it doesn't.

The resolver/fetcher layer is the system to use for handling type aware content operations.

That's the problem. There are duplicate stacks implemented atm that can't be used together for a lot of useful use-cases nor extended with new implementations. Code for copying content needs to be specific for the implementations and has a separate set of helper functions. Alternatively, this could be implemented with new fetcher/pusher interfaces that bring them to the same functionality of content.Provider/Writer and then removing all the use cases (I guess apart from that internal testcase use-case that you left a comment on) where the old types are currently being used.

AkihiroSuda · 2018-04-06T07:49:13Z

Closing temporarily, until we can reach consensus on #2129

AkihiroSuda · 2018-05-21T08:59:26Z

reopened PR

@stevvooe @tonistiigi @dmcgowan

AkihiroSuda · 2018-05-21T08:59:57Z

cc @Random-Liu
Is this correct way?

I believe the correct way is to update it in a fork and point the vendor to your fork. We should never do a merge into master without validated vendors.

dmcgowan · 2018-05-21T23:11:14Z

Should we handle the empty ref case now? This looks dangerous with an empty ref and could easily be omitted now.

updated to require non-empty ref

Signed-off-by: Akihiro Suda <[email protected]>

dmcgowan

LGTM

AkihiroSuda · 2018-05-26T09:08:08Z

rebased

AkihiroSuda · 2018-05-30T00:30:16Z

@stevvooe PTAL?

AkihiroSuda · 2018-05-31T07:36:26Z

rebased again

This change allows implementations to resolve the location of the actual data using OCI descriptor fields such as MediaType. No OCI descriptor field is written to the store. No change on gRPC API. Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2018-06-01T02:52:11Z

rererebased

dmcgowan · 2018-06-01T17:09:52Z

re-LGTM, let's get this merged because rebasing vendors like that is not fun

crosbymichael · 2018-06-01T17:14:31Z

LGTM

thaJeztah · 2018-06-01T18:57:58Z

-# cri dependencies
-github.com/containerd/cri b68fb075d49aa1c2885f45f2467142666c244f4a
+# #2135: cri is temporarily forked because of circular dependency. will be fixed immediately in a follow-up PR.
+github.com/containerd/cri 6e975823be192ad19f2ce7afcf6c57b88a991c30 https://github.com/AkihiroSuda/cri-containerd.git


@AkihiroSuda I spotted this one, but couldn't fine an associated PR in https://github.com/containerd/cri - are you working on this?

oh! never mind; I see that this was just merged; I looked at the commit-date.

For containerd/containerd#2135 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2018-06-02T02:44:49Z

opened containerd/cri#799

For containerd/containerd#2135 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda mentioned this pull request Feb 15, 2018

[RFC] content: change Writer/ReaderAt to take OCI descriptor #2129

Closed

AkihiroSuda force-pushed the oci-content-store branch 3 times, most recently from f290420 to c9a252a Compare February 15, 2018 11:06

AkihiroSuda commented Feb 15, 2018

View reviewed changes

AkihiroSuda force-pushed the oci-content-store branch from c9a252a to d4a1dc8 Compare February 15, 2018 11:21

AkihiroSuda force-pushed the oci-content-store branch from d4a1dc8 to e43e873 Compare February 16, 2018 03:05

tonistiigi reviewed Feb 16, 2018

View reviewed changes

AkihiroSuda force-pushed the oci-content-store branch from e43e873 to 32164da Compare February 19, 2018 05:26

stevvooe reviewed Feb 26, 2018

View reviewed changes

Comment thread content/helpers.go Outdated

Copy link
Copy Markdown

Member

stevvooe Feb 26, 2018

Choose a reason for hiding this comment

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

Please, no oci descriptor on write.

stevvooe reviewed Feb 26, 2018

View reviewed changes

AkihiroSuda force-pushed the oci-content-store branch 3 times, most recently from 3e037b0 to c878493 Compare March 22, 2018 06:42

AkihiroSuda mentioned this pull request Mar 22, 2018

services/content: fix reading a blob which is smaller than the read buffer #2221

Merged

AkihiroSuda closed this Apr 6, 2018

AkihiroSuda reopened this May 21, 2018

AkihiroSuda force-pushed the oci-content-store branch from c878493 to e7b4553 Compare May 21, 2018 08:58

AkihiroSuda commented May 21, 2018

View reviewed changes

AkihiroSuda mentioned this pull request May 21, 2018

decouple remotecache from Docker registry moby/buildkit#278

Merged

AkihiroSuda force-pushed the oci-content-store branch 2 times, most recently from 4178699 to 4d409bf Compare May 21, 2018 10:21

AkihiroSuda force-pushed the oci-content-store branch from 4d409bf to 184a351 Compare May 21, 2018 11:04

dmcgowan reviewed May 21, 2018

View reviewed changes

AkihiroSuda added a commit to AkihiroSuda/cri-containerd that referenced this pull request May 22, 2018

update containerd (containerd/containerd#2135)

b91ca77

Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the oci-content-store branch from 184a351 to 0f29852 Compare May 22, 2018 06:04

dmcgowan approved these changes May 22, 2018

View reviewed changes

AkihiroSuda force-pushed the oci-content-store branch from 0f29852 to 533229a Compare May 26, 2018 09:08

AkihiroSuda force-pushed the oci-content-store branch from 533229a to 1f8c612 Compare May 31, 2018 07:33

AkihiroSuda force-pushed the oci-content-store branch from 1f8c612 to c3ddc1e Compare May 31, 2018 08:04

AkihiroSuda force-pushed the oci-content-store branch from c3ddc1e to d88de4a Compare June 1, 2018 02:52

crosbymichael merged commit 5b1f69b into containerd:master Jun 1, 2018

thaJeztah reviewed Jun 1, 2018

View reviewed changes

thaJeztah mentioned this pull request Jun 1, 2018

Update containerd and dependencies moby/buildkit#422

Merged

AkihiroSuda added a commit to AkihiroSuda/cri-containerd that referenced this pull request Jun 2, 2018

vendor containerd (#2135)

f2dc3eb

For containerd/containerd#2135 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda mentioned this pull request Jun 2, 2018

vendor containerd (#2135) containerd/cri#799

Merged

AkihiroSuda added a commit to AkihiroSuda/cri-containerd that referenced this pull request Jun 2, 2018

vendor containerd (#2135)

1a38f7f

For containerd/containerd#2135 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda added a commit to AkihiroSuda/cri-containerd that referenced this pull request Jun 2, 2018

vendor containerd (#2135)

0972490

For containerd/containerd#2135 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda mentioned this pull request Jun 7, 2018

schema1 puller is not working (seems regression in #2135) #2389

Closed

Conversation

AkihiroSuda commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

codecov-io commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dmcgowan commented Feb 15, 2018

Uh oh!

AkihiroSuda commented Feb 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Feb 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Feb 26, 2018

Uh oh!

tonistiigi commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Apr 6, 2018

Uh oh!

AkihiroSuda commented May 21, 2018

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!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented May 26, 2018

Uh oh!

AkihiroSuda commented May 30, 2018

Uh oh!

AkihiroSuda commented May 31, 2018

Uh oh!

AkihiroSuda commented Jun 1, 2018

Uh oh!

dmcgowan commented Jun 1, 2018

Uh oh!

crosbymichael commented Jun 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AkihiroSuda commented Feb 15, 2018 •

edited

Loading

codecov-io commented Feb 15, 2018 •

edited

Loading

tonistiigi commented Feb 26, 2018 •

edited

Loading