Add WithProgress option for remote.Write

[tydra-wang]

As the title.

[DennisDenuto]

+1

I would find this also useful for remote.MultiWrite

[DennisDenuto]

Would it be correct to assume that WithProgress would work very similar to how it works for tarball.WriteToFile?

go-containerregistry/pkg/v1/progress.go

Line 21 in 77421d5

type Update struct {

i.e. MultiWrite would send progress information to the v1.Update channel?

[imjasonh]

Yeah, I'd like to have them behave similarly. The difference with MultiWrite is that there are multiple things having progress made, does it make sense for the update to include information about which things are progressing how?

MultiWrite already doesn't support streaming layers, so we can know ahead of time how many total bytes we anticipate writing. In theory we'd like to support streaming layers though, in which case the Total might not be known ahead-of-time.

I think we've also talked about it taking a chan of things to write, so the update's Total could increase as new things were requested. I don't know if that breaks any assumptions.

[DennisDenuto]

Thanks that is pretty helpful!

I would like to volunteer to work on this (This Friday will probably be the day I work on it).

wrt MultiWrite and streaming layers:
I will calculate the total bytes ahead-of-time before any blobs are uploaded (when / if streaming layers are supported, the implementation can be changed)

[imjasonh]

Sounds good @DennisDenuto, thanks!

Let us know if you have any questions or problems, we'd be happy to help.

[jonjohnsonjr]

The difference with MultiWrite is that there are multiple things having progress made, does it make sense for the update to include information about which things are progressing how?

The equivalent in docker land partitions upload progress by ID. I think that's usually layers.

I'm honestly not sure what to do here. A lot of the time, I want a single progress bar, and that's an easy model to program against... but folks might want to display more complex things than a single bar (e.g. per-layer progress), but making that change actually seems nontrivial, and I'm not sure how much it makes sense for MultiWrite (there can be hundreds of layers)...

I think we've also talked about it taking a chan of things to write, so the update's Total could increase as new things were requested. I don't know if that breaks any assumptions.

Yes, for tarball we take a channel:

go-containerregistry/pkg/v1/tarball/write.go

Line 396 in efb2d62

func WithProgress(updates chan<- v1.Update) WriteOption {

I imagine we want a uniform API.

[imjasonh]

I think we should start with WithProgress(chan<- v1.Update) for now. In the future if we decide we want a more granular progress option we can add WithGranularProgress(chan<- v1.GranularUpdate) or whatever makes sense then.

It sounds like that's sufficient for @DennisDenuto 's immediate use case, and I imagine it'll be good enough for most users at least for now.

[imjasonh]

There are some notable differences between tarball and remote wrt writing layer contents, that we should be careful to handle:

• remote.Write retries failed uploads, so a progress update might go back and forth if a partial upload fails and then succeeds on retry.
• remote.Write can also mount layer contents, and performs an existence check before writing layers, so a whole layer might get "uploaded" without involving any Writes.

I think we should try to get this landed in remote.Write first with tests, then extending it to remote.MultiWrite should be relatively straightforward.

[DennisDenuto]

Thanks for raising these differences.

I was thinking of writing an Example test similar to what tarball.Write does. But I'm leaning towards having tests with assertions for remote Writing, and having cases for the above might be a better approach.

Starting with the less complex Write and extending it to MultiWrite sounds like a good idea to me