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 <[email protected]>
containers · Apr 18, 2019 · 5ffc741 · 5ffc741
1 parent 1e10e9f
commit 5ffc741
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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