-
Notifications
You must be signed in to change notification settings - Fork 176
feat(helm): add affinity and tolerations to epp-deployment #1504
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
Conversation
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Welcome @hhk7734! |
Hi @hhk7734. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
Hi @hhk7734, can you please explain the motivation or use case for this change? |
I think this is for guiding the EPP image to land on specific nodes, which makes sense. We probably want this |
I created node groups based on their intended purpose and added labels and taints. To place epp in the desired groups, I added affinity and tolerations. |
yeah, makes sense. |
/ok-to-test |
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.
overall looks good.
I left just a few nits.
in addition, can you also add the relevant documentation here: https://github.com/kubernetes-sigs/gateway-api-inference-extension/tree/main/config/charts/inferencepool
- name: plugins-config-volume | ||
configMap: | ||
name: {{ include "gateway-api-inference-extension.name" . }} | ||
{{- with .Values.inferenceExtension.affinity }} |
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 we replace the “with” with “if”?
{{- with .Values.inferenceExtension.affinity }} | |
{{- if .Values.inferenceExtension.affinity }} |
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.
.Values.inferenceExtension.extraContainerPorts
and .Values.inferenceExtension.env
also use with
. Since this follows the same pattern, I’d suggest keeping it as is. Do you agree?
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.
To be honest, I’d change those to using “if” as well. I less like the use of “placeholders” and empty vars in the values file.
I personally prefer to have these options well documented and removed when not used.
I’ll defer to @kfswain to get his opinion.
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.
d6f2bcc
I modified it to use if
instead of with
.
affinity: | ||
{{- toYaml . | nindent 8 }} | ||
{{- end }} | ||
{{- with .Values.inferenceExtension.tolerations }} |
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.
ditto
{{- with .Values.inferenceExtension.tolerations }} | |
{{- if .Values.inferenceExtension.tolerations }} |
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 here as well.
- name: v | ||
value: 1 | ||
|
||
affinity: {} |
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.
and then we can remove empty values of affinity and tolerations from values.yaml
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 it should remain to show that affinity
and tolerations
can be set.
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
Signed-off-by: Hyeonki Hong <hhk7734@gmail.com>
@hhk7734 looks great! |
I only tested it with Helm templating, like the example below. test-values.yaml inferenceExtension:
affinity:
nodeAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
nodeSelectorTerms:
- matchExpressions:
- key: kubernetes.io/arch
operator: In
values:
- amd64 helm template gaie ./config/charts/inferencepool -f test-values.yaml > test.yaml test.yaml # ...
volumes:
- name: plugins-config-volume
configMap:
name: gaie-epp
affinity:
nodeAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
nodeSelectorTerms:
- matchExpressions:
- key: kubernetes.io/arch
operator: In
values:
- amd64
---
# ... |
/lgtm Thanks @hhk7734! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hhk7734, nirrozenbaum 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds support for configuring
affinity
andtolerations
in the epp-deployment chart template.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: