Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add options to control images pulling of kubelet #11094

Merged
merged 1 commit into from
Jun 21, 2024
Merged

Add options to control images pulling of kubelet #11094

merged 1 commit into from
Jun 21, 2024

Conversation

0ekk
Copy link
Member

@0ekk 0ekk commented Apr 17, 2024 •
edited by VannTen
Loading

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR provides a option to control the images pulling efficiency of kubelet.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`kubelet_max_parallel_image_pulls` represents the maximum number of image pulls in parallel
action required: if you were previously only setting `serializeImagePulls: false` to have unlimited parallel pulls, you will need to set `kubelet_max_parallel_images_pulls` to a suitable value instead

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 17, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 17, 2024
@0ekk 0ekk force-pushed the kubelet_pull_image branch 3 times, most recently from 2f3ca71 to 150c92e Compare April 17, 2024 11:09
MrFreezeex
MrFreezeex approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@MrFreezeex MrFreezeex left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution :D
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2024
@0ekk 0ekk force-pushed the kubelet_pull_image branch from 150c92e to 54660da Compare April 19, 2024 05:58
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 19, 2024
@MrFreezeex
Copy link
Member

(forgot to readd the lgtm label sorry)
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2024
@0ekk 0ekk force-pushed the kubelet_pull_image branch from 54660da to 962c371 Compare May 14, 2024 03:05
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2024
@0ekk 0ekk force-pushed the kubelet_pull_image branch 2 times, most recently from 5a2631c to 57d3aa4 Compare May 14, 2024 09:09
VannTen
VannTen requested changes May 14, 2024
View reviewed changes
Copy link
Contributor

@VannTen VannTen left a comment

Choose a reason for hiding this comment

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

See this : https://github.com/kubernetes/kubernetes/blob/68baa105f06c8a49eae242882ef531f7b5082fac/pkg/kubelet/apis/config/v1beta1/defaults.go#L209-L216

To me, this means we can simply get away with only 1 variable maxParrallelImagePulls, defaulting to 1 (that or nil has the exact same effect.
This would have the benefits of a simpler template (no need for is defined which is something I'm actively trying to get rid of 😆 )

@0ekk
Copy link
Member Author

0ekk commented May 15, 2024

See this : https://github.com/kubernetes/kubernetes/blob/68baa105f06c8a49eae242882ef531f7b5082fac/pkg/kubelet/apis/config/v1beta1/defaults.go#L209-L216

To me, this means we can simply get away with only 1 variable maxParrallelImagePulls, defaulting to 1 (that or nil has the exact same effect. This would have the benefits of a simpler template (no need for is defined which is something I'm actively trying to get rid of 😆 )

It's a nice solution for me. What do you think? @yankay @MrFreezeex

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2024
@yankay
Copy link
Member

yankay commented May 20, 2024

See this : https://github.com/kubernetes/kubernetes/blob/68baa105f06c8a49eae242882ef531f7b5082fac/pkg/kubelet/apis/config/v1beta1/defaults.go#L209-L216
To me, this means we can simply get away with only 1 variable maxParrallelImagePulls, defaulting to 1 (that or nil has the exact same effect. This would have the benefits of a simpler template (no need for is defined which is something I'm actively trying to get rid of 😆 )

It's a nice solution for me. What do you think? @yankay @MrFreezeex

Thanks @tu1h

I think it's a great solution.

maxParrallelImagePulls, defaulting to nil. If it is defined, set the SerializeImagePulls to false.

@0ekk 0ekk force-pushed the kubelet_pull_image branch from 57d3aa4 to af0fea3 Compare May 20, 2024 09:53
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 20, 2024
@0ekk
Copy link
Member Author

0ekk commented May 20, 2024

PTAL:-)
The new changes are simplified and use only one option to determine whether to adopt the parallel image pulling.

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 20, 2024
@VannTen
Copy link
Contributor

VannTen commented May 20, 2024 via email

@0ekk 0ekk force-pushed the kubelet_pull_image branch from af0fea3 to f0cd41a Compare May 20, 2024 10:31
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2024
@0ekk
Add options to control images pulling of kubelet
f24b1bd 
Signed-off-by: tu1h <lihai.tu@daocloud.io>
@0ekk 0ekk force-pushed the kubelet_pull_image branch from f0cd41a to f24b1bd Compare May 31, 2024 03:07
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2024
@VannTen
Copy link
Contributor

VannTen commented May 31, 2024 via email

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2024
@yankay
Copy link
Member

yankay commented May 31, 2024

Thanks @tu1h
/lgtm

@0ekk 0ekk requested a review from VannTen June 12, 2024 08:53
@VannTen
Copy link
Contributor

VannTen commented Jun 13, 2024 via email

@0ekk
Copy link
Member Author

0ekk commented Jun 13, 2024

Could you give approval for the pr? @yankay

@ant31
Copy link
Contributor

ant31 commented Jun 21, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ant31, MrFreezeex, tu1h

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 921b0c0 into kubernetes-sigs:master Jun 21, 2024
77 checks passed
Rickkwa pushed a commit to Rickkwa/kubespray that referenced this pull request Jun 26, 2024
@0ekk @Rickkwa
Add options to control images pulling of kubelet (kubernetes-sigs#11094)
586dc7f 
Signed-off-by: tu1h <lihai.tu@daocloud.io>
@yankay yankay mentioned this pull request Aug 28, 2024
Closed
@olevitt olevitt mentioned this pull request Oct 8, 2024
Closed
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 10, 2024
davidumea pushed a commit to elastisys/kubespray that referenced this pull request Oct 25, 2024
@0ekk @davidumea
Add options to control images pulling of kubelet (kubernetes-sigs#11094)
935aa05 
Signed-off-by: tu1h <lihai.tu@daocloud.io>
kpoxo6op pushed a commit to kpoxo6op/kubespray that referenced this pull request Dec 27, 2024
@0ekk @kpoxo6op
Add options to control images pulling of kubelet (kubernetes-sigs#11094)
789abb9 
Signed-off-by: tu1h <lihai.tu@daocloud.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrFreezeex MrFreezeex MrFreezeex approved these changes

@yankay yankay yankay left review comments

@cyclinder cyclinder Awaiting requested review from cyclinder

@VannTen VannTen Awaiting requested review from VannTen

Assignees

@ant31 ant31

@yankay yankay

@MrFreezeex MrFreezeex

@VannTen VannTen

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@0ekk @MrFreezeex @yankay @VannTen @ant31 @k8s-ci-robot