Add configuration options to local transfer service #8062
Conversation
|
Hi @fangn2. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
fangn2
force-pushed
the
config-options
branch
4 times, most recently
from
1e7a389 to
6441a40
Compare
fangn2
force-pushed
the
config-options
branch
4 times, most recently
from
3118d5d to
88c5582
Compare
fangn2
force-pushed
the
config-options
branch
3 times, most recently
from
d65b68d to
1600c32
Compare
|
@dmcgowan I addressed your comments, fixed CI failures, and rebased main. |
dmcgowan
left a comment
dmcgowan
left a comment
There was a problem hiding this comment.
A few minor things, then I think it is ready to go
Signed-off-by: Tony Fang <[email protected]>
fangn2
force-pushed
the
config-options
branch
from
1600c32 to
4730539
Compare
dmcgowan
left a comment
dmcgowan
left a comment
There was a problem hiding this comment.
Looks good, the tests can be in the follow up to not hold up the export PR.
fuweid
left a comment
fuweid
left a comment
There was a problem hiding this comment.
LGTM
The comment issue can be handled in follow-up.
|
|
||
| type TransferConfig struct { | ||
| // MaxConcurrentDownloads is the max concurrent content downloads for pull. | ||
| MaxConcurrentDownloads int `toml:"max_concurrent_downloads"` |
There was a problem hiding this comment.
Not related to this pr. I just see many issues saying that the image pulling brings more io pressure. So I am wondering about how to add a global limitation here. maybe introduces rate-limiter var?
There was a problem hiding this comment.
We do have a global limiter defined here which initialized using this MaxConcurrentDownloads
There was a problem hiding this comment.
Thanks! I misunderstand this part when I review it. It will change the existing CRI-plugin config definition right now.
Currently, the CRI-plugin limits the concurrent pulling numbers for each pull. It confuses that users from docker.
It looks good to me if the CRI-plugin config definition can align with transfer service. I was thinking about adding a global rate-limit for CRI-plugin.
Update fork-external/main with upstream main at commit [081d818](containerd@081d818) Marged upstream container/main into fork-external/main Related work items: containerd#7864, containerd#7954, containerd#8041, containerd#8044, containerd#8051, containerd#8062, containerd#8096, containerd#8103, containerd#8109, containerd#8110, containerd#8113, containerd#8114, containerd#8119, containerd#8120, containerd#8128, containerd#8130, containerd#8134, containerd#8140, containerd#8142, containerd#8143, containerd#8152, containerd#8154, containerd#8162, containerd#8164, containerd#8165, containerd#8172, containerd#8173, containerd#8177, containerd#8178, containerd#8181, containerd#8183, containerd#8187, containerd#8188, containerd#8189, containerd#8190, containerd#8191, containerd#8192, containerd#8193
4 participants
Part of #7592 Add configuration options to local transfer service.
Signed-off-by: Tony Fang [email protected]