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.
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
Add options to control images pulling of kubelet #11094
base: master
Are you sure you want to change the base?
Conversation
2f3ca71
to
150c92e
Compare
There was a problem hiding this 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
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: MrFreezeex, tu1h The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
150c92e
to
54660da
Compare
@@ -174,4 +174,8 @@ topologyManagerScope: {{ kubelet_topology_manager_scope }} | |||
tracing: | |||
endpoint: {{ kubelet_tracing_endpoint }} | |||
samplingRatePerMillion: {{ kubelet_tracing_sampling_rate_per_million }} | |||
{% endif %} | |||
serializeImagePulls: {{ kubelet_serialize_image_pulls | bool | lower }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @tu1h
Is it OK change it like to:
{% if kubelet_serialize_image_pulls is defined %}
....
{% endif %}
like the other optional variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but @MrFreezeex suggested to avoid using is defined
in Kubespray. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm perhaps we could be adding this line in the if in the line just below (and in that case it could be "hard coded" to false), there are a few other example that would be like that. It would prevent the usage of is defined
and not add those line by default (if this was the concern?)
(forgot to readd the lgtm label sorry) |
# Whether pull images one at a time | ||
kubelet_serialize_image_pulls: true | ||
# The maximum number of image pulls in parallel. This option takes effect if kubelet_serialize_image_pulls is false | ||
kubelet_max_parallel_image_pulls: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @tu1h
The maxParallelImagePulls
default value is nil: https://kubernetes.io/docs/reference/config-api/kubelet-config.v1beta1/#kubelet-config-k8s-io-v1beta1-KubeletConfiguration.
So we need to comment on it by default like:
# kubelet_max_parallel_image_pulls: 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it, please check if it is applicable.
New changes are detected. LGTM label has been removed. |
5a2631c
to
57d3aa4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
PTAL:-) |
Avoid using `default` if possible, rather define the variable in the defaults folder of the role. (Where you can also put comments for documentation).
Also, seems the effects are the same, I would rather have 1 than null.
|
Signed-off-by: tu1h <lihai.tu@daocloud.io>
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?: