Don't overcommit memory #913
Conversation
WalkthroughResource templates for multiple applications were updated to remove CPU limits and set memory limits equal to memory requests. Memory units were standardized, and some CPU and memory request values were adjusted. Chart versions were incremented for affected applications. The versions map was updated to reflect new commit hashes and HEAD pointers. No changes were made to exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm Chart
participant Kubernetes
User->>Helm Chart: Deploy app with resource preset
Helm Chart->>Kubernetes: Apply resource manifest
Kubernetes->>Kubernetes: Schedule pod with requests and limits
Note right of Kubernetes: CPU limits omitted,\nmemory limits = requests
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
82fc93c to
0164d27
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps/redis/templates/_resources.tpl (1)
40-41: Standardized 2xlarge preset memory units and removed CPU limits.This change completes the consistent application of the new resource configuration pattern across all presets. The 8Gi non-burstable memory allocation is appropriate for the largest tier.
Consider documenting the CPU limits removal rationale.
While the changes are sound, it might be helpful to add a comment explaining the architectural decision to remove CPU limits (to prevent throttling) and match memory requests/limits (to prevent OOM kills). This would help future maintainers understand the reasoning behind these configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/apps/clickhouse/templates/_resources.tpl(1 hunks)packages/apps/ferretdb/templates/_resources.tpl(1 hunks)packages/apps/http-cache/templates/_resources.tpl(1 hunks)packages/apps/kafka/templates/_resources.tpl(1 hunks)packages/apps/kubernetes/templates/_resources.tpl(1 hunks)packages/apps/mysql/templates/_resources.tpl(1 hunks)packages/apps/nats/templates/_resources.tpl(1 hunks)packages/apps/postgres/templates/_resources.tpl(1 hunks)packages/apps/rabbitmq/templates/_resources.tpl(1 hunks)packages/apps/redis/templates/_resources.tpl(1 hunks)packages/apps/tcp-balancer/templates/_resources.tpl(1 hunks)packages/apps/vpn/templates/_resources.tpl(1 hunks)packages/system/dashboard/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (29)
packages/apps/mysql/templates/_resources.tpl (1)
17-41: CPU limits removed and memory resources standardizedThese changes implement non-burstable memory allocation by setting memory limits equal to requests, remove CPU limits entirely, and standardize on Gi units for larger memory allocations. This aligns with Kubernetes best practices to avoid CPU throttling while maintaining predictable memory allocation.
packages/apps/rabbitmq/templates/_resources.tpl (1)
17-41: Resource configuration correctly updatedThe changes to RabbitMQ's resource presets follow the same pattern as the other applications - removing CPU limits, matching memory limits to requests, and standardizing memory units. This consistent approach across all applications will help prevent unexpected throttling and memory overcommitment issues.
packages/apps/tcp-balancer/templates/_resources.tpl (1)
17-41: Resource presets properly aligned with best practicesThe TCP balancer resource presets have been updated following the same pattern as other applications. CPU limits are removed to prevent throttling, memory allocations use consistent units, and memory limits match requests exactly to avoid overcommitment.
packages/apps/kubernetes/templates/_resources.tpl (1)
17-41: Resource configuration correctly modifiedKubernetes resource presets have been consistently updated to follow the same pattern as other applications - eliminating CPU limits to prevent throttling, ensuring memory requests and limits match exactly for predictable allocation, and using standardized units for clarity.
packages/apps/clickhouse/templates/_resources.tpl (3)
17-17: Improved resource handling by removing CPU limitsThe removal of CPU limits follows Kubernetes best practices and prevents potential CPU throttling. CPU is a compressible resource, and removing limits avoids unexpected performance issues while still maintaining control through requests.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Better resource scaling with updated CPU requests and standardized memory unitsThe changes to CPU requests (500m → 500m → 1 → 2 → 4) create a more meaningful differentiation between preset sizes. Converting memory values to Gi units improves readability and consistency.
Also applies to: 32-33, 36-37, 40-41
17-17: Non-burstable memory configuration helps prevent OOM issuesSetting memory limits equal to requests creates a non-burstable configuration, which ensures pods get exactly what they need and prevents unexpected OOM kills due to overcommitment. This is particularly important for database applications like ClickHouse.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
packages/apps/ferretdb/templates/_resources.tpl (3)
17-17: Improved resource handling by removing CPU limitsRemoving CPU limits follows Kubernetes best practices and prevents potential CPU throttling. This allows FerretDB to use additional CPU when available while still maintaining control through requests.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Better resource scaling with updated CPU requests and standardized memory unitsThe changes to CPU requests create a clearer progression between preset sizes (500m → 500m → 1 → 2 → 4), allowing better horizontal scaling decisions. The standardization to Gi units for larger memory values improves consistency.
Also applies to: 32-33, 36-37, 40-41
17-17: Non-burstable memory configuration prevents memory overcommitmentSetting memory limits equal to requests creates predictable memory behavior and prevents the system from overcommitting memory, which is crucial for database workloads like FerretDB that can experience memory spikes.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
packages/apps/postgres/templates/_resources.tpl (3)
17-17: Improved resource handling by removing CPU limitsRemoving CPU limits aligns with Kubernetes best practices and prevents potential CPU throttling issues. For database workloads like PostgreSQL, this provides better performance when CPU capacity is available.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Better resource scaling with updated CPU requests and standardized memory unitsThe updated CPU request progression (500m → 500m → 1 → 2 → 4) creates clearer differentiation between preset sizes. PostgreSQL performance scales well with additional CPU cores, so this progression makes sense for larger workloads.
Also applies to: 32-33, 36-37, 40-41
17-17: Non-burstable memory configuration critical for database stabilitySetting memory limits equal to requests is particularly important for PostgreSQL, as it relies on predictable memory behavior for operations like query execution and caching. This non-burstable configuration helps prevent OOM events that could lead to database restarts.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
packages/apps/vpn/templates/_resources.tpl (3)
17-17: Improved resource handling by removing CPU limitsRemoving CPU limits aligns with Kubernetes best practices and allows the VPN service to utilize additional CPU when available, which can be important for handling encryption/decryption tasks during peak loads.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Better resource scaling with updated CPU requests and standardized memory unitsThe updated CPU request progression (500m → 500m → 1 → 2 → 4) provides clearer differentiation between preset sizes, which is helpful for scaling VPN services based on expected concurrent connections.
Also applies to: 32-33, 36-37, 40-41
17-17: Non-burstable memory configuration ensures VPN service stabilitySetting memory limits equal to requests creates a non-burstable configuration that prevents memory overcommitment. For VPN services, predictable memory behavior is important to maintain connection state and avoid unexpected disconnections.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
packages/apps/kafka/templates/_resources.tpl (2)
17-17: Removed CPU limits from all resource presetsThe changes remove CPU limits from all presets, keeping only memory and ephemeral-storage limits. This change aligns with the PR objective of eliminating CPU limits while making memory allocation non-burstable.
This is a good practice in Kubernetes as CPU limits can sometimes cause throttling issues, while keeping them unlimited allows for better CPU utilization when capacity is available.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Standardized memory units to Gi notationChanged memory unit representation from "1024Mi" to "1Gi" for better readability, while ensuring memory limits match requests exactly (non-burstable).
packages/apps/http-cache/templates/_resources.tpl (2)
17-17: Removed CPU limits from all resource presetsCPU limits have been removed from all presets while keeping memory and ephemeral-storage limits. This is consistent with the approach taken in other templates and supports the PR objective to prevent overcommitting resources.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
28-29: Standardized memory units and made memory non-burstableMemory units have been standardized to use "1Gi" instead of "1024Mi" for better readability. Memory limits now exactly match memory requests, implementing non-burstable memory allocation.
packages/apps/nats/templates/_resources.tpl (2)
17-17: Removed CPU limits from all resource presetsCPU limits have been removed from all resource presets, which is consistent with the changes in other templates and the PR objective.
Also applies to: 21-21, 25-25, 29-29, 33-33, 37-37, 41-41
32-33: Adjusted CPU requests for larger presets and standardized memory allocationThe CPU requests for larger presets have been increased (large: 1, xlarge: 2, 2xlarge: 4), which provides better differentiation between preset sizes. Memory units are standardized and limits match requests exactly (non-burstable).
Also applies to: 36-37, 40-41
packages/system/dashboard/values.yaml (1)
24-32: Added Redis master resource configuration with non-burstable memoryNew configuration added for the Redis master with explicit resource requests and limits:
- CPU request of 200m (no CPU limit, consistent with other changes)
- Memory request and limit both set to 256Mi (non-burstable)
This addition makes the dashboard Redis configuration consistent with the resource management approach implemented across other components.
packages/apps/redis/templates/_resources.tpl (6)
17-17: Removed CPU limits in nano preset.The removal of CPU limits aligns with modern Kubernetes best practices. CPU limits can cause throttling, which often leads to unexpected performance issues, especially in Redis which is sensitive to latency spikes. By removing CPU limits while keeping requests, you allow for CPU bursting during peak loads without throttling.
21-21: Removed CPU limits in micro preset.This change is consistent with the removal of CPU limits across all presets. For Redis workloads, this is particularly beneficial as it allows better handling of temporary spikes in CPU usage without throttling.
25-25: Removed CPU limits in small preset.Consistent with the preceding changes. Removing CPU limits for Redis is a good approach as it prevents potential performance degradation due to CPU throttling.
28-29: Increased medium preset memory and removed CPU limits.The memory increase from 512Mi to 1Gi creates better differentiation between the small and medium tiers. The non-burstable memory allocation (request equals limit) prevents potential OOM issues that can be especially problematic for Redis.
32-33: Standardized large preset memory units and removed CPU limits.The change to express memory in Gi units improves readability and is more consistent with industry standards for expressing larger memory allocations. This change maintains the logical progression in the resource tier sizes.
36-37: Standardized xlarge preset memory units and removed CPU limits.Consistent with other presets, using Gi units for larger memory allocations improves readability. The non-burstable memory configuration is particularly important for Redis to ensure predictable performance.
0b2ec36 to
d9145d5
Compare
This patch recreates the resource presets with a non-burstable memory allocation (request==limit) and without CPU limits. With the new presets the difference between the larger presets became meaningless, so their values were adjusted. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
d9145d5 to
db34f31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
21-21: Improve hook metadata to reflect diff displayThe hook now prints diffs after generation, but neither the
namenor thedescriptionmentions this behavior. Consider updating them, for example:name: Run 'make generate' in all app directories and show diffs description: Execute code generation and display any changes with color highlighting
21-21: Simplify diff command using Git’s no-pager optionInstead of piping
git diff --color=always | cat, you can leverage Git’s built-in pager override:git --no-pager diff --colorThis preserves ANSI color codes without spawning an extra
catprocess.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.pre-commit-config.yaml(1 hunks)packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/_resources.tpl(1 hunks)packages/apps/ferretdb/Chart.yaml(1 hunks)packages/apps/ferretdb/templates/_resources.tpl(1 hunks)packages/apps/http-cache/Chart.yaml(1 hunks)packages/apps/http-cache/templates/_resources.tpl(1 hunks)packages/apps/kafka/Chart.yaml(1 hunks)packages/apps/kafka/templates/_resources.tpl(1 hunks)packages/apps/kubernetes/templates/_resources.tpl(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/mysql/templates/_resources.tpl(1 hunks)packages/apps/nats/Chart.yaml(1 hunks)packages/apps/nats/templates/_resources.tpl(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/templates/_resources.tpl(1 hunks)packages/apps/rabbitmq/Chart.yaml(1 hunks)packages/apps/rabbitmq/templates/_resources.tpl(1 hunks)packages/apps/redis/Chart.yaml(1 hunks)packages/apps/redis/templates/_resources.tpl(1 hunks)packages/apps/tcp-balancer/Chart.yaml(1 hunks)packages/apps/tcp-balancer/templates/_resources.tpl(1 hunks)packages/apps/versions_map(6 hunks)packages/apps/vpn/Chart.yaml(1 hunks)packages/apps/vpn/templates/_resources.tpl(1 hunks)packages/system/dashboard/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/apps/mysql/templates/_resources.tpl
- packages/apps/kafka/Chart.yaml
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/apps/nats/Chart.yaml
- packages/apps/tcp-balancer/Chart.yaml
- packages/apps/http-cache/Chart.yaml
- packages/apps/clickhouse/Chart.yaml
- packages/apps/redis/Chart.yaml
- packages/apps/postgres/Chart.yaml
- packages/apps/mysql/Chart.yaml
- packages/apps/vpn/Chart.yaml
- packages/apps/ferretdb/Chart.yaml
- packages/apps/http-cache/templates/_resources.tpl
- packages/apps/clickhouse/templates/_resources.tpl
- packages/apps/ferretdb/templates/_resources.tpl
- packages/apps/postgres/templates/_resources.tpl
- packages/apps/redis/templates/_resources.tpl
- packages/apps/vpn/templates/_resources.tpl
- packages/apps/rabbitmq/Chart.yaml
- packages/apps/kafka/templates/_resources.tpl
- packages/apps/nats/templates/_resources.tpl
- packages/system/dashboard/values.yaml
- packages/apps/kubernetes/templates/_resources.tpl
- packages/apps/rabbitmq/templates/_resources.tpl
- packages/apps/tcp-balancer/templates/_resources.tpl
- packages/apps/versions_map
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
This patch recreates the resource presets with a non-burstable memory allocation (request==limit) and without CPU limits. With the new presets the difference between the larger presets became meaningless, so their values were adjusted. Resolves #912 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Updated resource presets across all application charts to remove CPU limits, align memory limits with requests, and standardize memory units for consistency. - Adjusted CPU and memory request values for larger presets in several applications. - Updated chart versions for all affected applications. - Refreshed version mappings to reflect latest commit hashes. - Added explicit resource configuration for Redis in the dashboard configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 2c6338a) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
This patch recreates the resource presets with a non-burstable memory allocation (request==limit) and without CPU limits. With the new presets the difference between the larger presets became meaningless, so their values were adjusted.
Resolves #912
Summary by CodeRabbit