oras-project / oras-go

ORAS Go library
https://oras.land
Apache License 2.0
172 stars 94 forks source link

add support for optional progress updates channel #661

Open deitch opened 8 months ago

deitch commented 8 months ago

If CopyOptions.UpdateChannel is non-nil, then with each block copied during a Copy*(), it sends an update down the channel. The update includes the amount copied in this update, and the descriptor of what is being copied. This descriptor lets the consumer determine what blob it applies to, and what the expected total size is (which can be used, e.g. to calculate percentages or create a progress bar).

It does not include relative sizes of the whole root desc or tag in the Copy(), because we have no way of knowing that; you would have to walk the entire tree first. Granted, you can get it from manifests first, and only then the more expensive other elements, but that would require a complete restructure. This is more than good enough for know.

Discussed in Slack with @Wwwsylvia

deitch commented 8 months ago

Sylvia did raise the following questions. Adding here for follow-on discussion:

  1. It is not clear to me that how does the caller read the channel. Is the channel generated internally or is it passed in by the caller? Is it per-blob?
  2. How do we report progress for the skipped (existing) blobs? What if the caller (e.g. ORAS CLI) wants to show 100% for the skipped blobs (as shown in the screenshot)?
  3. What if the caller wants to track the progress of PushBytes? Is there a way to reuse this mechanism?

Basically, the idea is that we want to make sure the APIs of oras-go are generic and extensible.

codecov-commenter commented 8 months ago

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (0b78aa6) 75.46% compared to head (cd02b49) 75.27%.

Files Patch % Lines
progress_reader.go 0.00% 8 Missing :warning:
copy.go 12.50% 5 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #661 +/- ## ========================================== - Coverage 75.46% 75.27% -0.19% ========================================== Files 59 60 +1 Lines 5640 5654 +14 ========================================== Hits 4256 4256 - Misses 1019 1032 +13 - Partials 365 366 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

deitch commented 8 months ago

Some responses:

It is not clear to me that how does the caller read the channel. Is the channel generated internally or is it passed in by the caller?

Passed in by the caller, CopyOptions.UpdateChannel. If it is nil, we do nothing different; if it is non-nil, we send updates to it with each copy.

Is it per-blob?

No, one channel, but each update includes the descriptor. See the extended comment on UpdateChannel.

How do we report progress for the skipped (existing) blobs?

I hadn’t thought about that. I think I would send a single update of 100% on existing blobs. But that can be subject for discussion. I think I would do this first, then do that in a follow-on. One improvement at a time.

What if the caller wants to track the progress of PushBytes? Is there a way to reuse this mechanism?

I don’t see why not. All this is, is a mechanism for sending updates in a channel. The issue here is that there are no “opts” on which to add it

deitch commented 8 months ago

Rebased

deitch commented 8 months ago

Going to rebase to get it back in line with main; any thoughts on the feedback @shizhMSFT ?

deitch commented 8 months ago

Rebased

shizhMSFT commented 8 months ago

Going to rebase to get it back in line with main; any thoughts on the feedback @shizhMSFT ?

@deitch I think this PR is good brainstorming idea and worths an issue or discussion so that we can design it holistically with production-level quality.

deitch commented 8 months ago

PR is good brainstorming idea and worths an issue or discussion so that we can design it holistically with production-level quality

I have no idea what that means. If the overall direction is good, let's fix this and get it in. If not, let's redo it.

shizhMSFT commented 8 months ago

It means that we need an issue or discussion for a systematical design as this PR is a feature request proposal. The reason behind it is that oras-go/v2 is now at a stable state and we should introduce new a feature (i.e. merge to main) with the best design we could achieve to avoid future breaking changes as much as we can. (note: breaking changes requires us to introduce oras-go/v3).

Since issue and discussion are good place to propose a feature request, would you like to open one with details like the motivation of the feature request (i.e. what would you like to be added? why is this needed for oras?)? We should have an issue template for oras-go but it is still in progress (tracked by #641; the oras repo has one). With an open issue / discussion, more maintainers like @sajayantony, @TerryHowe, and @sabre1041 can join the discussion and give inputs so that this conversation is not just between you and me or @Wwwsylvia. Of course, you may reference this PR in the new issue / discussion as an implementation candidate (so do others). #650 and #651 by @ktarplee are a good example.

Meanwhile, I would like to continue the discussions on the aspects of API design, concurrency (go-routine safety), performance, and security so that we can iterate your idea and make oras-go a better library.

deitch commented 8 months ago

Sure. This all started with an extended discussion we had on Slack with @Wwwsylvia . Part of the issue is that this capability existed (in a different way, but still) in v1. The move to v2, with all of its many benefits, lost this capability. This is trying to get it back, so that downstream dependencies that want to switch from v1 to v2 can do so without loss of ability.

BTW, if this PR changes an API, and there is a way to do it without changing it (i.e. backwards-compatible), then by all means. If I recall correctly, that was the main driver behind the variadic options in the main calls in v1. We knew we could add features going forward without changing the API.

sajayantony commented 8 months ago

I think this is general goodness and so I'm in support of landing something that is feasible and generally stable. Can someone given an code example of how this API might be consumed with maybe a warning of how it can be abused as well?

@deitch maybe a short hackmd doc or something that we can use for API review from the consumer aspect?

deitch commented 8 months ago

Can someone given an code example of how this API might be consumed maybe a short hackmd doc or something

I will put something in a comment here. It looks like there is question about whether or not this API is the right one, or even about putting in the changes. Once we have general agreement on adding it and the approach, I am happy to put a formal example and doc anywhere we want.

deitch commented 8 months ago

Also rebased while I was at it

deitch commented 8 months ago

Simple example:

ch := make(chan oras.CopyUpdate, 1000)
opts := oras.CopyOptions{
    UpdateChannel: ch        
}
desc, err := oras.Copy(ctx, src, tagName, dst, tagName, opts)
go func(ch <- chan CopyUpdate) {
    for msg := range ch {
        fmt.Printf("copied %d bytes out of %d total for %s\n", msg.Copied, msg.Descriptor.Size, msg.Descriptor.Digest)
    }
}(ch)

I realize that we probably would need to extend it the channel type so there is a way to send "complete" or "error". But I am waiting until the approach is agreed upon.

As for abusing, it is pretty straightforward. Have a channel, do not buffer it, or do not read from it until the buffer is full.

We could make the channel send (in this PR) non-blocking. That means that it would risk lost messages, but I find that an acceptable risk, much more so than, "user abuses it and the whole thing blocks."