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

Fix concurrent uploads that share layers #20831

Merged
merged 1 commit into from Mar 2, 2016

Conversation

aaronlehmann
Copy link
Contributor

Concurrent uploads which share layers worked correctly as of #18353,
but unfortunately #18785 caused a regression. This PR removed the logic
that shares digests between different push sessions. This overlooked the
case where one session was waiting for another session to upload a
layer.

This commit adds back the ability to propagate this digest information,
using the distribution.Descriptor type because this is what is received
from stats and uploads, and also what is ultimately needed for building
the manifest.

Surprisingly, there was no test covering this case. This commit adds
one. It fails without the fix.

See recent comments on #9132.

Concurrent uploads which share layers worked correctly as of moby#18353,
but unfortunately moby#18785 caused a regression. This PR removed the logic
that shares digests between different push sessions. This overlooked the
case where one session was waiting for another session to upload a
layer.

This commit adds back the ability to propagate this digest information,
using the distribution.Descriptor type because this is what is received
from stats and uploads, and also what is ultimately needed for building
the manifest.

Surprisingly, there was no test covering this case. This commit adds
one. It fails without the fix.

See recent comments on moby#9132.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@calavera
Copy link
Contributor

calavera commented Mar 2, 2016

LGTM

@vdemeester
Copy link
Member

LGTM 🐰

vdemeester added a commit that referenced this pull request Mar 2, 2016
Fix concurrent uploads that share layers
@vdemeester vdemeester merged commit 621a148 into moby:master Mar 2, 2016
@thaJeztah thaJeztah added this to the 1.10.3 milestone Mar 2, 2016
@thaJeztah
Copy link
Member

@aaronlehmann I saw you got a panic when reproducing the issue; #9132 (comment). Should this be given an "priority/P.."? i.e. is it serious enough to warrant a patch release?

@aaronlehmann aaronlehmann deleted the concurrent-upload branch March 2, 2016 17:18
@aaronlehmann
Copy link
Contributor Author

If there is a 1.10.3 release, this should be included.

runcom added a commit to projectatomic/docker that referenced this pull request Mar 4, 2016
Backported and adapted from moby#20831

Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@tiborvass tiborvass mentioned this pull request Mar 7, 2016
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

5 participants