make seaweedfs and nginx resources configurable - #1413
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds per-replica resource configuration with preset fallback to the ingress chart and bumps its chart version. Adds per-component VPA support (including S3 and per-zone Volume VPAs), templates, schema and docs updates for SeaweedFS, bumps its chart version, updates versions_map, and adjusts system seaweedfs values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Values
participant H as Helm Template (ingress)
participant L as cozy-lib.resources.defaultingSanitize
participant K as Kubernetes API
U->>H: Provide `resources` or `resourcesPreset`
H->>L: Request sanitized resources (preset vs explicit)
L-->>H: Sanitized resources object
H->>K: Render & apply ingress-nginx controller with resources
sequenceDiagram
autonumber
participant U as Values.vpa
participant H as Helm (seaweedfs/templates/vpa.yaml)
participant K as Kubernetes API
participant V as VPA Controller
U->>H: Provide vpa.{filer,master,s3,volume}.{minAllowed,maxAllowed}
H->>K: Create/Update filer, master, s3 VPAs (templated min/max)
alt zones defined
H->>K: Create VPA per zone targeting each volume StatefulSet
else no zones
H->>K: Create single volume VPA targeting global StatefulSet
end
V-->>K: VPA recommends/applies resource adjustments (updateMode: Auto)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Summary of Changes
Hello klinch0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the flexibility of resource management for key components within the system. It introduces new configuration options for the NGINX Ingress Controller, enabling users to specify CPU and memory allocations directly or via presets. Concurrently, it exposes VPA settings for SeaweedFS's Filer, Master, and Volume pods, allowing for fine-tuned control over their resource scaling behavior. These changes empower users with greater control over resource utilization and performance optimization for these critical services.
Highlights
- NGINX Ingress Controller Resource Configuration: The NGINX Ingress Controller now supports explicit CPU and memory resource configuration, allowing users to define custom resource requests and limits or utilize predefined sizing presets.
- SeaweedFS Vertical Pod Autoscaler (VPA) Configuration: SeaweedFS components (Filer, Master, and Volume) can now have their Vertical Pod Autoscaler (VPA) minimum and maximum allowed CPU and memory resources configured, providing more granular control over dynamic scaling.
- Chart Version Updates: The NGINX Ingress Controller Helm chart has been updated from version 1.8.0 to 1.9.0, and the SeaweedFS Helm chart has been updated from 0.6.0 to 0.7.0.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | Gemini (@gemini-code-assist) | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces resource configurability for the ingress-nginx and seaweedfs Helm charts. The changes for ingress-nginx allow setting CPU and memory resources via resources and resourcesPreset values, which is a good improvement for flexibility. The changes for seaweedfs make the Vertical Pod Autoscaler (VPA) resource limits configurable.
I've found several instances of copy-paste errors in descriptions within the ingress-nginx chart files, where they incorrectly refer to "PostgreSQL replica" instead of "ingress-nginx replica". This can be confusing for users and I've left comments to correct them.
Also, the PR title and release note mention nats, but there are no changes related to a NATS chart in this PR. This might be a mistake and should be corrected for clarity.
| | `replicas` | Number of ingress-nginx replicas | `int` | `2` | | ||
| | `whitelist` | List of client networks | `[]*string` | `[]` | | ||
| | `clouflareProxy` | Restoring original visitor IPs when Cloudflare proxied is enabled | `bool` | `false` | | ||
| | `resources` | Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied. | `*object` | `{}` | |
There was a problem hiding this comment.
The description for resources incorrectly refers to "PostgreSQL replica". This appears to be a copy-paste error. Since this is an ingress-nginx chart, the description should refer to ingress-nginx replicas.
| | `resources` | Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied. | `*object` | `{}` | | |
| | `resources` | Explicit CPU and memory configuration for each ingress-nginx replica. When left empty, the preset defined in `resourcesPreset` is applied. | `*object` | `{}` | |
| "default": 2 | ||
| }, | ||
| "resources": { | ||
| "description": "Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied.", |
There was a problem hiding this comment.
The description for resources mentions "PostgreSQL replica", which is incorrect for an ingress-nginx chart. This should be updated to refer to ingress-nginx replicas to avoid confusion.
| "description": "Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied.", | |
| "description": "Explicit CPU and memory configuration for each ingress-nginx replica. When left empty, the preset defined in `resourcesPreset` is applied.", |
| ## @param clouflareProxy {bool} Restoring original visitor IPs when Cloudflare proxied is enabled | ||
| clouflareProxy: false | ||
|
|
||
| ## @param resources {*resources} Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied. |
There was a problem hiding this comment.
The comment description for resources seems to have been copied from another chart, as it mentions "PostgreSQL replica". Please correct it to refer to ingress-nginx replicas.
## @param resources {*resources} Explicit CPU and memory configuration for each ingress-nginx replica. When left empty, the preset defined in `resourcesPreset` is applied.There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extra/seaweedfs/templates/vpa.yaml (1)
51-99: Bug: empty or unset.Values.zonesdrops the global Volume VPAWith the current nested
if, whenzonesis unset the outerifis false, so neither per‑zone nor global Volume VPA is rendered. This is a behavioral regression.Simplify and make fallback explicit:
-{{- if .Values.zones }} - {{- if gt (len .Values.zones) 0 }} - {{- range $zoneName, $zoneSpec := .Values.zones }} +{{- $zones := (.Values.zones | default (dict)) }} +{{- if gt (len $zones) 0 }} + {{- range $zoneName, $zoneSpec := $zones }} @@ - {{- end }} - {{- else }} + {{- end }} +{{- else }} @@ - {{- end }} -{{- end }} +{{- end }}
🧹 Nitpick comments (8)
packages/extra/seaweedfs/values.schema.json (1)
75-80: Schema is too permissive for vpa; consider structuring it.
Define explicit shapes for filer/master/volume with minAllowed/maxAllowed (cpu/memory) to catch typos at validation time instead ofx-kubernetes-preserve-unknown-fields: true.Apply a structured schema like:
- "vpa": { - "description": "Vertical Pod Autoscaler configuration for each SeaweedFS component.", - "type": "object", - "default": {}, - "x-kubernetes-preserve-unknown-fields": true - }, + "vpa": { + "description": "Vertical Pod Autoscaler configuration for each SeaweedFS component.", + "type": "object", + "default": {}, + "properties": { + "filer": { "$ref": "#/definitions/vpaComponent" }, + "master": { "$ref": "#/definitions/vpaComponent" }, + "volume": { "$ref": "#/definitions/vpaComponent" } + }, + "additionalProperties": false + }and add a
definitions.vpaComponentwithminAllowed/maxAllowedobjects each havingcpu/memorystrings.packages/extra/seaweedfs/README.md (1)
23-41: Good VPA doc; minor copy issues.
- Table is clear. However, the file header says “Managed NATS Service” which doesn’t match SeaweedFS.
Proposed fixes:
-# Managed NATS Service +# SeaweedFSpackages/extra/ingress/README.md (1)
12-15: Copy/paste: mentions PostgreSQL instead of ingress.
Update wording to reflect ingress controller pods.-| `resources` | Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied. | `*object` | `{}` | +| `resources` | Explicit CPU and memory configuration for each ingress controller pod. When left empty, the preset defined in `resourcesPreset` is applied. | `*object` | `{}` | -| `resources.cpu` | CPU available to each replica | `*quantity` | `null` | +| `resources.cpu` | CPU available to each controller pod | `*quantity` | `null` | -| `resources.memory` | Memory (RAM) available to each replica | `*quantity` | `null` | +| `resources.memory` | Memory (RAM) available to each controller pod | `*quantity` | `null` |packages/extra/ingress/values.yaml (1)
17-25: Copy/paste in comments: PostgreSQL → ingress.
Clarify to avoid confusion.-## @param resources {*resources} Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied. -## @field resources.cpu {*quantity} CPU available to each replica -## @field resources.memory {*quantity} Memory (RAM) available to each replica +## @param resources {*resources} Explicit CPU and memory configuration for each ingress controller pod. When left empty, the preset defined in `resourcesPreset` is applied. +## @field resources.cpu {*quantity} CPU available to each controller pod +## @field resources.memory {*quantity} Memory (RAM) available to each controller podpackages/extra/seaweedfs/templates/vpa.yaml (1)
53-74: Deterministic output for per‑zone VPAsIterating a map yields non‑deterministic order. Sort keys for stable manifests and diffs.
- {{- range $zoneName, $zoneSpec := $zones }} + {{- range $zoneName := (keys $zones | sortAlpha) }} + {{- $zoneSpec := index $zones $zoneName }}packages/extra/ingress/values.schema.json (3)
15-47: Fix copy-paste: mentions PostgreSQL instead of ingress-nginxUpdate the description to reflect ingress resources; current wording is misleading.
- "description": "Explicit CPU and memory configuration for each PostgreSQL replica. When left empty, the preset defined in `resourcesPreset` is applied.", + "description": "Explicit CPU and memory configuration for the ingress-nginx controller Pods. When left empty, the preset defined in `resourcesPreset` is applied.",
15-47: Tighten schema: restrict unexpected fields underresourcesConsider preventing typos like
memroyby disallowing unknown keys."resources": { "type": "object", "default": {}, + "additionalProperties": false, "properties": { "cpu": { ... }, "memory": { ... } } },
48-61: Minor docs polish forresourcesPresetAdd examples to guide users and IDE tooltips.
"resourcesPreset": { "type": "string", "default": "micro", "enum": ["nano","micro","small","medium","large","xlarge","2xlarge"], + "examples": ["small"] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/templates/nginx-ingress.yaml(1 hunks)packages/extra/ingress/values.schema.json(1 hunks)packages/extra/ingress/values.yaml(1 hunks)packages/extra/seaweedfs/Chart.yaml(1 hunks)packages/extra/seaweedfs/README.md(1 hunks)packages/extra/seaweedfs/templates/vpa.yaml(4 hunks)packages/extra/seaweedfs/values.schema.json(1 hunks)packages/extra/seaweedfs/values.yaml(1 hunks)
🔇 Additional comments (3)
packages/extra/ingress/Chart.yaml (1)
6-6: Version bump LGTM.
Consistent with adding configurable resources.packages/extra/seaweedfs/Chart.yaml (1)
19-19: Version bump LGTM.
Matches the new VPA config surface.packages/extra/seaweedfs/values.yaml (1)
47-106: VPA defaults depend on templates; confirm sane behavior when fields are empty.
With{}defaults, ensuretemplates/vpa.yamlapplies safe min/max when not set and that per-zone Volume VPAs inherit component-level defaults as expected.
| controller: | ||
| replicaCount: {{ .Values.replicas }} | ||
| ingressClass: {{ .Release.Namespace }} | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 4 }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bad YAML indentation for included resources block.
nindent 4 under controller: will mis-indent the child map under resources: and break rendering. Use a newline and nindent 10 (child of an 8-space-indented key).
Apply:
- resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 4 }}
+ resources:
+ {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 10 }}Also verify the helper returns only the object body (requests/limits), not a top-level resources: key.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo
echo "=== Locate target file ==="
files=$(rg --hidden --no-ignore-vcs --files | rg -n 'packages/extra/ingress/templates/nginx-ingress.yaml|nginx-ingress.yaml' || true)
echo "$files"
echo
target="packages/extra/ingress/templates/nginx-ingress.yaml"
if [ -f "$target" ]; then
echo "=== Showing $target (first 300 lines) with numbers ==="
nl -ba "$target" | sed -n '1,300p'
echo
echo "=== Show lines around 'resources' and include patterns ==="
nl -ba "$target" | rg -n --line-number "resources:|include \"cozy-lib.resources.defaultingSanitize\"|defaultingSanitize" -n -C3 || true
else
echo "$target not found; printing any nginx-ingress.yaml files discovered above"
for f in $files; do
echo "---- $f ----"
nl -ba "$f" | sed -n '1,300p' || true
echo
done
fi
echo "=== Search for helper 'defaultingSanitize' ==="
helpers=$(rg -n --hidden --no-ignore-vcs "defaultingSanitize" || true)
echo "$helpers"
echo
if [ -n "$helpers" ]; then
echo "=== Showing files that contain 'defaultingSanitize' (up to first 400 lines each) ==="
for f in $(echo "$helpers" | cut -d: -f1 | sort -u); do
echo "---- file: $f ----"
nl -ba "$f" | sed -n '1,400p' || true
echo
done
else
echo "No helper named defaultingSanitize found."
fi
echo "=== Search for any helper that emits a top-level 'resources:' literal ==="
rg -n --hidden --no-ignore-vcs --line-number '\bresources:\b' | rg -v "templates/nginx-ingress.yaml" || true
echo "=== Done ==="Length of output: 264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "repo: $(pwd)"
target="packages/extra/ingress/templates/nginx-ingress.yaml"
echo
echo "== show target file (up to 400 lines) =="
if [ -f "$target" ]; then
sed -n '1,400p' "$target"
else
echo "target not found: $target"
fi
echo
echo "== show lines around 'resources' and include usage in target =="
rg -n -C3 --hidden --no-ignore-vcs 'resources:|include .*defaultingSanitize' "$target" || true
echo
echo "== search for helper definitions and related helpers =="
rg -n --hidden --no-ignore-vcs 'define ".+cozy-lib\.resources|define "cozy-lib\.resources|defaultingSanitize|cozy-lib\.resources' || true
echo
echo "== list _helpers.tpl files and show their contents (first 400 lines) =="
rg -n --hidden --no-ignore-vcs '_helpers.tpl' || true
for f in $(rg --hidden --no-ignore-vcs -g '!node_modules' --files | rg '_helpers.tpl' || true); do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
done
echo
echo "== search for any template helper that emits a top-level 'resources:' literal =="
rg -n --hidden --no-ignore-vcs --type-add 'tpl:*.tpl' '\bresources:\b' || true
echo
echo "done"Length of output: 13178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target1="packages/extra/ingress/templates/nginx-ingress.yaml"
helper="packages/library/cozy-lib/templates/_resources.tpl"
echo "== snippet (lines 24-36) from $target1 with leading-space counts =="
awk 'NR>=24 && NR<=36 {match($0,/^[ \t]*/); ws=RLENGTH; printf("%4d %2d|%s\n",NR,ws,substr($0,ws+1))}' "$target1" || true
echo
echo "== exact line 32 (raw) =="
sed -n '32p' "$target1" || true
echo
echo "== show define block for defaultingSanitize in $helper (lines 120-190) with line numbers =="
awk 'NR>=120 && NR<=190 {printf("%4d|%s\n",NR,$0)}' "$helper" || true
echo
echo "== search for literal 'resources:' inside helper file =="
rg -n --hidden --no-ignore-vcs --line-number '\bresources:\b' "$helper" || true
echo
echo "== Done =="Length of output: 3874
Fix YAML indentation for controller.resources in nginx-ingress.yaml
Replace the inline include with a block so the included mapping is indented as a child (2 spaces deeper than the controller keys). Apply:
- resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 4 }}
+ resources:
+ {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 10 }}Verified the helper returns the inner resources mapping (limits/requests), not a top-level resources: key.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 4 }} | |
| resources: | |
| {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.resourcesPreset .Values.resources $) | nindent 10 }} |
🤖 Prompt for AI Agents
packages/extra/ingress/templates/nginx-ingress.yaml around line 32: the current
single-line inline include emits the helper's mapping at the same indentation as
the controller keys instead of as the child of resources; change the inline
include into a block under the resources key so the helper's returned inner
mapping (limits/requests) is indented as the value of resources (i.e., keep a
literal resources: key on its own line and place the include on the next line
using the template indent helper to nindent the include 2 spaces deeper than the
controller keys — typically nindent 6 — so the mapping becomes a proper child of
resources).
| cpu: {{ default "25m" .Values.vpa.filer.minAllowed.cpu }} | ||
| memory: {{ default "64Mi" .Values.vpa.filer.minAllowed.memory }} | ||
| maxAllowed: | ||
| cpu: "1" | ||
| memory: 2048Mi | ||
| cpu: {{ default "1" .Values.vpa.filer.maxAllowed.cpu }} | ||
| memory: {{ default "2048Mi" .Values.vpa.filer.maxAllowed.memory }} |
There was a problem hiding this comment.
Avoid nil-chain panics in Helm: use dig when reading deeply nested values
Accessing .Values.vpa.*.*.* directly will error if any intermediate map is missing. Use dig + default to make lookups safe.
Suggested patch:
- cpu: {{ default "25m" .Values.vpa.filer.minAllowed.cpu }}
- memory: {{ default "64Mi" .Values.vpa.filer.minAllowed.memory }}
+ cpu: {{ default "25m" (dig "vpa" "filer" "minAllowed" "cpu" .Values) }}
+ memory: {{ default "64Mi" (dig "vpa" "filer" "minAllowed" "memory" .Values) }}
@@
- cpu: {{ default "1" .Values.vpa.filer.maxAllowed.cpu }}
- memory: {{ default "2048Mi" .Values.vpa.filer.maxAllowed.memory }}
+ cpu: {{ default "1" (dig "vpa" "filer" "maxAllowed" "cpu" .Values) }}
+ memory: {{ default "2048Mi" (dig "vpa" "filer" "maxAllowed" "memory" .Values) }}
@@
- cpu: {{ default "25m" .Values.vpa.master.minAllowed.cpu }}
- memory: {{ default "64Mi" .Values.vpa.master.minAllowed.memory }}
+ cpu: {{ default "25m" (dig "vpa" "master" "minAllowed" "cpu" .Values) }}
+ memory: {{ default "64Mi" (dig "vpa" "master" "minAllowed" "memory" .Values) }}
@@
- cpu: {{ default "1" .Values.vpa.master.maxAllowed.cpu }}
- memory: {{ default "2048Mi" .Values.vpa.master.maxAllowed.memory }}
+ cpu: {{ default "1" (dig "vpa" "master" "maxAllowed" "cpu" .Values) }}
+ memory: {{ default "2048Mi" (dig "vpa" "master" "maxAllowed" "memory" .Values) }}
@@
- cpu: {{ default "25m" .Values.vpa.volume.minAllowed.cpu }}
- memory: {{ default "64Mi" .Values.vpa.volume.minAllowed.memory }}
+ cpu: {{ default "25m" (dig "vpa" "volume" "minAllowed" "cpu" .Values) }}
+ memory: {{ default "64Mi" (dig "vpa" "volume" "minAllowed" "memory" .Values) }}
@@
- cpu: {{ default "1" .Values.vpa.volume.maxAllowed.cpu }}
- memory: {{ default "2048Mi" .Values.vpa.volume.maxAllowed.memory }}
+ cpu: {{ default "1" (dig "vpa" "volume" "maxAllowed" "cpu" .Values) }}
+ memory: {{ default "2048Mi" (dig "vpa" "volume" "maxAllowed" "memory" .Values) }}Also applies to: 42-46, 93-97
🤖 Prompt for AI Agents
In packages/extra/seaweedfs/templates/vpa.yaml around lines 18-22 (and similarly
at 42-46 and 93-97), the template is accessing deep nested values like
.Values.vpa.filer.minAllowed.* which can cause nil-chain panics if intermediate
maps are missing; change those lookups to use the helm sprig dig function
together with default (e.g. dig .Values "vpa" "filer" "minAllowed" "cpu" piped
to default) so each deep key is resolved safely and falls back to the same
defaults currently used.
b73faa4 to
4838a97
Compare
Signed-off-by: kklinch0 <kklinch0@gmail.com>
48245f8 to
46fea7e
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
46fea7e to
55af068
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/system/seaweedfs/values.yaml (1)
97-97: Ingress host and TLS hosts mismatch (will break certificate validation).
s3.ingress.hostis set to seaweedfs2.demo.cozystack.io (Line 97) but TLS hosts list seaweedfs.demo.cozystack.io (Lines 111-112). Clients will get a cert name mismatch.Apply this diff to align the names (pick the intended FQDN):
- host: "seaweedfs2.demo.cozystack.io" + host: "seaweedfs.demo.cozystack.io" @@ - - seaweedfs.demo.cozystack.io + - seaweedfs.demo.cozystack.ioIf "seaweedfs2" is the intended host, change TLS hosts accordingly instead.
Also applies to: 111-112
packages/extra/seaweedfs/values.schema.json (1)
8-12: Type mismatch for filer.whitelist default.
whitelistis an array but default is{}in the parent default object (Lines 8-12). This violates the schema.- "default": { - "grpcHost": "", - "grpcPort": 443, - "whitelist": {} - }, + "default": { + "grpcHost": "", + "grpcPort": 443, + "whitelist": [] + },Also applies to: 23-31
🧹 Nitpick comments (6)
packages/system/seaweedfs/values.yaml (1)
88-106: Verify HTTPS expectations vs. service ports.Probes/Ingress declare HTTPS (Lines 88-106) while
seaweedfs.s3.httpsPort: 0(Line 76). If the backend isn’t serving TLS, the ingress annotationbackend-protocol: "HTTPS"and probescheme: HTTPSwill fail.Please confirm the S3 container serves HTTPS on the target port or flip these to HTTP.
Also applies to: 115-121
packages/extra/seaweedfs/templates/seaweedfs.yaml (1)
153-159: Add S3 resources is good, but make them configurable.You hard-coded S3 container resources. Consider wiring them to values (with sane defaults) to match the PR’s “make resources configurable” goal, similar to ingress’ resources/resourcesPreset pattern.
Example:
- resources: - requests: - cpu: "100m" - memory: "128Mi" - limits: - cpu: "500m" - memory: "512Mi" + resources: + requests: + cpu: {{ default "100m" .Values.resources.s3.requests.cpu | quote }} + memory: {{ default "128Mi" .Values.resources.s3.requests.memory | quote }} + limits: + cpu: {{ default "500m" .Values.resources.s3.limits.cpu | quote }} + memory: {{ default "512Mi" .Values.resources.s3.limits.memory | quote }}If you prefer presets, introduce
.Values.resourcesPreset.s3and map to concrete values here.packages/extra/seaweedfs/README.md (2)
1-1: Fix title: this is SeaweedFS, not NATS.-# Managed NATS Service +# Managed SeaweedFS Service
26-57: “maxAllowed.*” rows incorrectly say “Minimum … request”.For all vpa.*.maxAllowed.{cpu,memory} entries, change “Minimum” to “Maximum”.
-| `vpa.master.maxAllowed.cpu` | Minimum CPU request | `*quantity` | `null` | -| `vpa.master.maxAllowed.memory` | Minimum memory request | `*quantity` | `null` | +| `vpa.master.maxAllowed.cpu` | Maximum CPU request | `*quantity` | `null` | +| `vpa.master.maxAllowed.memory` | Maximum memory request | `*quantity` | `null` | @@ -| `vpa.filer.maxAllowed.cpu` | Minimum CPU request | `*quantity` | `null` | -| `vpa.filer.maxAllowed.memory` | Minimum memory request | `*quantity` | `null` | +| `vpa.filer.maxAllowed.cpu` | Maximum CPU request | `*quantity` | `null` | +| `vpa.filer.maxAllowed.memory` | Maximum memory request | `*quantity` | `null` | @@ -| `vpa.volume.maxAllowed.cpu` | Minimum CPU request | `*quantity` | `null` | -| `vpa.volume.maxAllowed.memory` | Minimum memory request | `*quantity` | `null` | +| `vpa.volume.maxAllowed.cpu` | Maximum CPU request | `*quantity` | `null` | +| `vpa.volume.maxAllowed.memory` | Maximum memory request | `*quantity` | `null` | @@ -| `vpa.s3.maxAllowed.cpu` | Minimum CPU request | `*quantity` | `null` | -| `vpa.s3.maxAllowed.memory` | Minimum memory request | `*quantity` | `null` | +| `vpa.s3.maxAllowed.cpu` | Maximum CPU request | `*quantity` | `null` | +| `vpa.s3.maxAllowed.memory` | Maximum memory request | `*quantity` | `null` |packages/extra/seaweedfs/values.schema.json (2)
111-141: Fix “Maximum” vs “Minimum” descriptions in maxAllowed blocks.In every
*.maxAllowed.{cpu,memory}schema, the description says “Minimum …” (copy/paste). Should be “Maximum …”.Example fix (apply similarly to master/s3/volume sections):
- "cpu": { - "description": "Minimum CPU request", + "cpu": { + "description": "Maximum CPU request", @@ - "memory": { - "description": "Minimum memory request", + "memory": { + "description": "Maximum memory request",Also applies to: 145-175, 187-217, 221-251, 263-293, 297-327, 339-369, 373-403
75-101: Consider DRYing the quantity schema via $defs.The CPU/memory quantity schema is duplicated many times. Define it once under
$defs.quantityand$refit. Lower maintenance and fewer copy errors.Also applies to: 102-408
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extra/seaweedfs/README.md(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)packages/extra/seaweedfs/templates/vpa.yaml(4 hunks)packages/extra/seaweedfs/values.schema.json(1 hunks)packages/extra/seaweedfs/values.yaml(2 hunks)packages/system/seaweedfs/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extra/seaweedfs/values.yaml
🔇 Additional comments (3)
packages/system/seaweedfs/values.yaml (1)
73-74: Confirm chart support for seaweedfs.s3.replicas and its interaction with VPA.Replicas introduced for seaweedfs.s3 (Line 73). Ensure the underlying cozy-seaweedfs chart consumes this field and that scaling policy (HPA/VPA) doesn’t conflict.
Would you like a quick script to grep the chart for
.Values.seaweedfs.s3.replicasusage?packages/extra/seaweedfs/README.md (1)
16-17: Doc sync with schema: zones fields changed to optional — LGTM.packages/extra/seaweedfs/templates/vpa.yaml (1)
18-22: dig() argument order is correct — Sprig expects keys, optional default, then the dict.
Occurrences in packages/extra/seaweedfs/templates/vpa.yaml (lines 18–22, 42–46, 66–70, 93–97, 116–120) already follow that order; original comment is incorrect.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/extra/seaweedfs/templates/vpa.yaml (3)
18-22: Good fix: prevent nil-chain panics withdig.Using
dighere avoids Helm render failures when parts of.Values.vpaare missing.
66-70: Good fix: consistentdigdefaults for master VPA.Same pattern as filer—looks correct.
93-97: Good:digfor volume VPA (per-zone and single).The defaults and scoping with
$.Values.vpa/.Values.vpaare correct.Also applies to: 116-120
🧹 Nitpick comments (3)
packages/extra/seaweedfs/templates/vpa.yaml (2)
26-47: Gate S3 VPA on S3 being enabled and verify target/container names.Render the S3 VPA only when S3 is enabled to avoid dangling VPAs referencing non-existent targets. Also ensure the S3 Deployment name and container name are exactly
{{ .Release.Name }}-s3andseaweedfs.Apply this diff around the S3 block:
-# --- S3 --- +# --- S3 --- +{{- if or (dig "seaweedfs" "s3" "enabled" true .Values) (dig "s3" "enabled" true .Values) }} apiVersion: autoscaling.k8s.io/v1 kind: VerticalPodAutoscaler metadata: name: {{ .Release.Name }}-s3 spec: targetRef: apiVersion: apps/v1 kind: Deployment name: {{ .Release.Name }}-s3 @@ maxAllowed: cpu: {{ dig "s3" "maxAllowed" "cpu" "1" .Values.vpa }} memory: {{ dig "s3" "maxAllowed" "memory" "2048Mi" .Values.vpa }} --- +{{- end }}
75-99: YAML doc separators: ensure column-0 emission inside range loop.If there’s any leading whitespace before
---inside therange, some linters/parsers will complain. Make sure the generated---starts at column 0.Apply this trimming to be safe:
- {{- range $zoneName, $zoneSpec := .Values.zones }} ---- +{{- range $zoneName, $zoneSpec := .Values.zones }} +---packages/extra/seaweedfs/values.schema.json (1)
111-142: Fix incorrect descriptions under maxAllowed fields (says “Minimum” instead of “Maximum”).Descriptions for
maxAllowed.cpuandmaxAllowed.memoryshould say “Maximum …”. Update here and mirror the same fix formaster,s3, andvolumesections.Apply this diff for the filer block (repeat analogously for other components):
- "cpu": { - "description": "Minimum CPU request", + "cpu": { + "description": "Maximum CPU request", @@ - "memory": { - "description": "Minimum memory request", + "memory": { + "description": "Maximum memory request",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/extra/seaweedfs/README.md(1 hunks)packages/extra/seaweedfs/templates/seaweedfs.yaml(1 hunks)packages/extra/seaweedfs/templates/vpa.yaml(4 hunks)packages/extra/seaweedfs/values.schema.json(1 hunks)packages/extra/seaweedfs/values.yaml(2 hunks)packages/system/seaweedfs/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/extra/seaweedfs/values.yaml
- packages/extra/seaweedfs/templates/seaweedfs.yaml
- packages/extra/seaweedfs/README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Prepare environment
🔇 Additional comments (1)
packages/system/seaweedfs/values.yaml (1)
72-85: Confirm S3 replicas and probe scheme alignment.Setting
seaweedfs.s3.replicas: 2is fine. Double-check that:
- The S3 Deployment actually scales via this value (matches the template’s expected path).
- Readiness/liveness probes under top-level
s3.*use the correct scheme vsseaweedfs.s3.httpsPort(currently0). If HTTPS is disabled, probes should be HTTP to avoid false negatives.
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
Chores