-
Notifications
You must be signed in to change notification settings - Fork 463
controller/template: put the cloudconf for kubelet behind platform gate #672
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
controller/template: put the cloudconf for kubelet behind platform gate #672
Conversation
Previously, the assumption was that all platforms that had CloudProviderConfig set can have kubelet configured to use the config file. But this created a regression for user-provided-infrastructure VSphere case, as everybody like kube-controller-manager etc. except kubelet can use the cloud conf file from API for vSphere
|
/approve |
|
@kikisdeliveryservice can you also review this and put your lgtm to merge if it's good? |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, kikisdeliveryservice, runcom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
| --cloud-provider=vsphere \ | ||
| --volume-plugin-dir=/etc/kubernetes/kubelet-plugins/volume/exec \ | ||
| --cloud-config=/etc/kubernetes/cloud.conf \ | ||
| \ |
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.
let's remove the line entirely? same in the worker service below
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.
currently this is status for kubelet service files in master, and it would require changing the templates to allow skipping this file. Would it be okay to take it as a follow up to update the template to do this for all platforms?
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.
EDIT: also I want this PR's scope confined to changing the render function behavior
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.
no worries @abhinavdahiya
we can get this PR in and make whatever small changes as a followup.
|
I don't want to be a killjoy and put a |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Previously, the assumption was that all platforms that had CloudProviderConfig set can have kubelet configured to
use the config file. But this created a regression for user-provided-infrastructure VSphere case, as everybody like kube-controller-manager etc. except kubelet can use the cloud conf
file from API for vSphere
/cc @staebler