-
Notifications
You must be signed in to change notification settings - Fork 558
add GPU ExtendedResourceToleration admission controller support #3181
add GPU ExtendedResourceToleration admission controller support #3181
Conversation
This reverts commit c5594e7.
@@ -137,6 +137,9 @@ write_files: | |||
KUBELET_IMAGE={{WrapAsVariable "kubernetesHyperkubeSpec"}} | |||
KUBELET_REGISTER_SCHEDULABLE=true | |||
KUBELET_NODE_LABELS={{GetAgentKubernetesLabels . "',variables('labelResourceGroup'),'"}} | |||
{{if IsNVIDIADevicePluginEnabled}} |
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 think this needs to be wrapped with {{if IsNSeriesSKU .}}
so it won't apply to non-GPU pools
parts/k8s/kubernetesbase.t
Outdated
@@ -41,6 +41,9 @@ | |||
{{range $index, $agent := .AgentPoolProfiles}} | |||
"{{.Name}}Index": {{$index}}, | |||
{{template "k8s/kubernetesagentvars.t" .}} | |||
{{if IsNVIDIADevicePluginEnabled }} |
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.
same as above
Codecov Report
@@ Coverage Diff @@
## master #3181 +/- ##
==========================================
- Coverage 54.55% 54.55% -0.01%
==========================================
Files 104 104
Lines 15753 15758 +5
==========================================
+ Hits 8594 8596 +2
- Misses 6409 6411 +2
- Partials 750 751 +1 |
@@ -137,7 +137,7 @@ write_files: | |||
KUBELET_IMAGE={{WrapAsVariable "kubernetesHyperkubeSpec"}} | |||
KUBELET_REGISTER_SCHEDULABLE=true | |||
KUBELET_NODE_LABELS={{GetAgentKubernetesLabels . "',variables('labelResourceGroup'),'"}} | |||
{{if IsNVIDIADevicePluginEnabled}} | |||
{{if IsNSeriesSKU .}} |
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.
can you keep both of these checks? we want this to be enabled per GPU node and when NVIDIA device plugin enabled, but not enabled with k8s 1.8, 1.9 etc since were are not supporting device plugin in those version.
so it'll look something like:
{{if IsNSeriesSKU .}}
{{if IsNVIDIADevicePluginEnabled}}
...
{{end}}
{{end}}
parts/k8s/kubernetesbase.t
Outdated
@@ -41,7 +41,7 @@ | |||
{{range $index, $agent := .AgentPoolProfiles}} | |||
"{{.Name}}Index": {{$index}}, | |||
{{template "k8s/kubernetesagentvars.t" .}} | |||
{{if IsNVIDIADevicePluginEnabled }} | |||
{{if IsNSeriesSKU .}} |
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.
same as above
- key: nvidia.com/gpu | ||
effect: NoSchedule | ||
operator: Equal | ||
value: "true" |
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.
Since you are adding the toleration, can you also replace the snippet from below at /etc/docker/daemon.json"
in kubernetesagentcustomdata.yml
since we want it to be enabled for GPU pools only but not for CPU pools.
}{{if IsNSeriesSKU .}}{{if IsNVIDIADevicePluginEnabled}}
,"default-runtime": "nvidia",
"runtimes": {
"nvidia": {
"path": "/usr/bin/nvidia-container-runtime",
"runtimeArgs": []
}
}{{end}}{{end}}
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.
Fixed
BUGFIX - Update docker runtime on NON GPU enabled machines Add accelerator label to GPU enabled nodes Add nodeselector label match where accelerator=nvidia for nvidia-device-plugin
@sozercan this should be gtg. Tested against mixed gpu/cpu agent pool confirmed it works. |
Actually. It looks like DNS resolution to kube-dns from a POD running on a GPU machine.
|
Okay I've fixed that issue. I hadn't added the toleration to kube-proxy so that it runs on the gpu nodes. |
Tested that it runs correctly without the host volume mounts in the pod spec.
|
Let me also review the GPU docs to make sure they are good with these changes. |
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.
Just tested, LGTM! 👍
/assign @jackfrancis |
@@ -34,14 +34,14 @@ write_files: | |||
"log-opts": { | |||
"max-size": "50m", | |||
"max-file": "5" | |||
}{{if IsNVIDIADevicePluginEnabled}} | |||
}{{if IsNSeriesSKU .}}{{if IsNVIDIADevicePluginEnabled}} |
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 figure out how to simplify these guards.
- are there scenarios where we don't want to install the nvidia device plugin on an N Series VM SKU node?
- are there scenarios where we would ever want to install the plugin on a non-N Series VM SKU node?
I infer the answer to #2 is "no" based on the way this has been implemented here. For #1, if the answer to that is also "no", then we should combine these two checks: basically, we should get rid of IsNVIDIADevicePluginEnabled, or we should have IsNVIDIADevicePluginEnabled only return true if it's on an N Series node.
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.
Update file name to reflect nvidia-gpu Convert nodeSelector to affinity
Remove gpu from the resource name to fall in line with upstream
pkg/acsengine/k8s_versions.go
Outdated
"ratelimitbucket": k8sComponentVersions["1.11"]["ratelimitbucket"], | ||
"gchighthreshold": k8sComponentVersions["1.11"]["gchighthreshold"], | ||
"gclowthreshold": k8sComponentVersions["1.11"]["gclowthreshold"], | ||
DefaultNVIDIADevicePluginAddonName: k8sComponentVersions["1.11"]["nvidia-device-plugin"], |
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.
just curious, does NDP v1.10 work in k8s 1.11? when i tested NDP v1.9 with k8s 1.10 a while back, it didn't work
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.
It appears to work the same way it does in 1.10
Any update on the status on this PR? Hybrid gpu/cpu cluster is still broken due to #3192 |
@zzh8829 fixing this up this morning and should have it ready to merge |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, lachie83 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 |
Use-case - Users only want workloads that require GPU resources to be scheduled to nodes that have GPUs.
Adds the following support:
nvidia.com/gpu
to be auto-tolerated by the admission controller to enable scheduling to the GPU node pool(s) - https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#extendedresourcetoleration