From 977d86d995957e8ba683b0fd1921a383c203a828 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 4 Apr 2019 16:31:38 +0200 Subject: [PATCH 01/39] storage destination: commit layer in PutBlob() Commit a layer directly in PutBlob() to the storage. This will allow future commits to implement blob locking without leaking and deferring parts of the logic to callers. Note that API of PutBlob() and TryReusingBlob() are extended with an index corresponding to the layer's index in the image. Currently, only the storage destination is making use of the index. Signed-off-by: Valentin Rothberg --- copy/copy.go | 90 +++++---- directory/directory_dest.go | 4 +- directory/directory_test.go | 4 +- docker/docker_image_dest.go | 8 +- docker/tarfile/dest.go | 6 +- image/docker_schema2.go | 2 +- image/docker_schema2_test.go | 4 +- oci/archive/oci_dest.go | 8 +- oci/layout/oci_dest.go | 4 +- oci/layout/oci_dest_test.go | 2 +- openshift/openshift.go | 8 +- ostree/ostree_dest.go | 4 +- storage/storage_image.go | 349 ++++++++++++++++++++++++----------- storage/storage_test.go | 30 +-- types/types.go | 15 +- 15 files changed, 355 insertions(+), 183 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 3ed8a2b824..b1103062f0 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -454,10 +454,6 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { err error } - // copyGroup is used to determine if all layers are copied - copyGroup := sync.WaitGroup{} - copyGroup.Add(numLayers) - // copySemaphore is used to limit the number of parallel downloads to // avoid malicious images causing troubles and to be nice to servers. var copySemaphore *semaphore.Weighted @@ -467,43 +463,69 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { copySemaphore = semaphore.NewWeighted(int64(1)) } - data := make([]copyLayerData, numLayers) - copyLayerHelper := func(index int, srcLayer types.BlobInfo, pool *mpb.Progress) { - defer copySemaphore.Release(1) - defer copyGroup.Done() - cld := copyLayerData{} + // copyGroup is used to determine if all layers are copied + copyGroup := sync.WaitGroup{} + + // A context.WithCancel is needed when encountering an error while + // copying/downloading layers in parallel. + cancelCtx, cancelCopyLayer := context.WithCancel(ctx) + defer cancelCopyLayer() + progressPool, progressCleanup := ic.c.newProgressPool(cancelCtx) + defer progressCleanup() + + layerIndex := 0 // some layers might be skipped, so we need a dedicated counter + digestToCopyData := make(map[digest.Digest]*copyLayerData) + for _, srcLayer := range srcInfos { + // Check if we'are already copying the layer + if _, ok := digestToCopyData[srcLayer.Digest]; ok { + continue + } + + cld := ©LayerData{} + digestToCopyData[srcLayer.Digest] = cld if ic.c.dest.AcceptsForeignLayerURLs() && len(srcLayer.URLs) != 0 { // DiffIDs are, currently, needed only when converting from schema1. // In which case src.LayerInfos will not have URLs because schema1 // does not support them. if ic.diffIDsAreNeeded { - cld.err = errors.New("getting DiffID for foreign layers is unimplemented") + cancelCopyLayer() + return errors.New("getting DiffID for foreign layers is unimplemented") } else { + logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) cld.destInfo = srcLayer - logrus.Debugf("Skipping foreign layer %q copy to %s", cld.destInfo.Digest, ic.c.dest.Reference().Transport().Name()) + continue } - } else { - cld.destInfo, cld.diffID, cld.err = ic.copyLayer(ctx, srcLayer, pool) } - data[index] = cld - } - func() { // A scope for defer - progressPool, progressCleanup := ic.c.newProgressPool(ctx) - defer progressCleanup() + copySemaphore.Acquire(cancelCtx, 1) // limits parallel copy operations + copyGroup.Add(1) // allows the main routine to wait for all copy operations to finish - for i, srcLayer := range srcInfos { - copySemaphore.Acquire(ctx, 1) - go copyLayerHelper(i, srcLayer, progressPool) - } + // Copy the layer. Note that cld is a pointer; changes to it are + // propagated implicitly. + go func(index int, srcLayer types.BlobInfo, cld *copyLayerData) { + defer copySemaphore.Release(1) + defer copyGroup.Done() + cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer, index, progressPool) + if cld.err != nil { + // Note that the error will be caught below. + logrus.Errorf("copying layer %d failed: %v", index, cld.err) + cancelCopyLayer() + } + }(layerIndex, srcLayer, cld) - // Wait for all layers to be copied - copyGroup.Wait() - }() + layerIndex++ + } + + // Wait for all layer-copy operations to finish + copyGroup.Wait() destInfos := make([]types.BlobInfo, numLayers) diffIDs := make([]digest.Digest, numLayers) - for i, cld := range data { + for i, srcLayer := range srcInfos { + cld, ok := digestToCopyData[srcLayer.Digest] + if !ok { + return errors.Errorf("no copy data for layer %q", srcLayer.Digest) + } if cld.err != nil { return cld.err } @@ -625,7 +647,7 @@ func (c *copier) copyConfig(ctx context.Context, src types.Image) error { progressPool, progressCleanup := c.newProgressPool(ctx) defer progressCleanup() bar := c.createProgressBar(progressPool, srcInfo, "config", "done") - destInfo, err := c.copyBlobFromStream(ctx, bytes.NewReader(configBlob), srcInfo, nil, false, true, bar) + destInfo, err := c.copyBlobFromStream(ctx, bytes.NewReader(configBlob), srcInfo, -1, nil, false, true, bar) if err != nil { return types.BlobInfo{}, err } @@ -651,13 +673,13 @@ type diffIDResult struct { // copyLayer copies a layer with srcInfo (with known Digest and possibly known Size) in src to dest, perhaps compressing it if canCompress, // and returns a complete blobInfo of the copied layer, and a value for LayerDiffIDs if diffIDIsNeeded -func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, pool *mpb.Progress) (types.BlobInfo, digest.Digest, error) { +func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, layerIndexInImage int, pool *mpb.Progress) (types.BlobInfo, digest.Digest, error) { cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be "" diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" // If we already have the blob, and we don't need to compute the diffID, then we don't need to read it from the source. if !diffIDIsNeeded { - reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, ic.c.blobInfoCache, ic.canSubstituteBlobs) + reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs) if err != nil { return types.BlobInfo{}, "", errors.Wrapf(err, "Error trying to reuse blob %s at destination", srcInfo.Digest) } @@ -678,7 +700,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, po bar := ic.c.createProgressBar(pool, srcInfo, "blob", "done") - blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize}, diffIDIsNeeded, bar) + blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize}, layerIndexInImage, diffIDIsNeeded, bar) if err != nil { return types.BlobInfo{}, "", err } @@ -709,7 +731,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, po // perhaps compressing the stream if canCompress, // and returns a complete blobInfo of the copied blob and perhaps a <-chan diffIDResult if diffIDIsNeeded, to be read by the caller. func (ic *imageCopier) copyLayerFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, - diffIDIsNeeded bool, bar *mpb.Bar) (types.BlobInfo, <-chan diffIDResult, error) { + layerIndexInImage int, diffIDIsNeeded bool, bar *mpb.Bar) (types.BlobInfo, <-chan diffIDResult, error) { var getDiffIDRecorder func(compression.DecompressorFunc) io.Writer // = nil var diffIDChan chan diffIDResult @@ -733,7 +755,7 @@ func (ic *imageCopier) copyLayerFromStream(ctx context.Context, srcStream io.Rea return pipeWriter } } - blobInfo, err := ic.c.copyBlobFromStream(ctx, srcStream, srcInfo, getDiffIDRecorder, ic.canModifyManifest, false, bar) // Sets err to nil on success + blobInfo, err := ic.c.copyBlobFromStream(ctx, srcStream, srcInfo, layerIndexInImage, getDiffIDRecorder, ic.canModifyManifest, false, bar) // Sets err to nil on success return blobInfo, diffIDChan, err // We need the defer … pipeWriter.CloseWithError() to happen HERE so that the caller can block on reading from diffIDChan } @@ -768,7 +790,7 @@ func computeDiffID(stream io.Reader, decompressor compression.DecompressorFunc) // perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil, // perhaps compressing it if canCompress, // and returns a complete blobInfo of the copied blob. -func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, +func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, layerIndexInImage int, getOriginalLayerCopyWriter func(decompressor compression.DecompressorFunc) io.Writer, canModifyBlob bool, isConfig bool, bar *mpb.Bar) (types.BlobInfo, error) { // The copying happens through a pipeline of connected io.Readers. @@ -847,7 +869,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } // === Finally, send the layer stream to dest. - uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, c.blobInfoCache, isConfig) + uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig) if err != nil { return types.BlobInfo{}, errors.Wrap(err, "Error writing blob") } diff --git a/directory/directory_dest.go b/directory/directory_dest.go index 4b2ab022e2..3efd093b11 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -136,7 +136,7 @@ func (d *dirImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { blobFile, err := ioutil.TempFile(d.ref.path, "dir-put-blob") if err != nil { return types.BlobInfo{}, err @@ -182,7 +182,7 @@ func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/directory/directory_test.go b/directory/directory_test.go index 121b1cf964..ecf0a82586 100644 --- a/directory/directory_test.go +++ b/directory/directory_test.go @@ -65,7 +65,7 @@ func TestGetPutBlob(t *testing.T) { require.NoError(t, err) defer dest.Close() assert.Equal(t, types.PreserveOriginal, dest.DesiredLayerCompression()) - info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, cache, false) + info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, 0, cache, false) assert.NoError(t, err) err = dest.Commit(context.Background()) assert.NoError(t, err) @@ -123,7 +123,7 @@ func TestPutBlobDigestFailure(t *testing.T) { dest, err := ref.NewImageDestination(context.Background(), nil) require.NoError(t, err) defer dest.Close() - _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, cache, false) + _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false) assert.Error(t, err) assert.Contains(t, digestErrorString, err.Error()) err = dest.Commit(context.Background()) diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index c116cbec32..abe100fbe4 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -19,7 +19,7 @@ import ( "github.com/containers/image/pkg/blobinfocache/none" "github.com/containers/image/types" "github.com/docker/distribution/registry/api/errcode" - "github.com/docker/distribution/registry/api/v2" + v2 "github.com/docker/distribution/registry/api/v2" "github.com/docker/distribution/registry/client" "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -124,12 +124,12 @@ func (d *dockerImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { if inputInfo.Digest.String() != "" { // This should not really be necessary, at least the copy code calls TryReusingBlob automatically. // Still, we need to check, if only because the "initiate upload" endpoint does not have a documented "blob already exists" return value. // But we do that with NoCache, so that it _only_ checks the primary destination, instead of trying all mount candidates _again_. - haveBlob, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, none.NoCache, false) + haveBlob, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, none.NoCache, false) if err != nil { return types.BlobInfo{}, err } @@ -271,7 +271,7 @@ func (d *dockerImageDestination) mountBlob(ctx context.Context, srcRepo referenc // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *dockerImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *dockerImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/docker/tarfile/dest.go b/docker/tarfile/dest.go index 5f30eddbc7..270120ba77 100644 --- a/docker/tarfile/dest.go +++ b/docker/tarfile/dest.go @@ -94,7 +94,7 @@ func (d *Destination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { // Ouch, we need to stream the blob into a temporary file just to determine the size. // When the layer is decompressed, we also have to generate the digest on uncompressed datas. if inputInfo.Size == -1 || inputInfo.Digest.String() == "" { @@ -126,7 +126,7 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t } // Maybe the blob has been already sent - ok, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, cache, false) + ok, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, cache, false) if err != nil { return types.BlobInfo{}, err } @@ -164,7 +164,7 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *Destination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *Destination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf("Can not check for a blob with unknown digest") } diff --git a/image/docker_schema2.go b/image/docker_schema2.go index 351e73ea1d..1cd4f402b6 100644 --- a/image/docker_schema2.go +++ b/image/docker_schema2.go @@ -252,7 +252,7 @@ func (m *manifestSchema2) convertToManifestSchema1(ctx context.Context, dest typ logrus.Debugf("Uploading empty layer during conversion to schema 1") // Ideally we should update the relevant BlobInfoCache about this layer, but that would require passing it down here, // and anyway this blob is so small that it’s easier to just copy it than to worry about figuring out another location where to get it. - info, err := dest.PutBlob(ctx, bytes.NewReader(GzippedEmptyLayer), types.BlobInfo{Digest: GzippedEmptyLayerDigest, Size: int64(len(GzippedEmptyLayer))}, none.NoCache, false) + info, err := dest.PutBlob(ctx, bytes.NewReader(GzippedEmptyLayer), types.BlobInfo{Digest: GzippedEmptyLayerDigest, Size: int64(len(GzippedEmptyLayer))}, -1, none.NoCache, false) if err != nil { return nil, errors.Wrap(err, "Error uploading empty layer") } diff --git a/image/docker_schema2_test.go b/image/docker_schema2_test.go index 9d3f96fc6d..333010bbce 100644 --- a/image/docker_schema2_test.go +++ b/image/docker_schema2_test.go @@ -401,7 +401,7 @@ func (d *memoryImageDest) IgnoresEmbeddedDockerReference() bool { func (d *memoryImageDest) HasThreadSafePutBlob() bool { panic("Unexpected call to a mock function") } -func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { if d.storedBlobs == nil { d.storedBlobs = make(map[digest.Digest][]byte) } @@ -415,7 +415,7 @@ func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputIn d.storedBlobs[inputInfo.Digest] = contents return types.BlobInfo{Digest: inputInfo.Digest, Size: int64(len(contents))}, nil } -func (d *memoryImageDest) TryReusingBlob(context.Context, types.BlobInfo, types.BlobInfoCache, bool) (bool, types.BlobInfo, error) { +func (d *memoryImageDest) TryReusingBlob(context.Context, types.BlobInfo, int, types.BlobInfoCache, bool) (bool, types.BlobInfo, error) { panic("Unexpected call to a mock function") } func (d *memoryImageDest) PutManifest(ctx context.Context, m []byte) error { diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 9571c37e2b..7a29ea7481 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -90,8 +90,8 @@ func (d *ociArchiveImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { - return d.unpackedDest.PutBlob(ctx, stream, inputInfo, cache, isConfig) +func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { + return d.unpackedDest.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig) } // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination @@ -101,8 +101,8 @@ func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Read // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ociArchiveImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { - return d.unpackedDest.TryReusingBlob(ctx, info, cache, canSubstitute) +func (d *ociArchiveImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { + return d.unpackedDest.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute) } // PutManifest writes manifest to the destination diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index db102184db..a0f441648e 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -120,7 +120,7 @@ func (d *ociImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { blobFile, err := ioutil.TempFile(d.ref.dir, "oci-put-blob") if err != nil { return types.BlobInfo{}, err @@ -187,7 +187,7 @@ func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ociImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *ociImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/oci/layout/oci_dest_test.go b/oci/layout/oci_dest_test.go index 44316fabb4..27aaad508a 100644 --- a/oci/layout/oci_dest_test.go +++ b/oci/layout/oci_dest_test.go @@ -53,7 +53,7 @@ func TestPutBlobDigestFailure(t *testing.T) { dest, err := ref.NewImageDestination(context.Background(), nil) require.NoError(t, err) defer dest.Close() - _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, cache, false) + _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false) assert.Error(t, err) assert.Contains(t, digestErrorString, err.Error()) err = dest.Commit(context.Background()) diff --git a/openshift/openshift.go b/openshift/openshift.go index 814c3eea1d..2dae485a43 100644 --- a/openshift/openshift.go +++ b/openshift/openshift.go @@ -395,8 +395,8 @@ func (d *openshiftImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { - return d.docker.PutBlob(ctx, stream, inputInfo, cache, isConfig) +func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { + return d.docker.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig) } // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination @@ -406,8 +406,8 @@ func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reade // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *openshiftImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { - return d.docker.TryReusingBlob(ctx, info, cache, canSubstitute) +func (d *openshiftImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { + return d.docker.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute) } // PutManifest writes manifest to the destination. diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index c926e2b4b9..cf1028cffe 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -145,7 +145,7 @@ func (d *ostreeImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *ostreeImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *ostreeImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { tmpDir, err := ioutil.TempDir(d.tmpDirPath, "blob") if err != nil { return types.BlobInfo{}, err @@ -342,7 +342,7 @@ func (d *ostreeImageDestination) importConfig(repo *otbuiltin.Repo, blob *blobTo // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ostreeImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *ostreeImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { if d.repo == nil { repo, err := openRepo(d.ref.repo) if err != nil { diff --git a/storage/storage_image.go b/storage/storage_image.go index b39d2bcc04..0e2d17845e 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -54,16 +54,19 @@ type storageImageSource struct { } type storageImageDestination struct { - imageRef storageReference - directory string // Temporary directory where we store blobs until Commit() time - nextTempFileID int32 // A counter that we use for computing filenames to assign to blobs - manifest []byte // Manifest contents, temporary - signatures []byte // Signature contents, temporary - putBlobMutex sync.Mutex // Mutex to sync state for parallel PutBlob executions - blobDiffIDs map[digest.Digest]digest.Digest // Mapping from layer blobsums to their corresponding DiffIDs - fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes - filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them - SignatureSizes []int `json:"signature-sizes,omitempty"` // List of sizes of each signature slice + imageRef storageReference + directory string // Temporary directory where we store blobs until Commit() time + nextTempFileID int32 // A counter that we use for computing filenames to assign to blobs + manifest []byte // Manifest contents, temporary + signatures []byte // Signature contents, temporary + putBlobMutex sync.Mutex // Mutex to sync state for parallel PutBlob executions + blobDiffIDs map[digest.Digest]digest.Digest // Mapping from layer blobsums to their corresponding DiffIDs + blobLayerIDs map[digest.Digest]string // Mapping from layer blobsums to their corresponding storage layer ID + fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes + filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them + indexToStorageID map[int]string // Mapping from layer index to the layer IDs in the storage + indexToDoneChannel map[int]chan bool // Mapping from layer index to a channel to indicate the layer has been written to storage + SignatureSizes []int `json:"signature-sizes,omitempty"` // List of sizes of each signature slice } type storageImageCloser struct { @@ -323,16 +326,31 @@ func newImageDestination(imageRef storageReference) (*storageImageDestination, e return nil, errors.Wrapf(err, "error creating a temporary directory") } image := &storageImageDestination{ - imageRef: imageRef, - directory: directory, - blobDiffIDs: make(map[digest.Digest]digest.Digest), - fileSizes: make(map[digest.Digest]int64), - filenames: make(map[digest.Digest]string), - SignatureSizes: []int{}, + imageRef: imageRef, + directory: directory, + blobDiffIDs: make(map[digest.Digest]digest.Digest), + blobLayerIDs: make(map[digest.Digest]string), + fileSizes: make(map[digest.Digest]int64), + filenames: make(map[digest.Digest]string), + indexToStorageID: make(map[int]string), + indexToDoneChannel: make(map[int]chan bool), + SignatureSizes: []int{}, } return image, nil } +func (s *storageImageDestination) getChannelForLayer(layerIndexInImage int) chan bool { + s.putBlobMutex.Lock() + defer s.putBlobMutex.Unlock() + channel, ok := s.indexToDoneChannel[layerIndexInImage] + if !ok { + // A buffered channel to allow non-blocking sends + channel = make(chan bool, 1) + s.indexToDoneChannel[layerIndexInImage] = channel + } + return channel +} + // Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent, // e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects. func (s *storageImageDestination) Reference() types.ImageReference { @@ -365,10 +383,72 @@ func (s *storageImageDestination) HasThreadSafePutBlob() bool { // inputInfo.Size is the expected length of stream, if known. // inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (blob types.BlobInfo, err error) { + errorBlobInfo := types.BlobInfo{ + Digest: "", + Size: -1, + } + + blob, putBlobError := s.putBlob(ctx, stream, blobinfo, layerIndexInImage, cache, isConfig) + if putBlobError != nil { + return errorBlobInfo, putBlobError + } + + // Deferred call to an anonymous func to signal potentially waiting + // goroutines via the index-specific channel. + defer func() { + // No need to wait + if layerIndexInImage >= 0 { + // It's a buffered channel, so we don't wait for the message to be + // received + channel := s.getChannelForLayer(layerIndexInImage) + channel <- err == nil + if err != nil { + logrus.Errorf("error while committing blob %d: %v", layerIndexInImage, err) + } + } + }() + + // First, wait for the previous layer to be committed + previousID := "" + if layerIndexInImage > 0 { + channel := s.getChannelForLayer(layerIndexInImage - 1) + if committed := <-channel; !committed { + err := fmt.Errorf("committing previous layer %d failed", layerIndexInImage-1) + logrus.Errorf(err.Error()) + return errorBlobInfo, err + } + var ok bool + s.putBlobMutex.Lock() + previousID, ok = s.indexToStorageID[layerIndexInImage-1] + s.putBlobMutex.Unlock() + if !ok { + return errorBlobInfo, fmt.Errorf("error committing blob %q: could not find parent layer ID", blob.Digest.String()) + } + } + + // Commit the blob + if layerIndexInImage >= 0 { + id, err := s.commitBlob(ctx, blob, previousID) + if err == nil { + s.putBlobMutex.Lock() + s.blobLayerIDs[blob.Digest] = id + s.indexToStorageID[layerIndexInImage] = id + s.putBlobMutex.Unlock() + } else { + return errorBlobInfo, err + } + } + return blob, nil +} + +func (s *storageImageDestination) putBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { // Stores a layer or data blob in our temporary directory, checking that any information // in the blobinfo matches the incoming data. errorBlobInfo := types.BlobInfo{ @@ -425,21 +505,17 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, } // This is safe because we have just computed both values ourselves. cache.RecordDigestUncompressedPair(blobDigest, diffID.Digest()) - return types.BlobInfo{ + blob := types.BlobInfo{ Digest: blobDigest, Size: blobSize, MediaType: blobinfo.MediaType, - }, nil + } + + return blob, nil } -// TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination -// (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). -// info.Digest must not be empty. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. -// If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. -// May use and/or update cache. -func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +// tryReusingBlob is a helper method for TryReusingBlob to wrap it +func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { // lock the entire method as it executes fairly quickly s.putBlobMutex.Lock() defer s.putBlobMutex.Unlock() @@ -467,6 +543,10 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t if len(layers) > 0 { // Save this for completeness. s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest + s.blobLayerIDs[blobinfo.Digest] = layers[0].ID + if layerIndexInImage >= 0 { + s.indexToStorageID[layerIndexInImage] = layers[0].ID + } return true, types.BlobInfo{ Digest: blobinfo.Digest, Size: layers[0].UncompressedSize, @@ -482,6 +562,10 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t if len(layers) > 0 { // Record the uncompressed value so that we can use it to calculate layer IDs. s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest + s.blobLayerIDs[blobinfo.Digest] = layers[0].ID + if layerIndexInImage >= 0 { + s.indexToStorageID[layerIndexInImage] = layers[0].ID + } return true, types.BlobInfo{ Digest: blobinfo.Digest, Size: layers[0].CompressedSize, @@ -500,6 +584,10 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t } if len(layers) > 0 { s.blobDiffIDs[uncompressedDigest] = layers[0].UncompressedDigest + s.blobLayerIDs[blobinfo.Digest] = layers[0].ID + if layerIndexInImage >= 0 { + s.indexToStorageID[layerIndexInImage] = layers[0].ID + } return true, types.BlobInfo{ Digest: uncompressedDigest, Size: layers[0].UncompressedSize, @@ -513,6 +601,30 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t return false, types.BlobInfo{}, nil } +// TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination +// (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). +// info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. +// May use and/or update cache. +func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { + reusable, blob, err := s.tryReusingBlob(ctx, blobinfo, layerIndexInImage, cache, canSubstitute) + // If we can reuse the blob or hit an error trying to do so, we need to + // signal the result through the channel as another Goroutine is potentially + // waiting for it. If we can't resuse the blob and encountered no error, we + // need to copy it and will send the signal in PutBlob(). + if (layerIndexInImage >= 0) && (err != nil || reusable) { + channel := s.getChannelForLayer(layerIndexInImage) + channel <- (err == nil) + } + return reusable, blob, err +} + // computeID computes a recommended image ID based on information we have so far. If // the manifest is not of a type that we recognize, we return an empty value, indicating // that since we don't have a recommendation, a random ID should be used if one needs @@ -572,6 +684,102 @@ func (s *storageImageDestination) getConfigBlob(info types.BlobInfo) ([]byte, er return nil, errors.New("blob not found") } +// commitBlobs commits the specified blob to the storage. If not already done, +// it will block until the parent layer is committed to the storage. Note that +// the only caller of commitBlob() is PutBlob(), which is recording the results +// and send the error through the corresponding channel in s.previousLayerResult. +func (s *storageImageDestination) commitBlob(ctx context.Context, blob types.BlobInfo, previousID string) (string, error) { + logrus.Debugf("committing blob %q", blob.Digest) + // Check if there's already a layer with the ID that we'd give to the result of applying + // this layer blob to its parent, if it has one, or the blob's hex value otherwise. + s.putBlobMutex.Lock() + diffID, haveDiffID := s.blobDiffIDs[blob.Digest] + s.putBlobMutex.Unlock() + if !haveDiffID { + return "", errors.Errorf("we have blob %q, but don't know its uncompressed digest", blob.Digest.String()) + } + + id := diffID.Hex() + if previousID != "" { + id = digest.Canonical.FromBytes([]byte(previousID + "+" + diffID.Hex())).Hex() + } + if layer, err2 := s.imageRef.transport.store.Layer(id); layer != nil && err2 == nil { + logrus.Debugf("layer for blob %q already found in storage", blob.Digest) + return layer.ID, nil + } + // Check if we previously cached a file with that blob's contents. If we didn't, + // then we need to read the desired contents from a layer. + s.putBlobMutex.Lock() + filename, ok := s.filenames[blob.Digest] + s.putBlobMutex.Unlock() + if !ok { + // Try to find the layer with contents matching that blobsum. + layer := "" + layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(blob.Digest) + if err2 == nil && len(layers) > 0 { + layer = layers[0].ID + } else { + layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(blob.Digest) + if err2 == nil && len(layers) > 0 { + layer = layers[0].ID + } + } + if layer == "" { + return "", errors.Wrapf(err2, "error locating layer for blob %q", blob.Digest) + } + // Read the layer's contents. + noCompression := archive.Uncompressed + diffOptions := &storage.DiffOptions{ + Compression: &noCompression, + } + diff, err2 := s.imageRef.transport.store.Diff("", layer, diffOptions) + if err2 != nil { + return "", errors.Wrapf(err2, "error reading layer %q for blob %q", layer, blob.Digest) + } + // Copy the layer diff to a file. Diff() takes a lock that it holds + // until the ReadCloser that it returns is closed, and PutLayer() wants + // the same lock, so the diff can't just be directly streamed from one + // to the other. + filename = s.computeNextBlobCacheFile() + file, err := os.OpenFile(filename, os.O_CREATE|os.O_TRUNC|os.O_WRONLY|os.O_EXCL, 0600) + if err != nil { + diff.Close() + return "", errors.Wrapf(err, "error creating temporary file %q", filename) + } + // Copy the data to the file. + // TODO: This can take quite some time, and should ideally be cancellable using + // ctx.Done(). + _, err = io.Copy(file, diff) + diff.Close() + file.Close() + if err != nil { + return "", errors.Wrapf(err, "error storing blob to file %q", filename) + } + // Make sure that we can find this file later, should we need the layer's + // contents again. + s.putBlobMutex.Lock() + s.filenames[blob.Digest] = filename + s.putBlobMutex.Unlock() + } + // Read the cached blob and use it as a diff. + file, err := os.Open(filename) + if err != nil { + return "", errors.Wrapf(err, "error opening file %q", filename) + } + defer file.Close() + // Build the new layer using the diff, regardless of where it came from. + // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). + layer, _, err := s.imageRef.transport.store.PutLayer(id, previousID, nil, "", false, nil, file) + if err != nil && errors.Cause(err) != storage.ErrDuplicateID { + return "", errors.Wrapf(err, "error adding layer with blob %q", blob.Digest) + } + return layer.ID, nil +} + +// Commit marks the process of storing the image as successful and asks for the image to be persisted. +// WARNING: This does not have any transactional semantics: +// - Uploaded data MAY be visible to others before Commit() is called +// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed) func (s *storageImageDestination) Commit(ctx context.Context) error { // Find the list of layer blobs. if len(s.manifest) == 0 { @@ -581,6 +789,7 @@ func (s *storageImageDestination) Commit(ctx context.Context) error { if err != nil { return errors.Wrapf(err, "error parsing manifest") } + layerBlobs := man.LayerInfos() // Extract or find the layers. lastLayer := "" @@ -588,10 +797,7 @@ func (s *storageImageDestination) Commit(ctx context.Context) error { if blob.EmptyLayer { continue } - - // Check if there's already a layer with the ID that we'd give to the result of applying - // this layer blob to its parent, if it has one, or the blob's hex value otherwise. - diffID, haveDiffID := s.blobDiffIDs[blob.Digest] + _, haveDiffID := s.blobDiffIDs[blob.Digest] if !haveDiffID { // Check if it's elsewhere and the caller just forgot to pass it to us in a PutBlob(), // or to even check if we had it. @@ -600,90 +806,27 @@ func (s *storageImageDestination) Commit(ctx context.Context) error { // TryReusingBlob; not calling PutBlob already violates the documented API, so there’s only // so far we are going to accommodate that (if we should be doing that at all). logrus.Debugf("looking for diffID for blob %+v", blob.Digest) - has, _, err := s.TryReusingBlob(ctx, blob.BlobInfo, none.NoCache, false) + has, _, err := s.tryReusingBlob(ctx, blob.BlobInfo, -1, none.NoCache, false) if err != nil { return errors.Wrapf(err, "error checking for a layer based on blob %q", blob.Digest.String()) } if !has { return errors.Errorf("error determining uncompressed digest for blob %q", blob.Digest.String()) } - diffID, haveDiffID = s.blobDiffIDs[blob.Digest] + _, haveDiffID = s.blobDiffIDs[blob.Digest] if !haveDiffID { return errors.Errorf("we have blob %q, but don't know its uncompressed digest", blob.Digest.String()) } } - id := diffID.Hex() - if lastLayer != "" { - id = digest.Canonical.FromBytes([]byte(lastLayer + "+" + diffID.Hex())).Hex() - } - if layer, err2 := s.imageRef.transport.store.Layer(id); layer != nil && err2 == nil { - // There's already a layer that should have the right contents, just reuse it. - lastLayer = layer.ID - continue - } - // Check if we previously cached a file with that blob's contents. If we didn't, - // then we need to read the desired contents from a layer. - filename, ok := s.filenames[blob.Digest] - if !ok { - // Try to find the layer with contents matching that blobsum. - layer := "" - layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(blob.Digest) - if err2 == nil && len(layers) > 0 { - layer = layers[0].ID - } else { - layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(blob.Digest) - if err2 == nil && len(layers) > 0 { - layer = layers[0].ID - } - } - if layer == "" { - return errors.Wrapf(err2, "error locating layer for blob %q", blob.Digest) - } - // Read the layer's contents. - noCompression := archive.Uncompressed - diffOptions := &storage.DiffOptions{ - Compression: &noCompression, - } - diff, err2 := s.imageRef.transport.store.Diff("", layer, diffOptions) - if err2 != nil { - return errors.Wrapf(err2, "error reading layer %q for blob %q", layer, blob.Digest) - } - // Copy the layer diff to a file. Diff() takes a lock that it holds - // until the ReadCloser that it returns is closed, and PutLayer() wants - // the same lock, so the diff can't just be directly streamed from one - // to the other. - filename = s.computeNextBlobCacheFile() - file, err := os.OpenFile(filename, os.O_CREATE|os.O_TRUNC|os.O_WRONLY|os.O_EXCL, 0600) - if err != nil { - diff.Close() - return errors.Wrapf(err, "error creating temporary file %q", filename) - } - // Copy the data to the file. - // TODO: This can take quite some time, and should ideally be cancellable using - // ctx.Done(). - _, err = io.Copy(file, diff) - diff.Close() - file.Close() - if err != nil { - return errors.Wrapf(err, "error storing blob to file %q", filename) - } - // Make sure that we can find this file later, should we need the layer's - // contents again. - s.filenames[blob.Digest] = filename - } - // Read the cached blob and use it as a diff. - file, err := os.Open(filename) + newID, err := s.commitBlob(ctx, blob.BlobInfo, lastLayer) if err != nil { - return errors.Wrapf(err, "error opening file %q", filename) - } - defer file.Close() - // Build the new layer using the diff, regardless of where it came from. - // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). - layer, _, err := s.imageRef.transport.store.PutLayer(id, lastLayer, nil, "", false, nil, file) - if err != nil && errors.Cause(err) != storage.ErrDuplicateID { - return errors.Wrapf(err, "error adding layer with blob %q", blob.Digest) + return err } - lastLayer = layer.ID + lastLayer = newID + } + + if lastLayer == "" { + return fmt.Errorf("could not find top layer") } // If one of those blobs was a configuration blob, then we can try to dig out the date when the image diff --git a/storage/storage_test.go b/storage/storage_test.go index d91d7aaa03..cd8c28d479 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -380,11 +380,11 @@ func TestWriteRead(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination: %v", err) } t.Logf("Wrote randomly-generated layer %q (%d/%d bytes) to destination", digest, size, decompressedSize) - if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, cache, false); err != nil { + if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, false); err != nil { t.Fatalf("Error saving config to destination: %v", err) } manifest := strings.Replace(manifestFmt, "%lh", digest.String(), -1) @@ -541,7 +541,7 @@ func TestDuplicateName(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -576,7 +576,7 @@ func TestDuplicateName(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -628,7 +628,7 @@ func TestDuplicateID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -663,7 +663,7 @@ func TestDuplicateID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -718,7 +718,7 @@ func TestDuplicateNameID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -753,7 +753,7 @@ func TestDuplicateNameID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -850,21 +850,21 @@ func TestSize(t *testing.T) { if dest == nil { t.Fatalf("NewImageDestination(%q) returned no destination", ref.StringWithinTransport()) } - if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, cache, false); err != nil { + if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, true); err != nil { t.Fatalf("Error saving config to destination: %v", err) } digest1, usize1, size1, blob := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size1, Digest: digest1, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination: %v", err) } digest2, usize2, size2, blob := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size2, Digest: digest2, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination: %v", err) } manifest := fmt.Sprintf(` @@ -946,26 +946,26 @@ func TestDuplicateBlob(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob1), types.BlobInfo{ Size: size1, Digest: digest1, - }, cache, false); err != nil { + }, 0, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination (first copy): %v", err) } digest2, _, size2, blob2 := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob2), types.BlobInfo{ Size: size2, Digest: digest2, - }, cache, false); err != nil { + }, 1, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination (first copy): %v", err) } if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob1), types.BlobInfo{ Size: size1, Digest: digest1, - }, cache, false); err != nil { + }, 2, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination (second copy): %v", err) } if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob2), types.BlobInfo{ Size: size2, Digest: digest2, - }, cache, false); err != nil { + }, 3, cache, false); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination (second copy): %v", err) } manifest := fmt.Sprintf(` diff --git a/types/types.go b/types/types.go index 7895043481..3f0867b371 100644 --- a/types/types.go +++ b/types/types.go @@ -7,7 +7,7 @@ import ( "github.com/containers/image/docker/reference" "github.com/opencontainers/go-digest" - "github.com/opencontainers/image-spec/specs-go/v1" + v1 "github.com/opencontainers/image-spec/specs-go/v1" ) // ImageTransport is a top-level namespace for ways to to store/load an image. @@ -262,20 +262,27 @@ type ImageDestination interface { // inputInfo.Size is the expected length of stream, if known. // inputInfo.MediaType describes the blob format, if known. // May update cache. + // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of + // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. + // Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. + // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. - PutBlob(ctx context.Context, stream io.Reader, inputInfo BlobInfo, cache BlobInfoCache, isConfig bool) (BlobInfo, error) + PutBlob(ctx context.Context, stream io.Reader, inputInfo BlobInfo, layerIndexInImage int, cache BlobInfoCache, isConfig bool) (BlobInfo, error) // HasThreadSafePutBlob indicates whether PutBlob can be executed concurrently. HasThreadSafePutBlob() bool // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. + // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of + // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. + // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. - // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. + // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. - TryReusingBlob(ctx context.Context, info BlobInfo, cache BlobInfoCache, canSubstitute bool) (bool, BlobInfo, error) + TryReusingBlob(ctx context.Context, info BlobInfo, layerIndexInImage int, cache BlobInfoCache, canSubstitute bool) (bool, BlobInfo, error) // PutManifest writes manifest to the destination. // FIXME? This should also receive a MIME type if known, to differentiate between schema versions. // If the destination is in principle available, refuses this manifest type (e.g. it does not recognize the schema), From bbb1db4161feb0f24b849e941ddb56bf76d2e13f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 17 Apr 2019 15:56:37 +0200 Subject: [PATCH 02/39] add pkg/progress for progress bar management Adding this package allows us to implement abstractions required for showing the progress bars during image copy. Signed-off-by: Valentin Rothberg --- copy/copy.go | 81 +++++++++++++++-------------- pkg/progress/progress.go | 107 +++++++++++++++++++++++++++++++++++++++ vendor.conf | 2 +- 3 files changed, 150 insertions(+), 40 deletions(-) create mode 100644 pkg/progress/progress.go diff --git a/copy/copy.go b/copy/copy.go index b1103062f0..0414dc795c 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -18,6 +18,7 @@ import ( "github.com/containers/image/manifest" "github.com/containers/image/pkg/blobinfocache" "github.com/containers/image/pkg/compression" + "github.com/containers/image/pkg/progress" "github.com/containers/image/signature" "github.com/containers/image/transports" "github.com/containers/image/types" @@ -26,7 +27,6 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/vbauerster/mpb" - "github.com/vbauerster/mpb/decor" "golang.org/x/crypto/ssh/terminal" "golang.org/x/sync/semaphore" ) @@ -160,7 +160,6 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, // If reportWriter is not a TTY (e.g., when piping to a file), do not // print the progress bars to avoid long and hard to parse output. - // createProgressBar() will print a single line instead. progressOutput := reportWriter if !isTTY(reportWriter) { progressOutput = ioutil.Discard @@ -466,12 +465,13 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // copyGroup is used to determine if all layers are copied copyGroup := sync.WaitGroup{} + progressPool := progress.NewPool(ctx, ic.c.progressOutput) + defer progressPool.CleanUp() + // A context.WithCancel is needed when encountering an error while // copying/downloading layers in parallel. cancelCtx, cancelCopyLayer := context.WithCancel(ctx) defer cancelCopyLayer() - progressPool, progressCleanup := ic.c.newProgressPool(cancelCtx) - defer progressCleanup() layerIndex := 0 // some layers might be skipped, so we need a dedicated counter digestToCopyData := make(map[digest.Digest]*copyLayerData) @@ -606,32 +606,20 @@ func (c *copier) newProgressPool(ctx context.Context) (*mpb.Progress, func()) { } } -// createProgressBar creates a mpb.Bar in pool. Note that if the copier's reportWriter -// is ioutil.Discard, the progress bar's output will be discarded -func (c *copier) createProgressBar(pool *mpb.Progress, info types.BlobInfo, kind string, onComplete string) *mpb.Bar { +// blobInfoToProgressAction creates a string based on the blobinfo's short +// digest and size and kind, for instance, "Copying blob bdf0201b3a05". +// Note that kind must either be "blob" or "config". +func blobInfoToProgressAction(blobinfo types.BlobInfo, kind string) string { // shortDigestLen is the length of the digest used for blobs. const shortDigestLen = 12 - - prefix := fmt.Sprintf("Copying %s %s", kind, info.Digest.Encoded()) - // Truncate the prefix (chopping of some part of the digest) to make all progress bars aligned in a column. - maxPrefixLen := len("Copying blob ") + shortDigestLen - if len(prefix) > maxPrefixLen { - prefix = prefix[:maxPrefixLen] - } - - bar := pool.AddBar(info.Size, - mpb.BarClearOnComplete(), - mpb.PrependDecorators( - decor.Name(prefix), - ), - mpb.AppendDecorators( - decor.OnComplete(decor.CountersKibiByte("%.1f / %.1f"), " "+onComplete), - ), - ) - if c.progressOutput == ioutil.Discard { - c.Printf("Copying %s %s\n", kind, info.Digest) - } - return bar + action := fmt.Sprintf("Copying %s %s", kind, blobinfo.Digest.Encoded()) + // Truncate the string (chopping of some part of the digest) to make all + // progress bars aligned in a column. + maxLen := len("Copying blob ") + shortDigestLen + if len(action) > maxLen { + action = action[:maxLen] + } + return action } // copyConfig copies config.json, if any, from src to dest. @@ -644,14 +632,20 @@ func (c *copier) copyConfig(ctx context.Context, src types.Image) error { } destInfo, err := func() (types.BlobInfo, error) { // A scope for defer - progressPool, progressCleanup := c.newProgressPool(ctx) - defer progressCleanup() - bar := c.createProgressBar(progressPool, srcInfo, "config", "done") + pool := progress.NewPool(ctx, c.progressOutput) + defer pool.CleanUp() + + bar := pool.AddBar( + blobInfoToProgressAction(srcInfo, "config"), + srcInfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + destInfo, err := c.copyBlobFromStream(ctx, bytes.NewReader(configBlob), srcInfo, -1, nil, false, true, bar) if err != nil { return types.BlobInfo{}, err } - bar.SetTotal(int64(len(configBlob)), true) return destInfo, nil }() if err != nil { @@ -673,7 +667,7 @@ type diffIDResult struct { // copyLayer copies a layer with srcInfo (with known Digest and possibly known Size) in src to dest, perhaps compressing it if canCompress, // and returns a complete blobInfo of the copied layer, and a value for LayerDiffIDs if diffIDIsNeeded -func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, layerIndexInImage int, pool *mpb.Progress) (types.BlobInfo, digest.Digest, error) { +func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, layerIndexInImage int, pool *progress.Pool) (types.BlobInfo, digest.Digest, error) { cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be "" diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" @@ -685,8 +679,13 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la } if reused { logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest) - bar := ic.c.createProgressBar(pool, srcInfo, "blob", "skipped: already exists") - bar.SetTotal(0, true) + + pool.AddBar( + blobInfoToProgressAction(srcInfo, "blob"), + 0, + progress.BarOptions{ + StaticMessage: "skipped: already exists", + }) return blobInfo, cachedDiffID, nil } } @@ -698,7 +697,12 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la } defer srcStream.Close() - bar := ic.c.createProgressBar(pool, srcInfo, "blob", "done") + bar := pool.AddBar( + blobInfoToProgressAction(srcInfo, "blob"), + srcInfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize}, layerIndexInImage, diffIDIsNeeded, bar) if err != nil { @@ -722,7 +726,6 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la } } - bar.SetTotal(srcInfo.Size, true) return blobInfo, diffID, nil } @@ -731,7 +734,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la // perhaps compressing the stream if canCompress, // and returns a complete blobInfo of the copied blob and perhaps a <-chan diffIDResult if diffIDIsNeeded, to be read by the caller. func (ic *imageCopier) copyLayerFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, - layerIndexInImage int, diffIDIsNeeded bool, bar *mpb.Bar) (types.BlobInfo, <-chan diffIDResult, error) { + layerIndexInImage int, diffIDIsNeeded bool, bar *progress.Bar) (types.BlobInfo, <-chan diffIDResult, error) { var getDiffIDRecorder func(compression.DecompressorFunc) io.Writer // = nil var diffIDChan chan diffIDResult @@ -792,7 +795,7 @@ func computeDiffID(stream io.Reader, decompressor compression.DecompressorFunc) // and returns a complete blobInfo of the copied blob. func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, layerIndexInImage int, getOriginalLayerCopyWriter func(decompressor compression.DecompressorFunc) io.Writer, - canModifyBlob bool, isConfig bool, bar *mpb.Bar) (types.BlobInfo, error) { + canModifyBlob bool, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { // The copying happens through a pipeline of connected io.Readers. // === Input: srcStream diff --git a/pkg/progress/progress.go b/pkg/progress/progress.go new file mode 100644 index 0000000000..408128a844 --- /dev/null +++ b/pkg/progress/progress.go @@ -0,0 +1,107 @@ +// Package progress is exposes a convenience API and abstractions for creating +// and managing pools of multi-progress bars. +package progress + +import ( + "context" + "fmt" + "io" + + "github.com/vbauerster/mpb" + "github.com/vbauerster/mpb/decor" +) + +// Pool allows managing progress bars. +type Pool struct { + pool *mpb.Progress + cancel context.CancelFunc + writer io.Writer +} + +// Bar represents a progress bar. +type Bar struct { + bar *mpb.Bar + pool *Pool +} + +// BarOptions includes various options to control AddBar. +type BarOptions struct { + // Remove the bar on completion. + RemoveOnCompletion bool + // OnCompletionMessage will be shown on completion and replace the progress bar. + OnCompletionMessage string + // ReplaceBar is the bar to replace. + ReplaceBar *Bar + // StaticMessage, if set, will replace displaying the progress bar. Use this + // field for static progress bars that do not show progress. + StaticMessage string +} + +// NewPool returns a Pool. The caller must eventually call ProgressPoo.CleanUp() +// when the pool will no longer be updated. +func NewPool(ctx context.Context, writer io.Writer) *Pool { + ctx, cancelFunc := context.WithCancel(ctx) + return &Pool{ + pool: mpb.New(mpb.WithWidth(40), mpb.WithOutput(writer), mpb.WithContext(ctx)), + cancel: cancelFunc, + writer: writer, + } +} + +// CleanUp cleans up resources, such as remaining progress bars, and stops them +// if necessary. +func (p *Pool) CleanUp() { + p.cancel() + p.pool.Wait() +} + +// getWriter returns the Pool's writer. +func (p *Pool) getWriter() io.Writer { + return p.writer +} + +// AddBar adds a new Bar to the Pool. +func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { + var bar *mpb.Bar + + mpbOptions := []mpb.BarOption{ + mpb.PrependDecorators( + decor.Name(action), + ), + } + + if options.RemoveOnCompletion { + mpbOptions = append(mpbOptions, mpb.BarRemoveOnComplete()) + } + + if options.ReplaceBar != nil { + mpbOptions = append(mpbOptions, mpb.BarReplaceOnComplete(options.ReplaceBar.bar)) + defer options.ReplaceBar.bar.SetTotal(0, true) + } + + if options.StaticMessage == "" { + mpbOptions = append(mpbOptions, + mpb.AppendDecorators( + decor.OnComplete(decor.CountersKibiByte("%.1f / %.1f"), " "+options.OnCompletionMessage), + ), + ) + mpbOptions = append(mpbOptions, mpb.BarClearOnComplete()) + bar = p.pool.AddBar(size, mpbOptions...) + } else { + barFiller := mpb.FillerFunc( + func(w io.Writer, width int, st *decor.Statistics) { + fmt.Fprint(w, options.StaticMessage) + }) + bar = p.pool.Add(0, barFiller, mpbOptions...) + } + + return &Bar{ + bar: bar, + pool: p, + } +} + +// ProxyReader wraps the reader with metrics for progress tracking. +func (b *Bar) ProxyReader(reader io.Reader) io.ReadCloser { + return b.bar.ProxyReader(reader) +} diff --git a/vendor.conf b/vendor.conf index 438cab17ae..f8ad0f2496 100644 --- a/vendor.conf +++ b/vendor.conf @@ -46,6 +46,6 @@ github.com/etcd-io/bbolt v1.3.2 github.com/klauspost/pgzip v1.2.1 github.com/klauspost/compress v1.4.1 github.com/klauspost/cpuid v1.2.0 -github.com/vbauerster/mpb v3.3.4 +github.com/vbauerster/mpb v3.4.0 github.com/mattn/go-isatty v0.0.4 github.com/VividCortex/ewma v1.1.1 From 8146913f5dc79d33971163115be6170da4e23623 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 17 Apr 2019 17:14:41 +0200 Subject: [PATCH 03/39] Extend TryReusingBlob and PutBlob with a progress.Bar This allows for updating the progress.Bar individually for each ImageDestination. Signed-off-by: Valentin Rothberg --- copy/copy.go | 4 ++-- directory/directory_dest.go | 5 +++-- directory/directory_test.go | 4 ++-- docker/docker_image_dest.go | 7 ++++--- docker/tarfile/dest.go | 7 ++++--- image/docker_schema2.go | 2 +- image/docker_schema2_test.go | 5 +++-- oci/archive/oci_dest.go | 9 +++++---- oci/layout/oci_dest.go | 5 +++-- oci/layout/oci_dest_test.go | 2 +- openshift/openshift.go | 9 +++++---- ostree/ostree_dest.go | 5 +++-- storage/storage_image.go | 5 +++-- storage/storage_test.go | 30 +++++++++++++++--------------- types/types.go | 5 +++-- 15 files changed, 57 insertions(+), 47 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 0414dc795c..88fbd608b7 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -673,7 +673,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la // If we already have the blob, and we don't need to compute the diffID, then we don't need to read it from the source. if !diffIDIsNeeded { - reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs) + reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs, nil) if err != nil { return types.BlobInfo{}, "", errors.Wrapf(err, "Error trying to reuse blob %s at destination", srcInfo.Digest) } @@ -872,7 +872,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } // === Finally, send the layer stream to dest. - uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig) + uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig, nil) if err != nil { return types.BlobInfo{}, errors.Wrap(err, "Error writing blob") } diff --git a/directory/directory_dest.go b/directory/directory_dest.go index 3efd093b11..e9efadfac5 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/opencontainers/go-digest" "github.com/pkg/errors" @@ -136,7 +137,7 @@ func (d *dirImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { blobFile, err := ioutil.TempFile(d.ref.path, "dir-put-blob") if err != nil { return types.BlobInfo{}, err @@ -182,7 +183,7 @@ func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/directory/directory_test.go b/directory/directory_test.go index ecf0a82586..223d003932 100644 --- a/directory/directory_test.go +++ b/directory/directory_test.go @@ -65,7 +65,7 @@ func TestGetPutBlob(t *testing.T) { require.NoError(t, err) defer dest.Close() assert.Equal(t, types.PreserveOriginal, dest.DesiredLayerCompression()) - info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, 0, cache, false) + info, err := dest.PutBlob(context.Background(), bytes.NewReader(blob), types.BlobInfo{Digest: digest.Digest("sha256:digest-test"), Size: int64(9)}, 0, cache, false, nil) assert.NoError(t, err) err = dest.Commit(context.Background()) assert.NoError(t, err) @@ -123,7 +123,7 @@ func TestPutBlobDigestFailure(t *testing.T) { dest, err := ref.NewImageDestination(context.Background(), nil) require.NoError(t, err) defer dest.Close() - _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false) + _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false, nil) assert.Error(t, err) assert.Contains(t, digestErrorString, err.Error()) err = dest.Commit(context.Background()) diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index abe100fbe4..e2525d1397 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -17,6 +17,7 @@ import ( "github.com/containers/image/docker/reference" "github.com/containers/image/manifest" "github.com/containers/image/pkg/blobinfocache/none" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/docker/distribution/registry/api/errcode" v2 "github.com/docker/distribution/registry/api/v2" @@ -124,12 +125,12 @@ func (d *dockerImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { if inputInfo.Digest.String() != "" { // This should not really be necessary, at least the copy code calls TryReusingBlob automatically. // Still, we need to check, if only because the "initiate upload" endpoint does not have a documented "blob already exists" return value. // But we do that with NoCache, so that it _only_ checks the primary destination, instead of trying all mount candidates _again_. - haveBlob, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, none.NoCache, false) + haveBlob, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, none.NoCache, false, nil) if err != nil { return types.BlobInfo{}, err } @@ -271,7 +272,7 @@ func (d *dockerImageDestination) mountBlob(ctx context.Context, srcRepo referenc // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *dockerImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *dockerImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/docker/tarfile/dest.go b/docker/tarfile/dest.go index 270120ba77..6d37396034 100644 --- a/docker/tarfile/dest.go +++ b/docker/tarfile/dest.go @@ -15,6 +15,7 @@ import ( "github.com/containers/image/docker/reference" "github.com/containers/image/internal/tmpdir" "github.com/containers/image/manifest" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/opencontainers/go-digest" "github.com/pkg/errors" @@ -94,7 +95,7 @@ func (d *Destination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { // Ouch, we need to stream the blob into a temporary file just to determine the size. // When the layer is decompressed, we also have to generate the digest on uncompressed datas. if inputInfo.Size == -1 || inputInfo.Digest.String() == "" { @@ -126,7 +127,7 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t } // Maybe the blob has been already sent - ok, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, cache, false) + ok, reusedInfo, err := d.TryReusingBlob(ctx, inputInfo, layerIndexInImage, cache, false, nil) if err != nil { return types.BlobInfo{}, err } @@ -164,7 +165,7 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *Destination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *Destination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf("Can not check for a blob with unknown digest") } diff --git a/image/docker_schema2.go b/image/docker_schema2.go index 1cd4f402b6..924ec3671a 100644 --- a/image/docker_schema2.go +++ b/image/docker_schema2.go @@ -252,7 +252,7 @@ func (m *manifestSchema2) convertToManifestSchema1(ctx context.Context, dest typ logrus.Debugf("Uploading empty layer during conversion to schema 1") // Ideally we should update the relevant BlobInfoCache about this layer, but that would require passing it down here, // and anyway this blob is so small that it’s easier to just copy it than to worry about figuring out another location where to get it. - info, err := dest.PutBlob(ctx, bytes.NewReader(GzippedEmptyLayer), types.BlobInfo{Digest: GzippedEmptyLayerDigest, Size: int64(len(GzippedEmptyLayer))}, -1, none.NoCache, false) + info, err := dest.PutBlob(ctx, bytes.NewReader(GzippedEmptyLayer), types.BlobInfo{Digest: GzippedEmptyLayerDigest, Size: int64(len(GzippedEmptyLayer))}, -1, none.NoCache, false, nil) if err != nil { return nil, errors.Wrap(err, "Error uploading empty layer") } diff --git a/image/docker_schema2_test.go b/image/docker_schema2_test.go index 333010bbce..4b5668ca4f 100644 --- a/image/docker_schema2_test.go +++ b/image/docker_schema2_test.go @@ -12,6 +12,7 @@ import ( "github.com/containers/image/docker/reference" "github.com/containers/image/manifest" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -401,7 +402,7 @@ func (d *memoryImageDest) IgnoresEmbeddedDockerReference() bool { func (d *memoryImageDest) HasThreadSafePutBlob() bool { panic("Unexpected call to a mock function") } -func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { if d.storedBlobs == nil { d.storedBlobs = make(map[digest.Digest][]byte) } @@ -415,7 +416,7 @@ func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputIn d.storedBlobs[inputInfo.Digest] = contents return types.BlobInfo{Digest: inputInfo.Digest, Size: int64(len(contents))}, nil } -func (d *memoryImageDest) TryReusingBlob(context.Context, types.BlobInfo, int, types.BlobInfoCache, bool) (bool, types.BlobInfo, error) { +func (d *memoryImageDest) TryReusingBlob(context.Context, types.BlobInfo, int, types.BlobInfoCache, bool, *progress.Bar) (bool, types.BlobInfo, error) { panic("Unexpected call to a mock function") } func (d *memoryImageDest) PutManifest(ctx context.Context, m []byte) error { diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 7a29ea7481..6535dec56a 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -5,6 +5,7 @@ import ( "io" "os" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/containers/storage/pkg/archive" "github.com/pkg/errors" @@ -90,8 +91,8 @@ func (d *ociArchiveImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { - return d.unpackedDest.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig) +func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { + return d.unpackedDest.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig, bar) } // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination @@ -101,8 +102,8 @@ func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Read // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ociArchiveImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { - return d.unpackedDest.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute) +func (d *ociArchiveImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { + return d.unpackedDest.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute, bar) } // PutManifest writes manifest to the destination diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index a0f441648e..95212e48d2 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -10,6 +10,7 @@ import ( "runtime" "github.com/containers/image/manifest" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" digest "github.com/opencontainers/go-digest" imgspec "github.com/opencontainers/image-spec/specs-go" @@ -120,7 +121,7 @@ func (d *ociImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { blobFile, err := ioutil.TempFile(d.ref.dir, "oci-put-blob") if err != nil { return types.BlobInfo{}, err @@ -187,7 +188,7 @@ func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ociImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *ociImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { if info.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`"Can not check for a blob with unknown digest`) } diff --git a/oci/layout/oci_dest_test.go b/oci/layout/oci_dest_test.go index 27aaad508a..901eb50a26 100644 --- a/oci/layout/oci_dest_test.go +++ b/oci/layout/oci_dest_test.go @@ -53,7 +53,7 @@ func TestPutBlobDigestFailure(t *testing.T) { dest, err := ref.NewImageDestination(context.Background(), nil) require.NoError(t, err) defer dest.Close() - _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false) + _, err = dest.PutBlob(context.Background(), reader, types.BlobInfo{Digest: blobDigest, Size: -1}, 0, cache, false, nil) assert.Error(t, err) assert.Contains(t, digestErrorString, err.Error()) err = dest.Commit(context.Background()) diff --git a/openshift/openshift.go b/openshift/openshift.go index 2dae485a43..bd4dbaaae2 100644 --- a/openshift/openshift.go +++ b/openshift/openshift.go @@ -15,6 +15,7 @@ import ( "github.com/containers/image/docker" "github.com/containers/image/docker/reference" "github.com/containers/image/manifest" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/containers/image/version" "github.com/opencontainers/go-digest" @@ -395,8 +396,8 @@ func (d *openshiftImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { - return d.docker.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig) +func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { + return d.docker.PutBlob(ctx, stream, inputInfo, layerIndexInImage, cache, isConfig, bar) } // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination @@ -406,8 +407,8 @@ func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reade // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *openshiftImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { - return d.docker.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute) +func (d *openshiftImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { + return d.docker.TryReusingBlob(ctx, info, layerIndexInImage, cache, canSubstitute, bar) } // PutManifest writes manifest to the destination. diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index cf1028cffe..ad1098049f 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -21,6 +21,7 @@ import ( "unsafe" "github.com/containers/image/manifest" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/containers/storage/pkg/archive" "github.com/klauspost/pgzip" @@ -145,7 +146,7 @@ func (d *ostreeImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 *ostreeImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { +func (d *ostreeImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (types.BlobInfo, error) { tmpDir, err := ioutil.TempDir(d.tmpDirPath, "blob") if err != nil { return types.BlobInfo{}, err @@ -342,7 +343,7 @@ func (d *ostreeImageDestination) importConfig(repo *otbuiltin.Repo, blob *blobTo // If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (d *ostreeImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (d *ostreeImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { if d.repo == nil { repo, err := openRepo(d.ref.repo) if err != nil { diff --git a/storage/storage_image.go b/storage/storage_image.go index 0e2d17845e..8e009a6aa8 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -19,6 +19,7 @@ import ( "github.com/containers/image/internal/tmpdir" "github.com/containers/image/manifest" "github.com/containers/image/pkg/blobinfocache/none" + "github.com/containers/image/pkg/progress" "github.com/containers/image/types" "github.com/containers/storage" "github.com/containers/storage/pkg/archive" @@ -389,7 +390,7 @@ func (s *storageImageDestination) HasThreadSafePutBlob() bool { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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 (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (blob types.BlobInfo, err error) { +func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (blob types.BlobInfo, err error) { errorBlobInfo := types.BlobInfo{ Digest: "", Size: -1, @@ -612,7 +613,7 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. -func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { +func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { reusable, blob, err := s.tryReusingBlob(ctx, blobinfo, layerIndexInImage, cache, canSubstitute) // If we can reuse the blob or hit an error trying to do so, we need to // signal the result through the channel as another Goroutine is potentially diff --git a/storage/storage_test.go b/storage/storage_test.go index cd8c28d479..afbc7d3cb1 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -380,11 +380,11 @@ func TestWriteRead(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination: %v", err) } t.Logf("Wrote randomly-generated layer %q (%d/%d bytes) to destination", digest, size, decompressedSize) - if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, false); err != nil { + if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, false, nil); err != nil { t.Fatalf("Error saving config to destination: %v", err) } manifest := strings.Replace(manifestFmt, "%lh", digest.String(), -1) @@ -541,7 +541,7 @@ func TestDuplicateName(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -576,7 +576,7 @@ func TestDuplicateName(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -628,7 +628,7 @@ func TestDuplicateID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -663,7 +663,7 @@ func TestDuplicateID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -718,7 +718,7 @@ func TestDuplicateNameID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size, Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, first pass: %v", err) } manifest := fmt.Sprintf(` @@ -753,7 +753,7 @@ func TestDuplicateNameID(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: int64(size), Digest: digest, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer to destination, second pass: %v", err) } manifest = fmt.Sprintf(` @@ -850,21 +850,21 @@ func TestSize(t *testing.T) { if dest == nil { t.Fatalf("NewImageDestination(%q) returned no destination", ref.StringWithinTransport()) } - if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, true); err != nil { + if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, true, nil); err != nil { t.Fatalf("Error saving config to destination: %v", err) } digest1, usize1, size1, blob := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size1, Digest: digest1, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination: %v", err) } digest2, usize2, size2, blob := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size2, Digest: digest2, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination: %v", err) } manifest := fmt.Sprintf(` @@ -946,26 +946,26 @@ func TestDuplicateBlob(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob1), types.BlobInfo{ Size: size1, Digest: digest1, - }, 0, cache, false); err != nil { + }, 0, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination (first copy): %v", err) } digest2, _, size2, blob2 := makeLayer(t, archive.Gzip) if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob2), types.BlobInfo{ Size: size2, Digest: digest2, - }, 1, cache, false); err != nil { + }, 1, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination (first copy): %v", err) } if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob1), types.BlobInfo{ Size: size1, Digest: digest1, - }, 2, cache, false); err != nil { + }, 2, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 1 to destination (second copy): %v", err) } if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob2), types.BlobInfo{ Size: size2, Digest: digest2, - }, 3, cache, false); err != nil { + }, 3, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination (second copy): %v", err) } manifest := fmt.Sprintf(` diff --git a/types/types.go b/types/types.go index 3f0867b371..2cbf785902 100644 --- a/types/types.go +++ b/types/types.go @@ -6,6 +6,7 @@ import ( "time" "github.com/containers/image/docker/reference" + "github.com/containers/image/pkg/progress" "github.com/opencontainers/go-digest" v1 "github.com/opencontainers/image-spec/specs-go/v1" ) @@ -269,7 +270,7 @@ type ImageDestination interface { // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. - PutBlob(ctx context.Context, stream io.Reader, inputInfo BlobInfo, layerIndexInImage int, cache BlobInfoCache, isConfig bool) (BlobInfo, error) + PutBlob(ctx context.Context, stream io.Reader, inputInfo BlobInfo, layerIndexInImage int, cache BlobInfoCache, isConfig bool, bar *progress.Bar) (BlobInfo, error) // HasThreadSafePutBlob indicates whether PutBlob can be executed concurrently. HasThreadSafePutBlob() bool // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination @@ -282,7 +283,7 @@ type ImageDestination interface { // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. - TryReusingBlob(ctx context.Context, info BlobInfo, layerIndexInImage int, cache BlobInfoCache, canSubstitute bool) (bool, BlobInfo, error) + TryReusingBlob(ctx context.Context, info BlobInfo, layerIndexInImage int, cache BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, BlobInfo, error) // PutManifest writes manifest to the destination. // FIXME? This should also receive a MIME type if known, to differentiate between schema versions. // If the destination is in principle available, refuses this manifest type (e.g. it does not recognize the schema), From 39588b3f372200cc5eb05982261d9b3fe0cdb253 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 18 Apr 2019 08:03:02 +0200 Subject: [PATCH 04/39] copy: create replacable placeholder bar Create an initial replacably placeholder progress bar that can later be replaced with the action being taken. The transition may looks as follows: 1) Copying blob d172c7f0578d 2.1) Copying blob d172c7f0578d skipped: already exists 2.2) Copying blob d172c7f0578d [================>---------------------] 9.5MiB / 21.5MiB Signed-off-by: Valentin Rothberg --- copy/copy.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index 88fbd608b7..b4b3454c02 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -671,6 +671,14 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be "" diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" + progressBarAction := blobInfoToProgressAction(srcInfo, "blob") + bar := pool.AddBar( + progressBarAction, + 0, + progress.BarOptions{ + RemoveOnCompletion: true, + StaticMessage: " ", + }) // If we already have the blob, and we don't need to compute the diffID, then we don't need to read it from the source. if !diffIDIsNeeded { reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs, nil) @@ -685,6 +693,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la 0, progress.BarOptions{ StaticMessage: "skipped: already exists", + ReplaceBar: bar, }) return blobInfo, cachedDiffID, nil } @@ -697,11 +706,12 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la } defer srcStream.Close() - bar := pool.AddBar( + bar = pool.AddBar( blobInfoToProgressAction(srcInfo, "blob"), srcInfo.Size, progress.BarOptions{ OnCompletionMessage: "done", + ReplaceBar: bar, }) blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize}, layerIndexInImage, diffIDIsNeeded, bar) From 0d677370e642c23cfb6d6d6b790b817fc9cd2a6f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 18 Apr 2019 08:34:19 +0200 Subject: [PATCH 05/39] copy: move progress creation before copying the stream Also add a progress.Bar.ReplaceBar() method, and a progress.DigestToCopyAction() convenience function that can later be used in TryReusingBlob() and PutBlob(). Signed-off-by: Valentin Rothberg --- copy/copy.go | 50 +++++++++++++++------------------------- pkg/progress/progress.go | 27 +++++++++++++++++++++- 2 files changed, 44 insertions(+), 33 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index b4b3454c02..240b6de046 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -606,22 +606,6 @@ func (c *copier) newProgressPool(ctx context.Context) (*mpb.Progress, func()) { } } -// blobInfoToProgressAction creates a string based on the blobinfo's short -// digest and size and kind, for instance, "Copying blob bdf0201b3a05". -// Note that kind must either be "blob" or "config". -func blobInfoToProgressAction(blobinfo types.BlobInfo, kind string) string { - // shortDigestLen is the length of the digest used for blobs. - const shortDigestLen = 12 - action := fmt.Sprintf("Copying %s %s", kind, blobinfo.Digest.Encoded()) - // Truncate the string (chopping of some part of the digest) to make all - // progress bars aligned in a column. - maxLen := len("Copying blob ") + shortDigestLen - if len(action) > maxLen { - action = action[:maxLen] - } - return action -} - // copyConfig copies config.json, if any, from src to dest. func (c *copier) copyConfig(ctx context.Context, src types.Image) error { srcInfo := src.ConfigInfo() @@ -636,10 +620,11 @@ func (c *copier) copyConfig(ctx context.Context, src types.Image) error { defer pool.CleanUp() bar := pool.AddBar( - blobInfoToProgressAction(srcInfo, "config"), - srcInfo.Size, + progress.DigestToCopyAction(srcInfo.Digest, "config"), + 0, progress.BarOptions{ - OnCompletionMessage: "done", + RemoveOnCompletion: true, + StaticMessage: " ", }) destInfo, err := c.copyBlobFromStream(ctx, bytes.NewReader(configBlob), srcInfo, -1, nil, false, true, bar) @@ -671,7 +656,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be "" diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" - progressBarAction := blobInfoToProgressAction(srcInfo, "blob") + progressBarAction := progress.DigestToCopyAction(srcInfo.Digest, "blob") bar := pool.AddBar( progressBarAction, 0, @@ -688,12 +673,11 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la if reused { logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest) - pool.AddBar( - blobInfoToProgressAction(srcInfo, "blob"), + bar.ReplaceBar( + progressBarAction, 0, progress.BarOptions{ StaticMessage: "skipped: already exists", - ReplaceBar: bar, }) return blobInfo, cachedDiffID, nil } @@ -706,14 +690,6 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la } defer srcStream.Close() - bar = pool.AddBar( - blobInfoToProgressAction(srcInfo, "blob"), - srcInfo.Size, - progress.BarOptions{ - OnCompletionMessage: "done", - ReplaceBar: bar, - }) - blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize}, layerIndexInImage, diffIDIsNeeded, bar) if err != nil { return types.BlobInfo{}, "", err @@ -828,7 +804,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr return types.BlobInfo{}, errors.Wrapf(err, "Error reading blob %s", srcInfo.Digest) } isCompressed := decompressor != nil - destStream = bar.ProxyReader(destStream) // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. @@ -881,6 +856,17 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } } + kind := "blob" + if isConfig { + kind = "config" + } + bar = bar.ReplaceBar( + progress.DigestToCopyAction(srcInfo.Digest, kind), + srcInfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + destStream = bar.ProxyReader(destStream) // === Finally, send the layer stream to dest. uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig, nil) if err != nil { diff --git a/pkg/progress/progress.go b/pkg/progress/progress.go index 408128a844..22374943ad 100644 --- a/pkg/progress/progress.go +++ b/pkg/progress/progress.go @@ -7,6 +7,7 @@ import ( "fmt" "io" + "github.com/opencontainers/go-digest" "github.com/vbauerster/mpb" "github.com/vbauerster/mpb/decor" ) @@ -26,7 +27,8 @@ type Bar struct { // BarOptions includes various options to control AddBar. type BarOptions struct { - // Remove the bar on completion. + // Remove the bar on completion. This must be true if the bar will be + // replaced by another one. RemoveOnCompletion bool // OnCompletionMessage will be shown on completion and replace the progress bar. OnCompletionMessage string @@ -60,6 +62,21 @@ func (p *Pool) getWriter() io.Writer { return p.writer } +// DigestToCopyAction returns a string based on the blobinfo and kind. +// It's a convenience function for the c/image library when copying images. +func DigestToCopyAction(digest digest.Digest, kind string) string { + // shortDigestLen is the length of the digest used for blobs. + const shortDigestLen = 12 + const maxLen = len("Copying blob ") + shortDigestLen + // Truncate the string (chopping of some part of the digest) to make all + // progress bars aligned in a column. + copyAction := fmt.Sprintf("Copying %s %s", kind, digest.Encoded()) + if len(copyAction) > maxLen { + copyAction = copyAction[:maxLen] + } + return copyAction +} + // AddBar adds a new Bar to the Pool. func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { var bar *mpb.Bar @@ -101,6 +118,14 @@ func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { } } +// ReplaceBar returns a bar replacing the current one. Note that the current one +// will be terminated and should have been created with +// options.RemoveOnCompletion. +func (b *Bar) ReplaceBar(action string, size int64, options BarOptions) *Bar { + options.ReplaceBar = b + return b.pool.AddBar(action, size, options) +} + // ProxyReader wraps the reader with metrics for progress tracking. func (b *Bar) ProxyReader(reader io.Reader) io.ReadCloser { return b.bar.ProxyReader(reader) From da746eb589192efcc38fb42c495309f92a210f5a Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 18 Apr 2019 10:00:17 +0200 Subject: [PATCH 06/39] copy: let storage update the progress.Bar in {Put,TryResuing}Blob() Special-case the storage transprot when calling PutBlob() and TryReusingBlob() and let the storage transport replace the intial placeholder progress.Bar. This avoids "flickering" for cases when replacing a progress.Bar showing a progress with one that does not show a progress. Signed-off-by: Valentin Rothberg --- copy/copy.go | 49 ++++++++++++++++++++++-------------- storage/storage_image.go | 22 ++++++++++++++++ storage/storage_transport.go | 8 +++++- 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 240b6de046..7fda4b964b 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -20,6 +20,7 @@ import ( "github.com/containers/image/pkg/compression" "github.com/containers/image/pkg/progress" "github.com/containers/image/signature" + "github.com/containers/image/storage" "github.com/containers/image/transports" "github.com/containers/image/types" "github.com/klauspost/pgzip" @@ -666,19 +667,23 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la }) // If we already have the blob, and we don't need to compute the diffID, then we don't need to read it from the source. if !diffIDIsNeeded { - reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs, nil) + reused, blobInfo, err := ic.c.dest.TryReusingBlob(ctx, srcInfo, layerIndexInImage, ic.c.blobInfoCache, ic.canSubstituteBlobs, bar) if err != nil { return types.BlobInfo{}, "", errors.Wrapf(err, "Error trying to reuse blob %s at destination", srcInfo.Digest) } if reused { logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest) - - bar.ReplaceBar( - progressBarAction, - 0, - progress.BarOptions{ - StaticMessage: "skipped: already exists", - }) + // Instead of adding boilerplate code to ALL TryReusingBlob()s, just + // special case the storage transport, which is the only transport + // where we need control over the progress.Bar. + if ic.c.dest.Reference().Transport().Name() != storage.Name() { + bar.ReplaceBar( + progressBarAction, + 0, + progress.BarOptions{ + StaticMessage: "skipped: already exists", + }) + } return blobInfo, cachedDiffID, nil } } @@ -856,19 +861,25 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } } - kind := "blob" - if isConfig { - kind = "config" + // Instead of adding boilerplate code to ALL PutBlob()s, just special case + // the storage transport, which is the only transport where we need control + // over the progress.Bar. + if c.dest.Reference().Transport().Name() != storage.Name() { + kind := "blob" + if isConfig { + kind = "config" + } + bar = bar.ReplaceBar( + progress.DigestToCopyAction(srcInfo.Digest, kind), + srcInfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + destStream = bar.ProxyReader(destStream) } - bar = bar.ReplaceBar( - progress.DigestToCopyAction(srcInfo.Digest, kind), - srcInfo.Size, - progress.BarOptions{ - OnCompletionMessage: "done", - }) - destStream = bar.ProxyReader(destStream) + // === Finally, send the layer stream to dest. - uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig, nil) + uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig, bar) if err != nil { return types.BlobInfo{}, errors.Wrap(err, "Error writing blob") } diff --git a/storage/storage_image.go b/storage/storage_image.go index 8e009a6aa8..8cb1baf7d3 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -396,6 +396,20 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, Size: -1, } + if bar != nil { + kind := "blob" + if isConfig { + kind = "config" + } + bar = bar.ReplaceBar( + progress.DigestToCopyAction(blobinfo.Digest, kind), + blobinfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + stream = bar.ProxyReader(stream) + } + blob, putBlobError := s.putBlob(ctx, stream, blobinfo, layerIndexInImage, cache, isConfig) if putBlobError != nil { return errorBlobInfo, putBlobError @@ -623,6 +637,14 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t channel := s.getChannelForLayer(layerIndexInImage) channel <- (err == nil) } + if bar != nil && reusable { + bar.ReplaceBar( + progress.DigestToCopyAction(blobinfo.Digest, "blob"), + 0, + progress.BarOptions{ + StaticMessage: "skipped: already exists", + }) + } return reusable, blob, err } diff --git a/storage/storage_transport.go b/storage/storage_transport.go index c9a05e6c01..70c18640e4 100644 --- a/storage/storage_transport.go +++ b/storage/storage_transport.go @@ -38,6 +38,12 @@ var ( ErrPathNotAbsolute = errors.New("path name is not absolute") ) +// Name returns the name of the transport and can be used for type detection of +// a Transport object. +func Name() string { + return "containers-storage" +} + // StoreTransport is an ImageTransport that uses a storage.Store to parse // references, either its own default or one that it's told to use. type StoreTransport interface { @@ -71,7 +77,7 @@ type storageTransport struct { func (s *storageTransport) Name() string { // Still haven't really settled on a name. - return "containers-storage" + return Name() } // SetStore sets the Store object which the Transport will use for parsing From c85c1de539d9a4eb0425f5bc9d415a75332b85fe Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 18 Apr 2019 13:58:10 +0200 Subject: [PATCH 07/39] storage destination: blob-copy detection Make use of the digest locks from c/storage to detect if other processes are copying a given blob to effectively avoid redundantly copying the same blob to the storage. The heuristic to decide if a blob is currently being copied is to first acquire the digest lock and then wait for 200 ms; if the lock hasn't been acquired until then, we assume that another process is holding the lock and is hence copying the blob. Signed-off-by: Valentin Rothberg --- copy/copy.go | 13 ++- storage/storage_image.go | 183 +++++++++++++++++++++++++++++---------- 2 files changed, 141 insertions(+), 55 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 7fda4b964b..8c6f7595aa 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -466,6 +466,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // copyGroup is used to determine if all layers are copied copyGroup := sync.WaitGroup{} + // progressPool for creating progress bars progressPool := progress.NewPool(ctx, ic.c.progressOutput) defer progressPool.CleanUp() @@ -477,7 +478,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { layerIndex := 0 // some layers might be skipped, so we need a dedicated counter digestToCopyData := make(map[digest.Digest]*copyLayerData) for _, srcLayer := range srcInfos { - // Check if we'are already copying the layer + // Check if we're already copying the layer if _, ok := digestToCopyData[srcLayer.Digest]; ok { continue } @@ -489,13 +490,11 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // In which case src.LayerInfos will not have URLs because schema1 // does not support them. if ic.diffIDsAreNeeded { - cancelCopyLayer() return errors.New("getting DiffID for foreign layers is unimplemented") - } else { - logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) - cld.destInfo = srcLayer - continue } + logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) + cld.destInfo = srcLayer + continue // skip copying } copySemaphore.Acquire(cancelCtx, 1) // limits parallel copy operations @@ -508,8 +507,8 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { defer copyGroup.Done() cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer, index, progressPool) if cld.err != nil { - // Note that the error will be caught below. logrus.Errorf("copying layer %d failed: %v", index, cld.err) + // Stop all other running goroutines as we can't recover from an error cancelCopyLayer() } }(layerIndex, srcLayer, cld) diff --git a/storage/storage_image.go b/storage/storage_image.go index 8cb1baf7d3..fc0dcb29df 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -13,6 +13,7 @@ import ( "path/filepath" "sync" "sync/atomic" + "time" "github.com/containers/image/docker/reference" "github.com/containers/image/image" @@ -379,6 +380,69 @@ func (s *storageImageDestination) HasThreadSafePutBlob() bool { return true } +var errorAcquiringDigestLock = errors.New("") + +// tryReusingBlobFromOtherProcess is implementing a mechanism to detect if +// another process is already copying the blobinfo by making use of the +// blob-digest locks from containers/storage. The caller is expected to send a +// message through the done channel once the lock has been acquired. If we +// detect that another process is copying the blob, we wait until we own the +// lockfile and call tryReusingBlob() to check if we can reuse the committed +// layer. +// +// Note that the returned storage.Locker must be unlocked by the caller if +// the error is not a errorAcquiringDigestLock. +func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, done chan bool, bar *progress.Bar) (bool, types.BlobInfo, error) { + copiedByAnotherProcess := false + select { + case <-done: + case <-time.After(200 * time.Millisecond): + logrus.Debugf("blob %q is being copied by another process", blobinfo.Digest) + copiedByAnotherProcess = true + if bar != nil { + bar = bar.ReplaceBar( + progress.DigestToCopyAction(blobinfo.Digest, "blob"), + blobinfo.Size, + progress.BarOptions{ + StaticMessage: "paused: being copied by another process", + RemoveOnCompletion: true, + }) + } + // Wait until we acquired the lock or encountered an error. + <-done + } + + // Now, we own the lock. + // + // In case another process copied the blob, we should attempt reusing the + // blob. If it's not available, either the copy-detection heuristic failed + // (i.e., waiting 200 ms was not enough) or the other process failed to copy + // the blob. + if copiedByAnotherProcess { + reusable, blob, err := s.tryReusingBlob(ctx, blobinfo, layerIndexInImage, cache, false) + // If we can reuse the blob or hit an error trying to do so, we need to + // signal the result through the channel as another Goroutine is potentially + // waiting for it. If we can't resuse the blob and encountered no error, we + // need to copy it and will send the signal in PutBlob(). + if reusable { + logrus.Debugf("another process successfully copied blob %q", blobinfo.Digest) + if bar != nil { + bar = bar.ReplaceBar( + progress.DigestToCopyAction(blobinfo.Digest, "blob"), + blobinfo.Size, + progress.BarOptions{ + StaticMessage: "done: copied by another process", + }) + } + } else { + logrus.Debugf("another process must have failed copying blob %q", blobinfo.Digest) + } + return reusable, blob, err + } + + return false, types.BlobInfo{}, nil +} + // PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. @@ -391,30 +455,6 @@ func (s *storageImageDestination) HasThreadSafePutBlob() bool { // to any other readers for download using the supplied digest. // 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 (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (blob types.BlobInfo, err error) { - errorBlobInfo := types.BlobInfo{ - Digest: "", - Size: -1, - } - - if bar != nil { - kind := "blob" - if isConfig { - kind = "config" - } - bar = bar.ReplaceBar( - progress.DigestToCopyAction(blobinfo.Digest, kind), - blobinfo.Size, - progress.BarOptions{ - OnCompletionMessage: "done", - }) - stream = bar.ProxyReader(stream) - } - - blob, putBlobError := s.putBlob(ctx, stream, blobinfo, layerIndexInImage, cache, isConfig) - if putBlobError != nil { - return errorBlobInfo, putBlobError - } - // Deferred call to an anonymous func to signal potentially waiting // goroutines via the index-specific channel. defer func() { @@ -430,37 +470,84 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, } }() - // First, wait for the previous layer to be committed - previousID := "" - if layerIndexInImage > 0 { - channel := s.getChannelForLayer(layerIndexInImage - 1) - if committed := <-channel; !committed { - err := fmt.Errorf("committing previous layer %d failed", layerIndexInImage-1) - logrus.Errorf(err.Error()) - return errorBlobInfo, err + waitAndCommit := func(blob types.BlobInfo, err error) (types.BlobInfo, error) { + // First, wait for the previous layer to be committed + previousID := "" + if layerIndexInImage > 0 { + channel := s.getChannelForLayer(layerIndexInImage - 1) + if committed := <-channel; !committed { + err := fmt.Errorf("committing previous layer %d failed", layerIndexInImage-1) + logrus.Errorf(err.Error()) + return types.BlobInfo{}, err + } + var ok bool + s.putBlobMutex.Lock() + previousID, ok = s.indexToStorageID[layerIndexInImage-1] + s.putBlobMutex.Unlock() + if !ok { + return types.BlobInfo{}, fmt.Errorf("error committing blob %q: could not find parent layer ID", blob.Digest.String()) + } } - var ok bool - s.putBlobMutex.Lock() - previousID, ok = s.indexToStorageID[layerIndexInImage-1] - s.putBlobMutex.Unlock() - if !ok { - return errorBlobInfo, fmt.Errorf("error committing blob %q: could not find parent layer ID", blob.Digest.String()) + + // Commit the blob + if layerIndexInImage >= 0 { + id, err := s.commitBlob(ctx, blob, previousID) + if err == nil { + s.putBlobMutex.Lock() + s.blobLayerIDs[blob.Digest] = id + s.indexToStorageID[layerIndexInImage] = id + s.putBlobMutex.Unlock() + } else { + return types.BlobInfo{}, err + } } + return blob, nil } - // Commit the blob + // Check if another process is already copying the blob. Please refer to the + // doc of tryReusingBlobFromOtherProcess for further information. if layerIndexInImage >= 0 { - id, err := s.commitBlob(ctx, blob, previousID) - if err == nil { - s.putBlobMutex.Lock() - s.blobLayerIDs[blob.Digest] = id - s.indexToStorageID[layerIndexInImage] = id - s.putBlobMutex.Unlock() - } else { - return errorBlobInfo, err + // The reasoning for getting the locker here is to make the code paths + // of locking and unlocking it more obvious and simple. + locker, err := s.imageRef.transport.store.GetDigestLock(blobinfo.Digest) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(errorAcquiringDigestLock, "error acquiring lock for blob %q: %v", blobinfo.Digest) + } + done := make(chan bool, 1) + defer locker.Unlock() + go func() { + locker.Lock() + done <- true + }() + reusable, blob, err := s.tryReusingBlobFromOtherProcess(ctx, stream, blobinfo, layerIndexInImage, cache, done, bar) + if err != nil { + return blob, err + } + if reusable { + return waitAndCommit(blob, err) } } - return blob, nil + + if bar != nil { + kind := "blob" + if isConfig { + kind = "config" + } + bar = bar.ReplaceBar( + progress.DigestToCopyAction(blobinfo.Digest, kind), + blobinfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + stream = bar.ProxyReader(stream) + } + + blob, err = s.putBlob(ctx, stream, blobinfo, layerIndexInImage, cache, isConfig) + if err != nil { + return types.BlobInfo{}, err + } + + return waitAndCommit(blob, err) } func (s *storageImageDestination) putBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { From 4f9f19c08f5f8a11c0902a76efe49973fef513b2 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 23 Apr 2019 11:12:30 +0200 Subject: [PATCH 08/39] copy doc comments to all implementations of {Put,TryReusing}Blob Signed-off-by: Valentin Rothberg --- directory/directory_dest.go | 14 ++++++++++++-- docker/docker_image_dest.go | 14 ++++++++++++-- docker/tarfile/dest.go | 14 ++++++++++++-- oci/archive/oci_dest.go | 11 ++++++++++- oci/layout/oci_dest.go | 11 ++++++++++- openshift/openshift.go | 14 ++++++++++++-- ostree/ostree_dest.go | 11 ++++++++++- storage/storage_image.go | 4 +++- types/types.go | 2 ++ 9 files changed, 83 insertions(+), 12 deletions(-) diff --git a/directory/directory_dest.go b/directory/directory_dest.go index e9efadfac5..e0b5b94e6d 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -130,10 +130,16 @@ func (d *dirImageDestination) HasThreadSafePutBlob() bool { return false } -// PutBlob writes contents of stream and returns data representing the result (with all data filled in). +// PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. +// inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -179,8 +185,12 @@ func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *dirImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index e2525d1397..3fdc396cff 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -118,10 +118,16 @@ func (d *dockerImageDestination) HasThreadSafePutBlob() bool { return true } -// PutBlob writes contents of stream and returns data representing the result (with all data filled in). +// PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. +// inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -268,8 +274,12 @@ func (d *dockerImageDestination) mountBlob(ctx context.Context, srcRepo referenc // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *dockerImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/docker/tarfile/dest.go b/docker/tarfile/dest.go index 6d37396034..1be485e7c1 100644 --- a/docker/tarfile/dest.go +++ b/docker/tarfile/dest.go @@ -88,10 +88,16 @@ func (d *Destination) HasThreadSafePutBlob() bool { return false } -// PutBlob writes contents of stream and returns data representing the result (with all data filled in). +// PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. +// inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -161,8 +167,12 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *Destination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 6535dec56a..2596e53dea 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -88,6 +88,11 @@ func (d *ociArchiveImageDestination) HasThreadSafePutBlob() bool { // inputInfo.Size is the expected length of stream, if known. // inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -98,8 +103,12 @@ func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Read // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *ociArchiveImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 95212e48d2..39ae027730 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -118,6 +118,11 @@ func (d *ociImageDestination) HasThreadSafePutBlob() bool { // inputInfo.Size is the expected length of stream, if known. // inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -184,8 +189,12 @@ func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *ociImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/openshift/openshift.go b/openshift/openshift.go index bd4dbaaae2..3b50501e27 100644 --- a/openshift/openshift.go +++ b/openshift/openshift.go @@ -389,10 +389,16 @@ func (d *openshiftImageDestination) HasThreadSafePutBlob() bool { return false } -// PutBlob writes contents of stream and returns data representing the result (with all data filled in). +// PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. +// inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -403,8 +409,12 @@ func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reade // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *openshiftImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index ad1098049f..bd0b06e0aa 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -143,6 +143,11 @@ func (d *ostreeImageDestination) HasThreadSafePutBlob() bool { // inputInfo.Size is the expected length of stream, if known. // inputInfo.MediaType describes the blob format, if known. // May update cache. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -339,8 +344,12 @@ func (d *ostreeImageDestination) importConfig(repo *otbuiltin.Repo, blob *blobTo // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. +// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. -// If the blob has been succesfully reused, returns (true, info, nil); info must contain at least a digest and size. +// If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (d *ostreeImageDestination) TryReusingBlob(ctx context.Context, info types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { diff --git a/storage/storage_image.go b/storage/storage_image.go index fc0dcb29df..0d107e8658 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -450,7 +450,9 @@ func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Con // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. +// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -708,8 +710,8 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t // info.Digest must not be empty. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. diff --git a/types/types.go b/types/types.go index 2cbf785902..890e7df3a5 100644 --- a/types/types.go +++ b/types/types.go @@ -267,6 +267,7 @@ type ImageDestination interface { // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. // Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. + // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. // 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. @@ -279,6 +280,7 @@ type ImageDestination interface { // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. + // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. From fdaf13afd56a71c0f2f6870d061826fd1dc19d70 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 23 Apr 2019 12:22:05 +0200 Subject: [PATCH 09/39] pkg/progress: improve comments Signed-off-by: Valentin Rothberg --- pkg/progress/progress.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/progress/progress.go b/pkg/progress/progress.go index 22374943ad..58699b2a93 100644 --- a/pkg/progress/progress.go +++ b/pkg/progress/progress.go @@ -27,8 +27,8 @@ type Bar struct { // BarOptions includes various options to control AddBar. type BarOptions struct { - // Remove the bar on completion. This must be true if the bar will be - // replaced by another one. + // RemoveOnCompletion the bar on completion. This must be true if the bar + // will be replaced by another one. RemoveOnCompletion bool // OnCompletionMessage will be shown on completion and replace the progress bar. OnCompletionMessage string @@ -77,10 +77,12 @@ func DigestToCopyAction(digest digest.Digest, kind string) string { return copyAction } -// AddBar adds a new Bar to the Pool. +// AddBar adds a new Bar to the Pool. Use options to control the behavior and +// appearance of the bar. func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { var bar *mpb.Bar + // First decorator showing action (e.g., "Copying blob 123456abcd") mpbOptions := []mpb.BarOption{ mpb.PrependDecorators( decor.Name(action), @@ -93,9 +95,12 @@ func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { if options.ReplaceBar != nil { mpbOptions = append(mpbOptions, mpb.BarReplaceOnComplete(options.ReplaceBar.bar)) + // bar.SetTotal(0, true) will make sure that the bar is stopped defer options.ReplaceBar.bar.SetTotal(0, true) } + // If not static message is set, we display the progress bar. Otherwise, + // we'll display the message only. if options.StaticMessage == "" { mpbOptions = append(mpbOptions, mpb.AppendDecorators( @@ -118,8 +123,8 @@ func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { } } -// ReplaceBar returns a bar replacing the current one. Note that the current one -// will be terminated and should have been created with +// ReplaceBar is like Pool.AddBar but replace the bar in it's pool. Note that +// the bar be terminated and should have been created with // options.RemoveOnCompletion. func (b *Bar) ReplaceBar(action string, size int64, options BarOptions) *Bar { options.ReplaceBar = b From f05e672a3dcbddc20513aa7f922469ca548a2662 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 23 Apr 2019 14:28:16 +0200 Subject: [PATCH 10/39] copy: don't show progress when copying a config Configs are comparatively small and quickly downloaded, such that displaying a progress is more a distraction than an indicator. Signed-off-by: Valentin Rothberg --- pkg/progress/progress.go | 33 +++++++++++++++++++++++++-------- storage/storage_image.go | 7 +++++++ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/pkg/progress/progress.go b/pkg/progress/progress.go index 58699b2a93..2972e489e0 100644 --- a/pkg/progress/progress.go +++ b/pkg/progress/progress.go @@ -30,7 +30,9 @@ type BarOptions struct { // RemoveOnCompletion the bar on completion. This must be true if the bar // will be replaced by another one. RemoveOnCompletion bool - // OnCompletionMessage will be shown on completion and replace the progress bar. + // OnCompletionMessage will be shown on completion and replace the progress + // bar. Note that setting OnCompletionMessage will cause the progress bar + // (or the static message) to be cleared on completion. OnCompletionMessage string // ReplaceBar is the bar to replace. ReplaceBar *Bar @@ -99,7 +101,7 @@ func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { defer options.ReplaceBar.bar.SetTotal(0, true) } - // If not static message is set, we display the progress bar. Otherwise, + // If no static message is set, we display the progress bar. Otherwise, // we'll display the message only. if options.StaticMessage == "" { mpbOptions = append(mpbOptions, @@ -109,14 +111,29 @@ func (p *Pool) AddBar(action string, size int64, options BarOptions) *Bar { ) mpbOptions = append(mpbOptions, mpb.BarClearOnComplete()) bar = p.pool.AddBar(size, mpbOptions...) - } else { - barFiller := mpb.FillerFunc( - func(w io.Writer, width int, st *decor.Statistics) { - fmt.Fprint(w, options.StaticMessage) - }) - bar = p.pool.Add(0, barFiller, mpbOptions...) + return &Bar{ + bar: bar, + pool: p, + } } + barFiller := mpb.FillerFunc( + func(w io.Writer, width int, st *decor.Statistics) { + fmt.Fprint(w, options.StaticMessage) + }) + + // If OnCompletionMessage is set, we need to add the decorator and clear + // the bar on completion. + if options.OnCompletionMessage != "" { + mpbOptions = append(mpbOptions, + mpb.AppendDecorators( + decor.OnComplete(decor.Name(""), " "+options.OnCompletionMessage), + ), + ) + mpbOptions = append(mpbOptions, mpb.BarClearOnComplete()) + } + + bar = p.pool.Add(size, barFiller, mpbOptions...) return &Bar{ bar: bar, pool: p, diff --git a/storage/storage_image.go b/storage/storage_image.go index 0d107e8658..582d0762c5 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -532,13 +532,20 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, if bar != nil { kind := "blob" + message := "" if isConfig { kind = "config" + // Setting the StaticMessage for the config avoids displaying a + // progress bar. Configs are comparatively small and quickly + // downloaded, such that displaying a progress is more a distraction + // than an indicator. + message = " " } bar = bar.ReplaceBar( progress.DigestToCopyAction(blobinfo.Digest, kind), blobinfo.Size, progress.BarOptions{ + StaticMessage: message, OnCompletionMessage: "done", }) stream = bar.ProxyReader(stream) From edac09bdd34c17f7ffeb7d6a0f4d9a47353dcbad Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 23 Apr 2019 15:00:01 +0200 Subject: [PATCH 11/39] fix gofmt issues Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 582d0762c5..268d102a94 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -380,8 +380,6 @@ func (s *storageImageDestination) HasThreadSafePutBlob() bool { return true } -var errorAcquiringDigestLock = errors.New("") - // tryReusingBlobFromOtherProcess is implementing a mechanism to detect if // another process is already copying the blobinfo by making use of the // blob-digest locks from containers/storage. The caller is expected to send a @@ -389,9 +387,6 @@ var errorAcquiringDigestLock = errors.New("") // detect that another process is copying the blob, we wait until we own the // lockfile and call tryReusingBlob() to check if we can reuse the committed // layer. -// -// Note that the returned storage.Locker must be unlocked by the caller if -// the error is not a errorAcquiringDigestLock. func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, done chan bool, bar *progress.Bar) (bool, types.BlobInfo, error) { copiedByAnotherProcess := false select { @@ -513,7 +508,7 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, // of locking and unlocking it more obvious and simple. locker, err := s.imageRef.transport.store.GetDigestLock(blobinfo.Digest) if err != nil { - return types.BlobInfo{}, errors.Wrapf(errorAcquiringDigestLock, "error acquiring lock for blob %q: %v", blobinfo.Digest) + return types.BlobInfo{}, errors.Wrapf(err, "error acquiring lock for blob %q", blobinfo.Digest) } done := make(chan bool, 1) defer locker.Unlock() From f3cd7f25a14a67c89dbab64de7e547ed093e3ef7 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 26 Apr 2019 14:27:41 +0200 Subject: [PATCH 12/39] copy: remove leftover debug output Remove a leftover debugging output that was added while working on previous changes. Signed-off-by: Valentin Rothberg --- copy/copy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index 8c6f7595aa..c27f5fd6e3 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -507,7 +507,6 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { defer copyGroup.Done() cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer, index, progressPool) if cld.err != nil { - logrus.Errorf("copying layer %d failed: %v", index, cld.err) // Stop all other running goroutines as we can't recover from an error cancelCopyLayer() } From 263938fc1d6f25b939238d98199c842afe9e83dc Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 26 Apr 2019 14:39:32 +0200 Subject: [PATCH 13/39] copy: set ProyReader on decompressed destStream Signed-off-by: Valentin Rothberg --- copy/copy.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index c27f5fd6e3..5d583596c0 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -808,6 +808,23 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } isCompressed := decompressor != nil + // Instead of adding boilerplate code to ALL PutBlob()s, just special case + // the storage transport, which is the only transport where we need control + // over the progress.Bar. + if c.dest.Reference().Transport().Name() != storage.Name() { + kind := "blob" + if isConfig { + kind = "config" + } + bar = bar.ReplaceBar( + progress.DigestToCopyAction(srcInfo.Digest, kind), + srcInfo.Size, + progress.BarOptions{ + OnCompletionMessage: "done", + }) + destStream = bar.ProxyReader(destStream) + } + // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. if getOriginalLayerCopyWriter != nil { @@ -859,23 +876,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } } - // Instead of adding boilerplate code to ALL PutBlob()s, just special case - // the storage transport, which is the only transport where we need control - // over the progress.Bar. - if c.dest.Reference().Transport().Name() != storage.Name() { - kind := "blob" - if isConfig { - kind = "config" - } - bar = bar.ReplaceBar( - progress.DigestToCopyAction(srcInfo.Digest, kind), - srcInfo.Size, - progress.BarOptions{ - OnCompletionMessage: "done", - }) - destStream = bar.ProxyReader(destStream) - } - // === Finally, send the layer stream to dest. uploadedInfo, err := c.dest.PutBlob(ctx, destStream, inputInfo, layerIndexInImage, c.blobInfoCache, isConfig, bar) if err != nil { From 73569e9c37bcbbb18fad226ea27eb18f12c641bc Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 26 Apr 2019 14:44:09 +0200 Subject: [PATCH 14/39] storage: image: remove logrus.Errorf() Remove calls to Errof() or change them to Debugf(); they were used for debugging during development. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 268d102a94..269aec1de6 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -462,7 +462,7 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, channel := s.getChannelForLayer(layerIndexInImage) channel <- err == nil if err != nil { - logrus.Errorf("error while committing blob %d: %v", layerIndexInImage, err) + logrus.Debugf("error while committing blob %d: %v", layerIndexInImage, err) } } }() @@ -474,7 +474,6 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, channel := s.getChannelForLayer(layerIndexInImage - 1) if committed := <-channel; !committed { err := fmt.Errorf("committing previous layer %d failed", layerIndexInImage-1) - logrus.Errorf(err.Error()) return types.BlobInfo{}, err } var ok bool From f5f95add61f49c8900878b8f6beaa7b56539e91b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 26 Apr 2019 16:25:49 +0200 Subject: [PATCH 15/39] storage: remove Name() func Remove the Name() func as users can conveniently use `storage.Transport.Name()` instead. Signed-off-by: Valentin Rothberg --- copy/copy.go | 4 ++-- storage/storage_transport.go | 8 +------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 5d583596c0..ac809385b1 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -674,7 +674,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, la // Instead of adding boilerplate code to ALL TryReusingBlob()s, just // special case the storage transport, which is the only transport // where we need control over the progress.Bar. - if ic.c.dest.Reference().Transport().Name() != storage.Name() { + if ic.c.dest.Reference().Transport().Name() != storage.Transport.Name() { bar.ReplaceBar( progressBarAction, 0, @@ -811,7 +811,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr // Instead of adding boilerplate code to ALL PutBlob()s, just special case // the storage transport, which is the only transport where we need control // over the progress.Bar. - if c.dest.Reference().Transport().Name() != storage.Name() { + if c.dest.Reference().Transport().Name() != storage.Transport.Name() { kind := "blob" if isConfig { kind = "config" diff --git a/storage/storage_transport.go b/storage/storage_transport.go index 70c18640e4..c9a05e6c01 100644 --- a/storage/storage_transport.go +++ b/storage/storage_transport.go @@ -38,12 +38,6 @@ var ( ErrPathNotAbsolute = errors.New("path name is not absolute") ) -// Name returns the name of the transport and can be used for type detection of -// a Transport object. -func Name() string { - return "containers-storage" -} - // StoreTransport is an ImageTransport that uses a storage.Store to parse // references, either its own default or one that it's told to use. type StoreTransport interface { @@ -77,7 +71,7 @@ type storageTransport struct { func (s *storageTransport) Name() string { // Still haven't really settled on a name. - return Name() + return "containers-storage" } // SetStore sets the Store object which the Transport will use for parsing From 02c3576dc2cb177b814a3daf57c52b178f9cbf88 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 14:19:44 +0200 Subject: [PATCH 16/39] copy: remove unused newProgressPool Signed-off-by: Valentin Rothberg --- copy/copy.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index ac809385b1..01c72aedcd 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -27,7 +27,6 @@ import ( digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" "github.com/sirupsen/logrus" - "github.com/vbauerster/mpb" "golang.org/x/crypto/ssh/terminal" "golang.org/x/sync/semaphore" ) @@ -594,17 +593,6 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context) ([]byte return manifest, nil } -// newProgressPool creates a *mpb.Progress and a cleanup function. -// The caller must eventually call the returned cleanup function after the pool will no longer be updated. -func (c *copier) newProgressPool(ctx context.Context) (*mpb.Progress, func()) { - ctx, cancel := context.WithCancel(ctx) - pool := mpb.New(mpb.WithWidth(40), mpb.WithOutput(c.progressOutput), mpb.WithContext(ctx)) - return pool, func() { - cancel() - pool.Wait() - } -} - // copyConfig copies config.json, if any, from src to dest. func (c *copier) copyConfig(ctx context.Context, src types.Image) error { srcInfo := src.ConfigInfo() From c930452ff531b612a922ec18b68cdd7df004e946 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 14:23:14 +0200 Subject: [PATCH 17/39] pkg/progress: remove unused getWriter Also fix some typos in the comments. Signed-off-by: Valentin Rothberg --- pkg/progress/progress.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/pkg/progress/progress.go b/pkg/progress/progress.go index 2972e489e0..d5dd20f56d 100644 --- a/pkg/progress/progress.go +++ b/pkg/progress/progress.go @@ -1,5 +1,5 @@ -// Package progress is exposes a convenience API and abstractions for creating -// and managing pools of multi-progress bars. +// Package progress exposes a convenience API and abstractions for creating and +// managing pools of multi-progress bars. package progress import ( @@ -41,7 +41,7 @@ type BarOptions struct { StaticMessage string } -// NewPool returns a Pool. The caller must eventually call ProgressPoo.CleanUp() +// NewPool returns a Pool. The caller must eventually call ProgressPool.CleanUp() // when the pool will no longer be updated. func NewPool(ctx context.Context, writer io.Writer) *Pool { ctx, cancelFunc := context.WithCancel(ctx) @@ -59,11 +59,6 @@ func (p *Pool) CleanUp() { p.pool.Wait() } -// getWriter returns the Pool's writer. -func (p *Pool) getWriter() io.Writer { - return p.writer -} - // DigestToCopyAction returns a string based on the blobinfo and kind. // It's a convenience function for the c/image library when copying images. func DigestToCopyAction(digest digest.Digest, kind string) string { From 95226e3d949d97ea8d50b0b59fb9d9cba7cc6fb8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 14:27:01 +0200 Subject: [PATCH 18/39] storage_test: correct arguments for PutBlob() The recent changes to PutBlob() require to correctly specify the layer's index in the image and the isConfig parameter. Signed-off-by: Valentin Rothberg --- storage/storage_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/storage_test.go b/storage/storage_test.go index afbc7d3cb1..98141ca8ee 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -384,7 +384,7 @@ func TestWriteRead(t *testing.T) { t.Fatalf("Error saving randomly-generated layer to destination: %v", err) } t.Logf("Wrote randomly-generated layer %q (%d/%d bytes) to destination", digest, size, decompressedSize) - if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, 0, cache, false, nil); err != nil { + if _, err := dest.PutBlob(context.Background(), bytes.NewBufferString(config), configInfo, -1, cache, true, nil); err != nil { t.Fatalf("Error saving config to destination: %v", err) } manifest := strings.Replace(manifestFmt, "%lh", digest.String(), -1) @@ -864,7 +864,7 @@ func TestSize(t *testing.T) { if _, err := dest.PutBlob(context.Background(), bytes.NewBuffer(blob), types.BlobInfo{ Size: size2, Digest: digest2, - }, 0, cache, false, nil); err != nil { + }, 1, cache, false, nil); err != nil { t.Fatalf("Error saving randomly-generated layer 2 to destination: %v", err) } manifest := fmt.Sprintf(` From d2f52332f9e5494ac13da1c3a9971e01d4d4ce51 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 14:42:49 +0200 Subject: [PATCH 19/39] update docs for {TryReusing,Put}Blob Signed-off-by: Valentin Rothberg --- directory/directory_dest.go | 14 ++++++-------- docker/docker_image_dest.go | 14 ++++++-------- docker/tarfile/dest.go | 14 ++++++-------- oci/archive/oci_dest.go | 14 ++++++-------- oci/layout/oci_dest.go | 14 ++++++-------- openshift/openshift.go | 14 ++++++-------- ostree/ostree_dest.go | 14 ++++++-------- storage/storage_image.go | 6 +++--- types/types.go | 14 ++++++-------- 9 files changed, 51 insertions(+), 67 deletions(-) diff --git a/directory/directory_dest.go b/directory/directory_dest.go index e0b5b94e6d..cfb346c639 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -136,9 +136,9 @@ func (d *dirImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -185,11 +185,9 @@ func (d *dirImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index 3fdc396cff..6ea3f60459 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -124,9 +124,9 @@ func (d *dockerImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -274,11 +274,9 @@ func (d *dockerImageDestination) mountBlob(ctx context.Context, srcRepo referenc // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/docker/tarfile/dest.go b/docker/tarfile/dest.go index 1be485e7c1..bcf0241fe0 100644 --- a/docker/tarfile/dest.go +++ b/docker/tarfile/dest.go @@ -94,9 +94,9 @@ func (d *Destination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -167,11 +167,9 @@ func (d *Destination) PutBlob(ctx context.Context, stream io.Reader, inputInfo t // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 2596e53dea..7a505a35e0 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -89,9 +89,9 @@ func (d *ociArchiveImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -103,11 +103,9 @@ func (d *ociArchiveImageDestination) PutBlob(ctx context.Context, stream io.Read // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 39ae027730..ad1afd44b5 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -119,9 +119,9 @@ func (d *ociImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -189,11 +189,9 @@ func (d *ociImageDestination) PutBlob(ctx context.Context, stream io.Reader, inp // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/openshift/openshift.go b/openshift/openshift.go index 3b50501e27..71a03ce0f3 100644 --- a/openshift/openshift.go +++ b/openshift/openshift.go @@ -395,9 +395,9 @@ func (d *openshiftImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -409,11 +409,9 @@ func (d *openshiftImageDestination) PutBlob(ctx context.Context, stream io.Reade // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index bd0b06e0aa..52586b1721 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -144,9 +144,9 @@ func (d *ostreeImageDestination) HasThreadSafePutBlob() bool { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -344,11 +344,9 @@ func (d *ostreeImageDestination) importConfig(repo *otbuiltin.Repo, blob *blobTo // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. -// layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. -// Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. -// If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. diff --git a/storage/storage_image.go b/storage/storage_image.go index 269aec1de6..c560472bac 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -444,9 +444,9 @@ func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Con // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of -// PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. -// Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. -// Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. +// layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of +// PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. +// The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. diff --git a/types/types.go b/types/types.go index 890e7df3a5..7cf4c38141 100644 --- a/types/types.go +++ b/types/types.go @@ -264,9 +264,9 @@ type ImageDestination interface { // inputInfo.MediaType describes the blob format, if known. // May update cache. // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of - // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. - // Non-layer blobs (e.g., configs or throwaway layers) must have a value < 0. - // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. + // layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of + // PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. + // The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. // WARNING: The contents of stream are being verified on the fly. Until stream.Read() returns io.EOF, the contents of the data SHOULD NOT be available // to any other readers for download using the supplied digest. @@ -277,11 +277,9 @@ type ImageDestination interface { // TryReusingBlob checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination // (e.g. if the blob is a filesystem layer, this signifies that the changes it describes need to be applied again when composing a filesystem tree). // info.Digest must not be empty. - // layerIndexInImage must be properly set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of - // PutBlob() and TryReusingBlob() where the layers must be written to the backend storage in sequential order. A value >= indicates that the blob a layer. - // Note that only the containers-storage destination is sensitive to the layerIndexInImage parameter. Other transport destinations ignore it. - // Same applies to bar, which is used in the containers-storage destination to update the progress bars displayed in the terminal. If it's nil, it will be ignored. - // If canSubstitute, TryReusingBlob can use an equivalent equivalent of the desired blob; in that case the returned info may not match the input. + // layerIndexInImage is set to the layer index of the corresponding blob in the image. This value is required to allow parallel executions of + // PutBlob() and TryReusingBlob() where the layers must be written to the destination in sequential order. A value >= 0 indicates that the blob is a layer. + // The bar can optionally be specified to allow replacing/updating it. Note that only the containers-storage transport updates the bar; other transports ignore it. // If the blob has been successfully reused, returns (true, info, nil); info must contain at least a digest and size. // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. From 20558546db0f3f1a4e54dac7154b66923c17c10f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 14:49:49 +0200 Subject: [PATCH 20/39] storageImageDestination: update docs of internal members Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index c560472bac..fefd112ff7 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -66,8 +66,8 @@ type storageImageDestination struct { blobLayerIDs map[digest.Digest]string // Mapping from layer blobsums to their corresponding storage layer ID fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them - indexToStorageID map[int]string // Mapping from layer index to the layer IDs in the storage - indexToDoneChannel map[int]chan bool // Mapping from layer index to a channel to indicate the layer has been written to storage + indexToStorageID map[int]string // Mapping from layer index to the layer IDs in the storage. Only valid after receiving `true` from the corresponding `indexToDoneChannel`. + indexToDoneChannel map[int]chan bool // Mapping from layer index to a channel to indicate the layer has been written to storage. True is written when the corresponding index/layer has successfully been written to the storage. SignatureSizes []int `json:"signature-sizes,omitempty"` // List of sizes of each signature slice } From affe61d4ae0e633d621a0195cfdf87375fb63d77 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 15:01:35 +0200 Subject: [PATCH 21/39] storageImageDestination: remove unused blobLayerIDs Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index fefd112ff7..1e659c9fb6 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -63,7 +63,6 @@ type storageImageDestination struct { signatures []byte // Signature contents, temporary putBlobMutex sync.Mutex // Mutex to sync state for parallel PutBlob executions blobDiffIDs map[digest.Digest]digest.Digest // Mapping from layer blobsums to their corresponding DiffIDs - blobLayerIDs map[digest.Digest]string // Mapping from layer blobsums to their corresponding storage layer ID fileSizes map[digest.Digest]int64 // Mapping from layer blobsums to their sizes filenames map[digest.Digest]string // Mapping from layer blobsums to names of files we used to hold them indexToStorageID map[int]string // Mapping from layer index to the layer IDs in the storage. Only valid after receiving `true` from the corresponding `indexToDoneChannel`. @@ -331,7 +330,6 @@ func newImageDestination(imageRef storageReference) (*storageImageDestination, e imageRef: imageRef, directory: directory, blobDiffIDs: make(map[digest.Digest]digest.Digest), - blobLayerIDs: make(map[digest.Digest]string), fileSizes: make(map[digest.Digest]int64), filenames: make(map[digest.Digest]string), indexToStorageID: make(map[int]string), @@ -490,7 +488,6 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, id, err := s.commitBlob(ctx, blob, previousID) if err == nil { s.putBlobMutex.Lock() - s.blobLayerIDs[blob.Digest] = id s.indexToStorageID[layerIndexInImage] = id s.putBlobMutex.Unlock() } else { @@ -648,7 +645,6 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t if len(layers) > 0 { // Save this for completeness. s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest - s.blobLayerIDs[blobinfo.Digest] = layers[0].ID if layerIndexInImage >= 0 { s.indexToStorageID[layerIndexInImage] = layers[0].ID } @@ -667,7 +663,6 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t if len(layers) > 0 { // Record the uncompressed value so that we can use it to calculate layer IDs. s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest - s.blobLayerIDs[blobinfo.Digest] = layers[0].ID if layerIndexInImage >= 0 { s.indexToStorageID[layerIndexInImage] = layers[0].ID } @@ -689,7 +684,6 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t } if len(layers) > 0 { s.blobDiffIDs[uncompressedDigest] = layers[0].UncompressedDigest - s.blobLayerIDs[blobinfo.Digest] = layers[0].ID if layerIndexInImage >= 0 { s.indexToStorageID[layerIndexInImage] = layers[0].ID } From b377743a5fcd17724eac573c06b26f2a5950348f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 15:05:56 +0200 Subject: [PATCH 22/39] storage: TryReusingBlob: defer writing to done channel We use a channel to signal potentially waiting goroutines when a layer has been written or when we found it in the storage. Move writing to the channel to a deferred statement to be **very** sure it's always being executed. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 1e659c9fb6..553cfebe7a 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -718,8 +718,10 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t // waiting for it. If we can't resuse the blob and encountered no error, we // need to copy it and will send the signal in PutBlob(). if (layerIndexInImage >= 0) && (err != nil || reusable) { - channel := s.getChannelForLayer(layerIndexInImage) - channel <- (err == nil) + defer func() { // Defer to be **very** sure to always execute the code. + channel := s.getChannelForLayer(layerIndexInImage) + channel <- (err == nil) + }() } if bar != nil && reusable { bar.ReplaceBar( From 74e23192f86cf53cf7ecd574423da92b040d8d35 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 15:18:34 +0200 Subject: [PATCH 23/39] storage: PutBlob: make index checks as early as possible Improves readability and slightly improves performance. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 553cfebe7a..47789259e5 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -452,9 +452,8 @@ func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Con func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool, bar *progress.Bar) (blob types.BlobInfo, err error) { // Deferred call to an anonymous func to signal potentially waiting // goroutines via the index-specific channel. - defer func() { - // No need to wait - if layerIndexInImage >= 0 { + if layerIndexInImage >= 0 { + defer func() { // It's a buffered channel, so we don't wait for the message to be // received channel := s.getChannelForLayer(layerIndexInImage) @@ -462,10 +461,15 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, if err != nil { logrus.Debugf("error while committing blob %d: %v", layerIndexInImage, err) } - } - }() + + }() + } waitAndCommit := func(blob types.BlobInfo, err error) (types.BlobInfo, error) { + if layerIndexInImage >= 0 { + return blob, err + } + // First, wait for the previous layer to be committed previousID := "" if layerIndexInImage > 0 { @@ -484,15 +488,13 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, } // Commit the blob - if layerIndexInImage >= 0 { - id, err := s.commitBlob(ctx, blob, previousID) - if err == nil { - s.putBlobMutex.Lock() - s.indexToStorageID[layerIndexInImage] = id - s.putBlobMutex.Unlock() - } else { - return types.BlobInfo{}, err - } + id, err := s.commitBlob(ctx, blob, previousID) + if err == nil { + s.putBlobMutex.Lock() + s.indexToStorageID[layerIndexInImage] = id + s.putBlobMutex.Unlock() + } else { + return types.BlobInfo{}, err } return blob, nil } From 21c925426a99f0a66d9bbde844fb77b30c84c7c8 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 15:38:38 +0200 Subject: [PATCH 24/39] storage: TryReusingBlob: s/reusable/reused/ Rename the variable to reused to be explicit about the fact that the internal state already implies that the layer is reused. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 47789259e5..098cdca8d5 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -714,18 +714,18 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t // If the transport can not reuse the requested blob, TryReusingBlob returns (false, {}, nil); it returns a non-nil error only on an unexpected failure. // May use and/or update cache. func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool, bar *progress.Bar) (bool, types.BlobInfo, error) { - reusable, blob, err := s.tryReusingBlob(ctx, blobinfo, layerIndexInImage, cache, canSubstitute) + reused, blob, err := s.tryReusingBlob(ctx, blobinfo, layerIndexInImage, cache, canSubstitute) // If we can reuse the blob or hit an error trying to do so, we need to // signal the result through the channel as another Goroutine is potentially - // waiting for it. If we can't resuse the blob and encountered no error, we + // waiting for it. If we can't reuse the blob and encountered no error, we // need to copy it and will send the signal in PutBlob(). - if (layerIndexInImage >= 0) && (err != nil || reusable) { + if (layerIndexInImage >= 0) && (err != nil || reused) { defer func() { // Defer to be **very** sure to always execute the code. channel := s.getChannelForLayer(layerIndexInImage) channel <- (err == nil) }() } - if bar != nil && reusable { + if bar != nil && reused { bar.ReplaceBar( progress.DigestToCopyAction(blobinfo.Digest, "blob"), 0, @@ -733,7 +733,7 @@ func (s *storageImageDestination) TryReusingBlob(ctx context.Context, blobinfo t StaticMessage: "skipped: already exists", }) } - return reusable, blob, err + return reused, blob, err } // computeID computes a recommended image ID based on information we have so far. If From c4ae4e9a9fb487bd04fe37b7e60259390316ba96 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 15:42:14 +0200 Subject: [PATCH 25/39] storage: Commit: no layers: change error string Change the error to: "image does not contain a non-empty or non-throwaway layer" This should give the users a better guidance than the previous "could not find top layer" error message. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 098cdca8d5..7e81d6c128 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -937,7 +937,7 @@ func (s *storageImageDestination) Commit(ctx context.Context) error { } if lastLayer == "" { - return fmt.Errorf("could not find top layer") + return fmt.Errorf("image does not contain a non-empty or non-throwaway layer") } // If one of those blobs was a configuration blob, then we can try to dig out the date when the image From 621d10b45590e1621e2dccc0d9eba0b36484520c Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Apr 2019 17:54:32 +0200 Subject: [PATCH 26/39] storageImageDestination: commitBlob: add TODO comment Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 7e81d6c128..2bbca53441 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -818,8 +818,12 @@ func (s *storageImageDestination) commitBlob(ctx context.Context, blob types.Blo logrus.Debugf("layer for blob %q already found in storage", blob.Digest) return layer.ID, nil } - // Check if we previously cached a file with that blob's contents. If we didn't, - // then we need to read the desired contents from a layer. + // Check if we previously cached a file with that blob's contents. If we + // didn't, then we need to read the desired contents from a layer. + // + // TODO: the lookup here is only required when being called for an entirely + // new layer or reused ones. Maybe we should split that into separate + // functions? s.putBlobMutex.Lock() filename, ok := s.filenames[blob.Digest] s.putBlobMutex.Unlock() From 83a1dab8458e5df7eda958e319307a4822121322 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 30 Apr 2019 14:33:43 +0200 Subject: [PATCH 27/39] storageImageDestination: cleanup docs of commitBlob() Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 2bbca53441..02e49f3210 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -795,10 +795,7 @@ func (s *storageImageDestination) getConfigBlob(info types.BlobInfo) ([]byte, er return nil, errors.New("blob not found") } -// commitBlobs commits the specified blob to the storage. If not already done, -// it will block until the parent layer is committed to the storage. Note that -// the only caller of commitBlob() is PutBlob(), which is recording the results -// and send the error through the corresponding channel in s.previousLayerResult. +// commitBlobs commits the specified blob to the storage. func (s *storageImageDestination) commitBlob(ctx context.Context, blob types.BlobInfo, previousID string) (string, error) { logrus.Debugf("committing blob %q", blob.Digest) // Check if there's already a layer with the ID that we'd give to the result of applying From 5cc5f9e6acc5a8308b2eb10225ab280ae6f72625 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 30 Apr 2019 17:39:16 +0200 Subject: [PATCH 28/39] copy: set index of empty layers to -1 Signed-off-by: Valentin Rothberg --- copy/copy.go | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 01c72aedcd..b37f0a62c1 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -433,7 +433,6 @@ func isTTY(w io.Writer) bool { // copyLayers copies layers from ic.src/ic.c.rawSource to dest, using and updating ic.manifestUpdates if necessary and ic.canModifyManifest. func (ic *imageCopier) copyLayers(ctx context.Context) error { srcInfos := ic.src.LayerInfos() - numLayers := len(srcInfos) updatedSrcInfos, err := ic.src.LayerInfosForCopy(ctx) if err != nil { return err @@ -447,6 +446,17 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { srcInfosUpdated = true } + mblob, mtype, err := ic.src.Manifest(ctx) + if err != nil { + return err + } + man, err := manifest.FromBlob(mblob, mtype) + if err != nil { + return err + } + layerInfos := man.LayerInfos() + numLayers := len(layerInfos) + type copyLayerData struct { destInfo types.BlobInfo diffID digest.Digest @@ -476,7 +486,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { layerIndex := 0 // some layers might be skipped, so we need a dedicated counter digestToCopyData := make(map[digest.Digest]*copyLayerData) - for _, srcLayer := range srcInfos { + for _, srcLayer := range layerInfos { // Check if we're already copying the layer if _, ok := digestToCopyData[srcLayer.Digest]; ok { continue @@ -492,7 +502,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { return errors.New("getting DiffID for foreign layers is unimplemented") } logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) - cld.destInfo = srcLayer + cld.destInfo = srcLayer.BlobInfo continue // skip copying } @@ -501,17 +511,22 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { // Copy the layer. Note that cld is a pointer; changes to it are // propagated implicitly. - go func(index int, srcLayer types.BlobInfo, cld *copyLayerData) { + go func(index int, srcLayer manifest.LayerInfo, cld *copyLayerData) { defer copySemaphore.Release(1) defer copyGroup.Done() - cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer, index, progressPool) + if srcLayer.EmptyLayer { + index = -1 + } + cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer.BlobInfo, index, progressPool) if cld.err != nil { // Stop all other running goroutines as we can't recover from an error cancelCopyLayer() } }(layerIndex, srcLayer, cld) - layerIndex++ + if !srcLayer.EmptyLayer { + layerIndex++ + } } // Wait for all layer-copy operations to finish @@ -519,7 +534,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { destInfos := make([]types.BlobInfo, numLayers) diffIDs := make([]digest.Digest, numLayers) - for i, srcLayer := range srcInfos { + for i, srcLayer := range layerInfos { cld, ok := digestToCopyData[srcLayer.Digest] if !ok { return errors.Errorf("no copy data for layer %q", srcLayer.Digest) From 7e5a85b66b1f4b14509d2a6db12c0d5955d0c457 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 30 Apr 2019 18:00:54 +0200 Subject: [PATCH 29/39] copy: use "Internal error: " prefix in copyLayers Signed-off-by: Valentin Rothberg --- copy/copy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index b37f0a62c1..cb800871b8 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -537,7 +537,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { for i, srcLayer := range layerInfos { cld, ok := digestToCopyData[srcLayer.Digest] if !ok { - return errors.Errorf("no copy data for layer %q", srcLayer.Digest) + return errors.Errorf("Internal error: no copy data for layer %q", srcLayer.Digest) } if cld.err != nil { return cld.err From 116a38a90a9df1469449b7ecf4624330f5c7c3ce Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 7 May 2019 14:01:40 +0200 Subject: [PATCH 30/39] copy: don't skip already copied layer Do not skip an already copied layer and let the image destinations take care of that. Otherwise, we're breaking the layer-index semantics of {TryReusing,Put}Blob. Signed-off-by: Valentin Rothberg --- copy/copy.go | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index cb800871b8..18e3623afb 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -484,16 +484,9 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { cancelCtx, cancelCopyLayer := context.WithCancel(ctx) defer cancelCopyLayer() + copyData := make([]copyLayerData, numLayers) layerIndex := 0 // some layers might be skipped, so we need a dedicated counter - digestToCopyData := make(map[digest.Digest]*copyLayerData) - for _, srcLayer := range layerInfos { - // Check if we're already copying the layer - if _, ok := digestToCopyData[srcLayer.Digest]; ok { - continue - } - - cld := ©LayerData{} - digestToCopyData[srcLayer.Digest] = cld + for i, srcLayer := range layerInfos { if ic.c.dest.AcceptsForeignLayerURLs() && len(srcLayer.URLs) != 0 { // DiffIDs are, currently, needed only when converting from schema1. // In which case src.LayerInfos will not have URLs because schema1 @@ -502,27 +495,28 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { return errors.New("getting DiffID for foreign layers is unimplemented") } logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) - cld.destInfo = srcLayer.BlobInfo + copyData[i].destInfo = srcLayer.BlobInfo continue // skip copying } copySemaphore.Acquire(cancelCtx, 1) // limits parallel copy operations copyGroup.Add(1) // allows the main routine to wait for all copy operations to finish - // Copy the layer. Note that cld is a pointer; changes to it are - // propagated implicitly. - go func(index int, srcLayer manifest.LayerInfo, cld *copyLayerData) { + // Copy the layer. + go func(index, layerIndex int, srcLayer manifest.LayerInfo) { defer copySemaphore.Release(1) defer copyGroup.Done() if srcLayer.EmptyLayer { - index = -1 + layerIndex = -1 } - cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer.BlobInfo, index, progressPool) + cld := copyLayerData{} + cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer.BlobInfo, layerIndex, progressPool) if cld.err != nil { // Stop all other running goroutines as we can't recover from an error cancelCopyLayer() } - }(layerIndex, srcLayer, cld) + copyData[index] = cld + }(i, layerIndex, srcLayer) if !srcLayer.EmptyLayer { layerIndex++ @@ -534,11 +528,8 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { destInfos := make([]types.BlobInfo, numLayers) diffIDs := make([]digest.Digest, numLayers) - for i, srcLayer := range layerInfos { - cld, ok := digestToCopyData[srcLayer.Digest] - if !ok { - return errors.Errorf("Internal error: no copy data for layer %q", srcLayer.Digest) - } + for i := range copyData { + cld := copyData[i] if cld.err != nil { return cld.err } From fbd2a0c097b6d7734ef373145bbdf1c8e72e45e5 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 7 May 2019 14:10:59 +0200 Subject: [PATCH 31/39] copy: return non-context.Canceled error The copying of layers happens in parallel but will be canceled via the context if we hit an error. Hence, skip all context.Canceled errors to return the real error cause. Signed-off-by: Valentin Rothberg --- copy/copy.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/copy/copy.go b/copy/copy.go index 18e3623afb..530699557a 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -531,6 +531,10 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { for i := range copyData { cld := copyData[i] if cld.err != nil { + // Skip context.Canceled errors to determine the real error. + if cld.err == context.Canceled { + continue + } return cld.err } destInfos[i] = cld.destInfo From 90d77b23a0ffceb1fba67bb015e63200f449ad6b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Tue, 7 May 2019 16:04:13 +0200 Subject: [PATCH 32/39] storage: PutBlob: fix commit condition Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 02e49f3210..914f95b2ac 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -466,10 +466,6 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, } waitAndCommit := func(blob types.BlobInfo, err error) (types.BlobInfo, error) { - if layerIndexInImage >= 0 { - return blob, err - } - // First, wait for the previous layer to be committed previousID := "" if layerIndexInImage > 0 { @@ -489,7 +485,7 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, // Commit the blob id, err := s.commitBlob(ctx, blob, previousID) - if err == nil { + if err == nil && layerIndexInImage >= 0 { s.putBlobMutex.Lock() s.indexToStorageID[layerIndexInImage] = id s.putBlobMutex.Unlock() From e8162438388249da605e7e5145e41ec409e2d331 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 8 May 2019 15:10:23 +0200 Subject: [PATCH 33/39] storage: refactor commit logic Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 356 +++++++++++++++++++++------------------ 1 file changed, 195 insertions(+), 161 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 914f95b2ac..058b75a6d5 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -436,6 +436,31 @@ func (s *storageImageDestination) tryReusingBlobFromOtherProcess(ctx context.Con return false, types.BlobInfo{}, nil } +// getPreviousLayerID returns the layer ID of the previous layer of +// layerIndexInImage. It might wait until the previous layer ID has been +// computed by another goroutine. +// Note that it returns "" if layerIndexInImage <= 0. +func (s *storageImageDestination) getPreviousLayerID(layerIndexInImage int) (string, error) { + if layerIndexInImage <= 0 { + return "", nil + } + + channel := s.getChannelForLayer(layerIndexInImage - 1) + if committed := <-channel; !committed { + err := fmt.Errorf("committing parent layer %d failed", layerIndexInImage-1) + return "", err + } + + s.putBlobMutex.Lock() + previousID, ok := s.indexToStorageID[layerIndexInImage-1] + s.putBlobMutex.Unlock() + if !ok { + return "", fmt.Errorf("could not find parent layer ID for layer %d", layerIndexInImage) + } + + return previousID, nil +} + // PutBlob writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it. // inputInfo.Size is the expected length of stream, if known. @@ -465,36 +490,6 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, }() } - waitAndCommit := func(blob types.BlobInfo, err error) (types.BlobInfo, error) { - // First, wait for the previous layer to be committed - previousID := "" - if layerIndexInImage > 0 { - channel := s.getChannelForLayer(layerIndexInImage - 1) - if committed := <-channel; !committed { - err := fmt.Errorf("committing previous layer %d failed", layerIndexInImage-1) - return types.BlobInfo{}, err - } - var ok bool - s.putBlobMutex.Lock() - previousID, ok = s.indexToStorageID[layerIndexInImage-1] - s.putBlobMutex.Unlock() - if !ok { - return types.BlobInfo{}, fmt.Errorf("error committing blob %q: could not find parent layer ID", blob.Digest.String()) - } - } - - // Commit the blob - id, err := s.commitBlob(ctx, blob, previousID) - if err == nil && layerIndexInImage >= 0 { - s.putBlobMutex.Lock() - s.indexToStorageID[layerIndexInImage] = id - s.putBlobMutex.Unlock() - } else { - return types.BlobInfo{}, err - } - return blob, nil - } - // Check if another process is already copying the blob. Please refer to the // doc of tryReusingBlobFromOtherProcess for further information. if layerIndexInImage >= 0 { @@ -515,7 +510,7 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, return blob, err } if reusable { - return waitAndCommit(blob, err) + return blob, nil } } @@ -545,7 +540,27 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, return types.BlobInfo{}, err } - return waitAndCommit(blob, err) + // First, wait for the previous layer to be committed + previousID := "" + if layerIndexInImage > 0 { + var err error + previousID, err = s.getPreviousLayerID(layerIndexInImage) + if err != nil { + return types.BlobInfo{}, err + } + } + + // Commit the blob + id, err := s.commitBlob(ctx, blob, previousID) + if err == nil && layerIndexInImage >= 0 { + s.putBlobMutex.Lock() + s.indexToStorageID[layerIndexInImage] = id + s.putBlobMutex.Unlock() + } else { + return types.BlobInfo{}, err + } + + return blob, nil } func (s *storageImageDestination) putBlob(ctx context.Context, stream io.Reader, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, isConfig bool) (types.BlobInfo, error) { @@ -614,11 +629,126 @@ func (s *storageImageDestination) putBlob(ctx context.Context, stream io.Reader, return blob, nil } +// tryCopyingLayer looks up a matching layer for the blob in the storage and +// creates a new layer for it. It returns false if no matching layer has been +// found in the storage. Callers are expected to decide if that's an error or +// not. Note that layerID and previousID must either be left empty or both be +// specified. +func (s *storageImageDestination) tryCopyingLayer(ctx context.Context, blob types.BlobInfo, layerIndexInImage int, layerID, previousID string, byCompressed, byUncompressed bool) (string, types.BlobInfo, error) { + if !byCompressed && !byUncompressed { + return "", types.BlobInfo{}, errors.New("Internal error: copyLayer without compressed and uncompressed argument") + } + + // Check if we previously cached a file with that blob's contents. If we + // didn't, then we need to read the desired contents from a layer. + // + // TODO: the lookup here is only required when being called for an entirely + // new layer or reused ones. Maybe we should split that into separate + // functions?bool + s.putBlobMutex.Lock() + filename, ok := s.filenames[blob.Digest] + s.putBlobMutex.Unlock() + + id := "" + if !ok { + // Try to find the layer with contents matching that blobsum. + layer := "" + if byUncompressed { + layers, err := s.imageRef.transport.store.LayersByUncompressedDigest(blob.Digest) + if err == nil && len(layers) > 0 { + layer = layers[0].ID + id = layers[0].UncompressedDigest.Hex() + } + } + if byCompressed && layer == "" { + layers, err := s.imageRef.transport.store.LayersByCompressedDigest(blob.Digest) + if err == nil && len(layers) > 0 { + layer = layers[0].ID + id = layers[0].UncompressedDigest.Hex() + } + } + // No layer with a matching digest found, so there's nothing to copy. + if layer == "" { + logrus.Debugf("no matching layer found for blob %q", blob.Digest.String()) + return "", types.BlobInfo{}, nil + } + + // Read the layer's contents. + noCompression := archive.Uncompressed + diffOptions := &storage.DiffOptions{ + Compression: &noCompression, + } + diff, err := s.imageRef.transport.store.Diff("", layer, diffOptions) + if err != nil { + return "", types.BlobInfo{}, errors.Wrapf(err, "error reading layer %q for blob %q", layer, blob.Digest) + } + // Copy the layer diff to a file. Diff() takes a lock that it holds + // until the ReadCloser that it returns is closed, and PutLayer() wants + // the same lock, so the diff can't just be directly streamed from one + // to the other. + filename = s.computeNextBlobCacheFile() + file, err := os.OpenFile(filename, os.O_CREATE|os.O_TRUNC|os.O_WRONLY|os.O_EXCL, 0600) + if err != nil { + diff.Close() + return "", types.BlobInfo{}, errors.Wrapf(err, "error creating temporary file %q", filename) + } + // Copy the data to the file. + // TODO: This can take quite some time, and should ideally be cancellable using + // ctx.Done(). + _, err = io.Copy(file, diff) + diff.Close() + file.Close() + if err != nil { + return "", types.BlobInfo{}, errors.Wrapf(err, "error storing blob to file %q", filename) + } + } + + // Read the cached blob and use it as a diff. + file, err := os.Open(filename) + if err != nil { + return "", types.BlobInfo{}, errors.Wrapf(err, "error opening file %q", filename) + } + defer file.Close() + + // If layerID is provided, use it. + if layerID != "" { + id = layerID + } + // If no previousID is provided, wait for it to be computed. + if previousID == "" { + var err error + // Wait for the previous layer to be computed. + previousID, err = s.getPreviousLayerID(layerIndexInImage) + if err != nil { + return "", types.BlobInfo{}, err + } + if previousID != "" { + id = digest.Canonical.FromBytes([]byte(previousID + "+" + id)).Hex() + } + } + + // Build the new layer using the diff, regardless of where it came from. + // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). + layer, _, err := s.imageRef.transport.store.PutLayer(id, previousID, nil, "", false, nil, file) + if err != nil && errors.Cause(err) != storage.ErrDuplicateID { + return "", types.BlobInfo{}, errors.Wrapf(err, "error adding layer with blob %q", blob.Digest) + } + + // Record all data at once to avoid taking the lock too many times. + s.putBlobMutex.Lock() + s.indexToStorageID[layerIndexInImage] = layer.ID + s.blobDiffIDs[blob.Digest] = layer.UncompressedDigest + s.filenames[blob.Digest] = filename + s.putBlobMutex.Unlock() + + blob.Size = layer.UncompressedSize + + return layer.ID, blob, nil +} + // tryReusingBlob is a helper method for TryReusingBlob to wrap it func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo types.BlobInfo, layerIndexInImage int, cache types.BlobInfoCache, canSubstitute bool) (bool, types.BlobInfo, error) { // lock the entire method as it executes fairly quickly - s.putBlobMutex.Lock() - defer s.putBlobMutex.Unlock() if blobinfo.Digest == "" { return false, types.BlobInfo{}, errors.Errorf(`Can not check for a blob with unknown digest`) } @@ -627,69 +757,34 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t } // Check if we've already cached it in a file. - if size, ok := s.fileSizes[blobinfo.Digest]; ok { - return true, types.BlobInfo{ - Digest: blobinfo.Digest, - Size: size, - MediaType: blobinfo.MediaType, - }, nil - } - - // Check if we have a wasn't-compressed layer in storage that's based on that blob. - layers, err := s.imageRef.transport.store.LayersByUncompressedDigest(blobinfo.Digest) - if err != nil && errors.Cause(err) != storage.ErrLayerUnknown { - return false, types.BlobInfo{}, errors.Wrapf(err, `Error looking for layers with digest %q`, blobinfo.Digest) - } - if len(layers) > 0 { - // Save this for completeness. - s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest - if layerIndexInImage >= 0 { - s.indexToStorageID[layerIndexInImage] = layers[0].ID - } - return true, types.BlobInfo{ - Digest: blobinfo.Digest, - Size: layers[0].UncompressedSize, - MediaType: blobinfo.MediaType, - }, nil + s.putBlobMutex.Lock() + size, ok := s.fileSizes[blobinfo.Digest] + s.putBlobMutex.Unlock() + if ok { + blobinfo.Size = size + return true, blobinfo, nil } - // Check if we have a was-compressed layer in storage that's based on that blob. - layers, err = s.imageRef.transport.store.LayersByCompressedDigest(blobinfo.Digest) - if err != nil && errors.Cause(err) != storage.ErrLayerUnknown { - return false, types.BlobInfo{}, errors.Wrapf(err, `Error looking for compressed layers with digest %q`, blobinfo.Digest) - } - if len(layers) > 0 { - // Record the uncompressed value so that we can use it to calculate layer IDs. - s.blobDiffIDs[blobinfo.Digest] = layers[0].UncompressedDigest - if layerIndexInImage >= 0 { - s.indexToStorageID[layerIndexInImage] = layers[0].ID - } - return true, types.BlobInfo{ - Digest: blobinfo.Digest, - Size: layers[0].CompressedSize, - MediaType: blobinfo.MediaType, - }, nil + // Check if we can copy an already existing {un}compressed layer. + if layerID, blob, err := s.tryCopyingLayer(ctx, blobinfo, layerIndexInImage, "", "", true, true); err != nil { + return false, blob, errors.Wrapf(err, `Error looking for layers with digest %q`, blobinfo.Digest) + } else if layerID != "" { + return true, blob, nil } - // Does the blob correspond to a known DiffID which we already have available? - // Because we must return the size, which is unknown for unavailable compressed blobs, the returned BlobInfo refers to the + // Does the blob correspond to a known DiffID which we already have + // available? Because we must return the size, which is unknown for + // unavailable compressed blobs, the returned BlobInfo refers to the // uncompressed layer, and that can happen only if canSubstitute. if canSubstitute { - if uncompressedDigest := cache.UncompressedDigest(blobinfo.Digest); uncompressedDigest != "" && uncompressedDigest != blobinfo.Digest { - layers, err := s.imageRef.transport.store.LayersByUncompressedDigest(uncompressedDigest) - if err != nil && errors.Cause(err) != storage.ErrLayerUnknown { - return false, types.BlobInfo{}, errors.Wrapf(err, `Error looking for layers with digest %q`, uncompressedDigest) - } - if len(layers) > 0 { - s.blobDiffIDs[uncompressedDigest] = layers[0].UncompressedDigest - if layerIndexInImage >= 0 { - s.indexToStorageID[layerIndexInImage] = layers[0].ID - } - return true, types.BlobInfo{ - Digest: uncompressedDigest, - Size: layers[0].UncompressedSize, - MediaType: blobinfo.MediaType, - }, nil + uncompressedDigest := cache.UncompressedDigest(blobinfo.Digest) + if uncompressedDigest != "" && uncompressedDigest != blobinfo.Digest { + // Check if we can copy an existing uncompressed layer. + blobinfo.Digest = uncompressedDigest + if layerID, blob, err := s.tryCopyingLayer(ctx, blobinfo, layerIndexInImage, "", "", false, true); err != nil { + return false, blob, errors.Wrapf(err, `Error looking for layers with digest %q`, blobinfo.Digest) + } else if layerID != "" { + return true, blob, nil } } } @@ -808,80 +903,19 @@ func (s *storageImageDestination) commitBlob(ctx context.Context, blob types.Blo id = digest.Canonical.FromBytes([]byte(previousID + "+" + diffID.Hex())).Hex() } if layer, err2 := s.imageRef.transport.store.Layer(id); layer != nil && err2 == nil { - logrus.Debugf("layer for blob %q already found in storage", blob.Digest) + logrus.Debugf("layer for blob %q already found in storage", blob.Digest.String()) return layer.ID, nil } - // Check if we previously cached a file with that blob's contents. If we - // didn't, then we need to read the desired contents from a layer. - // - // TODO: the lookup here is only required when being called for an entirely - // new layer or reused ones. Maybe we should split that into separate - // functions? - s.putBlobMutex.Lock() - filename, ok := s.filenames[blob.Digest] - s.putBlobMutex.Unlock() - if !ok { - // Try to find the layer with contents matching that blobsum. - layer := "" - layers, err2 := s.imageRef.transport.store.LayersByUncompressedDigest(blob.Digest) - if err2 == nil && len(layers) > 0 { - layer = layers[0].ID - } else { - layers, err2 = s.imageRef.transport.store.LayersByCompressedDigest(blob.Digest) - if err2 == nil && len(layers) > 0 { - layer = layers[0].ID - } - } - if layer == "" { - return "", errors.Wrapf(err2, "error locating layer for blob %q", blob.Digest) - } - // Read the layer's contents. - noCompression := archive.Uncompressed - diffOptions := &storage.DiffOptions{ - Compression: &noCompression, - } - diff, err2 := s.imageRef.transport.store.Diff("", layer, diffOptions) - if err2 != nil { - return "", errors.Wrapf(err2, "error reading layer %q for blob %q", layer, blob.Digest) - } - // Copy the layer diff to a file. Diff() takes a lock that it holds - // until the ReadCloser that it returns is closed, and PutLayer() wants - // the same lock, so the diff can't just be directly streamed from one - // to the other. - filename = s.computeNextBlobCacheFile() - file, err := os.OpenFile(filename, os.O_CREATE|os.O_TRUNC|os.O_WRONLY|os.O_EXCL, 0600) - if err != nil { - diff.Close() - return "", errors.Wrapf(err, "error creating temporary file %q", filename) - } - // Copy the data to the file. - // TODO: This can take quite some time, and should ideally be cancellable using - // ctx.Done(). - _, err = io.Copy(file, diff) - diff.Close() - file.Close() - if err != nil { - return "", errors.Wrapf(err, "error storing blob to file %q", filename) - } - // Make sure that we can find this file later, should we need the layer's - // contents again. - s.putBlobMutex.Lock() - s.filenames[blob.Digest] = filename - s.putBlobMutex.Unlock() - } - // Read the cached blob and use it as a diff. - file, err := os.Open(filename) + + layerID, _, err := s.tryCopyingLayer(ctx, blob, -1, id, previousID, true, true) if err != nil { - return "", errors.Wrapf(err, "error opening file %q", filename) + return "", err } - defer file.Close() - // Build the new layer using the diff, regardless of where it came from. - // TODO: This can take quite some time, and should ideally be cancellable using ctx.Done(). - layer, _, err := s.imageRef.transport.store.PutLayer(id, previousID, nil, "", false, nil, file) - if err != nil && errors.Cause(err) != storage.ErrDuplicateID { - return "", errors.Wrapf(err, "error adding layer with blob %q", blob.Digest) + if layerID == "" { + return "", fmt.Errorf("Internal error: could not locate layer for blob %q", blob.Digest.String()) } - return layer.ID, nil + + return layerID, nil } // Commit marks the process of storing the image as successful and asks for the image to be persisted. @@ -910,7 +944,7 @@ func (s *storageImageDestination) Commit(ctx context.Context) error { // Check if it's elsewhere and the caller just forgot to pass it to us in a PutBlob(), // or to even check if we had it. // Use none.NoCache to avoid a repeated DiffID lookup in the BlobInfoCache; a caller - // that relies on using a blob digest that has never been seeen by the store had better call + // that relies on using a blob digest that has never been seen by the store had better call // TryReusingBlob; not calling PutBlob already violates the documented API, so there’s only // so far we are going to accommodate that (if we should be doing that at all). logrus.Debugf("looking for diffID for blob %+v", blob.Digest) From a0f636540b94430469f2c3acc36d510bb4388f33 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 13 May 2019 15:38:00 +0200 Subject: [PATCH 34/39] storage: PutBlob: return correct BlobInfo Fix a bug where a wrong BlobInfo was returned for layers with an index less than zero. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 058b75a6d5..9362fbb6de 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -552,12 +552,13 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, // Commit the blob id, err := s.commitBlob(ctx, blob, previousID) - if err == nil && layerIndexInImage >= 0 { + if err != nil { + return types.BlobInfo{}, err + } + if layerIndexInImage >= 0 { s.putBlobMutex.Lock() s.indexToStorageID[layerIndexInImage] = id s.putBlobMutex.Unlock() - } else { - return types.BlobInfo{}, err } return blob, nil From 9cc1fd7bfc738512375cde9598d50eba6cd2a8e5 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 15 May 2019 11:32:47 +0200 Subject: [PATCH 35/39] storage: PutBlob: do not commit config Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/storage/storage_image.go b/storage/storage_image.go index 9362fbb6de..1cd896c3f5 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -540,6 +540,10 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, return types.BlobInfo{}, err } + if isConfig { + return blob, err + } + // First, wait for the previous layer to be committed previousID := "" if layerIndexInImage > 0 { From 88210a17d42c39bb56d53799e8438b377d3811f4 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 15 May 2019 13:50:28 +0200 Subject: [PATCH 36/39] copy: don't use the manifest's LayerInfos for copying They can vary from the actual source BlobInfos. The manifest has previously been used to check if a given layer is empty. Do this now via the corresponding index. Signed-off-by: Valentin Rothberg --- copy/copy.go | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 530699557a..941a3837c9 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -433,6 +433,7 @@ func isTTY(w io.Writer) bool { // copyLayers copies layers from ic.src/ic.c.rawSource to dest, using and updating ic.manifestUpdates if necessary and ic.canModifyManifest. func (ic *imageCopier) copyLayers(ctx context.Context) error { srcInfos := ic.src.LayerInfos() + numLayers := len(srcInfos) updatedSrcInfos, err := ic.src.LayerInfosForCopy(ctx) if err != nil { return err @@ -446,6 +447,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { srcInfosUpdated = true } + // Create a map from the manifest to check if a srcLayer may be empty. mblob, mtype, err := ic.src.Manifest(ctx) if err != nil { return err @@ -454,8 +456,10 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { if err != nil { return err } - layerInfos := man.LayerInfos() - numLayers := len(layerInfos) + emptyLayerMap := make(map[digest.Digest]bool) + for _, info := range man.LayerInfos() { + emptyLayerMap[info.Digest] = info.EmptyLayer + } type copyLayerData struct { destInfo types.BlobInfo @@ -486,7 +490,7 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { copyData := make([]copyLayerData, numLayers) layerIndex := 0 // some layers might be skipped, so we need a dedicated counter - for i, srcLayer := range layerInfos { + for i, srcLayer := range srcInfos { if ic.c.dest.AcceptsForeignLayerURLs() && len(srcLayer.URLs) != 0 { // DiffIDs are, currently, needed only when converting from schema1. // In which case src.LayerInfos will not have URLs because schema1 @@ -495,30 +499,33 @@ func (ic *imageCopier) copyLayers(ctx context.Context) error { return errors.New("getting DiffID for foreign layers is unimplemented") } logrus.Debugf("Skipping foreign layer %q copy to %s", srcLayer.Digest, ic.c.dest.Reference().Transport().Name()) - copyData[i].destInfo = srcLayer.BlobInfo + copyData[i].destInfo = srcLayer continue // skip copying } copySemaphore.Acquire(cancelCtx, 1) // limits parallel copy operations copyGroup.Add(1) // allows the main routine to wait for all copy operations to finish + index := layerIndex + emptyLayer := false + if ok, empty := emptyLayerMap[srcLayer.Digest]; ok && empty { + emptyLayer = true + index = -1 + } // Copy the layer. - go func(index, layerIndex int, srcLayer manifest.LayerInfo) { + go func(dataIndex, layerIndex int, srcLayer types.BlobInfo) { defer copySemaphore.Release(1) defer copyGroup.Done() - if srcLayer.EmptyLayer { - layerIndex = -1 - } cld := copyLayerData{} - cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer.BlobInfo, layerIndex, progressPool) + cld.destInfo, cld.diffID, cld.err = ic.copyLayer(cancelCtx, srcLayer, layerIndex, progressPool) if cld.err != nil { // Stop all other running goroutines as we can't recover from an error cancelCopyLayer() } - copyData[index] = cld - }(i, layerIndex, srcLayer) + copyData[dataIndex] = cld + }(i, index, srcLayer) - if !srcLayer.EmptyLayer { + if !emptyLayer { layerIndex++ } } From 1fde49725c4c2360d9a124744fc8c56f257be25b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 16 May 2019 13:45:28 +0200 Subject: [PATCH 37/39] storage: digest lock: use RecursiveLock() Call RecursiveLock() instead of Lock() for locking the blob's digest lock. Otherwise, there is a potential to run into deadlocks when an image references a given blob multiple times and a goroutines with a higher layer index already acquired the lock but can't release it. Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 1cd896c3f5..34d6aca175 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -502,7 +502,7 @@ func (s *storageImageDestination) PutBlob(ctx context.Context, stream io.Reader, done := make(chan bool, 1) defer locker.Unlock() go func() { - locker.Lock() + locker.RecursiveLock() done <- true }() reusable, blob, err := s.tryReusingBlobFromOtherProcess(ctx, stream, blobinfo, layerIndexInImage, cache, done, bar) From 31890b5be23ba1538997c6353936463cfb89e83e Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 16 May 2019 15:39:59 +0200 Subject: [PATCH 38/39] storage: TryReusingBlob: create layer for reoccurring blob Remove a check in tryReusingBlob() to create a new layer even if have already seen the same blob. Fixes pulling docker/whalesay@sha256:178598e51a26abbc958b8a2e48825c90bc22e641 Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index 34d6aca175..c89745fbdd 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -761,15 +761,6 @@ func (s *storageImageDestination) tryReusingBlob(ctx context.Context, blobinfo t return false, types.BlobInfo{}, errors.Wrapf(err, `Can not check for a blob with invalid digest`) } - // Check if we've already cached it in a file. - s.putBlobMutex.Lock() - size, ok := s.fileSizes[blobinfo.Digest] - s.putBlobMutex.Unlock() - if ok { - blobinfo.Size = size - return true, blobinfo, nil - } - // Check if we can copy an already existing {un}compressed layer. if layerID, blob, err := s.tryCopyingLayer(ctx, blobinfo, layerIndexInImage, "", "", true, true); err != nil { return false, blob, errors.Wrapf(err, `Error looking for layers with digest %q`, blobinfo.Digest) From 1326dce39639394e393a96721bc840050081d373 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 17 May 2019 18:18:50 +0200 Subject: [PATCH 39/39] storage: PutBlob: don't change the blob's Signed-off-by: Valentin Rothberg --- storage/storage_image.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/storage/storage_image.go b/storage/storage_image.go index c89745fbdd..c09ae8fb42 100644 --- a/storage/storage_image.go +++ b/storage/storage_image.go @@ -746,8 +746,6 @@ func (s *storageImageDestination) tryCopyingLayer(ctx context.Context, blob type s.filenames[blob.Digest] = filename s.putBlobMutex.Unlock() - blob.Size = layer.UncompressedSize - return layer.ID, blob, nil }