Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c8d/pull: Progress fixes #47432

Merged
merged 3 commits into from Feb 26, 2024
Merged

c8d/pull: Progress fixes #47432

merged 3 commits into from Feb 26, 2024

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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants