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

Allow InstanceGroups to override Kubelet config #1931

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
5 participants
@mzsanford
Contributor

mzsanford commented Feb 16, 2017

This allows the option of altering kubelet DAEMON_ARGS on a per-InstanceGroup basis. The core use case is currently the ability to maintain an InstanceGroup with nvidiaGPUs enabled without requiring the whole cluster to meet that requirement.


This change is Reviewable

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Feb 16, 2017

Contributor

Hi @mzsanford. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions 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. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Feb 16, 2017

Hi @mzsanford. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions 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. I understand the commands that are listed here.

@geojaz geojaz self-assigned this Feb 16, 2017

@geojaz geojaz self-requested a review Feb 16, 2017

@geojaz

This comment has been minimized.

Show comment
Hide comment
@geojaz

geojaz Feb 16, 2017

Member

@k8s-bot ok to test

Member

geojaz commented Feb 16, 2017

@k8s-bot ok to test

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Feb 16, 2017

Member

This is good, but it definitely has deeper implications that I want to consider!

Your use-case is for GPUs, right? Do you think we should auto-detect GPUs and pass the flag to kubelet instead?

Member

justinsb commented Feb 16, 2017

This is good, but it definitely has deeper implications that I want to consider!

Your use-case is for GPUs, right? Do you think we should auto-detect GPUs and pass the flag to kubelet instead?

@mzsanford

This comment has been minimized.

Show comment
Hide comment
@mzsanford

mzsanford Feb 16, 2017

Contributor

I considered using the presence of /dev/nvidia0 to set this using some shell-fu like --experimental-nvidia-gpus=$(ls -l /dev/nvidia0 2>/dev/null | wc -l) but I opted to try and build on #1728 after seeing that.

The /dev/nvidia0 as feature detection seems like an equally valid approach but I wasn't sure how portable that command would need to be.

Contributor

mzsanford commented Feb 16, 2017

I considered using the presence of /dev/nvidia0 to set this using some shell-fu like --experimental-nvidia-gpus=$(ls -l /dev/nvidia0 2>/dev/null | wc -l) but I opted to try and build on #1728 after seeing that.

The /dev/nvidia0 as feature detection seems like an equally valid approach but I wasn't sure how portable that command would need to be.

@justinsb justinsb modified the milestone: 1.5.3 Feb 23, 2017

@mzsanford

This comment has been minimized.

Show comment
Hide comment
@mzsanford

mzsanford Feb 27, 2017

Contributor

I did another branch with the /dev/nvidia0 approach at master...mzsanford:nvidia_auto_detect but it does not feel quite as clean to me since NvidiaGPUs is an int field. Happy to close this and submit the other if it would be preferred.

Contributor

mzsanford commented Feb 27, 2017

I did another branch with the /dev/nvidia0 approach at master...mzsanford:nvidia_auto_detect but it does not feel quite as clean to me since NvidiaGPUs is an int field. Happy to close this and submit the other if it would be preferred.

@macat

This comment has been minimized.

Show comment
Hide comment
@macat

macat Mar 14, 2017

This change would be great generally as it would allow slightly different configuration for instance groups.

I'd love to see this merged.

(BTW, Kubernetes v1.6 relies on Accelerators flag to turn on gpu support, so kops will need to change when that's released.)

macat commented Mar 14, 2017

This change would be great generally as it would allow slightly different configuration for instance groups.

I'd love to see this merged.

(BTW, Kubernetes v1.6 relies on Accelerators flag to turn on gpu support, so kops will need to change when that's released.)

@justinsb

LGTM

@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Mar 21, 2017

Member

@mzsanford I've decided - after an overly long period of deliberation that I can only apologize for - that you are right. This meshes nicely in with where componentconfig is going (I believe), and the GPU discussion is orthogonal. Even if GPU discovery is completely automatic in 1.6 (fingers crossed), we would still want the capability to specify kubelet options per instance group.

Anything else you want to tweak or are you OK if I merge it?

Member

justinsb commented Mar 21, 2017

@mzsanford I've decided - after an overly long period of deliberation that I can only apologize for - that you are right. This meshes nicely in with where componentconfig is going (I believe), and the GPU discussion is orthogonal. Even if GPU discovery is completely automatic in 1.6 (fingers crossed), we would still want the capability to specify kubelet options per instance group.

Anything else you want to tweak or are you OK if I merge it?

@mzsanford

This comment has been minimized.

Show comment
Hide comment
@mzsanford

mzsanford Mar 21, 2017

Contributor

I'm good with merging this

Contributor

mzsanford commented Mar 21, 2017

I'm good with merging this

@justinsb justinsb merged commit 8712a72 into kubernetes:master Mar 22, 2017

3 checks passed

Jenkins Kubernetes AWS e2e Build succeeded.
Details
cla/linuxfoundation mzsanford authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@justinsb

This comment has been minimized.

Show comment
Hide comment
@justinsb

justinsb Mar 22, 2017

Member

Thanks @mzsanford - and thanks for the patience also!

Member

justinsb commented Mar 22, 2017

Thanks @mzsanford - and thanks for the patience also!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment