Skip to content

Add image pull concurrency limit.#2920

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:max-concurrent-download
Jan 29, 2019
Merged

Add image pull concurrency limit.#2920
crosbymichael merged 1 commit intocontainerd:masterfrom
Random-Liu:max-concurrent-download

Conversation

@Random-Liu
Copy link
Copy Markdown
Member

@Random-Liu Random-Liu commented Jan 9, 2019 •
edited
Loading

For #2886
Fixes #2195

The result is almost the same with docker now! :)

Signed-off-by: Lantao Liu lantaol@google.com

@Random-Liu Random-Liu added this to the 1.3 milestone Jan 9, 2019
@Random-Liu Random-Liu mentioned this pull request Jan 9, 2019
Closed
AkihiroSuda
AkihiroSuda reviewed Jan 10, 2019
View reviewed changes
pkg/concurrencylimiter/limiter.go Outdated
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return error if limit < 1 ?

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fuweid
fuweid reviewed Jan 10, 2019
View reviewed changes
pkg/concurrencylimiter/limiter.go Outdated
Copy link
Copy Markdown
Member

@fuweid fuweid Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use ctx.Err() here because it might be timeout caused by deadline.

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Random-Liu Random-Liu force-pushed the max-concurrent-download branch 2 times, most recently from 84ae6ec to 0f97382 Compare January 10, 2019 09:15
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 10, 2019 •
edited
Loading

Codecov Report

Merging #2920 into master will decrease coverage by 2.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2920      +/-   ##
==========================================
- Coverage   43.97%   41.41%   -2.56%     
==========================================
  Files         102       70      -32     
  Lines       10874     9457    -1417     
==========================================
- Hits         4782     3917     -865     
+ Misses       5359     4979     -380     
+ Partials      733      561     -172
Flag Coverage Δ
#linux ?
#windows 41.41% <ø> (+0.22%) ⬆️
Impacted Files Coverage Δ
snapshots/native/native.go 1.78% <0%> (-41.27%) ⬇️
archive/tar.go 16.99% <0%> (-26.8%) ⬇️
metadata/snapshot.go 21.53% <0%> (-24.28%) ⬇️
content/local/writer.go 56.86% <0%> (-0.99%) ⬇️
gc/scheduler/scheduler.go 66.34% <0%> (-0.97%) ⬇️
metadata/db.go 64.98% <0%> (-0.93%) ⬇️
oci/spec_opts.go 26.8% <0%> (-0.27%) ⬇️
metadata/content.go 43.8% <0%> (-0.18%) ⬇️
archive/tar_windows.go 8.78% <0%> (-0.09%) ⬇️
content/local/locks.go 100% <0%> (ø) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5abeeff...d7ed403. Read the comment docs.

cpuguy83
cpuguy83 reviewed Jan 10, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty straight forward. Not a huge fan of adding a new argument to Dispatch, but I don't have a good suggestion at the moment for another approach.

pkg/concurrencylimiter/limiter.go Outdated
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this could just be https://godoc.org/golang.org/x/sync/semaphore

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I didn't know there is such package, this is exactly what I want at the beginning. Didn't find it in golang sync, thus implemented one with channel myself.

Will use this instead.

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Random-Liu Random-Liu force-pushed the max-concurrent-download branch from 0f97382 to a8e32b3 Compare January 10, 2019 18:42
stevvooe
stevvooe reviewed Jan 10, 2019
View reviewed changes
images/handlers.go Outdated
Copy link
Copy Markdown
Member

@stevvooe stevvooe Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to update the godoc.

Copy link
Copy Markdown
Member Author

@Random-Liu Random-Liu Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Random-Liu Random-Liu force-pushed the max-concurrent-download branch from a8e32b3 to 5104855 Compare January 10, 2019 20:53
cpuguy83
cpuguy83 approved these changes Jan 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ehazlett
Copy link
Copy Markdown
Member

ehazlett commented Jan 29, 2019

@Random-Liu needs a rebase. thanks!

@Random-Liu
Add image pull concurrency limit.
d7ed403 
Signed-off-by: Lantao Liu <lantaol@google.com>
@Random-Liu Random-Liu force-pushed the max-concurrent-download branch from 5104855 to d7ed403 Compare January 29, 2019 18:29
@Random-Liu
Copy link
Copy Markdown
Member Author

Random-Liu commented Jan 29, 2019

@ehazlett Done

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Jan 29, 2019

LGTM

1 similar comment
@ehazlett
Copy link
Copy Markdown
Member

ehazlett commented Jan 29, 2019

LGTM

@crosbymichael crosbymichael merged commit 7fd3d02 into containerd:master Jan 29, 2019
@Random-Liu Random-Liu deleted the max-concurrent-download branch January 29, 2019 23:17
This was referenced Aug 18, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@fuweid fuweid fuweid left review comments

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

@dmcgowan dmcgowan

Labels

None yet

Projects

None yet

Milestone

1.3

Development

Successfully merging this pull request may close these issues.

9 participants

@Random-Liu @codecov-io @ehazlett @crosbymichael @stevvooe @cpuguy83 @AkihiroSuda @fuweid @dmcgowan