Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve progress bars, and update mpb #1530

Merged
merged 11 commits into from Apr 27, 2022
Merged

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Apr 26, 2022

Resolves #1464 (see there for rationale), and replaces #1473 . The general principle is that progress bars track object size at the source transport now, consistently everywhere.

Also various other related cleanups in this area.

See individual commit messages for details.

… tiny bit

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@rhatdan
Copy link
Member

@rhatdan rhatdan commented Apr 27, 2022

LGTM
There were a couple of typos in your commit messages, but nothing requiring a repush. Excellent job.
@vrothberg PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Changes LGTM.

Something I would love to see in the UX is to draw all progress bars before pulling. Currently, a bar is drawn the moment we start pulling the blob.

It would be nice to have them all drawn before. Unrelated to this PR though.

@mtrmac feel free to merge.

@mtrmac
Copy link
Contributor Author

@mtrmac mtrmac commented Apr 27, 2022

Something I would love to see in the UX is to draw all progress bars before pulling.

That’s a good idea — but not something I want to take on during this PR, which is primarily so that we can update the MPB dependency and don’t have to worry about a c/image user updating MPB itself, breaking the code.

So, I have filed #1532 for that. One issue resolved, one issue added, so it goes…

mtrmac added 10 commits Apr 27, 2022
…s.go

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č <mitr@redhat.com>
Second part: Move blobChunkAccessorProxy to the newly established
progress_bars.go.

Only moves unchanged code, should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... 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č <mitr@redhat.com>
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č <mitr@redhat.com>
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č <mitr@redhat.com>
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č <mitr@redhat.com>
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č <mitr@redhat.com>
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č <mitr@redhat.com>
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č <mitr@redhat.com>
... so that we test all of this with the new semantics.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac merged commit 50b0657 into containers:main Apr 27, 2022
9 checks passed
@mtrmac mtrmac deleted the progress-updates branch Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants