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.

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 extra-args configs to kubernetes-worker charm #55334

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Add extra-args configs to kubernetes-worker charm #55334

merged 1 commit into from
Nov 9, 2017

Conversation

Cynerva
Copy link
Contributor

@Cynerva Cynerva commented Nov 8, 2017

What this PR does / why we need it:

This adds kubelet-extra-args and proxy-extra-args config options to the kubernetes-worker charm, which allow operators to pass arbitrary configuraton to the kubelet and kube-proxy services.

This also fixes a bug where the equivalent config options on kubernetes-master (api-extra-args, controller-manager-extra-args, scheduler-extra-args) don't remove arguments from the services when they are removed from the config.

Release note:

Add extra-args configs to kubernetes-worker charm

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 8, 2017
@Cynerva
Copy link
Contributor Author

Cynerva commented Nov 8, 2017

/approve no-issue

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2017
kubelet_opts['experimental-nvidia-gpus'] = '1'
else:
hookenv.log('Adding --feature-gates=Accelerators=true to kubelet')
kubelet_opts['feature-gates'] = 'Accelerators=true'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to test that the GPU functionality still works with this change. I don't think our automated tests cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested with a GPU worker. This worked how I would expect, so we should be good.

@Cynerva
Copy link
Contributor Author

Cynerva commented Nov 8, 2017

/assign @ktsakalozos

Can you review this please? This should fix the bug that our CI tests are failing on. Thanks!

@ktsakalozos
Copy link
Contributor

Less code, less room for bugs. +1

@ktsakalozos
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017
@ktsakalozos
Copy link
Contributor

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cynerva, ktsakalozos

Associated issue requirement bypassed by: Cynerva

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55044, 55257, 55334). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit ce910f2 into kubernetes:master Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants