From 4ba557564d792660bf70d5bae7f5556f37a444c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:52:53 +0200 Subject: [PATCH 01/11] Improve the documentation of the objects in copy/progress_reader.go a tiny bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/progress_reader.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/copy/progress_reader.go b/copy/progress_reader.go index de23cec1b..514a888c8 100644 --- a/copy/progress_reader.go +++ b/copy/progress_reader.go @@ -10,7 +10,7 @@ import ( "github.com/vbauerster/mpb/v7" ) -// progressReader is a reader that reports its progress on an interval. +// progressReader is a reader that reports its progress to a types.ProgressProperties channel on an interval. type progressReader struct { source io.Reader channel chan<- types.ProgressProperties @@ -81,8 +81,8 @@ func (r *progressReader) Read(p []byte) (int, error) { return n, err } -// blobChunkAccessorProxy wraps a BlobChunkAccessor and keeps track of how many bytes -// are received. +// blobChunkAccessorProxy wraps a BlobChunkAccessor and updates a *mpb.Bar +// with the number of received bytes. type blobChunkAccessorProxy struct { wrapped private.BlobChunkAccessor // The underlying BlobChunkAccessor bar *mpb.Bar // A progress bar updated with the number of bytes read so far From 66100ac0e289826224369d085ad03f76f474946b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:56:34 +0200 Subject: [PATCH 02/11] Move progress-bar-specific utilities from copy.go to new progress_bars.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's try to make copy.go shorter, and consolidate progress bar code to new copy/progress_bars.go. First, move the code from copy.go. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 80 -------------------------------------- copy/progress_bars.go | 89 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 80 deletions(-) create mode 100644 copy/progress_bars.go diff --git a/copy/copy.go b/copy/copy.go index 644f82615..8e3fca5b7 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -32,7 +32,6 @@ import ( "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/vbauerster/mpb/v7" - "github.com/vbauerster/mpb/v7/decor" "golang.org/x/sync/semaphore" "golang.org/x/term" ) @@ -1069,85 +1068,6 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc return man, manifestDigest, nil } -// newProgressPool creates a *mpb.Progress. -// The caller must eventually call pool.Wait() after the pool will no longer be updated. -// NOTE: Every progress bar created within the progress pool must either successfully -// complete or be aborted, or pool.Wait() will hang. That is typically done -// using "defer bar.Abort(false)", which must be called BEFORE pool.Wait() is called. -func (c *copier) newProgressPool() *mpb.Progress { - return mpb.New(mpb.WithWidth(40), mpb.WithOutput(c.progressOutput)) -} - -// customPartialBlobDecorFunc implements mpb.DecorFunc for the partial blobs retrieval progress bar -func customPartialBlobDecorFunc(s decor.Statistics) string { - if s.Total == 0 { - pairFmt := "%.1f / %.1f (skipped: %.1f)" - return fmt.Sprintf(pairFmt, decor.SizeB1024(s.Current), decor.SizeB1024(s.Total), decor.SizeB1024(s.Refill)) - } - pairFmt := "%.1f / %.1f (skipped: %.1f = %.2f%%)" - percentage := 100.0 * float64(s.Refill) / float64(s.Total) - return fmt.Sprintf(pairFmt, decor.SizeB1024(s.Current), decor.SizeB1024(s.Total), decor.SizeB1024(s.Refill), percentage) -} - -// createProgressBar creates a mpb.Bar in pool. Note that if the copier's reportWriter -// is io.Discard, the progress bar's output will be discarded -// NOTE: Every progress bar created within a progress pool must either successfully -// complete or be aborted, or pool.Wait() will hang. That is typically done -// using "defer bar.Abort(false)", which must happen BEFORE pool.Wait() is called. -func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *mpb.Bar { - // 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] - } - - // onComplete will replace prefix once the bar/spinner has completed - onComplete = prefix + " " + onComplete - - // Use a normal progress bar when we know the size (i.e., size > 0). - // Otherwise, use a spinner to indicate that something's happening. - var bar *mpb.Bar - if info.Size > 0 { - if partial { - bar = pool.AddBar(info.Size, - mpb.BarFillerClearOnComplete(), - mpb.PrependDecorators( - decor.OnComplete(decor.Name(prefix), onComplete), - ), - mpb.AppendDecorators( - decor.Any(customPartialBlobDecorFunc), - ), - ) - } else { - bar = pool.AddBar(info.Size, - mpb.BarFillerClearOnComplete(), - mpb.PrependDecorators( - decor.OnComplete(decor.Name(prefix), onComplete), - ), - mpb.AppendDecorators( - decor.OnComplete(decor.CountersKibiByte("%.1f / %.1f"), ""), - ), - ) - } - } else { - bar = pool.New(0, - mpb.SpinnerStyle(".", "..", "...", "....", "").PositionLeft(), - mpb.BarFillerClearOnComplete(), - mpb.PrependDecorators( - decor.OnComplete(decor.Name(prefix), onComplete), - ), - ) - } - if c.progressOutput == io.Discard { - c.Printf("Copying %s %s\n", kind, info.Digest) - } - return bar -} - // copyConfig copies config.json, if any, from src to dest. func (c *copier) copyConfig(ctx context.Context, src types.Image) error { srcInfo := src.ConfigInfo() diff --git a/copy/progress_bars.go b/copy/progress_bars.go new file mode 100644 index 000000000..4e84f73a4 --- /dev/null +++ b/copy/progress_bars.go @@ -0,0 +1,89 @@ +package copy + +import ( + "fmt" + "io" + + "github.com/containers/image/v5/types" + "github.com/vbauerster/mpb/v7" + "github.com/vbauerster/mpb/v7/decor" +) + +// newProgressPool creates a *mpb.Progress. +// The caller must eventually call pool.Wait() after the pool will no longer be updated. +// NOTE: Every progress bar created within the progress pool must either successfully +// complete or be aborted, or pool.Wait() will hang. That is typically done +// using "defer bar.Abort(false)", which must be called BEFORE pool.Wait() is called. +func (c *copier) newProgressPool() *mpb.Progress { + return mpb.New(mpb.WithWidth(40), mpb.WithOutput(c.progressOutput)) +} + +// customPartialBlobDecorFunc implements mpb.DecorFunc for the partial blobs retrieval progress bar +func customPartialBlobDecorFunc(s decor.Statistics) string { + if s.Total == 0 { + pairFmt := "%.1f / %.1f (skipped: %.1f)" + return fmt.Sprintf(pairFmt, decor.SizeB1024(s.Current), decor.SizeB1024(s.Total), decor.SizeB1024(s.Refill)) + } + pairFmt := "%.1f / %.1f (skipped: %.1f = %.2f%%)" + percentage := 100.0 * float64(s.Refill) / float64(s.Total) + return fmt.Sprintf(pairFmt, decor.SizeB1024(s.Current), decor.SizeB1024(s.Total), decor.SizeB1024(s.Refill), percentage) +} + +// createProgressBar creates a mpb.Bar in pool. Note that if the copier's reportWriter +// is io.Discard, the progress bar's output will be discarded +// NOTE: Every progress bar created within a progress pool must either successfully +// complete or be aborted, or pool.Wait() will hang. That is typically done +// using "defer bar.Abort(false)", which must happen BEFORE pool.Wait() is called. +func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *mpb.Bar { + // 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] + } + + // onComplete will replace prefix once the bar/spinner has completed + onComplete = prefix + " " + onComplete + + // Use a normal progress bar when we know the size (i.e., size > 0). + // Otherwise, use a spinner to indicate that something's happening. + var bar *mpb.Bar + if info.Size > 0 { + if partial { + bar = pool.AddBar(info.Size, + mpb.BarFillerClearOnComplete(), + mpb.PrependDecorators( + decor.OnComplete(decor.Name(prefix), onComplete), + ), + mpb.AppendDecorators( + decor.Any(customPartialBlobDecorFunc), + ), + ) + } else { + bar = pool.AddBar(info.Size, + mpb.BarFillerClearOnComplete(), + mpb.PrependDecorators( + decor.OnComplete(decor.Name(prefix), onComplete), + ), + mpb.AppendDecorators( + decor.OnComplete(decor.CountersKibiByte("%.1f / %.1f"), ""), + ), + ) + } + } else { + bar = pool.New(0, + mpb.SpinnerStyle(".", "..", "...", "....", "").PositionLeft(), + mpb.BarFillerClearOnComplete(), + mpb.PrependDecorators( + decor.OnComplete(decor.Name(prefix), onComplete), + ), + ) + } + if c.progressOutput == io.Discard { + c.Printf("Copying %s %s\n", kind, info.Digest) + } + return bar +} From 9c46add0020e44435a19bfa0a5b9ddce5de2ba45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 23:08:58 +0200 Subject: [PATCH 03/11] Move blobChunkAccessorProxy to copy/progress_bars.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second part: Move blobChunkAccessorProxy to the newly established progress_bars.go. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/progress_bars.go | 26 ++++++++++++++++++++++++++ copy/progress_reader.go | 27 --------------------------- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/copy/progress_bars.go b/copy/progress_bars.go index 4e84f73a4..832be4e83 100644 --- a/copy/progress_bars.go +++ b/copy/progress_bars.go @@ -1,9 +1,11 @@ package copy import ( + "context" "fmt" "io" + "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/types" "github.com/vbauerster/mpb/v7" "github.com/vbauerster/mpb/v7/decor" @@ -87,3 +89,27 @@ func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types. } return bar } + +// blobChunkAccessorProxy wraps a BlobChunkAccessor and updates a *mpb.Bar +// with the number of received bytes. +type blobChunkAccessorProxy struct { + wrapped private.BlobChunkAccessor // The underlying BlobChunkAccessor + bar *mpb.Bar // A progress bar updated with the number of bytes read so far +} + +// GetBlobAt returns a sequential channel of readers that contain data for the requested +// blob chunks, and a channel that might get a single error value. +// The specified chunks must be not overlapping and sorted by their offset. +// The readers must be fully consumed, in the order they are returned, before blocking +// to read the next chunk. +func (s *blobChunkAccessorProxy) GetBlobAt(ctx context.Context, info types.BlobInfo, chunks []private.ImageSourceChunk) (chan io.ReadCloser, chan error, error) { + rc, errs, err := s.wrapped.GetBlobAt(ctx, info, chunks) + if err == nil { + total := int64(0) + for _, c := range chunks { + total += int64(c.Length) + } + s.bar.IncrInt64(total) + } + return rc, errs, err +} diff --git a/copy/progress_reader.go b/copy/progress_reader.go index 514a888c8..d5e9e09bd 100644 --- a/copy/progress_reader.go +++ b/copy/progress_reader.go @@ -1,13 +1,10 @@ package copy import ( - "context" "io" "time" - "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/types" - "github.com/vbauerster/mpb/v7" ) // progressReader is a reader that reports its progress to a types.ProgressProperties channel on an interval. @@ -80,27 +77,3 @@ func (r *progressReader) Read(p []byte) (int, error) { } return n, err } - -// blobChunkAccessorProxy wraps a BlobChunkAccessor and updates a *mpb.Bar -// with the number of received bytes. -type blobChunkAccessorProxy struct { - wrapped private.BlobChunkAccessor // The underlying BlobChunkAccessor - bar *mpb.Bar // A progress bar updated with the number of bytes read so far -} - -// GetBlobAt returns a sequential channel of readers that contain data for the requested -// blob chunks, and a channel that might get a single error value. -// The specified chunks must be not overlapping and sorted by their offset. -// The readers must be fully consumed, in the order they are returned, before blocking -// to read the next chunk. -func (s *blobChunkAccessorProxy) GetBlobAt(ctx context.Context, info types.BlobInfo, chunks []private.ImageSourceChunk) (chan io.ReadCloser, chan error, error) { - rc, errs, err := s.wrapped.GetBlobAt(ctx, info, chunks) - if err == nil { - total := int64(0) - for _, c := range chunks { - total += int64(c.Length) - } - s.bar.IncrInt64(total) - } - return rc, errs, err -} From c8c17e298f61fa265c12ba46517a542117b20f4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 23:10:04 +0200 Subject: [PATCH 04/11] Rename progress_reader* to progress_channel* MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to cleanly differentiate the code related to chan<- types.ProgressProperties from the code related to progress bars. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/{progress_reader.go => progress_channel.go} | 0 copy/{progress_reader_test.go => progress_channel_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename copy/{progress_reader.go => progress_channel.go} (100%) rename copy/{progress_reader_test.go => progress_channel_test.go} (100%) diff --git a/copy/progress_reader.go b/copy/progress_channel.go similarity index 100% rename from copy/progress_reader.go rename to copy/progress_channel.go diff --git a/copy/progress_reader_test.go b/copy/progress_channel_test.go similarity index 100% rename from copy/progress_reader_test.go rename to copy/progress_channel_test.go From 5a56b1e41a05a306a3e5bfca9e56f589e3644d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:29:39 +0200 Subject: [PATCH 05/11] Remove a redundant bar.SetTotal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same srcInfo.size was passed to createProgressBar, so this does nothing at best if size is known; if it is unknown, it seems to effectively also be a no-op with a recent version of mpb (it sets the total value to 0, the current progress at the time, but it doesn't enable auto-complete, so it doesn't make a difference). Signed-off-by: Miloslav Trmač --- copy/copy.go | 1 - 1 file changed, 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index 8e3fca5b7..505acb230 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1207,7 +1207,6 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to wrapped: ic.c.rawSource, bar: bar, } - bar.SetTotal(srcInfo.Size, false) info, err := ic.c.dest.PutBlobPartial(ctx, &proxy, srcInfo, ic.c.blobInfoCache) if err == nil { bar.SetRefill(srcInfo.Size - bar.Current()) From 69dde2ee4a3930100a1d1d73eccd4f039088c965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:32:13 +0200 Subject: [PATCH 06/11] Add more operations within the scope of progress bars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Already draw the progress bar before opening the source's layer or config blob, so that the users can have a visual indication of something related to that blob happening if it takes too long. (For the .ConfigBlob() call, this might not make a difference, because in _some_ cases we trigger reading the config via .OCIConfig in checkImageDestinationForCurrentRuntime(), and the config blob is cached, but it might make a difference in the other cases, and it doesn't hurt in this one.) Signed-off-by: Miloslav Trmač --- copy/copy.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 505acb230..3d1d0beca 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1078,17 +1078,17 @@ func (c *copier) copyConfig(ctx context.Context, src types.Image) error { } defer c.concurrentBlobCopiesSemaphore.Release(1) - configBlob, err := src.ConfigBlob(ctx) - if err != nil { - return errors.Wrapf(err, "reading config blob %s", srcInfo.Digest) - } - destInfo, err := func() (types.BlobInfo, error) { // A scope for defer progressPool := c.newProgressPool() defer progressPool.Wait() bar := c.createProgressBar(progressPool, false, srcInfo, "config", "done") defer bar.Abort(false) + configBlob, err := src.ConfigBlob(ctx) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "reading config blob %s", srcInfo.Digest) + } + destInfo, err := c.copyBlobFromStream(ctx, bytes.NewReader(configBlob), srcInfo, nil, false, true, false, bar, -1, false) if err != nil { return types.BlobInfo{}, err @@ -1224,16 +1224,16 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to } // Fallback: copy the layer, computing the diffID if we need to do so - srcStream, srcBlobSize, err := ic.c.rawSource.GetBlob(ctx, srcInfo, ic.c.blobInfoCache) - if err != nil { - return types.BlobInfo{}, "", errors.Wrapf(err, "reading blob %s", srcInfo.Digest) - } - defer srcStream.Close() - return func() (types.BlobInfo, digest.Digest, error) { // A scope for defer bar := ic.c.createProgressBar(pool, false, srcInfo, "blob", "done") defer bar.Abort(false) + srcStream, srcBlobSize, err := ic.c.rawSource.GetBlob(ctx, srcInfo, ic.c.blobInfoCache) + if err != nil { + return types.BlobInfo{}, "", errors.Wrapf(err, "reading blob %s", srcInfo.Digest) + } + defer srcStream.Close() + blobInfo, diffIDChan, err := ic.copyLayerFromStream(ctx, srcStream, types.BlobInfo{Digest: srcInfo.Digest, Size: srcBlobSize, MediaType: srcInfo.MediaType, Annotations: srcInfo.Annotations}, diffIDIsNeeded, toEncrypt, bar, layerIndex, emptyLayer) if err != nil { return types.BlobInfo{}, "", err From 83bc18acfd1310ce91a7cef47ca39beabf9ff699 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:58:38 +0200 Subject: [PATCH 07/11] Track the original source blob's size in the progress bar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In particular, if the source is compressed, track the full size of the original compressed blob in the progress bar, even if the decompressed version has a different size (e.g. because the encryption adds a header or a MAC). This original size is easy to determine and eaiser to explain to users; so let's do the simple thing. Signed-off-by: Miloslav Trmač --- copy/copy.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 3d1d0beca..19189d5b2 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1363,6 +1363,9 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } var destStream io.Reader = digestingReader + // === Update progress bars + destStream = bar.ProxyReader(destStream) + // === Decrypt the stream, if required. var decrypted bool if isOciEncrypted(srcInfo.MediaType) && c.ociDecryptConfig != nil { @@ -1397,9 +1400,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) } - // === Update progress bars - 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 { From c444bc31496309fd97c67be64306150d1db465e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 23:29:55 +0200 Subject: [PATCH 08/11] Wrap a *mpb.Bar in a *progressBar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will allow us to add more state and methods to the bar object, without copy&pasting. For now, this just wraps one pointer in another, and should not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 ++-- copy/progress_bars.go | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 19189d5b2..2dc87e443 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1266,7 +1266,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to // perhaps (de/re/)compressing the stream, // 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, toEncrypt bool, bar *mpb.Bar, layerIndex int, emptyLayer bool) (types.BlobInfo, <-chan diffIDResult, error) { + diffIDIsNeeded bool, toEncrypt bool, bar *progressBar, layerIndex int, emptyLayer bool) (types.BlobInfo, <-chan diffIDResult, error) { var getDiffIDRecorder func(compressiontypes.DecompressorFunc) io.Writer // = nil var diffIDChan chan diffIDResult @@ -1343,7 +1343,7 @@ func (r errorAnnotationReader) Read(b []byte) (n int, err error) { // and returns a complete blobInfo of the copied blob. func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, getOriginalLayerCopyWriter func(decompressor compressiontypes.DecompressorFunc) io.Writer, - canModifyBlob bool, isConfig bool, toEncrypt bool, bar *mpb.Bar, layerIndex int, emptyLayer bool) (types.BlobInfo, error) { + canModifyBlob bool, isConfig bool, toEncrypt bool, bar *progressBar, layerIndex int, emptyLayer bool) (types.BlobInfo, error) { if isConfig { // This is guaranteed by the caller, but set it here to be explicit. canModifyBlob = false } diff --git a/copy/progress_bars.go b/copy/progress_bars.go index 832be4e83..300fed3d7 100644 --- a/copy/progress_bars.go +++ b/copy/progress_bars.go @@ -31,12 +31,17 @@ func customPartialBlobDecorFunc(s decor.Statistics) string { return fmt.Sprintf(pairFmt, decor.SizeB1024(s.Current), decor.SizeB1024(s.Total), decor.SizeB1024(s.Refill), percentage) } -// createProgressBar creates a mpb.Bar in pool. Note that if the copier's reportWriter +// progressBar wraps a *mpb.Bar, allowing us to add extra state and methods. +type progressBar struct { + *mpb.Bar +} + +// createProgressBar creates a progressBar in pool. Note that if the copier's reportWriter // is io.Discard, the progress bar's output will be discarded // NOTE: Every progress bar created within a progress pool must either successfully // complete or be aborted, or pool.Wait() will hang. That is typically done // using "defer bar.Abort(false)", which must happen BEFORE pool.Wait() is called. -func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *mpb.Bar { +func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *progressBar { // shortDigestLen is the length of the digest used for blobs. const shortDigestLen = 12 @@ -87,14 +92,16 @@ func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types. if c.progressOutput == io.Discard { c.Printf("Copying %s %s\n", kind, info.Digest) } - return bar + return &progressBar{ + Bar: bar, + } } -// blobChunkAccessorProxy wraps a BlobChunkAccessor and updates a *mpb.Bar +// blobChunkAccessorProxy wraps a BlobChunkAccessor and updates a *progressBar // with the number of received bytes. type blobChunkAccessorProxy struct { wrapped private.BlobChunkAccessor // The underlying BlobChunkAccessor - bar *mpb.Bar // A progress bar updated with the number of bytes read so far + bar *progressBar // A progress bar updated with the number of bytes read so far } // GetBlobAt returns a sequential channel of readers that contain data for the requested From f090300acabfc4dcdd3ea582fb557a84208ac7ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Apr 2022 22:59:18 +0200 Subject: [PATCH 09/11] Add progressBar.mark100PercentComplete, and use it on all progress bars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This correctly handles both the "size known" and the "size unknown" cases, and because we now record the originally-configured progress bar size in the progressBar struct, the caller doesn't need to track it. Signed-off-by: Miloslav Trmač --- copy/copy.go | 12 ++++++------ copy/progress_bars.go | 28 +++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 2dc87e443..c6f31ad8b 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1093,7 +1093,8 @@ func (c *copier) copyConfig(ctx context.Context, src types.Image) error { if err != nil { return types.BlobInfo{}, err } - bar.SetTotal(int64(len(configBlob)), true) + + bar.mark100PercentComplete() return destInfo, nil }() if err != nil { @@ -1162,9 +1163,9 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to if reused { logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest) func() { // A scope for defer - bar := ic.c.createProgressBar(pool, false, srcInfo, "blob", "skipped: already exists") + bar := ic.c.createProgressBar(pool, false, types.BlobInfo{Digest: blobInfo.Digest, Size: 0}, "blob", "skipped: already exists") defer bar.Abort(false) - bar.SetTotal(0, true) + bar.mark100PercentComplete() }() // Throw an event that the layer has been skipped @@ -1210,8 +1211,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to info, err := ic.c.dest.PutBlobPartial(ctx, &proxy, srcInfo, ic.c.blobInfoCache) if err == nil { bar.SetRefill(srcInfo.Size - bar.Current()) - bar.SetCurrent(srcInfo.Size) - bar.SetTotal(srcInfo.Size, true) + bar.mark100PercentComplete() hideProgressBar = false logrus.Debugf("Retrieved partial blob %v", srcInfo.Digest) return true, info @@ -1256,7 +1256,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to } } - bar.SetTotal(srcInfo.Size, true) + bar.mark100PercentComplete() return blobInfo, diffID, nil }() } diff --git a/copy/progress_bars.go b/copy/progress_bars.go index 300fed3d7..585d86057 100644 --- a/copy/progress_bars.go +++ b/copy/progress_bars.go @@ -34,13 +34,19 @@ func customPartialBlobDecorFunc(s decor.Statistics) string { // progressBar wraps a *mpb.Bar, allowing us to add extra state and methods. type progressBar struct { *mpb.Bar + originalSize int64 // or -1 if unknown } // createProgressBar creates a progressBar in pool. Note that if the copier's reportWriter // is io.Discard, the progress bar's output will be discarded +// // NOTE: Every progress bar created within a progress pool must either successfully // complete or be aborted, or pool.Wait() will hang. That is typically done // using "defer bar.Abort(false)", which must happen BEFORE pool.Wait() is called. +// +// As a convention, most users of progress bars should call mark100PercentComplete on full success; +// by convention, we don't leave progress bars in partial state when fully done +// (even if we copied much less data than anticipated). func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types.BlobInfo, kind string, onComplete string) *progressBar { // shortDigestLen is the length of the digest used for blobs. const shortDigestLen = 12 @@ -93,7 +99,27 @@ func (c *copier) createProgressBar(pool *mpb.Progress, partial bool, info types. c.Printf("Copying %s %s\n", kind, info.Digest) } return &progressBar{ - Bar: bar, + Bar: bar, + originalSize: info.Size, + } +} + +// mark100PercentComplete marks the progres bars as 100% complete; +// it may do so by possibly advancing the current state if it is below the known total. +func (bar *progressBar) mark100PercentComplete() { + if bar.originalSize > 0 { + // We can't call bar.SetTotal even if we wanted to; the total can not be changed + // after a progress bar is created with a definite total. + bar.SetCurrent(bar.originalSize) // This triggers the completion condition. + } else { + // -1 = unknown size + // 0 is somewhat of a a special case: Unlike c/image, where 0 is a definite known + // size (possible at least in theory), in mpb, zero-sized progress bars are treated + // as unknown size, in particular they are not configured to be marked as + // complete on bar.Current() reaching bar.total (because that would happen already + // when creating the progress bar). + // That means that we are both _allowed_ to call SetTotal, and we _have to_. + bar.SetTotal(-1, true) // total < 0 = set it to bar.Current(), report it; and mark the bar as complete. } } From 2837c2a3e3c79c7e3779267c77fa93a7876f6571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 27 Apr 2022 00:27:08 +0200 Subject: [PATCH 10/11] Don't call bar.SetRefill with negative numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If srcInfo.Size == -1, the progress bar is not showing the refill value anyway (customPartialBlobDecorFunc is only used if size is known), so don't bother calling this function with nonsense data, if anything, at least to emphasize that this condition is, in principle, possible. Should not affect behavior, hopefully. Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index c6f31ad8b..74da68eb3 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -1210,7 +1210,9 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to } info, err := ic.c.dest.PutBlobPartial(ctx, &proxy, srcInfo, ic.c.blobInfoCache) if err == nil { - bar.SetRefill(srcInfo.Size - bar.Current()) + if srcInfo.Size != -1 { + bar.SetRefill(srcInfo.Size - bar.Current()) + } bar.mark100PercentComplete() hideProgressBar = false logrus.Debugf("Retrieved partial blob %v", srcInfo.Digest) From 3a9550c3e8f911483ee2687a3854063b02888cb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 27 Apr 2022 00:29:31 +0200 Subject: [PATCH 11/11] Update to github.com/vbauerster/mpb/v7 v7.4.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we test all of this with the new semantics. Signed-off-by: Miloslav Trmač --- go.mod | 4 ++-- go.sum | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 2356864cb..97ef70f13 100644 --- a/go.mod +++ b/go.mod @@ -32,14 +32,14 @@ require ( github.com/sylabs/sif/v2 v2.7.0 github.com/ulikunitz/xz v0.5.10 github.com/vbatts/tar-split v0.11.2 - github.com/vbauerster/mpb/v7 v7.3.2 + github.com/vbauerster/mpb/v7 v7.4.1 github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect github.com/xeipuuv/gojsonschema v1.2.0 go.etcd.io/bbolt v1.3.6 golang.org/x/crypto v0.0.0-20211215153901-e495a2d5b3d3 golang.org/x/net v0.0.0-20220225172249-27dd8689420f golang.org/x/sync v0.0.0-20210220032951-036812b2e83c - golang.org/x/sys v0.0.0-20220227234510-4e6760a101f9 // indirect + golang.org/x/sys v0.0.0-20220422013727-9388b58f7150 // indirect golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 google.golang.org/genproto v0.0.0-20220304144024-325a89244dc8 // indirect ) diff --git a/go.sum b/go.sum index 8da0595b3..3bed5a092 100644 --- a/go.sum +++ b/go.sum @@ -884,8 +884,8 @@ github.com/urfave/cli v1.22.2/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtX github.com/urfave/cli v1.22.4/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0= github.com/vbatts/tar-split v0.11.2 h1:Via6XqJr0hceW4wff3QRzD5gAk/tatMw/4ZA7cTlIME= github.com/vbatts/tar-split v0.11.2/go.mod h1:vV3ZuO2yWSVsz+pfFzDG/upWH1JhjOiEaWq6kXyQ3VI= -github.com/vbauerster/mpb/v7 v7.3.2 h1:tCuxMy8G9cLdjb61b6wO7I1vRT/LyMEzRbr3xCC0JPU= -github.com/vbauerster/mpb/v7 v7.3.2/go.mod h1:wfxIZcOJq/bG1/lAtfzMXcOiSvbqVi/5GX5WCSi+IsA= +github.com/vbauerster/mpb/v7 v7.4.1 h1:NhLMWQ3gNg2KJR8oeA9lO8Xvq+eNPmixDmB6JEQOUdA= +github.com/vbauerster/mpb/v7 v7.4.1/go.mod h1:Ygg2mV9Vj9sQBWqsK2m2pidcf9H3s6bNKtqd3/M4gBo= github.com/vishvananda/netlink v0.0.0-20181108222139-023a6dafdcdf/go.mod h1:+SR5DhBJrl6ZM7CoCKvpw5BKroDKQ+PJqOg65H/2ktk= github.com/vishvananda/netlink v1.1.0/go.mod h1:cTgwzPIzzgDAYoQrMm0EdrjRUBkTqKYppBueQtXaqoE= github.com/vishvananda/netlink v1.1.1-0.20201029203352-d40f9887b852/go.mod h1:twkDnbuQxJYemMlGd4JFIcuhgX83tXhKS2B/PRMpOho= @@ -1202,9 +1202,9 @@ golang.org/x/sys v0.0.0-20210906170528-6f6e22806c34/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20211025201205-69cdffdb9359/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211116061358-0a5406a5449c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220114195835-da31bd327af9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.0.0-20220227234510-4e6760a101f9 h1:nhht2DYV/Sn3qOayu8lM+cU1ii9sTLUeBQwQQfUHtrs= -golang.org/x/sys v0.0.0-20220227234510-4e6760a101f9/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220422013727-9388b58f7150 h1:xHms4gcpe1YE7A3yIllJXP16CMAGuqwO2lX1mTyyRRc= +golang.org/x/sys v0.0.0-20220422013727-9388b58f7150/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201117132131-f5c789dd3221/go.mod h1:Nr5EML6q2oocZ2LXRh80K7BxOlk5/8JxuGnuhpl+muw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=