Improve progress reader to report start/end and offset update #732

saschagrunert · 2019-10-22T12:27:47Z

The progress reader will be now created via the new newProgressReader
function. This way we have the possibility to trigger a new event called
ProgressEventNewArtifact and ProgressEventDone to indicate the
boundaries of the artifact download to the API consumer. During the
download, we additionally provide the OffsetUpdate, which can be used
to report the downloaded data during the last elapsed time interval.

Unit tests have been added as well to the progress reader, whereas the
documentation for the new types has been enhanced as well.

Needed for metrics improvements for CRI-O: cri-o/cri-o#2912

mtrmac

Overall, sure, this works, but I’m a bit worried that it may not do quite what the caller expects.

Is “the first time Read returns” really exactly the right moment? If the source is very slow, the caller won’t receive any report about trying to work with the blob until after the first data/timeout arrives. Should the first progress indication about the artifact happen at the very start of copyBlobFromStream, before asking for the first byte? (Or, actually, all the way up before TryReusingBlob and the like, so that it captures the delays communicating over HTTP and the like, and setting up the streams, not just waiting for data?)
Note that PutBlob may never actually read all of the data (well, it may not read any), e.g. if the blob already exists on the destination (well, if it was uploaded after TryReusingBlob fails). Should the last update be conditioned on r.artifact.Size, or be determined by code flow using a defer or something like that? (r.artifact.Size can even be -1 to mean “unknown”!) And should we, in that case, somehow tell the caller that the artifact is done (by lying and saying Offset = Size, or by decreasing Size)?

Neither is really a big show-stopper to me, and I’m not sure that CRI-O cares about either of the two at all. Thinking too far in this direction would lead to a complete redesign of the progress interface, which is currently rather a crude hack, to be much more detailed about the semantics, and I suppose that’s not all that needed right now — or is it?

types/types.go

saschagrunert · 2019-10-24T07:48:16Z

Overall, sure, this works, but I’m a bit worried that it may not do quite what the caller expects.

Hey @mtrmac, thanks for the first look. My main intention was to have something which changes
the library as little as possible, so I'm totally open to some rework here.

Is “the first time Read returns” really exactly the right moment? If the source is very slow, the caller won’t receive any report about trying to work with the blob until after the first data/timeout arrives. Should the first progress indication about the artifact happen at the very start of copyBlobFromStream, before asking for the first byte? (Or, actually, all the way up before TryReusingBlob and the like, so that it captures the delays communicating over HTTP and the like, and setting up the streams, not just waiting for data?)

I would expect (as a user) that the progress fires the first time a connection with the server (stream) has been successfully setup and no data got transferred. Maybe a progress state (CONNECTED) would be a good indicator for this.

Note that PutBlob may never actually read all of the data (well, it may not read any), e.g. if the blob already exists on the destination (well, if it was uploaded after TryReusingBlob fails). Should the last update be conditioned on r.artifact.Size, or be determined by code flow using a defer or something like that? (r.artifact.Size can even be -1 to mean “unknown”!) And should we, in that case, somehow tell the caller that the artifact is done (by lying and saying Offset = Size, or by decreasing Size)?

The caller needs the information that data has been skipped, because this will be an additional metric in CRI-O.

Neither is really a big show-stopper to me, and I’m not sure that CRI-O cares about either of the two at all. Thinking too far in this direction would lead to a complete redesign of the progress interface, which is currently rather a crude hack, to be much more detailed about the semantics, and I suppose that’s not all that needed right now — or is it?

Yeah, I'm not sure. The first time I used this API I must honestly admit that I expected something different from a usage perspective. I would assume that the progress bar display is handled by a more higher level library (like podman), whereas the API here works like:

Tell the library that I want to copy an image from A to B, where the copy function only returns a channel
I can now listen on that channel and get events (connected, pulling, interrupted, done), whereas the pulling events get triggered periodically and the library calculates the overall download status for me (packed in events data).

mtrmac · 2019-10-24T13:42:21Z

My main intention was to have something which changes
the library as little as possible

That’s very fair, and I don’t at at all want to insist on a big (API-breaking) redesign. It’s perfectly fine to do a small incremental improvement, as long as it’s clear that it still has limitations.

Neither is really a big show-stopper to me, and I’m not sure that CRI-O cares about either of the two at all. Thinking too far in this direction would lead to a complete redesign of the progress interface, which is currently rather a crude hack, to be much more detailed about the semantics, and I suppose that’s not all that needed right now — or is it?

Yeah, I'm not sure. The first time I used this API I must honestly admit that I expected something different from a usage perspective. I would assume that the progress bar display is handled by a more higher level library (like podman), whereas the API here works like:

The current progress bars are handled by copy.Image directly, and that is managed in more places than progressReader. Yes, it would be conceptually attractive for both to be built on the same abstraction. OTOH, that’s a fair amount of work — especially knowing about #611 , which among other things, included fairly significant extensions to the progress bar model as well (to be able to switch between status updates / progress status, for the same blob / unit of work).

I would expect (as a user) that the progress fires the first time a connection with the server (stream) has been successfully setup and no data got transferred.
The caller needs the information that data has been skipped, because this will be an additional metric in CRI-O.

Doing just these two might be easy enough in the ~current model, see the code comments:

mtrmac

My only real quibble is with the LastRead field name; these are just possible ways to extend this PR.

mtrmac · 2019-10-24T13:48:35Z

copy/copy.go

@@ -1159,7 +1159,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
 			channel:  c.progress,
 			interval: c.progressInterval,
 			artifact: srcInfo,
-			lastTime: time.Now(),
+			lastTime: time.Time{},


copyBlobFromStream should ideally not need to know about the time.Time{} special case; this rather calls for a newProgressReader() constructor that handles that.

That constructor could legitimately fire a “dealing with new artifact, 0% progress” event, without making this code any more complex.

It would then be very reasonable to add a defer func() { progressReader.reportDone(didReadAll, errorIfAny) } or something like that (let’s not add data that no-one needs), and ensure that there really is a 100%/skipped report.

Perhaps we could also split the constructor from a progressReader.assignSource(destStream) call, and call the constructor earlier than here, to get the 0% indication in the right place.

mtrmac · 2019-10-24T13:50:49Z

OTOH, that’s a fair amount of work — especially knowing about #611 , which among other things, included fairly significant extensions to the progress bar model as well (to be able to switch between status updates / progress status, for the same blob / unit of work).

(To be clear, that work was good and it would be a shame to lose it — and splitting that work into a smaller PR focused just on progress bars could help. Still, designing a well-typed stable API with detailed events about the progress is much more work than this PR.)

saschagrunert · 2019-10-25T09:20:43Z

Ready for review, @mtrmac and @vrothberg PTAL.

mtrmac

Thanks, this is great!

I’ll probably not get to make a careful review before Tuesday, so just a quick pass — other than the test question, it’s all nits and I don’t expect anything more serious to come up.

copy/progress_reader.go

mtrmac · 2019-10-25T19:25:09Z

copy/progress_reader_test.go

