-
Notifications
You must be signed in to change notification settings - Fork 542
Add GPU support to kubernetes_scale on EKS Karpenter #6313
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
base: master
Are you sure you want to change the base?
Add GPU support to kubernetes_scale on EKS Karpenter #6313
Conversation
| name: {{ Name }} | ||
| spec: | ||
| {%- if NvidiaGpuRequest and Cloud == 'aws' %} | ||
| nodeSelector: |
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.
Zach recently changed how the nodeSelectors area applied. Take a look at #6291, and in particular, the (new) GetNodeSelectors() methods in the providers. (e.g. within providers/aws/elastic_kubernetes_service.py).
That said, your nodeSelector here refers back to your aws-gpu-nodepool.yaml.j2 file, so I think the new GetNodeSelectors() method probably doesn't apply - keeping this in the yaml.j2 file seems like the right approach here.
No action required (though take a quick look at that #6291.)
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 the pointer, I checked #6291.
This does not apply in our case, so you are right, no changes are needed here. Thanks.
| operator: "Exists" | ||
| effect: "NoExecute" | ||
| tolerationSeconds: {{ PodTimeout }} | ||
| {%- if NvidiaGpuRequest and Cloud == 'aws' %} |
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.
Similarly, that same PR introduced ModifyPodSpecPlacyementYaml method, which seems like it could apply here. But again, this block refers back to your nodepool yaml.j2 file, so I think you should probably leave it as is.
No action required.
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 the pointer and the context.
| ) | ||
|
|
||
| if is_eks_karpenter_aws_gpu: | ||
| cluster.ApplyManifest( |
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.
NB: Because ScaleUpPods is called twice, you'll end up applying this nodepool manifest twice. Which is fine - the second time k8s will notice it's already present and do nothing.
But perhaps we should put this into the Prepare() function instead? (And if that works, maybe we should put the first invocation of ScaleUpPods there too... though if you want to do that, then a separate PR might be better.)
No description provided.