[bugfix] add-resource-quotas-for-pg-jobs-and-fix-install-generate - #1051
Conversation
WalkthroughThe PostgreSQL Helm chart version was updated to 0.13.0. New Helm template helpers were added to define default resource requests and limits for PostgreSQL jobs and to sanitize these resources via an external helper. These helpers were integrated into the backup CronJob and init job container specifications. The version mapping for PostgreSQL was updated. Additionally, the Node.js setup in the pre-commit GitHub Actions workflow was revised, changing the installation method and package versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm Chart
participant K8s as Kubernetes API
participant Namespace as Namespace ResourceQuota
Helm->>K8s: Lookup ResourceQuota in release namespace
K8s-->>Helm: Return ResourceQuota (if exists)
Helm->>Helm: Evaluate resource limits/requests (from quota or defaults)
Helm->>K8s: Render Job/CronJob manifest with computed resources
K8s-->>Namespace: Deploy Job/CronJob with appropriate resources
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/postgres/templates/_postgresjobs.tpl (1)
29-45: Nit – quote defaults for consistencyThe quota-derived values are wrapped in
| quote, whereas the hard-coded defaults (500m,512Mi, etc.) are not.
Quoting them as well avoids type surprises in downstream tooling and keeps YAML style consistent.- cpu: 500m + cpu: "500m" … - memory: 512Mi + memory: "512Mi" … - cpu: 250m + cpu: "250m" … - memory: 256Mi + memory: "256Mi"packages/apps/postgres/Chart.yaml (1)
19-19: Chart version bumped – remember to update CHANGELOGVersion
0.13.0is fine; ensure the chart’s CHANGELOG (if any) captures the new helper and the behavioural change around job resources.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/templates/_postgresjobs.tpl(1 hunks)packages/apps/postgres/templates/backup-cronjob.yaml(1 hunks)packages/apps/postgres/templates/init-job.yaml(1 hunks)packages/apps/versions_map(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/apps/postgres/templates/backup-cronjob.yaml (1)
84-85: Indentation looks correct but please validate withyamllint
nindent 12should alignresources:withsecurityContext:(12 spaces) given the current structure.
A quickhelm template | yamllintrun will confirm there’s no shift that would pushresourcesoutside the container block.packages/apps/postgres/templates/init-job.yaml (1)
53-54: Same indentation caveat as backup CronJob
nindent 8is expected to alignresources:withvolumeMounts:inside the container spec.
Please lint the rendered manifest to ensure the block is in the correct location.packages/apps/versions_map (1)
112-113: HEAD pointer update LGTMMapping is consistent with the chart bump.
| {{- if $hasLimitsCPU }} | ||
| cpu: {{ $rq.spec.hard.limits.cpu | quote }} | ||
| {{- else }} | ||
| cpu: 500m | ||
| {{- end }} | ||
| {{- if $hasLimitsMemory }} | ||
| memory: {{ $rq.spec.hard.limits.memory | quote }} | ||
| {{- else }} | ||
| memory: 512Mi | ||
| {{- end }} | ||
| requests: | ||
| {{- if $hasRequestsCPU }} | ||
| cpu: {{ $rq.spec.hard.requests.cpu | quote }} | ||
| {{- else }} | ||
| cpu: 250m | ||
| {{- end }} | ||
| {{- if $hasRequestsMemory }} | ||
| memory: {{ $rq.spec.hard.requests.memory | quote }} | ||
| {{- else }} | ||
| memory: 256Mi | ||
| {{- end }} |
There was a problem hiding this comment.
Bug – ResourceQuota keys containing dots are being dereferenced incorrectly
limits.cpu, limits.memory, requests.cpu, and requests.memory are flat keys in spec.hard.
Using dot-notation ($rq.spec.hard.limits.cpu) splits the key at the first . and therefore always yields <no value>, causing Helm rendering failures or empty values.
Fix by indexing the map with the full key string:
- cpu: {{ $rq.spec.hard.limits.cpu | quote }}
+ cpu: {{ index $rq.spec.hard "limits.cpu" | quote }}
- memory: {{ $rq.spec.hard.limits.memory | quote }}
+ memory: {{ index $rq.spec.hard "limits.memory" | quote }}
- cpu: {{ $rq.spec.hard.requests.cpu | quote }}
+ cpu: {{ index $rq.spec.hard "requests.cpu" | quote }}
- memory: {{ $rq.spec.hard.requests.memory | quote }}
+ memory: {{ index $rq.spec.hard "requests.memory" | quote }}📝 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.
| {{- if $hasLimitsCPU }} | |
| cpu: {{ $rq.spec.hard.limits.cpu | quote }} | |
| {{- else }} | |
| cpu: 500m | |
| {{- end }} | |
| {{- if $hasLimitsMemory }} | |
| memory: {{ $rq.spec.hard.limits.memory | quote }} | |
| {{- else }} | |
| memory: 512Mi | |
| {{- end }} | |
| requests: | |
| {{- if $hasRequestsCPU }} | |
| cpu: {{ $rq.spec.hard.requests.cpu | quote }} | |
| {{- else }} | |
| cpu: 250m | |
| {{- end }} | |
| {{- if $hasRequestsMemory }} | |
| memory: {{ $rq.spec.hard.requests.memory | quote }} | |
| {{- else }} | |
| memory: 256Mi | |
| {{- end }} | |
| {{- if $hasLimitsCPU }} | |
| cpu: {{ index $rq.spec.hard "limits.cpu" | quote }} | |
| {{- else }} | |
| cpu: 500m | |
| {{- end }} | |
| {{- if $hasLimitsMemory }} | |
| memory: {{ index $rq.spec.hard "limits.memory" | quote }} | |
| {{- else }} | |
| memory: 512Mi | |
| {{- end }} | |
| requests: | |
| {{- if $hasRequestsCPU }} | |
| cpu: {{ index $rq.spec.hard "requests.cpu" | quote }} | |
| {{- else }} | |
| cpu: 250m | |
| {{- end }} | |
| {{- if $hasRequestsMemory }} | |
| memory: {{ index $rq.spec.hard "requests.memory" | quote }} | |
| {{- else }} | |
| memory: 256Mi | |
| {{- end }} |
🤖 Prompt for AI Agents
In packages/apps/postgres/templates/_postgresjobs.tpl between lines 26 and 46,
the resource quota keys like limits.cpu and requests.memory are accessed using
dot notation, which incorrectly splits the keys and results in no values. To fix
this, replace dot notation with map indexing using the full key string in
quotes, for example, use $rq.spec.hard["limits.cpu"] instead of
$rq.spec.hard.limits.cpu, and similarly for the other keys.
4967dbc to
6d07123
Compare
| {{/* | ||
| Generate resource requirements based on ResourceQuota named as the namespace. | ||
| */}} | ||
| {{- define "postgresjobs.resources" }} | ||
| {{- $rq := (lookup "v1" "ResourceQuota" .Release.Namespace .Release.Namespace) }} | ||
| {{- $hasLimitsCPU := false }} | ||
| {{- $hasLimitsMemory := false }} | ||
| {{- $hasRequestsCPU := false }} | ||
| {{- $hasRequestsMemory := false }} | ||
| {{- if $rq }} | ||
| {{- if hasKey $rq.spec.hard "limits.cpu" }} | ||
| {{- $hasLimitsCPU = true }} | ||
| {{- end }} | ||
| {{- if hasKey $rq.spec.hard "limits.memory" }} | ||
| {{- $hasLimitsMemory = true }} | ||
| {{- end }} | ||
| {{- if hasKey $rq.spec.hard "requests.cpu" }} | ||
| {{- $hasRequestsCPU = true }} | ||
| {{- end }} | ||
| {{- if hasKey $rq.spec.hard "requests.memory" }} | ||
| {{- $hasRequestsMemory = true }} | ||
| {{- end }} | ||
| {{- end }} | ||
| resources: | ||
| limits: | ||
| {{- if $hasLimitsCPU }} | ||
| cpu: {{ $rq.spec.hard.limits.cpu | quote }} | ||
| {{- else }} | ||
| cpu: 500m | ||
| {{- end }} | ||
| {{- if $hasLimitsMemory }} | ||
| memory: {{ $rq.spec.hard.limits.memory | quote }} | ||
| {{- else }} | ||
| memory: 512Mi | ||
| {{- end }} | ||
| requests: | ||
| {{- if $hasRequestsCPU }} | ||
| cpu: {{ $rq.spec.hard.requests.cpu | quote }} | ||
| {{- else }} | ||
| cpu: 250m | ||
| {{- end }} | ||
| {{- if $hasRequestsMemory }} | ||
| memory: {{ $rq.spec.hard.requests.memory | quote }} | ||
| {{- else }} | ||
| memory: 256Mi | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
| {{/* | |
| Generate resource requirements based on ResourceQuota named as the namespace. | |
| */}} | |
| {{- define "postgresjobs.resources" }} | |
| {{- $rq := (lookup "v1" "ResourceQuota" .Release.Namespace .Release.Namespace) }} | |
| {{- $hasLimitsCPU := false }} | |
| {{- $hasLimitsMemory := false }} | |
| {{- $hasRequestsCPU := false }} | |
| {{- $hasRequestsMemory := false }} | |
| {{- if $rq }} | |
| {{- if hasKey $rq.spec.hard "limits.cpu" }} | |
| {{- $hasLimitsCPU = true }} | |
| {{- end }} | |
| {{- if hasKey $rq.spec.hard "limits.memory" }} | |
| {{- $hasLimitsMemory = true }} | |
| {{- end }} | |
| {{- if hasKey $rq.spec.hard "requests.cpu" }} | |
| {{- $hasRequestsCPU = true }} | |
| {{- end }} | |
| {{- if hasKey $rq.spec.hard "requests.memory" }} | |
| {{- $hasRequestsMemory = true }} | |
| {{- end }} | |
| {{- end }} | |
| resources: | |
| limits: | |
| {{- if $hasLimitsCPU }} | |
| cpu: {{ $rq.spec.hard.limits.cpu | quote }} | |
| {{- else }} | |
| cpu: 500m | |
| {{- end }} | |
| {{- if $hasLimitsMemory }} | |
| memory: {{ $rq.spec.hard.limits.memory | quote }} | |
| {{- else }} | |
| memory: 512Mi | |
| {{- end }} | |
| requests: | |
| {{- if $hasRequestsCPU }} | |
| cpu: {{ $rq.spec.hard.requests.cpu | quote }} | |
| {{- else }} | |
| cpu: 250m | |
| {{- end }} | |
| {{- if $hasRequestsMemory }} | |
| memory: {{ $rq.spec.hard.requests.memory | quote }} | |
| {{- else }} | |
| memory: 256Mi | |
| {{- end }} | |
| {{- end }} | |
| {{/* | |
| Generate resource requirements based on ResourceQuota named as the namespace. | |
| */}} | |
| {{- define "postgresjobs.defaultResources" }} | |
| resources: | |
| cpu: "1" | |
| memory: 512Mi | |
| {{- end }} | |
| {{- define "postgresjobs.resources" }} | |
| {{- include "cozy-lib.resources.sanitize" (list (include "postgresjobs.defaultResources" $ | fromYaml) $) }} | |
| {{- end }} |
6d07123 to
a0c1177
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/apps/postgres/templates/_postgresjobs.tpl (2)
2-3: Comment is misleading – implementation no longer referencesResourceQuotaThe header still claims resources are “based on ResourceQuota”, but the helper now hard-codes static defaults and delegates everything else to
cozy-lib.resources.sanitize.
Please update the comment (or re-introduce the dynamic logic) to avoid future confusion.
10-10: Indent the rendered block to match surrounding YAMLWhen this helper is inserted in
backup-cronjob.yaml/init-job.yaml, it appears under the container stanza that is already indented.
Piping throughnindent(orindent) avoids malformed YAML:- {{- include "cozy-lib.resources.sanitize" (list (include "postgresjobs.defaultResources" $ | fromYaml) $) }} + {{- include "cozy-lib.resources.sanitize" (list (include "postgresjobs.defaultResources" $ | fromYaml) $) | nindent 2 }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pre-commit.yml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/templates/_postgresjobs.tpl(1 hunks)packages/apps/postgres/templates/backup-cronjob.yaml(1 hunks)packages/apps/postgres/templates/init-job.yaml(1 hunks)packages/apps/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/apps/postgres/templates/backup-cronjob.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/apps/versions_map
- packages/apps/postgres/Chart.yaml
- .github/workflows/pre-commit.yml
- packages/apps/postgres/templates/init-job.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
| {{- define "postgresjobs.defaultResources" }} | ||
| resources: | ||
| cpu: "1" | ||
| memory: 512Mi | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Default map shape is not a valid Kubernetes resources object
resources: must contain requests and/or limits sub-maps, not raw cpu/memory keys.
Placing the current output inside a container spec will render as
resources:
resources:
cpu: "1"
memory: 512Miwhich is invalid.
Suggested fix:
-{{- define "postgresjobs.defaultResources" }}
-resources:
- cpu: "1"
- memory: 512Mi
+{{- define "postgresjobs.defaultResources" }}
+requests:
+ cpu: "100m"
+ memory: "512Mi"
+limits:
+ cpu: "1"
+ memory: "1Gi"
{{- end }}This keeps sane defaults and produces a structure the API server accepts.
🤖 Prompt for AI Agents
In packages/apps/postgres/templates/_postgresjobs.tpl lines 4 to 8, the
resources map incorrectly uses raw cpu and memory keys directly under resources,
which is invalid in Kubernetes. To fix this, nest the cpu and memory values
inside a requests sub-map under resources, so the output has resources:
requests: cpu and memory, ensuring it matches the valid Kubernetes resource
specification.
a0c1177 to
9c962b2
Compare
Signed-off-by: kklinch0 <kklinch0@gmail.com>
9c962b2 to
8e79f24
Compare
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin release-0.31
git worktree add -d .worktree/backport-1051-to-release-0.31 origin/release-0.31
cd .worktree/backport-1051-to-release-0.31
git switch --create backport-1051-to-release-0.31
git cherry-pick -x 8e79f24c5ba40421514cf30203f965250140fcaa |
Summary by CodeRabbit