+) *progressReader {
+	artifact := types.BlobInfo{Size: 10}
+
+	go func() {


Note to self: does the testing framework know it has to wait for all spawned goroutines?

mtrmac · 2019-10-25T19:25:32Z

copy/progress_reader_test.go

@@ -0,0 +1,94 @@
+package copy


Yay, tests. Great!

types/types.go

vrothberg · 2019-10-28T11:30:58Z

Needs rebase.

The progress reader will be now created via the new `newProgressReader` function. This way we have the possibility to trigger a new event called `ProgressEventNewArtifact` and `ProgressEventDone` to indicate the boundaries of the artifact download to the API consumer. During the download, we additionally provide the `OffsetUpdate`, which can be used to report the downloaded data during the last elapsed time interval. Unit tests have been added as well to the progress reader, whereas the documentation for the new types has been enhanced as well. Signed-off-by: Sascha Grunert <[email protected]>

saschagrunert · 2019-10-28T11:36:03Z

Rebased on top of the latest master branch.

vrothberg

LGTM

TomSweeneyRedHat · 2019-10-28T12:47:57Z

LGTM and happy green test buttons. Would like a final head nod from @mtrmac

rhatdan · 2019-10-28T16:18:30Z

Hopefully this does not require a new version? :^(

vrothberg · 2019-10-28T16:20:51Z

Hopefully this does not require a new version? :^(

Only a minor bump is required :)

saschagrunert mentioned this pull request Oct 22, 2019

Extend image pull metrics cri-o/cri-o#2912

Merged

mtrmac reviewed Oct 23, 2019

View reviewed changes

types/types.go Outdated Show resolved Hide resolved

mtrmac reviewed Oct 24, 2019

View reviewed changes

saschagrunert force-pushed the reader-adaptions branch 2 times, most recently from daf6d55 to 42fdc78 Compare October 25, 2019 09:12

saschagrunert changed the title ~~Add last read value to progress reader~~ Improve progress reader to report start/end and offset update Oct 25, 2019

saschagrunert marked this pull request as ready for review October 25, 2019 09:12

saschagrunert force-pushed the reader-adaptions branch from 42fdc78 to cd6d90d Compare October 25, 2019 09:20

mtrmac reviewed Oct 25, 2019

View reviewed changes

saschagrunert force-pushed the reader-adaptions branch from cd6d90d to 0cb2995 Compare October 28, 2019 08:43

saschagrunert force-pushed the reader-adaptions branch from 0cb2995 to 01369d9 Compare October 28, 2019 11:35

saschagrunert force-pushed the reader-adaptions branch from 01369d9 to dbbb10b Compare October 28, 2019 11:35

vrothberg reviewed Oct 28, 2019

View reviewed changes

rhatdan merged commit 6bbebfc into containers:master Oct 30, 2019

saschagrunert deleted the reader-adaptions branch October 30, 2019 17:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve progress reader to report start/end and offset update #732

Improve progress reader to report start/end and offset update #732

saschagrunert commented Oct 22, 2019 •

edited

Loading

mtrmac left a comment

saschagrunert commented Oct 24, 2019 •

edited

Loading

mtrmac commented Oct 24, 2019

mtrmac left a comment

mtrmac Oct 24, 2019

mtrmac commented Oct 24, 2019

saschagrunert commented Oct 25, 2019

mtrmac left a comment •

edited

Loading

mtrmac Oct 25, 2019

mtrmac Oct 25, 2019

vrothberg commented Oct 28, 2019

saschagrunert commented Oct 28, 2019

vrothberg left a comment

TomSweeneyRedHat commented Oct 28, 2019

rhatdan commented Oct 28, 2019

vrothberg commented Oct 28, 2019

Improve progress reader to report start/end and offset update #732

Improve progress reader to report start/end and offset update #732

Conversation

saschagrunert commented Oct 22, 2019 • edited Loading

mtrmac left a comment

Choose a reason for hiding this comment

saschagrunert commented Oct 24, 2019 • edited Loading

mtrmac commented Oct 24, 2019

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Oct 24, 2019

Choose a reason for hiding this comment

mtrmac commented Oct 24, 2019

saschagrunert commented Oct 25, 2019

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

mtrmac Oct 25, 2019

Choose a reason for hiding this comment

mtrmac Oct 25, 2019

Choose a reason for hiding this comment

vrothberg commented Oct 28, 2019

saschagrunert commented Oct 28, 2019

vrothberg left a comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Oct 28, 2019

rhatdan commented Oct 28, 2019

vrothberg commented Oct 28, 2019

saschagrunert commented Oct 22, 2019 •

edited

Loading

saschagrunert commented Oct 24, 2019 •

edited

Loading

mtrmac left a comment •

edited

Loading