X Tutup
Skip to content

c8d/pull: Progress fixes#47432

Merged
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-pull-fslayer
Feb 26, 2024
Merged

c8d/pull: Progress fixes#47432
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-pull-fslayer

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Feb 22, 2024

- What I did

  • Emit Pulling fs layer (fixes issue in Docker Desktop)
  • Guard io.Writer in progressOutput with a mutex
  • Don't emit Downloading when no progress was made yet

- How I did it
See commits.

- How to verify it

- Description for the changelog

containerd image store: Fix image pull not emitting `Pulling fs layer` status

- A picture of a cute animal (not mandatory but encouraged)

Sync access to the underlying `io.Writer` with a mutex.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
To align with the graphdrivers behavior and don't send unnecessary
progress messages.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added area/distribution Image Distribution process/cherry-pick kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration process/cherry-pick/25.0 labels Feb 22, 2024
@vvoland vvoland added this to the 26.0.0 milestone Feb 22, 2024
@vvoland vvoland self-assigned this Feb 22, 2024
@dmcgowan
Copy link
Member

Guard io.Writer in progressOutput with a mutex

What is the issue seen for this? Is prog.LastUpdate not actually the last update or is the writer itself not protected?

@vvoland
Copy link
Contributor Author

vvoland commented Feb 22, 2024

The writer itself is not protected. I noticed that after I experienced random failures when adding that additional progress update. This happens because HandlerFunc is executed with images.Dispatch on containerd side so using non-thread safe things in the handler func isn't safe.

@thaJeztah
Copy link
Member

Failures are the expected ones (containerd snapshotter on Windows)

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/distribution Image Distribution containerd-integration Issues and PRs related to containerd integration impact/changelog kind/bugfix PR's that fix bugs process/cherry-picked status/4-merge

Projects

Development

Successfully merging this pull request may close these issues.

6 participants

X Tutup