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

audit Bar.SetTotal calls in copy.go #1464

Closed
mtrmac opened this issue Feb 12, 2022 · 4 comments · Fixed by #1530
Closed

audit Bar.SetTotal calls in copy.go #1464

mtrmac opened this issue Feb 12, 2022 · 4 comments · Fixed by #1530

Comments

@mtrmac
Copy link
Collaborator

mtrmac commented Feb 12, 2022

  • Some seem to be unnecessary — createProgressBar already uses blobInfo.Size
  • copyLayer, after a copy is done, calls SetTotal with the source info (which might not contain a size), when it certainly has a size from the updated result.

It’s probably just bugs, but review also the history in case some of those are non-obvious and necessary.

@vrothberg
Copy link
Member

IIRC, those were "needed" for bars to be filled. I'm sure some are not needed and maybe there's a better way.

@vrothberg
Copy link
Member

Ah, yes. There are two cases where I used the SetTotal "trick" for the bars to finish rendering and display the "done" state:

  1. In case the blob is already on present and copying is skipped
  2. For v2s1 images where the blob sizes are unknown; unconditionally calling SetTotal was attractive instead of carrying the information around whether we know the size or not.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 24, 2022

Notes to self, so that I don’t forget. Referring to the ≥ 7.4.1 MPB API:

  • If the source has a size, we need
    // from createProgressBar: bState.total is set to sourceSize, bState.triggerComplete is set
    bar.SetCurrent(sourceSize)     // Even if the blob already exists on the destination and we didn’t need to actually copy it, report 100% progress.
    Notably we don’t want to use destSize at all, because we can’t change bState.total any more, and we want the progress bar to go to 100%
  • If the source does not have a size, we need
    // from createProgressBar: bState.total is 0, bState.triggerComplete is unset
    bar.SetTotal(destSize, true) // Report the final size and 100% progress
  • The partial pull case is the same as the ordinary cases, except that we also set the “refill” value.
  • The TryReusingBlob success case is special (bState.total set to 0 does not immediately trigger a complete state)

@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 24, 2022

  • If the source does not have a size …

we should, if possible, still count in “source size” units, not the “destination size” units (to use a consistent measure in the hypothetical case of a single (multi-arch?) image that contains both size-known and size-unknown blobs).

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 a pull request may close this issue.

2 participants