Skip to content

chore: Normalize how resources are set for each container #175

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

Merged
merged 11 commits into from
Apr 11, 2025

Conversation

lindhe
Copy link
Contributor

@lindhe lindhe commented Apr 7, 2025

Description

This PR normalizes how we set resources for each container and initContainer, first by standardizing how we include the values in the templates and secondly by resouces to {} by default, aligned with Bitnami's standard.

Here's one example of Bitnami documenting their reasoning for it:

We usually recommend not to specify default resources and to leave this as a conscious choice for the user. This also increases chances charts run on environments with little resources, such as Minikube.

https://github.com/bitnami/charts/blob/7345c0bb2842b73f299aacd2af37147bd7b714cd/bitnami/nginx/values.yaml#L331-L334

We don't have to follow Bitnami's standard, but I think it's reasonable to do so. Unless we are very confident that a container has some upper/lower bound on its resource utilization, it's probably best to condition the chart user to always set the resources explicitly.

Considering how Kubernetes hide the exact capacity of CPU cores etc., it's pretty much impossible to set resource requests/limits that work the same for everyone. This strategy also reduces the need to make additional changes when updating the chart's default images in the future.

Fixes #109

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

@lindhe lindhe requested review from egabancho and Copilot April 7, 2025 12:52
@lindhe lindhe self-assigned this Apr 7, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

charts/invenio/templates/worker-beat-deployment.yaml:91

  • Unconditionally rendering an empty resources block may lead to unexpected behavior if Kubernetes does not accept an empty mapping. Consider conditionally rendering the resources block only when the provided value is non-empty.
resources: {{- toYaml .Values.workerBeat.resources | nindent 10 }}

@lindhe
Copy link
Contributor Author

lindhe commented Apr 7, 2025

There may be additional initContainers that I have not considered. But I think those has to be postponed until later, because it's practically impossible to work with initContainers in the chart right now. The chart has to be restructured w.r.t. initContainers before it makes any sense to spend time on them, in my opinion.

@lindhe lindhe marked this pull request as ready for review April 7, 2025 13:30
Copy link
Member

@egabancho egabancho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to questions:

  • Could we comment similarly to the one you linked on the nginx chart? We usually recommend not to specify default resources and to leave this as a conscious...
  • Since we are, essentially, removing default values for resources, do you think there is a way to warn users about this change? Other than release notes ☺️

@lindhe
Copy link
Contributor Author

lindhe commented Apr 7, 2025

  • Could we comment similarly to the one you linked on the nginx chart? We usually recommend not to specify default resources and to leave this as a conscious...

Excellent suggestion! I'll see to it.

  • Since we are, essentially, removing default values for resources, do you think there is a way to warn users about this change? Other than release notes ☺️

Oh yeah, right. That. 🫤 I guess we could add a warning in NOTES.txt that shows up if resources are unset. I don't think I've seen that before, and it might be a bit annoying to maintain over time, but I think that would provide a pretty good help to users. Sounds reasonable?

@lindhe lindhe force-pushed the lindhe/resources branch from e0f84bb to e390271 Compare April 8, 2025 13:13
@lindhe
Copy link
Contributor Author

lindhe commented Apr 8, 2025

@egabancho I've updated the comments now, so I think that's been resolved.

I've also added some logic to NOTES for printing a warning if resources is unset for any of our internal resources (not dependencies; I'm leaving that for Bitnami to implement). Please take a look and see if you like the solution. It turned out to be more complex that I first thought it would be, so maybe I've missed something obvious.

Resulting message looks like this:

image

@lindhe lindhe requested a review from egabancho April 8, 2025 13:18
@egabancho
Copy link
Member

LGTM!

@lindhe lindhe merged commit 69c204f into master Apr 11, 2025
1 check passed
@lindhe lindhe deleted the lindhe/resources branch April 11, 2025 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configurable resources for deployments
2 participants