Repository navigation
fix(helm): stop migrations Job pods matching the litellm Service selector - #40393
JoaVirtudes19 wants to merge 10 commits into
Conversation
…ticated requests Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…stency Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…nd Model Hub UI Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…mutation Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis pull request contains a broad set of changes beyond the stated Helm fix, including migration-pod label isolation, managed Bedrock file deletion, Azure AI pricing and transcription updates, MCP lifecycle and logging changes, guardrail scan metadata, spend-log metadata recovery, and extensive E2E coverage.
Confidence Score: 3/5The pull request is not safe to merge until migration labels remain valid for long name overrides and cannot be changed back to the proxy Service selector by global pod labels. Two reachable Helm configurations break the core migration fix: long supported names render an invalid Kubernetes label, while a global name label can make migration pods match the Service selector again. Files Needing Attention: helm/litellm-helm/templates/_helpers.tpl, helm/litellm-helm/templates/migrations-job.yaml
|
| Filename | Overview |
|---|---|
| helm/litellm-helm/templates/_helpers.tpl | Adds dedicated migration-pod labels, but the suffixed name can violate Kubernetes label-length limits. |
| helm/litellm-helm/templates/migrations-job.yaml | Merges migration labels with global pod labels, but override precedence can restore the Service-selector collision. |
| helm/litellm-helm/tests/migrations-job_tests.yaml | Adds direct coverage for default migration labels and global label overrides, but does not cover long names or selector-restoring overrides. |
| enterprise/litellm_enterprise/proxy/hooks/managed_files.py | Forwards deployment credentials for managed deletion and returns the unified managed file identifier. |
| litellm/llms/bedrock/files/transformation.py | Implements bucket-restricted, SigV4-signed S3 file deletion through the shared files handler. |
| litellm/llms/azure_ai/cost_calculator.py | Separates routed-model token pricing from the Azure Model Router fee and avoids duplicate charges. |
| litellm/proxy/_experimental/mcp_server/server.py | Adds terminal logging for MCP proxy calls, adjusts manager shutdown order, and resolves namespaced server metadata. |
| litellm/proxy/spend_tracking/key_metadata_recovery.py | Adds bounded, cached SpendLogs fallback queries for recovering missing key metadata. |
| litellm/proxy/common_utils/callback_utils.py | Records structured guardrail scan metadata and emits it through a bounded response header. |
Reviews (1): Last reviewed commit: "fix(helm): stop migrations Job pods matc..." | Re-trigger Greptile
| */}} | ||
| {{- define "litellm.migrationsLabels" -}} | ||
| helm.sh/chart: {{ include "litellm.chart" . }} | ||
| app.kubernetes.io/name: {{ include "litellm.name" . }}-migrations |
There was a problem hiding this comment.
A nameOverride can already be 63 characters because litellm.name truncates it to that limit. Appending -migrations afterward can make app.kubernetes.io/name 74 characters long. Kubernetes rejects label values longer than 63 characters, so a release using a long supported override cannot create the migration pod and its installation or upgrade fails.
| app.kubernetes.io/name: {{ include "litellm.name" . }}-migrations | |
| app.kubernetes.io/name: {{ printf "%s-migrations" (include "litellm.name" .) | trunc 63 | trimSuffix "-" }} |
Knowledge Base Used: Bound Helm migration Jobs so blocked releases fail visibly
There was a problem hiding this comment.
Good catch, fixed. app.kubernetes.io/name is now built as printf "%s-migrations" (include "litellm.name" .) | trunc 63 | trimSuffix "-". Added a regression test with a 60-character nameOverride, which produced a 71-character label before the fix.
| {{- with .Values.podLabels }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| {{- toYaml (merge (deepCopy (default dict .Values.podLabels)) $chartLabels) | nindent 8 }} |
There was a problem hiding this comment.
Overrides Restore Service Match
The merge lets the global podLabels map override app.kubernetes.io/name. An existing setting such as podLabels.app.kubernetes.io/name: litellm therefore restores the Service selector on migration pods. Because the same global map is used for deployment pods, this is a realistic configuration that defeats the fix and puts migration pods back into the Service and PodDisruptionBudget selections.
Knowledge Base Used: Bound Helm migration Jobs so blocked releases fail visibly
There was a problem hiding this comment.
Agreed, that defeated the fix. podLabels still wins for every other key, but app.kubernetes.io/name is now forced back from the chart labels after the merge, since that key is what the Service, metrics Service, ServiceMonitor and PDB select on. Added a regression test that sets podLabels."app.kubernetes.io/name": litellm and asserts the pods stay out of the selector.
…ow read fails Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…iption feat(model_hub): surface model_info.description in Model Hub
…ip_save_on_failed_read fix(health): skip background health check DB writes when the latest-row read fails
feat(dd_span_tagger): emit litellm.user_email span tag for JWT-authenticated requests
…ctor The migrations Job's pod template labels came from `litellm.labels`, which embeds `litellm.selectorLabels` (`app.kubernetes.io/name` + `app.kubernetes.io/instance`). That is the exact selector of the litellm Service, the metrics Service, the ServiceMonitor and the PodDisruptionBudget, so every migrations pod is selected by all four while its `prisma-migrations` container exposes no ports. The Service resolves a named `targetPort: http` that the pod cannot satisfy, so each PreSync/pre-upgrade run briefly perturbs the Service's EndpointSlices. On NGINX Ingress Controller 5.3.4 (OSS) this was enough to make the controller compute zero endpoints for the upstream and keep serving its no-endpoints placeholder (`upstream_addr=127.0.0.1:8181`, HTTP 502) long after the Job pod had exited and the new proxy pods were healthy — until the controller itself was restarted. The same overlap makes `pdb.minAvailable` percentages count a pod that never serves traffic, which is why the chart's PDB can only take integers. Give those pods their own labels: `app.kubernetes.io/name: <name>-migrations` plus `app.kubernetes.io/component: migrations`, mirroring what the newer `helm/litellm` chart already does for its migrations Job. Overriding `name` is what breaks the match — adding only a component label would not, since a Service selector matches on a subset. The proxy Deployment's own selector cannot be narrowed instead, because `spec.selector` is immutable and any change there would break `helm upgrade` on existing releases. `podLabels` are now merged rather than appended, so an override replaces a chart label instead of rendering the key twice. The Job object's own metadata labels are left untouched.
…vice selector Truncate app.kubernetes.io/name to 63 characters, since "litellm.name" can already return 63 and appending "-migrations" produced a value Kubernetes rejects. Force the same key back after merging podLabels: that map is shared with the deployment pods, so an override of it restored the selector match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c86ccdf to
f8abefd
Compare
|
@yassin-berriai @yuneng-berri mind taking a look at this one? Quick heads-up on the red checks: this PR only touches Helm templates and their unit tests, yet |
The problem
helm/litellm-helm's migrations Job labels its pods withlitellm.labels, which embedslitellm.selectorLabels:That is the exact selector of four other objects in the same chart:
service.yamllitellm.selectorLabelsservice-metrics.yamllitellm.selectorLabelsservicemonitor.yamllitellm.selectorLabelspoddisruptionbudget.yamllitellm.selectorLabelsSo every migrations pod is selected by all four, even though its
prisma-migrationscontainer exposes no ports. ThelitellmService resolves a namedtargetPort: httpthat the pod cannot satisfy, so eachpre-upgrade/PreSyncrun perturbs the Service's EndpointSlices for the lifetime of the Job pod.How it surfaced
On a cluster fronted by NGINX Ingress Controller 5.3.4 (OSS, NGINX 1.29.5), that was enough to make the controller compute zero endpoints for the litellm upstream and serve its no-endpoints placeholder for every request:
127.0.0.1:8181is the controller's internal "service has no active endpoints" backend. It kept returning 502 long after the Job pod had exited (~12 s) and the new proxy pods were healthy — in one case for 63 minutes, across every controller pod, until the ingress controller itself was restarted. Correlation over 14 days was exact: every 502 burst followed a migrations Job, and pod replacements without a chart sync produced none.The same overlap is why
pdb.minAvailablecan only take an integer in this chart: a percentage would be computed against a pod count that includes a Job pod which never serves traffic.The fix
Give the migrations pods their own labels —
app.kubernetes.io/name: <name>-migrationsplusapp.kubernetes.io/component: migrations— mirroring what the newerhelm/litellmchart already does for its own migrations Job (litellm.commonLabels+ acomponentlabel, where every component selector carries acomponent).Two notes on why it is done this way:
app.kubernetes.io/nameis what breaks the match. Adding onlyapp.kubernetes.io/component: migrationswould not: a Service selector matches on a subset, so a pod carryingname+instance+componentstill matches a{name, instance}selector.component: proxylabel tolitellm.selectorLabelswould changeDeployment.spec.selector, which is immutable —helm upgradeon any existing release would fail outright, and the Services would briefly select nothing mid-upgrade.podLabelsare now merged rather than appended, so an override replaces a chart label instead of rendering the key twice. This is the same pattern and comment already used inhelm/litellm/templates/migrations-job.yaml.The Job object's own
metadata.labelsare deliberately left untouched, to keep the change to the one thing that causes the bug.Compatibility
The migrations pods'
app.kubernetes.io/namechanges fromlitellmtolitellm-migrations, and they gainapp.kubernetes.io/component: migrations. Anything selecting migrations pods byapp.kubernetes.io/name=litellm(a custom NetworkPolicy or log query, say) would need updating — which is precisely the overlap being removed.kubectl get jobs -l app.kubernetes.io/name=litellmis unaffected.A Job's
spec.templateis immutable, so an in-place update of an existing Job would be rejected. Both hook paths delete and recreate the Job (helm.sh/hook-delete-policy: before-hook-creation/argocd.argoproj.io/hook-delete-policy: BeforeHookCreation) plusttlSecondsAfterFinished, so they are unaffected. Running with both hooks disabled already fails to upgrade today, because the Job'schecksum/configpod annotation changes with any values change and is equally immutable.Testing
Two cases added to
helm/litellm-helm/tests/migrations-job_tests.yaml:nameislitellm-migrations,componentismigrations,instancestill the release)podLabelsstill land on the pods and can override a chart label without duplicating the keyNo new suite is declared, so the
helm_unit_test.ymldeclared-vs-executed guard still balances at 14.helm/litellmis untouched.Rendered before/after for the Job's pod labels:
template: metadata: labels: - helm.sh/chart: litellm-helm-1.1.3 - app.kubernetes.io/name: litellm - app.kubernetes.io/instance: r - app.kubernetes.io/version: "v1.85.1" - app.kubernetes.io/managed-by: Helm + app.kubernetes.io/component: migrations + app.kubernetes.io/instance: r + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/name: litellm-migrations + app.kubernetes.io/version: v1.85.1 + helm.sh/chart: litellm-helm-1.1.3