Add explicit resource blocks for managed workloads#32
Conversation
Explicitly set the resource block on all managed workloads (default empty on all to avoid breaking changes). Also allow setting the resource values through the Values file for all managed workloads: - DB init job - Grafana deployment - Prometheus deployment - PGWatch deployment - The Metrics Database backend
There was a problem hiding this comment.
Overall, the PR looks good to me, thank you @adrianeliasson .
One small nitpick about the naming conventions, variables start with lowercase letters.
Other than that, if you want, you can add low values you mentioned in your comment as commented-out example suggestions for init jobs and initContainers. However, I'd rather not add active defaults or recommendations for the main workloads such as PostgreSQL, Prometheus, Grafana, or Pgwatch itself.
Resource requirements vary significantly depending on workload size, scrape interval, number of monitored DBs, retention settings, dashboards, etc. In my opinion, bad defaults are worse than no defaults. Because of that, I would suggest keeping the scope of this implementation focused on providing the functionality, while leaving actual resource sizing decisions to users.
Could you also add a short section to the README.md describing how to configure resource blocks for the built-in workloads, so users are aware of this functionality?
After your fixes, I think the PR is good to go. Thanks again for your work. 👏
| size: '10Gi' | ||
| storageClass: 'standard' | ||
| # -- Resource requests and limits for the Metrics Database init job. | ||
| DbInitJob: |
There was a problem hiding this comment.
| DbInitJob: | |
| dbInitJob: |
According to Helm Naming Conventions:
Variable names should begin with a lowercase letter, and words should be separated with camelcase:
There was a problem hiding this comment.
Thanks for pointing it out, fixed!
|
Hi @adrianeliasson 👋 I'd be happy to include this in the next release once the PR is ready to merge. If you need anything, please don't hesitate to nudge :) |
- Adds a readme entry detailing how to manually set resource limits and requests - Adds example resource requests and limit for init jobs - Use helm naming convention for all values names (lower camelcase)
…ource-block-for-managed-workloads
|
Hey @serdardalgic, thanks for your feedback. See changes:
Please let me know what you think :) |
serdardalgic
left a comment
serdardalgic
left a comment
There was a problem hiding this comment.
Thanks @adrianeliasson , that's solid, LGTM 👍
Once the Changelog entry is in the right place, I'd happily approve the PR and merge into main.
| ### Added | ||
|
|
||
| - `seccompProfile.type: RuntimeDefault` in global `securityContext.pod` defaults when `securityContext.enabled=true` ([#31](https://github.com/cybertec-postgresql/pgwatch-charts/issues/31)). | ||
| - The ability to manually specify `resources` blocks on managed workloads.([#30](https://github.com/cybertec-postgresql/pgwatch-charts/issues/30)). |
There was a problem hiding this comment.
This should go to the Unreleased part of Changelog. 4.0.1 is already released.
There was a problem hiding this comment.
Ah, now I see it. Thanks!
There was a problem hiding this comment.
Now it should be in the right place.
serdardalgic
left a comment
serdardalgic
left a comment
There was a problem hiding this comment.
Great! Thank you very much for your contribution! 🎉
2 participants
Closes #30
Explicitly set the resource block on all managed workloads (default empty object on all workloads to avoid breaking changes). Also allow setting the resource values through the Values file for all managed workloads: