Fix PushHandler cannot push image that contains duplicated blobs#5379
Fix PushHandler cannot push image that contains duplicated blobs#5379estesp merged 1 commit intocontainerd:masterfrom
Conversation
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded.
|
From this description and the code, it sounds like the If so, would you mind updating the documentation on |
|
@samuelkarp Thanks for reviewing this.
No, that isn't. Because the duplication of pushes is the source of this reported bug, too. For example if we have 20 bytes blob (ref = "A") and multiple (duplicated) calls to upload it occur, the following will happen:
This PR tries to solve this by de-duplicating push (i.e. introducing Ingester API). Another possible (ideal) solution is to give unique ref per push even if they point to the same blob. But this solution will break backward compatibility because The lock on
This prevents the race condition that happens during deciding which push to be deduplicated, like the following (please see also the code snippet):
func (p dockerPusher) push(ctx context.Context, desc ocispec.Descriptor, ref string, unavailableOnFail bool) (content.Writer, error) {
...(omit)...
status, err := p.tracker.GetStatus(ref)
if err == nil {
if status.Committed && status.Offset == status.Total {
return nil, errors.Wrapf(errdefs.ErrAlreadyExists, "ref %v", ref)
}
...(omit)...
} else if !errdefs.IsNotFound(err) {
return nil, errors.Wrap(err, "failed to get status")
}
...(omit)... |
|
Build succeeded.
|
|
@ktock Thanks for the explanation! Would you mind updating the documentation for |
|
@samuelkarp Thanks for the comment. Added the comment to |
|
Build succeeded.
|
|
Build succeeded.
|
samuelkarp
left a comment
samuelkarp
left a comment
There was a problem hiding this comment.
LGTM, thanks for bearing with me!
Signed-off-by: Kohei Tokunaga <[email protected]>
|
Build succeeded.
|
|
Hi. We currently see this defect in our environment. I can't really figure out what for what release this defect was fixed? |
|
If you click on a commit hash, you should go to the commit (instead of PR) view and GitHub will show you all the tags in which the commit is included. In this case it is every release from v1.5.0 through the current v1.6.0 release. However, a bug in this code was just fixed 22 days ago in #6243 which is only in the v1.6.0 release at this time. As a bug fix it should be backported to |
Excellent. Thank you. |
|
We are using the latest Docker version 20.10.12 on both Linux and Windows, but as far as I can tell this version uses containerd v1.4.12. I don't see any way for us to update to something where this fix is included. What are our options here, expect for waiting until containerd is updated in the Docker distributions? Is there a workaround for this issue. Would it e.g. be possible to configure containerd to not push images in parallel |
Our issue is not so much with containerd, but with https://github.com/getporter/porter that depends on containerd 1.3.0 |
5 participants
Fixes: #2706
Currently,
remotes.PushHandlerfails to push images that contain duplicated blobs. This is because when multiple and parallel calls ofdockerPusher.Pushwith the same digest occur, each call races to updateStatusTracker. This breaks theStatusof the upload anddockerPusher.Commit()fails occasionally.This commit tries to solve this issue without breaking backward compatibility.
This commit introduces a new backward-compatible extension for
StatusTrackercalledStatusTrackLockerwhich implements named mutex for ensuring atomic update.Additionally, this commit introduces
content.IngesterAPI todockerPusher. If there is already an ongoing upload of blob with ref "A", the second call with "A" returnsErrUnavailable(This behaviour follows the local content store'sIngester.Writer).PushHandlerretries to get the writer usingcontent.OpenWriter. This is somewhat similar to the way ofremotes.FetchHandlerhandling duplicated blobs.Test
The following image used in CRI test has a lot of duplicated blobs.
Click here to show the manifest index of the image
ctr i pushfails with the master version of containerd:But the above succeeds with this PR.