-
Notifications
You must be signed in to change notification settings - Fork 681
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 specifying flyte pod webhook deployment resources #4988
Conversation
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
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.
LGTM, we could add reasonable resources.requests
but omit resources.limits
. Requests defaults but are not immediately necessary. It may make more sense to have a single issue/PR set reasonable defaults missing for all workloads so folks can track what's changed more easily.
@@ -103,6 +103,9 @@ spec: | |||
allowPrivilegeEscalation: false | |||
capabilities: | |||
drop: ["ALL"] | |||
{{- if .Values.webhook.resources -}} |
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.
nit: Not important in this case but we could also do the following.
{{- with .Values.webhook.resources }}
resources: {{- toYaml . | nindent 12 }}
{{- end }}
This is not really that important as it's just syntactic difference, only really benefits for more complex config. We can always update later.
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 @mhotan updated to address both your comments, although having a hard time reasoning about what sensible defaults would be here esp given the recent ephemeral storage incident. figure it might be safer to err on overprovisioning for now
Signed-off-by: Katrina Rogan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4988 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 645 645
Lines 55562 55562
=======================================
Hits 32767 32767
Misses 20200 20200
Partials 2595 2595
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Katrina Rogan <[email protected]>
Why are the changes needed?
Various resources recommend setting resources and limits for k8s deployments, e.g: https://www.cncf.io/blog/2022/10/20/kubernetes-best-practice-how-to-correctly-set-resource-requests-and-limits/
What changes were proposed in this pull request?
Optionally allows specifying flyte pod deployment resource requests and limits.
How was this patch tested?
In values.yaml
verified the rendered webhook deployment included
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link