image/copy: Fix missing progress reporting for chunked layers by simek-m · Pull Request #848 · podman-container-tools/container-libs

simek-m · 2026-05-15T15:25:49Z

The PutBlobPartial code path only updated a progress bar, but it did not report its progress to the copy.Options.Progress channel for chunked layers.
This PR adds missing reporting and refactors parts shared with progressReader.

The Podman REST API images/pull with the pullProgress flag introduced in podman-container-tools/podman#28224 did not get progress updates from the channel and the output stream lacked events for partial blobs.

Test added in: podman-container-tools/podman#28713

Fixes: #469

API pullProgress stream with this fix

➜  bin/podman system service --log-level=trace --time=0 tcp://localhost:8888 2> >(grep -i partial >&2)
time="2026-05-14T15:18:48Z" level=debug msg="Retrieved partial blob sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b"
time="2026-05-14T15:18:49Z" level=debug msg="Retrieved partial blob sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0"
time="2026-05-14T15:18:49Z" level=debug msg="Retrieved partial blob sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538"
time="2026-05-14T15:18:49Z" level=debug msg="Retrieved partial blob sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51"
time="2026-05-14T15:18:49Z" level=debug msg="Retrieved partial blob sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936"
time="2026-05-14T15:18:49Z" level=debug msg="Retrieved partial blob sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4"

➜  ~ podman rmi -f localhost:15000/chunked-test
Untagged: localhost:15000/chunked-test:latest
Deleted: be9027b559bccd87b7e10e472e464beadace2012e4dd69615a3a72682d83f006
➜  ~ curl --silent -X POST 'http://localhost:8888/v6.0.0/libpod/images/pull?reference=localhost:15000/chunked-test:latest&pullProgress=true' | jq .
{
  "status": "pulling",
  "stream": "Trying to pull localhost:15000/chunked-test:latest...\n"
}
{
  "status": "pulling",
  "stream": "Getting image source signatures\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:09aa76a8de5384893277e1a0ba676ed7d063e4089c5b2966362e4de9eb37efb6\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51\n"
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0\n"
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 10499078,
    "progressComponentID": "sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538"
  }
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 5250753,
    "progressComponentID": "sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4"
  }
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 6299391,
    "progressComponentID": "sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b"
  }
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 6299400,
    "progressComponentID": "sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51"
  }
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 5250775,
    "progressComponentID": "sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0"
  }
}
{
  "status": "pulling",
  "stream": "Artifact already exists",
  "pullProgress": {
    "status": "skipped",
    "progressComponentID": "sha256:09aa76a8de5384893277e1a0ba676ed7d063e4089c5b2966362e4de9eb37efb6"
  }
}
{
  "status": "pulling",
  "stream": "Copying blob sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936\n"
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 6299404,
    "progressComponentID": "sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6086,
    "total": 6299391,
    "progressComponentID": "sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 10246,
    "total": 10499078,
    "progressComponentID": "sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6042,
    "total": 5250753,
    "progressComponentID": "sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6089,
    "total": 6299400,
    "progressComponentID": "sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6058,
    "total": 5250775,
    "progressComponentID": "sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6086,
    "total": 6299404,
    "progressComponentID": "sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6299225,
    "total": 6299391,
    "progressComponentID": "sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 10498912,
    "total": 10499078,
    "progressComponentID": "sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6299234,
    "total": 6299400,
    "progressComponentID": "sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 6299237,
    "total": 6299404,
    "progressComponentID": "sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 5250586,
    "total": 5250753,
    "progressComponentID": "sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 6299225,
    "total": 6299391,
    "progressComponentID": "sha256:0ea8cd0810b88e14631c1064bc882a6ff5e664860ff768c70102716ab64e7f0b"
  }
}
{
  "status": "pulling",
  "stream": "Pulling artifact",
  "pullProgress": {
    "status": "pulling",
    "current": 5250607,
    "total": 5250775,
    "progressComponentID": "sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 5250607,
    "total": 5250775,
    "progressComponentID": "sha256:da9bf6c3f7cfad233b1e4de2f6dbf010d48a73dfc14395bf634f45a413e4dbb0"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 10498912,
    "total": 10499078,
    "progressComponentID": "sha256:8ad5c254127956ee8bea277ea593be376859bff00be2e987b4e6905c2e8d5538"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 6299234,
    "total": 6299400,
    "progressComponentID": "sha256:7fd8aa9622b5f5f23bee3c4035235d929a4fe5da860b52bdda81ff432cb82b51"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 6299237,
    "total": 6299404,
    "progressComponentID": "sha256:fc285a90f91602992c8cb458431ffe63fcb61295489191d153e366704885d936"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 5250586,
    "total": 5250753,
    "progressComponentID": "sha256:400129928e227203c8f2146c61b5142a362ba9afdbd37fb459d61f7829983fe4"
  }
}
{
  "status": "pulling",
  "stream": "Copying config sha256:be9027b559bccd87b7e10e472e464beadace2012e4dd69615a3a72682d83f006\n"
}
{
  "status": "pulling",
  "stream": "Starting to pull artifact",
  "pullProgress": {
    "status": "pulling",
    "total": 1859,
    "progressComponentID": "sha256:be9027b559bccd87b7e10e472e464beadace2012e4dd69615a3a72682d83f006"
  }
}
{
  "status": "pulling",
  "stream": "Artifact pulled successfully",
  "pullProgress": {
    "status": "success",
    "current": 1859,
    "total": 1859,
    "progressComponentID": "sha256:be9027b559bccd87b7e10e472e464beadace2012e4dd69615a3a72682d83f006"
  }
}
{
  "status": "pulling",
  "stream": "Writing manifest to image destination\n"
}
{
  "status": "success",
  "images": [
    "be9027b559bccd87b7e10e472e464beadace2012e4dd69615a3a72682d83f006"
  ]

simek-m · 2026-05-15T15:30:05Z

 			}
+			// Setup progress reporting and report a new artifact event
+			// if the channel available and a non-zero interval set.
+			if ic.c.options.Progress != nil && ic.c.options.ProgressInterval > 0 {


I was thinking of moving the nil checks from the caller side to the methods and allow nil receivers to clean it up a bit, but there is only one special-cased instance of this pattern (or two) in this codebase and it would be confusing, I think.

This is not immediately relevant (bar.ProxyReader breaks it anyway), but various code in the standard library special-cases known implementations of io.Reader and the like (e.g., but not only, checking for things like WriterTo); so adding proxy objects might have a non-obvious performance cost — it’s better to make the whole proxy usage conditional than to always interpose a proxy and then have it be a no-op under some conditions.

(This does not really apply to GetBlobAt because that’s our private interface.)

mtrmac

Thanks!

Most importantly, the Podman-side tests seem broken/insufficient.

I don’t feel strongly about adding the extra progressReporter abstraction — but if we have it, it should be used consistently in the two users.

mtrmac · 2026-05-15T17:00:13Z

 			}
+			// Setup progress reporting and report a new artifact event
+			// if the channel available and a non-zero interval set.
+			if ic.c.options.Progress != nil && ic.c.options.ProgressInterval > 0 {


This is not immediately relevant (bar.ProxyReader breaks it anyway), but various code in the standard library special-cases known implementations of io.Reader and the like (e.g., but not only, checking for things like WriterTo); so adding proxy objects might have a non-obvious performance cost — it’s better to make the whole proxy usage conditional than to always interpose a proxy and then have it be a no-op under some conditions.

(This does not really apply to GetBlobAt because that’s our private interface.)

simek-m · 2026-05-15T17:40:07Z

@mtrmac Thank you, I responded to some of the more obvious comments. I'll go through the rest later and change the code accordingly.

simek-m · 2026-05-20T11:27:56Z

+// TestNewProgressReporter verifies that constructing a reporter
+// signals a new artifact event.
+func TestNewProgressReporter(t *testing.T) {
+	channel := make(chan types.ProgressProperties, 1)


The buffered channel is used for simplicity to avoid a race because the newProgressReporter constructor sends the event and then returns.

simek-m · 2026-05-20T11:50:51Z

I hope changes in 80883b6 reflect your comments and what we discussed.

Now reportDone() should only be called on success (there's no event for errors) and it's the responsibility of the caller.
The ErrFallbackToOrdinaryLayerDownload path resets progress of the re-used reporter.

Edit: There was an omission in the previous commit.
However, I don't know if it's correct not to have a reporter for imageCopier.copyConfig and I'm not sure what I did is a good approach and if there should be any reset/reuse.

simek-m · 2026-05-21T12:23:09Z

+// it's higher than the offset reached before the restart to
+// avoid confusing behavior in consumers of the events
+// (skipping back).
+type channelProgressReporter struct {


Just to make sure, is it ok not worry about thread-safety here?
There was no synchronization in progressReader before my changes.

Yes; we have one goroutine per blob, so that naturally synchronizes the per-blob operations; and io.Reader is generally not thread-safe, so this is not making anything worse.

mtrmac

Thanks! Looks pretty good overall.

(Please squash the commits, at least in the current form they seem not to give meaningful structure individually to be worth preserving in the history.)

Note to self: I didn’t review the tests.

mtrmac

A full review now; just some nits / cleanups, mostly in tests.

mtrmac · 2026-06-08T15:09:43Z

 			if err != nil {
 				return types.BlobInfo{}, err
 			}
+			reporter.reportDone()


(Absolutely non-blocking: Being consistent about the location of the empty line, and the relative ordering of reportDone/mark100PercentComplete would make it clearer that the 3 code paths are consistent.)

mtrmac · 2026-06-08T15:34:14Z

+
+		// After interval, reportRead(15): nothing (15 < 20 already reported).
+		time.Sleep(2 * interval)
+		r.reportRead(15)


Non-blocking: A test for /* no sleep */ reportRead(); reportDone() in ~this situation wouldn’t hurt.

Added a new test TestProgressReporterResetIntervalNotElapsed.

For the record, that test does not test the "15 < 20 already reported" case.

The `PutBlobPartial` code path only updated a progress bar, but it did not report its progress to the `copy.Options.Progress` channel for chunked layers. Add missing progress reporting, tests and refactor parts shared with `progressReader`. Fixes: podman-container-tools#469 Signed-off-by: Marek Simek <msimek@redhat.com>

The reader tests did not use the tested progressReader wrapper. Use progressReader in the tests and remove goroutines. Signed-off-by: Marek Simek <msimek@redhat.com>

mtrmac

Thank you!

github-actions Bot added the image Related to "image" package label May 15, 2026

simek-m commented May 15, 2026

View reviewed changes

mtrmac reviewed May 15, 2026

View reviewed changes

Comment thread image/copy/progress_channel.go Outdated

mtrmac reviewed May 15, 2026

View reviewed changes

Comment thread image/copy/blob_chunk_accessor_proxy.go

simek-m force-pushed the 469-chunked-progress branch from e67f898 to 80883b6 Compare May 20, 2026 11:24

simek-m commented May 20, 2026

View reviewed changes

mtrmac reviewed May 20, 2026

View reviewed changes

simek-m commented May 21, 2026

View reviewed changes

simek-m force-pushed the 469-chunked-progress branch from 1c7ef0e to bcc8be6 Compare June 8, 2026 09:34

mtrmac reviewed Jun 8, 2026

View reviewed changes

Comment thread image/copy/progress.go Outdated

Comment thread image/copy/single.go Outdated

Comment thread image/copy/single.go

mtrmac reviewed Jun 8, 2026

View reviewed changes

simek-m force-pushed the 469-chunked-progress branch from bcc8be6 to c6c5ff4 Compare June 9, 2026 10:48

image/copy: Fix reader tests in progress_channel_test.go

dfc3483

The reader tests did not use the tested progressReader wrapper. Use progressReader in the tests and remove goroutines. Signed-off-by: Marek Simek <msimek@redhat.com>

mtrmac approved these changes Jun 9, 2026

View reviewed changes

mtrmac merged commit 3a8ceb7 into podman-container-tools:main Jun 9, 2026
19 checks passed

Conversation

simek-m commented May 15, 2026

API pullProgress stream with this fix

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

simek-m commented May 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simek-m commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

simek-m commented May 20, 2026 •

edited

Loading

mtrmac left a comment •

edited

Loading