fix: Add TTL cleanup for OpenFGA migration jobs to resolve HPA metric failures #281
Conversation
WalkthroughThe Helm chart is bumped from version 0.2.51 to 0.2.52 and adds support for configuring automatic cleanup of migration Job pods through a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@charts/openfga/templates/job.yaml`:
- Around line 16-18: The template currently omits ttlSecondsAfterFinished when
the value is 0 because the truthy check drops falsy values; update the
conditional to detect presence instead of truthiness so zero is rendered.
Replace the existing "if .Values.migrate.ttlSecondsAfterFinished" check with a
presence check such as "if hasKey .Values.migrate \"ttlSecondsAfterFinished\""
(or use a default-nil comparison like "if (not (eq
(.Values.migrate.ttlSecondsAfterFinished | default nil) nil))") so the
ttlSecondsAfterFinished field is emitted even when set to 0.
In `@charts/openfga/values.yaml`:
- Around line 356-358: The values.yaml currently leaves
migrate.ttlSecondsAfterFinished empty so Helm won’t set the runtime default;
update the chart values to set migrate.ttlSecondsAfterFinished: 300 so the
migration Job pods are automatically deleted after 300 seconds (ensure the key
name migrate.ttlSecondsAfterFinished is used exactly and saved in
charts/openfga/values.yaml).
| {{- if .Values.migrate.ttlSecondsAfterFinished }} | ||
| ttlSecondsAfterFinished: {{ .Values.migrate.ttlSecondsAfterFinished }} | ||
| {{- end }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "(job\.yaml|Chart\.yaml)"Repository: openfga/helm-charts
Length of output: 151
🏁 Script executed:
cat -n charts/openfga/templates/job.yaml | head -30Repository: openfga/helm-charts
Length of output: 1213
🏁 Script executed:
fd -e "values\.yaml" charts/openfgaRepository: openfga/helm-charts
Length of output: 424
🏁 Script executed:
fd "values" charts/openfgaRepository: openfga/helm-charts
Length of output: 123
🏁 Script executed:
cat -n charts/openfga/values.yaml | grep -A 10 -B 5 "ttlSecondsAfterFinished"Repository: openfga/helm-charts
Length of output: 711
🏁 Script executed:
cat -n charts/openfga/values.schema.json | grep -A 20 -B 5 "ttlSecondsAfterFinished"Repository: openfga/helm-charts
Length of output: 1326
Allow 0 TTL values to render.
Using a truthy check skips the field when users set 0 for immediate cleanup.
🔧 Proposed fix
- {{- if .Values.migrate.ttlSecondsAfterFinished }}
+ {{- if ne .Values.migrate.ttlSecondsAfterFinished nil }}
ttlSecondsAfterFinished: {{ .Values.migrate.ttlSecondsAfterFinished }}
{{- end }}📝 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 .Values.migrate.ttlSecondsAfterFinished }} | |
| ttlSecondsAfterFinished: {{ .Values.migrate.ttlSecondsAfterFinished }} | |
| {{- end }} | |
| {{- if ne .Values.migrate.ttlSecondsAfterFinished nil }} | |
| ttlSecondsAfterFinished: {{ .Values.migrate.ttlSecondsAfterFinished }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In `@charts/openfga/templates/job.yaml` around lines 16 - 18, The template
currently omits ttlSecondsAfterFinished when the value is 0 because the truthy
check drops falsy values; update the conditional to detect presence instead of
truthiness so zero is rendered. Replace the existing "if
.Values.migrate.ttlSecondsAfterFinished" check with a presence check such as "if
hasKey .Values.migrate \"ttlSecondsAfterFinished\"" (or use a default-nil
comparison like "if (not (eq (.Values.migrate.ttlSecondsAfterFinished | default
nil) nil))") so the ttlSecondsAfterFinished field is emitted even when set to 0.
| # Automatically delete migration Job pods after completion (in seconds). | ||
| # Prevents completed pods from interfering with HPA metric calculations. | ||
| ttlSecondsAfterFinished: |
There was a problem hiding this comment.
Set the runtime default for migrate.ttlSecondsAfterFinished.
Right now it’s empty in values.yaml, so Helm won’t apply the intended default of 300s (schema defaults don’t set runtime values). That leaves cleanup disabled by default.
🔧 Proposed fix
- ttlSecondsAfterFinished:
+ ttlSecondsAfterFinished: 300📝 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.
| # Automatically delete migration Job pods after completion (in seconds). | |
| # Prevents completed pods from interfering with HPA metric calculations. | |
| ttlSecondsAfterFinished: | |
| # Automatically delete migration Job pods after completion (in seconds). | |
| # Prevents completed pods from interfering with HPA metric calculations. | |
| ttlSecondsAfterFinished: 300 |
🤖 Prompt for AI Agents
In `@charts/openfga/values.yaml` around lines 356 - 358, The values.yaml currently
leaves migrate.ttlSecondsAfterFinished empty so Helm won’t set the runtime
default; update the chart values to set migrate.ttlSecondsAfterFinished: 300 so
the migration Job pods are automatically deleted after 300 seconds (ensure the
key name migrate.ttlSecondsAfterFinished is used exactly and saved in
charts/openfga/values.yaml).
1 participant
Description
What problem is being solved?
When running OpenFGA with Horizontal Pod Autoscaler (HPA) in Kubernetes, completed migration Job pods can cause HPA metric collection failures. Since migration pods lack resource requests, the HPA reports FailedGetResourceMetric errors and shows for CPU and memory utilization, preventing proper autoscaling.
How is it being solved?
By adding a configurable ttlSecondsAfterFinished option to the migration Job spec. This leverages Kubernetes' built-in TTL controller to automatically clean up completed Job pods after a specified duration.
What changes are made to solve it?
References
N/A
Review Checklist
mainSummary by CodeRabbit
Chores
New Features
✏️ Tip: You can customize this high-level summary in your review settings.