Docker pull/push with max concurrency limits. #22445

yongtang · 2016-05-01T21:45:44Z

- What I did

This fix tries to address issues raised in #20936 and #22443 where docker pull or docker push fails because of the concurrent connection failing.
Currently, the number of maximum concurrent connections is controlled by maxDownloadConcurrency and maxUploadConcurrency which are set to 3 and 5 respectively. Therefore, in situations where network connections don't support multiple downloads/uploads, failures may encounter for docker push or docker pull.

- How I did it

This fix changes maxDownloadConcurrency and maxUploadConcurrency to adjustable by passing --max-concurrent-uploads and --max-concurrent-downloads to docker daemon command.

The documentation related to docker daemon has been updated.

- How to verify it

Additional test case have been added to cover the changes in this fix.

- Description for the changelog

Add --max--concurrent-uploads and --max--concurrent-downloads to docker daemon command so that docker pull and docker push could control the max number of concurrent connections during uploads or downloads.

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

This fix fixes #20936. This fix fixes #22443.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

thaJeztah · 2016-05-01T22:24:24Z

Thanks!

ping @aaronlehmann wdyt?

aaronlehmann · 2016-05-02T17:29:45Z

Thanks for working on this.

I wasn't sure whether this should be a daemon-level flag or a flag that can be passed to the client and transmitted through the API. Now that I think about it some more, I think a client flag would be very unfriendly, because it would have to be specified on every pull. So I think the approach here makes sense.

I'd be interested to hear opinions on this, though.

aaronlehmann · 2016-05-02T17:33:34Z

daemon/config.go

@@ -18,6 +18,11 @@ import (
 )

 const (
+	defaultMaxDownloadConcurrency = 5
+	defaultMaxUploadConcurrency   = 3


I noticed that this reverses the old defaults. The old code was:

// maxDownloadConcurrency is the maximum number of downloads that // may take place at a time for each pull. maxDownloadConcurrency = 3 // maxUploadConcurrency is the maximum number of uploads that // may take place at a time for each push. maxUploadConcurrency = 5

I was a little surprised to see that there are more simultaneous uploads allowed than downloads, but I think I remember why it's like this. Registries often have high latencies to a storage backend, such as S3. At the end of a layer upload, there are multiple steps that happen to commit that upload, and because of the latency, this can take several seconds. If a push is uploading many small layers, it's faster overall to upload more at the same time, so less time is wasted in between uploads waiting for the registry to do these commits.

Thanks @aaronlehmann that was a typo I must have been confused about which one is which when I was working on it.

LK4D4 · 2016-05-02T17:42:34Z

I'm okay with flags. However now we need validation and probably a way to specify "unlimited".

MHBauer · 2016-05-02T18:05:56Z

@aaronlehmann There is a client-side config file that could be used for this flag in the client.

I like 'max' something as the flag. That is the word I would first look for.

yongtang · 2016-05-03T02:35:09Z

Thanks @LK4D4 I just updated the validation to the added flags. Please let me know if there are any issues.

aaronlehmann · 2016-05-03T16:57:06Z

Thanks @yongtang, this looks good.

To address @LK4D4's suggestion of allowing unlimited concurrency, could we make 0 mean unlimited?

As for the flag names, maybe -max-downloads and -max-uploads?

thaJeztah · 2016-05-03T19:02:17Z

Yes, was also thinking along the lines of --max-downloads, possibly --max-concurrent-downloads, but that's quite long again.

yongtang · 2016-05-04T02:19:30Z

Thanks @LK4D4 @aaronlehmann @thaJeztah I just updated the pull request to change the names and make 0 as unlimited. Please let me know if there are any issues.

aaronlehmann · 2016-05-04T16:42:00Z

LGTM

thaJeztah · 2016-05-04T16:58:39Z

hm, before it's merged; should this setting be "reloadable"? https://docs.docker.com/engine/reference/commandline/daemon/#configuration-reloading, so that it can be re-configured without restarting the daemon?

aaronlehmann · 2016-05-04T17:03:13Z

@thaJeztah: It already is; see the changes to the Reload function. I requested this during the review.

aaronlehmann · 2016-05-04T17:08:13Z

Looks like this doc needs to be updated though - is that what you mean?

thaJeztah · 2016-05-04T17:17:24Z

Oh! Missed that that was implemented now, didn't look at the code. But yes, it should be mentioned in the docs, below this header https://github.com/docker/docker/blob/master/docs/reference/commandline/dockerd.md#configuration-reloading

yongtang · 2016-05-04T17:25:24Z

Thanks @aaronlehmann @thaJeztah I just updated the docs. Please let me know if there are any other issues.

thaJeztah · 2016-05-10T17:29:07Z

@LK4D4 I think we can wait one day so that @yongtang can add the man page (the "primary" documentation is complete then for this feature)

thaJeztah · 2016-05-10T17:33:14Z

Haha, so no day waiting is needed. LGTM

icecrime · 2016-05-11T23:03:36Z

Sorry but I really dislike the names --max-uploads and --max-downloads, it feels like I'm going to have to pay after the first n images downloaded 😕

Explicit is better than short, can we please rename to --max-concurrent-downloads and --max-concurrent-uploads? Thanks, and sorry for being late to the party.

This fix tries to address issues raised in moby#20936 and moby#22443 where `docker pull` or `docker push` fails because of the concurrent connection failing. Currently, the number of maximum concurrent connections is controlled by `maxDownloadConcurrency` and `maxUploadConcurrency` which are hardcoded to 3 and 5 respectively. Therefore, in situations where network connections don't support multiple downloads/uploads, failures may encounter for `docker push` or `docker pull`. This fix tries changes `maxDownloadConcurrency` and `maxUploadConcurrency` to adjustable by passing `--max-concurrent-uploads` and `--max-concurrent-downloads` to `docker daemon` command. The documentation related to docker daemon has been updated. Additional test case have been added to cover the changes in this fix. This fix fixes moby#20936. This fix fixes moby#22443. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-05-12T02:51:43Z

Thanks @icecrime just updated the PR. Please let me know if there are any other issues.

aaronlehmann · 2016-05-12T05:16:42Z

re-LGTM

thaJeztah · 2016-05-12T11:18:10Z

LGTM (--max-concurrent-downloads was my second choice, so 👍 )

icecrime · 2016-05-12T15:51:00Z

@yongtang Thanks a lot for the rapid update!
@aaronlehmann @thaJeztah Thanks a lot for the rapid re-review!

thaJeztah · 2016-05-12T16:26:44Z

ping @albers @sdurrheimer think this needs changes to the completion scripts. Sorry forgot to ping earlier ❤️

albers · 2016-05-12T19:22:28Z

I'll add it to bash completion. Thanks for the ping.

This fix tries to address a separate issue raised during the review of another PR moby#22445. The issue was raised because currently, daemon reload will skip the action if the field (e.g., `debug`, `labels`, etc) is not specified. This potentially could cause some confusion: 1. Users will need to explicitly set the field if they want to unset and they cannot assume `default` value any more 2. Users have to check the previous state of the daemon in order to figure out the expected behavior of a reload, instead of relying on the config file they intend to reload. Without knowing the previous state of the daemon the behavior will be unpredictable. In this fix, we always unset the value if not specified so that users know what to expect based on the config file to be loaded, instead of the previous state of the daemon (before reload). Additional test has been added to cover the changes in this fix. This fix is related to moby#22445. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

GordonTheTurtle added the status/0-triage label May 1, 2016

yongtang mentioned this pull request May 1, 2016

docker pull fails because of a concurrent connection failing #22443

Closed

thaJeztah added status/1-design-review and removed status/0-triage labels May 1, 2016

aaronlehmann added status/2-code-review and removed status/1-design-review labels May 2, 2016

aaronlehmann reviewed May 2, 2016
View reviewed changes

yongtang force-pushed the 20936-22443-concurrent-connection branch 4 times, most recently from 194f9ae to 7a18b4e Compare May 3, 2016 02:34

yongtang force-pushed the 20936-22443-concurrent-connection branch from 7a18b4e to b9bd316 Compare May 4, 2016 02:18

yongtang force-pushed the 20936-22443-concurrent-connection branch from b9bd316 to 811ea10 Compare May 4, 2016 13:57

yongtang force-pushed the 20936-22443-concurrent-connection branch from 811ea10 to 720595a Compare May 4, 2016 17:24

thaJeztah added status/4-merge and removed status/3-docs-review labels May 10, 2016

yujuhong mentioned this pull request May 10, 2016

System pods are not starting because they cannot pull image kubernetes/kubernetes#25408

Closed

icecrime added status/2-code-review and removed status/4-merge labels May 11, 2016

yongtang force-pushed the 20936-22443-concurrent-connection branch from 113b3af to 7368e41 Compare May 12, 2016 02:47

thaJeztah added status/4-merge and removed status/2-code-review labels May 12, 2016

icecrime merged commit e911757 into moby:master May 12, 2016

yongtang deleted the 20936-22443-concurrent-connection branch May 12, 2016 15:54

albers mentioned this pull request May 13, 2016

bash completion for docker daemon --max-concurrent-{down,up}load #22713

Merged

sdurrheimer mentioned this pull request May 14, 2016

Add zsh completion for 'docker daemon --max-concurrent-downloads --ma… #22739

Merged

thaJeztah mentioned this pull request Aug 13, 2016

Push Serialized On Low Bandwith #25679

Closed

AkihiroSuda mentioned this pull request Dec 6, 2016

--max-concurrent-downloads,--max-concurrent-uploads must great than or equal to 0 #28279

Merged

supereagle mentioned this pull request Jan 10, 2017

In Docker 1.10, it is very slow when push multiple images at the same time supereagle/experiences#1

Closed

komuw mentioned this pull request Apr 11, 2017

increase docker max-concurrent-downloads value komuw/meli#4

Closed

thaJeztah mentioned this pull request Jun 8, 2017

Add runtime option to throttle I/O/network bandwidth? #4763

Open

sami-alajrami mentioned this pull request Jul 5, 2017

Docker on windows fails to pull linux images behind proxy docker/for-win#894

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker pull/push with max concurrency limits. #22445

Docker pull/push with max concurrency limits. #22445

yongtang commented May 1, 2016 •

edited

Loading

thaJeztah commented May 1, 2016

aaronlehmann commented May 2, 2016

aaronlehmann May 2, 2016

yongtang May 3, 2016

LK4D4 commented May 2, 2016

MHBauer commented May 2, 2016

yongtang commented May 3, 2016

aaronlehmann commented May 3, 2016

thaJeztah commented May 3, 2016

yongtang commented May 4, 2016

aaronlehmann commented May 4, 2016

thaJeztah commented May 4, 2016

aaronlehmann commented May 4, 2016

aaronlehmann commented May 4, 2016

thaJeztah commented May 4, 2016

yongtang commented May 4, 2016

thaJeztah commented May 10, 2016

thaJeztah commented May 10, 2016

icecrime commented May 11, 2016

yongtang commented May 12, 2016

aaronlehmann commented May 12, 2016

thaJeztah commented May 12, 2016

icecrime commented May 12, 2016

thaJeztah commented May 12, 2016

albers commented May 12, 2016

Docker pull/push with max concurrency limits. #22445

Docker pull/push with max concurrency limits. #22445

Conversation

yongtang commented May 1, 2016 • edited Loading

thaJeztah commented May 1, 2016

aaronlehmann commented May 2, 2016

aaronlehmann May 2, 2016

Choose a reason for hiding this comment

yongtang May 3, 2016

Choose a reason for hiding this comment

LK4D4 commented May 2, 2016

MHBauer commented May 2, 2016

yongtang commented May 3, 2016

aaronlehmann commented May 3, 2016

thaJeztah commented May 3, 2016

yongtang commented May 4, 2016

aaronlehmann commented May 4, 2016

thaJeztah commented May 4, 2016

aaronlehmann commented May 4, 2016

aaronlehmann commented May 4, 2016

thaJeztah commented May 4, 2016

yongtang commented May 4, 2016

thaJeztah commented May 10, 2016

thaJeztah commented May 10, 2016

icecrime commented May 11, 2016

yongtang commented May 12, 2016

aaronlehmann commented May 12, 2016

thaJeztah commented May 12, 2016

icecrime commented May 12, 2016

thaJeztah commented May 12, 2016

albers commented May 12, 2016

yongtang commented May 1, 2016 •

edited

Loading