Skip to content

[release/1.4] Improve image pull performance from http 1.1 container registries#4751

Merged
fuweid merged 3 commits intocontainerd:release/1.4from
amrmahdi:amrh/cherry-pick-cc3785-1.4
Nov 20, 2020
Merged

[release/1.4] Improve image pull performance from http 1.1 container registries#4751
fuweid merged 3 commits intocontainerd:release/1.4from
amrmahdi:amrh/cherry-pick-cc3785-1.4

Conversation

@amrmahdi
Copy link
Copy Markdown
Contributor

@amrmahdi amrmahdi commented Nov 18, 2020

Improve image pull performance from http 1.1 container registries

(cherry picked from commit cc3785c)
Signed-off-by: Amr Mahdi [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Nov 18, 2020

Hi @amrmahdi. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Nov 18, 2020

/ok-to-test

@cpuguy83 cpuguy83 added this to the 1.4.2 milestone Nov 18, 2020
cpuguy83
cpuguy83 approved these changes Nov 18, 2020
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

@fuweid fuweid changed the title backport pull request #4653 to 1.4 [release/1.4] Improve image pull performance from http 1.1 container registries Nov 18, 2020
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Nov 18, 2020

backport from #4653

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 19, 2020

Build succeeded.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Nov 19, 2020

@amrmahdi is that merged commit? Just see @estesp as a author

@amrmahdi
Copy link
Copy Markdown
Contributor Author

amrmahdi commented Nov 19, 2020

@amrmahdi is that merged commit? Just see @estesp as a author

Yes, see #4653

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 19, 2020 •
edited
Loading

@amrmahdi is that merged commit? Just see @estesp as a author
Yes, see #4653

I think we generally cherry-pick the commit itself, not the merge commit, so in this case it would be the three commits from the PR; 289130b, f6834d4, and b81917e

Could you re-do the cherry-pick with those?;

git cherry-pick -s -x 289130b8a7760b813c7bcfdd37f978a17f10c31a f6834d4c0b9a0433d6723ea172ad333ddc44936b b81917ee72a8e705127006084619b5c0ef76aa8e

@amrmahdi
Improve image pull performance from http 1.1 container registries
9b2156a 
Private registries that does not support http 2.0 such as Azure Container Registry streams back content in a max of 16KB chunks (max TLS record size). The small chunks introduce an overhead when copying the layers to the content store sine each chunk incurs the overhead of  grpc message that has to be sent to the content store.

This change reduces this overhead by buffering the chunks into 1MB chunks and only then writes a message to the content store.

Below is a per comparsion between the 2 approaches using a couple of large images that are being pulled from the docker hub (http 2.0) and a private Azure CR (http 1.1) in seconds.

image                                                   | Buffered copy | master
-------                                                 |---------------|----------
docker.io/pytorch/pytorch:latest                        |  55.63        | 58.33
docker.io/nvidia/cuda:latest                            |  72.05        | 75.98
containerdpulltest.azurecr.io/pytorch/pytorch:latest    | 61.45         | 77.1
containerdpulltest.azurecr.io/nvidia/cuda:latest        | 77.13         | 85.47

Signed-off-by: Amr Mahdi <[email protected]>

(cherry picked from commit 289130b)

Signed-off-by: Amr Mahdi <[email protected]>
@amrmahdi
replicate io.Copy optimizations
a2ebee3 
Signed-off-by: Amr Mahdi <[email protected]>
(cherry picked from commit f6834d4)
Signed-off-by: Amr Mahdi <[email protected]>
@amrmahdi
Add comments clarifying copyWithBuffer implementation
5618423 
Signed-off-by: Amr Mahdi <[email protected]>
(cherry picked from commit b81917e)
Signed-off-by: Amr Mahdi <[email protected]>
@amrmahdi amrmahdi force-pushed the amrh/cherry-pick-cc3785-1.4 branch from 6d65464 to 5618423 Compare November 19, 2020 00:59
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Nov 19, 2020

Build succeeded.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 19, 2020

Thanks!

@thaJeztah thaJeztah requested a review from cpuguy83 November 19, 2020 01:18
Zyqsempai
Zyqsempai approved these changes Nov 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Zyqsempai Zyqsempai left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah approved these changes Nov 19, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

fuweid
fuweid approved these changes Nov 20, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 8d839c4 into containerd:release/1.4 Nov 20, 2020
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Nov 20, 2020

Thanks @amrmahdi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@fuweid fuweid fuweid approved these changes

+1 more reviewer

@Zyqsempai Zyqsempai Zyqsempai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

1.4.2

Development

Successfully merging this pull request may close these issues.

6 participants

@amrmahdi @k8s-ci-robot @cpuguy83 @fuweid @thaJeztah @Zyqsempai