-
Notifications
You must be signed in to change notification settings - Fork 71
fix: set a different component label on migration Job #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
WalkthroughVersion bump to 0.2.46 accompanied by introduction of a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
1ec7368 to
3bd55ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/openfga/templates/_helpers.tpl (1)
66-85: LGTM: Well-documented migration labels template.The new
migrate.labelstemplate correctly provides a distinct label set withcomponent: migration, preventing HPA from selecting migration Job pods. The explanatory comment clearly documents the rationale.Optional: Consider reducing duplication with a parameterized helper.
The
migrate.labelstemplate duplicates most ofopenfga.labels. For better maintainability, you could create a base label template that accepts a component parameter:{{- define "openfga.baseLabels" -}} helm.sh/chart: {{ include "openfga.chart" . }} app.kubernetes.io/name: {{ include "openfga.name" . }} app.kubernetes.io/instance: {{ .Release.Name }} app.kubernetes.io/component: {{ .component }} {{- with .root.Values.commonLabels }} {{ . | toYaml }} {{- end }} {{- if .root.Chart.AppVersion }} app.kubernetes.io/version: {{ .root.Chart.AppVersion | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .root.Release.Service }} app.kubernetes.io/part-of: openfga {{- end }}Then call it from both templates:
{{- define "openfga.labels" -}} {{- include "openfga.baseLabels" (dict "component" "authorization-controller" "root" .) }} {{- end }} {{- define "migrate.labels" -}} {{- include "openfga.baseLabels" (dict "component" "migration" "root" .) }} {{- end }}This approach would centralize the label structure and reduce future maintenance burden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
charts/openfga/Chart.yaml(1 hunks)charts/openfga/templates/_helpers.tpl(1 hunks)charts/openfga/templates/job.yaml(2 hunks)
🔇 Additional comments (3)
charts/openfga/Chart.yaml (1)
6-6: LGTM: Appropriate version bump.The patch version increment is correct for this bug fix.
charts/openfga/templates/job.yaml (1)
7-7: LGTM: Correctly applies migration-specific labels.The Job manifest now uses
migrate.labelsfor both the Job metadata and Pod template, ensuring these pods havecomponent: migrationinstead ofcomponent: authorization-controller. This prevents the HPA from inadvertently selecting migration Job pods.Also applies to: 23-23
charts/openfga/templates/_helpers.tpl (1)
63-64: LGTM: Component label correctly positioned in selector labels for proper pod targeting.The change to include
app.kubernetes.io/component: authorization-controllerinopenfga.selectorLabelsis correct. This ensures that:
- Deployment selectors (deployment.yaml) explicitly match pods with this label
- Service and ServiceMonitor selectors properly target these pods
- Migration Job pods (using
migrate.labelswith component:migration) remain separate and won't interfere with HPA scalingHPA correctly targets the Deployment by name rather than label selectors, so this change has no negative impact. The label structure maintains proper pod segregation and is backwards compatible.
61736d5 to
550342b
Compare
If the migration Job has the same set of label as the ones referenced in the Deployment match labels then the HorizontalPodAutoscaler will match the Job containers as being part of the replicaset. This addresses this issue by setting the component lable to "migration" on the migration Job and the migration Job Pods. The pattern for setting the component label is now better aligned with other open-source Helm charts. Signed-off-by: Alexandre-P <alexandre.picosson@owkin.com>
550342b to
b5ed816
Compare
Description
What problem is being solved?
I deploy OpenFGA using the chart with the migration Job and the Horizontal Pod Autoscaler (HPA) enabled. This creates an issue where the HPA is matching the migration Job pod because the matching of the HPA is based on the matchLabels of the Deployment. This issue is known and tracked upstream here: kubernetes/enhancements#5325. While we wait for the Kubernetes team to address it, the best solution is to have a unique set of matchLabels for the Deployment that should not be present on other pods.
How is it being solved?
I changed the
app.kubernetes.io/componentlabel for the migration Job and the Pods managed by this Job as well as the test Pod.Applying the component label outside of the common labels seems to be quite standard across charts I frequently use.
What changes are made to solve it?
Here is a diff of the template rendered with the following command:
References
Review Checklist
mainSummary by CodeRabbit
Chores
New Features