Skip to content

Commit 5c99eeb

Browse files
author
Aaron Lehmann
committed
Fix concurrent uploads that share layers
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>
1 parent d883002 commit 5c99eeb

File tree

4 files changed

+103
-32
lines changed

4 files changed

+103
-32
lines changed

‎distribution/push_v2.go‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ type v2Pusher struct {
4242
config *ImagePushConfig
4343
repo distribution.Repository
4444

45-
// pushState is state built by the Download functions.
45+
// pushState is state built by the Upload functions.
4646
pushState pushState
4747
}
4848

@@ -224,6 +224,7 @@ type v2PushDescriptor struct {
224224
repoInfo reference.Named
225225
repo distribution.Repository
226226
pushState *pushState
227+
remoteDescriptor distribution.Descriptor
227228
}
228229

229230
func (pd *v2PushDescriptor) Key() string {
@@ -238,16 +239,16 @@ func (pd *v2PushDescriptor) DiffID() layer.DiffID {
238239
return pd.layer.DiffID()
239240
}
240241

241-
func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.Output) error {
242+
func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.Output) (distribution.Descriptor, error) {
242243
diffID := pd.DiffID()
243244

244245
pd.pushState.Lock()
245-
if _, ok := pd.pushState.remoteLayers[diffID]; ok {
246+
if descriptor, ok := pd.pushState.remoteLayers[diffID]; ok {
246247
// it is already known that the push is not needed and
247248
// therefore doing a stat is unnecessary
248249
pd.pushState.Unlock()
249250
progress.Update(progressOutput, pd.ID(), "Layer already exists")
250-
return nil
251+
return descriptor, nil
251252
}
252253
pd.pushState.Unlock()
253254

@@ -257,14 +258,14 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
257258
descriptor, exists, err := layerAlreadyExists(ctx, v2Metadata, pd.repoInfo, pd.repo, pd.pushState)
258259
if err != nil {
259260
progress.Update(progressOutput, pd.ID(), "Image push failed")
260-
return retryOnError(err)
261+
return distribution.Descriptor{}, retryOnError(err)
261262
}
262263
if exists {
263264
progress.Update(progressOutput, pd.ID(), "Layer already exists")
264265
pd.pushState.Lock()
265266
pd.pushState.remoteLayers[diffID] = descriptor
266267
pd.pushState.Unlock()
267-
return nil
268+
return descriptor, nil
268269
}
269270
}
270271

@@ -328,9 +329,9 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
328329

329330
// Cache mapping from this layer's DiffID to the blobsum
330331
if err := pd.v2MetadataService.Add(diffID, metadata.V2Metadata{Digest: mountFrom.Digest, SourceRepository: pd.repoInfo.FullName()}); err != nil {
331-
return xfer.DoNotRetry{Err: err}
332+
return distribution.Descriptor{}, xfer.DoNotRetry{Err: err}
332333
}
333-
return nil
334+
return err.Descriptor, nil
334335
case nil:
335336
// blob upload session created successfully, so begin the upload
336337
mountAttemptsRemaining = 0
@@ -345,14 +346,14 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
345346
if layerUpload == nil {
346347
layerUpload, err = bs.Create(ctx)
347348
if err != nil {
348-
return retryOnError(err)
349+
return distribution.Descriptor{}, retryOnError(err)
349350
}
350351
}
351352
defer layerUpload.Close()
352353

353354
arch, err := pd.layer.TarStream()
354355
if err != nil {
355-
return xfer.DoNotRetry{Err: err}
356+
return distribution.Descriptor{}, xfer.DoNotRetry{Err: err}
356357
}
357358

358359
// don't care if this fails; best effort
@@ -371,20 +372,20 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
371372
nn, err := layerUpload.ReadFrom(tee)
372373
compressedReader.Close()
373374
if err != nil {
374-
return retryOnError(err)
375+
return distribution.Descriptor{}, retryOnError(err)
375376
}
376377

377378
pushDigest := digester.Digest()
378379
if _, err := layerUpload.Commit(ctx, distribution.Descriptor{Digest: pushDigest}); err != nil {
379-
return retryOnError(err)
380+
return distribution.Descriptor{}, retryOnError(err)
380381
}
381382

382383
logrus.Debugf("uploaded layer %s (%s), %d bytes", diffID, pushDigest, nn)
383384
progress.Update(progressOutput, pd.ID(), "Pushed")
384385

385386
// Cache mapping from this layer's DiffID to the blobsum
386387
if err := pd.v2MetadataService.Add(diffID, metadata.V2Metadata{Digest: pushDigest, SourceRepository: pd.repoInfo.FullName()}); err != nil {
387-
return xfer.DoNotRetry{Err: err}
388+
return distribution.Descriptor{}, xfer.DoNotRetry{Err: err}
388389
}
389390

390391
pd.pushState.Lock()
@@ -393,23 +394,24 @@ func (pd *v2PushDescriptor) Upload(ctx context.Context, progressOutput progress.
393394
// speaks the v2 protocol.
394395
pd.pushState.confirmedV2 = true
395396

396-
pd.pushState.remoteLayers[diffID] = distribution.Descriptor{
397+
descriptor := distribution.Descriptor{
397398
Digest: pushDigest,
398399
MediaType: schema2.MediaTypeLayer,
399400
Size: nn,
400401
}
402+
pd.pushState.remoteLayers[diffID] = descriptor
401403

402404
pd.pushState.Unlock()
403405

404-
return nil
406+
return descriptor, nil
407+
}
408+
409+
func (pd *v2PushDescriptor) SetRemoteDescriptor(descriptor distribution.Descriptor) {
410+
pd.remoteDescriptor = descriptor
405411
}
406412

407413
func (pd *v2PushDescriptor) Descriptor() distribution.Descriptor {
408-
// Not necessary to lock pushStatus because this is always
409-
// called after all the mutation in pushStatus.
410-
// By the time this function is called, every layer will have
411-
// an entry in remoteLayers.
412-
return pd.pushState.remoteLayers[pd.DiffID()]
414+
return pd.remoteDescriptor
413415
}
414416

415417
// layerAlreadyExists checks if the registry already know about any of the

‎distribution/xfer/upload.go‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"time"
66

77
"github.com/Sirupsen/logrus"
8+
"github.com/docker/distribution"
89
"github.com/docker/docker/layer"
910
"github.com/docker/docker/pkg/progress"
1011
"golang.org/x/net/context"
@@ -28,8 +29,8 @@ func NewLayerUploadManager(concurrencyLimit int) *LayerUploadManager {
2829
type uploadTransfer struct {
2930
Transfer
3031

31-
diffID layer.DiffID
32-
err error
32+
remoteDescriptor distribution.Descriptor
33+
err error
3334
}
3435

3536
// An UploadDescriptor references a layer that may need to be uploaded.
@@ -41,7 +42,12 @@ type UploadDescriptor interface {
4142
// DiffID should return the DiffID for this layer.
4243
DiffID() layer.DiffID
4344
// Upload is called to perform the Upload.
44-
Upload(ctx context.Context, progressOutput progress.Output) error
45+
Upload(ctx context.Context, progressOutput progress.Output) (distribution.Descriptor, error)
46+
// SetRemoteDescriptor provides the distribution.Descriptor that was
47+
// returned by Upload. This descriptor is not to be confused with
48+
// the UploadDescriptor interface, which is used for internally
49+
// identifying layers that are being uploaded.
50+
SetRemoteDescriptor(descriptor distribution.Descriptor)
4551
}
4652

4753
// Upload is a blocking function which ensures the listed layers are present on
@@ -50,7 +56,7 @@ type UploadDescriptor interface {
5056
func (lum *LayerUploadManager) Upload(ctx context.Context, layers []UploadDescriptor, progressOutput progress.Output) error {
5157
var (
5258
uploads []*uploadTransfer
53-
dedupDescriptors = make(map[string]struct{})
59+
dedupDescriptors = make(map[string]*uploadTransfer)
5460
)
5561

5662
for _, descriptor := range layers {
@@ -60,12 +66,12 @@ func (lum *LayerUploadManager) Upload(ctx context.Context, layers []UploadDescri
6066
if _, present := dedupDescriptors[key]; present {
6167
continue
6268
}
63-
dedupDescriptors[key] = struct{}{}
6469

6570
xferFunc := lum.makeUploadFunc(descriptor)
6671
upload, watcher := lum.tm.Transfer(descriptor.Key(), xferFunc, progressOutput)
6772
defer upload.Release(watcher)
6873
uploads = append(uploads, upload.(*uploadTransfer))
74+
dedupDescriptors[key] = upload.(*uploadTransfer)
6975
}
7076

7177
for _, upload := range uploads {
@@ -78,6 +84,9 @@ func (lum *LayerUploadManager) Upload(ctx context.Context, layers []UploadDescri
7884
}
7985
}
8086
}
87+
for _, l := range layers {
88+
l.SetRemoteDescriptor(dedupDescriptors[l.Key()].remoteDescriptor)
89+
}
8190

8291
return nil
8392
}
@@ -86,7 +95,6 @@ func (lum *LayerUploadManager) makeUploadFunc(descriptor UploadDescriptor) DoFun
8695
return func(progressChan chan<- progress.Progress, start <-chan struct{}, inactive chan<- struct{}) Transfer {
8796
u := &uploadTransfer{
8897
Transfer: NewTransfer(),
89-
diffID: descriptor.DiffID(),
9098
}
9199

92100
go func() {
@@ -105,8 +113,9 @@ func (lum *LayerUploadManager) makeUploadFunc(descriptor UploadDescriptor) DoFun
105113

106114
retries := 0
107115
for {
108-
err := descriptor.Upload(u.Transfer.Context(), progressOutput)
116+
remoteDescriptor, err := descriptor.Upload(u.Transfer.Context(), progressOutput)
109117
if err == nil {
118+
u.remoteDescriptor = remoteDescriptor
110119
break
111120
}
112121

‎distribution/xfer/upload_test.go‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"testing"
77
"time"
88

9+
"github.com/docker/distribution"
910
"github.com/docker/distribution/digest"
1011
"github.com/docker/docker/layer"
1112
"github.com/docker/docker/pkg/progress"
@@ -35,32 +36,36 @@ func (u *mockUploadDescriptor) DiffID() layer.DiffID {
3536
return u.diffID
3637
}
3738

39+
// SetRemoteDescriptor is not used in the mock.
40+
func (u *mockUploadDescriptor) SetRemoteDescriptor(remoteDescriptor distribution.Descriptor) {
41+
}
42+
3843
// Upload is called to perform the upload.
39-
func (u *mockUploadDescriptor) Upload(ctx context.Context, progressOutput progress.Output) error {
44+
func (u *mockUploadDescriptor) Upload(ctx context.Context, progressOutput progress.Output) (distribution.Descriptor, error) {
4045
if u.currentUploads != nil {
4146
defer atomic.AddInt32(u.currentUploads, -1)
4247

4348
if atomic.AddInt32(u.currentUploads, 1) > maxUploadConcurrency {
44-
return errors.New("concurrency limit exceeded")
49+
return distribution.Descriptor{}, errors.New("concurrency limit exceeded")
4550
}
4651
}
4752

4853
// Sleep a bit to simulate a time-consuming upload.
4954
for i := int64(0); i <= 10; i++ {
5055
select {
5156
case <-ctx.Done():
52-
return ctx.Err()
57+
return distribution.Descriptor{}, ctx.Err()
5358
case <-time.After(10 * time.Millisecond):
5459
progressOutput.WriteProgress(progress.Progress{ID: u.ID(), Current: i, Total: 10})
5560
}
5661
}
5762

5863
if u.simulateRetries != 0 {
5964
u.simulateRetries--
60-
return errors.New("simulating retry")
65+
return distribution.Descriptor{}, errors.New("simulating retry")
6166
}
6267

63-
return nil
68+
return distribution.Descriptor{}, nil
6469
}
6570

6671
func uploadDescriptors(currentUploads *int32) []UploadDescriptor {

‎integration-cli/docker_cli_push_test.go‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,61 @@ func (s *DockerSchema1RegistrySuite) TestPushEmptyLayer(c *check.C) {
148148
testPushEmptyLayer(c)
149149
}
150150

151+
// testConcurrentPush pushes multiple tags to the same repo
152+
// concurrently.
153+
func testConcurrentPush(c *check.C) {
154+
repoName := fmt.Sprintf("%v/dockercli/busybox", privateRegistryURL)
155+
156+
repos := []string{}
157+
for _, tag := range []string{"push1", "push2", "push3"} {
158+
repo := fmt.Sprintf("%v:%v", repoName, tag)
159+
_, err := buildImage(repo, fmt.Sprintf(`
160+
FROM busybox
161+
ENTRYPOINT ["/bin/echo"]
162+
ENV FOO foo
163+
ENV BAR bar
164+
CMD echo %s
165+
`, repo), true)
166+
c.Assert(err, checker.IsNil)
167+
repos = append(repos, repo)
168+
}
169+
170+
// Push tags, in parallel
171+
results := make(chan error)
172+
173+
for _, repo := range repos {
174+
go func(repo string) {
175+
_, _, err := runCommandWithOutput(exec.Command(dockerBinary, "push", repo))
176+
results <- err
177+
}(repo)
178+
}
179+
180+
for range repos {
181+
err := <-results
182+
c.Assert(err, checker.IsNil, check.Commentf("concurrent push failed with error: %v", err))
183+
}
184+
185+
// Clear local images store.
186+
args := append([]string{"rmi"}, repos...)
187+
dockerCmd(c, args...)
188+
189+
// Re-pull and run individual tags, to make sure pushes succeeded
190+
for _, repo := range repos {
191+
dockerCmd(c, "pull", repo)
192+
dockerCmd(c, "inspect", repo)
193+
out, _ := dockerCmd(c, "run", "--rm", repo)
194+
c.Assert(strings.TrimSpace(out), checker.Equals, "/bin/sh -c echo "+repo)
195+
}
196+
}
197+
198+
func (s *DockerRegistrySuite) TestConcurrentPush(c *check.C) {
199+
testConcurrentPush(c)
200+
}
201+
202+
func (s *DockerSchema1RegistrySuite) TestConcurrentPush(c *check.C) {
203+
testConcurrentPush(c)
204+
}
205+
151206
func (s *DockerRegistrySuite) TestCrossRepositoryLayerPush(c *check.C) {
152207
sourceRepoName := fmt.Sprintf("%v/dockercli/busybox", privateRegistryURL)
153208
// tag the image to upload it to the private registry

0 commit comments

Comments
 (0)