Skip to content

[Issue#21344]: avoid migration hook serviceaccount dependency cycle #21405

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.

Sign up for GitHub

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

Merged
2 commits merged into from
Mar 4, 2026
+83 −2
Merged

[Issue#21344]: avoid migration hook serviceaccount dependency cycle #21405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7644217
helm cyclic dependency fix
devarakondasrikanth Feb 17, 2026
3fab2a8
updating test case for handling edge cases
devarakondasrikanth Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions deploy/charts/litellm-helm/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,20 @@ Create the name of the service account to use
{{- end }}
{{- end }}

{{/*
Create the service account name used by migration jobs.
When Helm hooks are enabled, pre-install/pre-upgrade hooks run before normal resources.
If this chart is creating the ServiceAccount, it is not yet available for the hook job,
so fall back to "default" (or an explicit override) to avoid a cyclic dependency.
*/}}
{{- define "litellm.migrationServiceAccountName" -}}
{{- if and .Values.migrationJob.hooks.helm.enabled .Values.serviceAccount.create }}
{{- default "default" .Values.migrationJob.serviceAccountName }}
{{- else }}
{{- include "litellm.serviceAccountName" . }}
{{- end }}
{{- end }}

{{/*
Get redis service name
*/}}
Expand Down
2 changes: 1 addition & 1 deletion deploy/charts/litellm-helm/templates/migrations-job.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ spec:
imagePullSecrets:
{{- toYaml . | nindent 8 }}
{{- end }}
serviceAccountName: {{ include "litellm.serviceAccountName" . }}
serviceAccountName: {{ include "litellm.migrationServiceAccountName" . }}
{{- with .Values.migrationJob.extraInitContainers }}
initContainers:
{{- toYaml . | nindent 8 }}
Expand Down
65 changes: 64 additions & 1 deletion deploy/charts/litellm-helm/tests/migrations-job_tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,67 @@ tests:
- notContains:
path: spec.template.spec.containers[0].env
content:
name: DATABASE_URL
name: DATABASE_URL

- it: should use default service account for helm hooks when serviceAccount.create is true
template: migrations-job.yaml
set:
migrationJob:
enabled: true
hooks:
helm:
enabled: true
serviceAccount:
create: true
asserts:
- equal:
path: spec.template.spec.serviceAccountName
value: default

- it: should use migrationJob.serviceAccountName override for helm hooks when serviceAccount.create is true
template: migrations-job.yaml
set:
migrationJob:
enabled: true
serviceAccountName: migration-sa
hooks:
helm:
enabled: true
serviceAccount:
create: true
asserts:
- equal:
path: spec.template.spec.serviceAccountName
value: migration-sa

- it: should use chart service account when helm hooks are disabled
template: migrations-job.yaml
set:
migrationJob:
enabled: true
hooks:
helm:
enabled: false
serviceAccount:
create: true
name: my-custom-sa
asserts:
- equal:
path: spec.template.spec.serviceAccountName
value: my-custom-sa
Comment on lines +160 to +174

@greptile-apps greptile-apps Bot Feb 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test for pre-existing SA with hooks

The tests cover the three main scenarios well. One additional edge case worth testing: helm.enabled=true with serviceAccount.create=false and an explicit serviceAccount.name. In this case, the SA already exists (not managed by the chart), so there's no cyclic dependency and the helper should pass through the pre-existing SA name rather than falling back to default. This would confirm the else branch of the new helper behaves correctly when hooks are active but the SA isn't chart-managed.

  - it: should use pre-existing service account when helm hooks are enabled but serviceAccount.create is false
    template: migrations-job.yaml
    set:
      migrationJob:
        enabled: true
        hooks:
          helm:
            enabled: true
      serviceAccount:
        create: false
        name: pre-existing-sa
    asserts:
      - equal:
          path: spec.template.spec.serviceAccountName
          value: pre-existing-sa

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@devarakondasrikanth devarakondasrikanth Feb 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the tests to handle above mentioned issue.


- it: should use pre-existing service account when helm hooks are enabled but serviceAccount.create is false
template: migrations-job.yaml
set:
migrationJob:
enabled: true
hooks:
helm:
enabled: true
serviceAccount:
create: false
name: pre-existing-sa
asserts:
- equal:
path: spec.template.spec.serviceAccountName
value: pre-existing-sa
4 changes: 4 additions & 0 deletions deploy/charts/litellm-helm/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,10 @@ migrationJob:
retries: 3 # Number of retries for the Job in case of failure
backoffLimit: 4 # Backoff limit for Job restarts
disableSchemaUpdate: false # Skip schema migrations for specific environments. When True, the job will exit with code 0.
# Optional service account for the migration job.
# Only used when migrationJob.hooks.helm.enabled=true and serviceAccount.create=true.
# In that case, pre-install/pre-upgrade hooks run before normal resources, so this defaults to "default".
serviceAccountName: ""
annotations: {}
ttlSecondsAfterFinished: 120
resources: {}
Expand Down
Loading