[redis] Auto-set maxmemory from resource limits#2339
[redis] Auto-set maxmemory from resource limits#2339officialasishkumar wants to merge 1 commit intocozystack:mainfrom
Conversation
Automatically compute Redis maxmemory as 80% of the pod memory resource limit (4/5 of the limit in bytes). This prevents OOM kills by ensuring Redis stays within its allocated memory, leaving 20% headroom for replication buffers, the exporter sidecar, and Redis internal overhead. The maxmemory value is derived from the resolved resources (either the explicit resources parameter or the selected resourcesPreset) using cozy-lib.resources.defaultingSanitize and toFloat helpers. Users can override the auto-computed value by setting the new maxmemory parameter explicitly. Fixes cozystack#2155 Signed-off-by: Asish Kumar <[email protected]>
📝 WalkthroughWalkthroughThe Redis Helm chart now includes automatic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces an optional 'maxmemory' configuration for Redis, which defaults to 80% of the memory resource limit if not explicitly set. The review feedback suggests refining the conditional logic in the template to correctly handle a value of '0', updating the JSON schema to support both integer and string types for the 'maxmemory' parameter to improve usability, and ensuring the documentation reflects these supported types.
| - appendonly no | ||
| - save "" | ||
| {{- end }} | ||
| {{- if .Values.maxmemory }} |
There was a problem hiding this comment.
Using if .Values.maxmemory will evaluate to false if the value is 0, causing the auto-computation to trigger even when an explicit 0 (unlimited) is desired. Comparing against an empty string is more robust for optional parameters that can accept numeric zero.
{{- if ne (toString .Values.maxmemory) "" }}| }, | ||
| "maxmemory": { | ||
| "description": "Override the auto-computed maxmemory (bytes). When omitted, maxmemory is set to 80% of the memory resource limit.", | ||
| "type": "string", |
There was a problem hiding this comment.
The maxmemory parameter is currently restricted to a string type. Allowing both integer and string (via anyOf) improves usability by allowing users to provide raw byte counts without quotes, while still supporting strings with units (e.g., '100mb'). This is consistent with how other quantity fields like size are defined in this schema.
| "type": "string", | |
| "anyOf": [ | |
| { "type": "integer" }, | |
| { "type": "string" } | |
| ], |
| ## @section Application-specific parameters | ||
| ## | ||
|
|
||
| ## @param {string} [maxmemory] - Override the auto-computed maxmemory (bytes). When omitted, maxmemory is set to 80% of the memory resource limit. |
There was a problem hiding this comment.
Update the parameter type documentation to reflect that both strings and integers are supported for the maxmemory override.
## @param {string|int} [maxmemory] - Override the auto-computed maxmemory (bytes). When omitted, maxmemory is set to 80% of the memory resource limit.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd44c569e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| "maxmemory": { | ||
| "description": "Override the auto-computed maxmemory (bytes). When omitted, maxmemory is set to 80% of the memory resource limit.", | ||
| "type": "string", |
There was a problem hiding this comment.
Allow numeric maxmemory overrides in values schema
Defining maxmemory as only "type": "string" makes common byte-style overrides fail schema validation when users provide an unquoted number in values.yaml (e.g., maxmemory: 536870912), even though the template itself can render numeric values. This effectively breaks the new override path for a natural input format; the schema should accept integers (or int-or-string) for this field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/apps/redis/templates/redisfailover.yaml (1)
67-73: Reuse resolved resources instead of recalculating insidecustomConfig.Line 70 recomputes
defaultingSanitizeeven though the same effective resources are already rendered earlier. Caching once improves readability and avoids future drift.♻️ Suggested refactor
apiVersion: databases.spotahome.com/v1 kind: RedisFailover metadata: name: {{ .Release.Name }} @@ spec: + {{- $resolvedResources := include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | fromYaml }} sentinel: replicas: 3 - resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} + resources: {{- $resolvedResources | toYaml | nindent 6 }} redis: image: "redis:{{ include "redis.versionMap" $ }}" - resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 6 }} + resources: {{- $resolvedResources | toYaml | nindent 6 }} @@ {{- if .Values.maxmemory }} - maxmemory {{ .Values.maxmemory }} {{- else }} - {{- $resolved := include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | fromYaml }} - {{- $memLimitBytes := include "cozy-lib.resources.toFloat" $resolved.limits.memory | float64 | int64 }} + {{- $memLimitBytes := include "cozy-lib.resources.toFloat" $resolvedResources.limits.memory | float64 | int64 }} - maxmemory {{ div (mul $memLimitBytes 4) 5 }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/apps/redis/templates/redisfailover.yaml` around lines 67 - 73, The template currently recomputes the sanitized resources inside the customConfig block by calling include "cozy-lib.resources.defaultingSanitize" again and deriving $memLimitBytes; instead, reuse the previously-resolved resources object (e.g. the existing $resolved or a renamed shared variable) and compute maxmemory from that single resolved.limits.memory to avoid duplication and drift; update the customConfig block to reference the shared $resolved and remove the second include and $memLimitBytes calculation so maxmemory uses div (mul (include "cozy-lib.resources.toFloat" $resolved.limits.memory | float64 | int64) 4) 5 (or equivalent) with the shared variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/apps/redis/templates/redisfailover.yaml`:
- Around line 67-73: The template currently recomputes the sanitized resources
inside the customConfig block by calling include
"cozy-lib.resources.defaultingSanitize" again and deriving $memLimitBytes;
instead, reuse the previously-resolved resources object (e.g. the existing
$resolved or a renamed shared variable) and compute maxmemory from that single
resolved.limits.memory to avoid duplication and drift; update the customConfig
block to reference the shared $resolved and remove the second include and
$memLimitBytes calculation so maxmemory uses div (mul (include
"cozy-lib.resources.toFloat" $resolved.limits.memory | float64 | int64) 4) 5 (or
equivalent) with the shared variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e18f094e-ef70-47ca-a4c6-c78786a7d7da
📒 Files selected for processing (3)
packages/apps/redis/templates/redisfailover.yamlpackages/apps/redis/values.schema.jsonpackages/apps/redis/values.yaml
What this PR does
Automatically sets Redis
maxmemoryto 80% of the pod's memory resource limit. This prevents OOM kills by ensuring Redis respects its container memory budget, leaving 20% headroom for replication buffers, the exporter sidecar, and Redis internal overhead.How it works:
resources.memoryparameter or the selectedresourcesPresetusing the existingcozy-lib.resources.defaultingSanitizeandtoFloathelpersmaxmemoryasmemoryLimitBytes * 4 / 5(integer arithmetic, equivalent to 80%)spec.redis.customConfigasmaxmemory <bytes>Override: Users can set an explicit
maxmemoryvalue (in bytes) to bypass the auto-computation.Examples by preset:
Fixes #2155
Release note
Summary by CodeRabbit
maxmemoryparameter for Redis memory limit management