OCI: add a sharedBlobDir mode #369

jonboulle · 2017-10-26T14:30:13Z

This is a POC/RFC for allowing users to specify an external blob directory for OCI image layout sources and destinations. The OCI image spec stipulates that image layouts must have a blobs subdirectory, but that it may be empty and implementations may fetch blobs from other sources)

For our use case, we're forced to use a Docker V2 registry as a canonical store but prefer to use OCI when building and manipulating images locally. So a typical workflow involves a skopeo copy docker://image oci:image; umoci unpack; umoci repack; skopeo copy oci:image docker:// sequence. Since we have various shared layers we want to improve the performance of the copies as much as possible by sharing blobs, but this isn't possible with the current implementation because blobs are always stored within each independent layout.

Here's an example of this in action in conjunction with the corresponding umoci change (see https://github.com/openSUSE/umoci/pull/190) and a small skopeo patch (https://github.com/nstack/skopeo/commit/1854cbea233a234fe0dfb72bf97de25405dcb853):

# /home/jon/src/skopeo/skopeo copy docker://busybox:latest oci:busybox-1:latest --dest-shared-blob-dir=./blobs
Getting image source signatures
Copying blob sha256:03b1be98f3f9b05cb57782a3a71a44aaf6ec695de5f4f8e6c1058cd42f04953e
 698.42 KB / 698.42 KB [====================================================] 0s
Copying config sha256:cbd374fddd3ee5b8792fb6ae179676cfd3673e91c6afee89b7286ff0c2c0ce61
 575 B / 575 B [============================================================] 0s
Writing manifest to image destination
Storing signatures
# /home/jon/src/skopeo/skopeo copy docker://busybox:latest oci:busybox-2:latest --dest-shared-blob-dir=./blobs
Getting image source signatures
Skipping fetch of repeat blob sha256:03b1be98f3f9b05cb57782a3a71a44aaf6ec695de5f4f8e6c1058cd42f04953e
Copying config sha256:cbd374fddd3ee5b8792fb6ae179676cfd3673e91c6afee89b7286ff0c2c0ce61
 575 B / 575 B [============================================================] 0s
Writing manifest to image destination
Storing signatures
# find 
.
./busybox-2
./busybox-2/index.json
./busybox-2/oci-layout
./blobs
./blobs/sha256
./blobs/sha256/cbd374fddd3ee5b8792fb6ae179676cfd3673e91c6afee89b7286ff0c2c0ce61
./blobs/sha256/57b4b433672b7d0ee3c3ea274bda013bf090610cbf5c68455839f7c1a94673fa
./blobs/sha256/03b1be98f3f9b05cb57782a3a71a44aaf6ec695de5f4f8e6c1058cd42f04953e
./busybox-1
./busybox-1/index.json
./busybox-1/oci-layout
# /home/jon/src/umoci/umoci unpack --shared-cas $(pwd)/blobs/ --image busybox-1 busybox-unpacked 
# ls busybox-unpacked/
config.json  rootfs  sha256_57b4b433672b7d0ee3c3ea274bda013bf090610cbf5c68455839f7c1a94673fa.mtree  umoci.json

@runcom @mtrmac would love to have some feedback on whether you would take something like this upstream. It's pretty critical for our performance requirements so we might end up maintaining a branch if not, but it would be great if there's some path we can agree on.

Also cc @cyphar

runcom · 2017-10-26T14:36:06Z

Looks fine to me actually 🤔

mtrmac

Same; having this as an opt-in feature does sound useful, especially if the feature is in the spec already.

mtrmac · 2017-10-26T15:12:15Z

oci/layout/oci_dest.go

 // If stream.Read() at any time, ESPECIALLY at end of input, returns an error, PutBlob MUST 1) fail, and 2) delete any data stored so far.
 func (d *ociImageDestination) PutBlob(stream io.Reader, inputInfo types.BlobInfo) (types.BlobInfo, error) {
+	// This should have been handled during newImageDestination, but just in case
 	if err := ensureDirectoryExists(d.ref.dir); err != nil {


If newImageDestination always creates it, I’d prefer dropping the redundant code. Unless there is some special reason why creating this directory again is actually needed?

mtrmac · 2017-10-26T15:13:14Z

oci/layout/oci_dest.go

+	if err := ensureDirectoryExists(d.ref.dir); err != nil {
+		return nil, err
+	}
+	// https: //github.com/opencontainers/image-spec/blame/7c889fafd04a893f5c5f50b7ab9963d5d64e5242/image-layout.md#L19


Extra space after https:. (Preferably, quote that single sentence in the comment as well, to save future readers’ time.)

mtrmac · 2017-10-26T15:19:01Z

oci/layout/oci_dest.go

 		},
 	}
-	return &ociImageDestination{ref: ref, index: index}, nil
+	d := &ociImageDestination{ref: ref, index: index, sharedBlobDir: ctx.OCISharedBlobDirPath}


ctx may be nil; it might be easier to pass the *types.SystemContext all the way through to blobPath.

jonboulle · 2017-10-30T16:13:32Z

@runcom @mtrmac sounds like you're both open to this? Any other changes in the design you'd like to see? if not I'll add some tests and get it ready for merge

runcom · 2017-10-30T16:18:15Z

if not I'll add some tests and get it ready for merge

let's add some tests if you could and I think we're both good with this.

If supplied, this will be used instead of the `blobs` subdirectory within OCI image layouts (which, per the OCI spec, may be empty), when getting or putting blobs. Signed-off-by: Jonathan Boulle <[email protected]>

jonboulle · 2017-11-06T11:50:24Z

test added but lmk any others you'd like to see

runcom · 2017-11-06T11:54:30Z

LGTM

runcom · 2017-11-06T11:54:35Z

@mtrmac PTAL

TomSweeneyRedHat · 2017-11-06T12:11:38Z

oci/layout/oci_dest_test.go

 	dirRef, ok := ref.(ociReference)
 	require.True(t, ok)
-	blobPath, err := dirRef.blobPath(blobDigest)
+	blobPath, err := dirRef.blobPath(blobDigest, "")


Would it be possible to create a test that uses a sharedblobdir?

it's here https://github.com/containers/image/pull/369/files#diff-b9397fa64a169d9bf8d84edd77f3b245R291 no?

Oh Jeez Louise, ignore me. Spot on @runcom, I don't know how I managed to miss that. Obviously time to dive into my first cup of tea for the day.
Straight up LGTM.

TomSweeneyRedHat

LGTM, one small question for consideration that's not a holder for this PR.

mtrmac · 2017-11-06T13:25:02Z

👍

mtrmac · 2017-11-06T13:27:30Z

Thanks!

mtrmac reviewed Oct 26, 2017

View reviewed changes

oci: add a sharedBlobDir to sources and dests

567057c

If supplied, this will be used instead of the `blobs` subdirectory within OCI image layouts (which, per the OCI spec, may be empty), when getting or putting blobs. Signed-off-by: Jonathan Boulle <[email protected]>

jonboulle changed the title ~~RFC: add a sharedBlobDir mode to OCI layouts~~ OCI: add a sharedBlobDir mode Nov 6, 2017

TomSweeneyRedHat reviewed Nov 6, 2017

View reviewed changes

mtrmac merged commit 2d9d9c1 into containers:master Nov 6, 2017

jonboulle deleted the shared-blobs branch November 6, 2017 14:20

jonboulle mentioned this pull request Nov 6, 2017

copy: add shared blob directory support for OCI sources/destinations containers/skopeo#443

Merged

OCI: add a sharedBlobDir mode #369

OCI: add a sharedBlobDir mode #369

Uh oh!

Conversation

jonboulle commented Oct 26, 2017

Uh oh!

runcom commented Oct 26, 2017

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

jonboulle commented Oct 30, 2017

Uh oh!

runcom commented Oct 30, 2017

Uh oh!

jonboulle commented Nov 6, 2017

Uh oh!

runcom commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Nov 6, 2017

Uh oh!

TomSweeneyRedHat Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

runcom Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat Nov 6, 2017

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Nov 6, 2017 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Nov 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

runcom commented Nov 6, 2017 •

edited

Loading

mtrmac commented Nov 6, 2017 •

edited by runcom

Loading