fix(helm): scale the classic chart's HPA out at the documented 60 percent CPU - #35975
fix(helm): scale the classic chart's HPA out at the documented 60 percent CPU#35975yassin-berriai wants to merge 1 commit into
Conversation
…cent CPU The litellm-helm chart shipped targetCPUUtilizationPercentage: 80, which is unexamined helm create scaffold rather than a chosen number. It arrived packaged with the stock minReplicas: 1, maxReplicas: 100, a commented-out targetMemoryUtilizationPercentage: 80, and the boilerplate "such as Minikube" comment, the same provenance as the 128Mi resource example this file just corrected. 60 is the documented recommendation. The mechanism behind it is scale-up lag: the chart's own startupProbe is failureThreshold: 30 times periodSeconds: 10, so a replica can take up to 300 seconds to become ready, and a pod added at 80 percent utilization arrives minutes after saturation. The memory target stays commented out on purpose. The prisma query engine's resident memory is a high-water mark that ratchets to the pod's worst-ever write and is never returned, so a memory-target HPA reads the largest write a pod ever did rather than what it is doing now, and replicas ratchet up without scaling back in. hpa_tests.yaml carried its second suite after a YAML document separator, and helm-unittest loads only the first document per file, so that suite never ran; an assertion planted in it still passed. Fold it into the one live suite and add coverage pinning the rendered CPU target, the absence of a memory metric by default, and that overrides still take effect. Bump the chart to 1.1.2, since rendered output changes for anyone running with autoscaling enabled.
|
|
Greptile SummaryThis PR lowers the classic Helm chart's default HPA CPU target from 80% to 60%, increments the chart version, and consolidates the HPA tests so previously skipped and newly added cases run.
Confidence Score: 4/5The PR appears safe to merge after the non-blocking repository-guidance issue concerning newly added comments is addressed. The HPA default, tests, and chart version are internally consistent; the only accepted concern is that the values file introduces comments disallowed by the repository's contribution guidance. Files Needing Attention: helm/litellm-helm/values.yaml
|
| Filename | Overview |
|---|---|
| helm/litellm-helm/values.yaml | Lowers the default HPA CPU threshold correctly, but adds comments prohibited by repository guidance. |
| helm/litellm-helm/tests/hpa_tests.yaml | Consolidates the suites into one discoverable YAML document and adds focused HPA rendering assertions without losing existing coverage. |
| helm/litellm-helm/Chart.yaml | Applies the expected patch-level chart version increment for the rendered behavior change. |
Reviews (1): Last reviewed commit: "fix(helm): scale the classic chart's HPA..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # 60 is the documented recommendation. See "Recommended Machine Specifications" | ||
| # in https://docs.litellm.ai/docs/proxy/prod. A new replica clears the startupProbe | ||
| # above only after up to failureThreshold x periodSeconds = 300 seconds, so a target | ||
| # high enough to trip near saturation adds capacity minutes after it was needed. | ||
| targetCPUUtilizationPercentage: 60 | ||
| # Deliberately left unset rather than given a value. The prisma query engine's | ||
| # resident memory is a high-water mark that ratchets to the pod's worst-ever write | ||
| # and is never returned, so a memory target reads the largest write a pod ever did | ||
| # rather than what it is doing now, and replicas ratchet up without scaling back in. | ||
| # Memory is a floor to provision under 'resources', not a signal to scale on. | ||
| # targetMemoryUtilizationPercentage: 80 |
There was a problem hiding this comment.
The new CPU and memory explanations add maintenance comments despite the repository guidance prohibiting new comments unless explicitly requested, so this block should retain only the configuration values.
| # 60 is the documented recommendation. See "Recommended Machine Specifications" | |
| # in https://docs.litellm.ai/docs/proxy/prod. A new replica clears the startupProbe | |
| # above only after up to failureThreshold x periodSeconds = 300 seconds, so a target | |
| # high enough to trip near saturation adds capacity minutes after it was needed. | |
| targetCPUUtilizationPercentage: 60 | |
| # Deliberately left unset rather than given a value. The prisma query engine's | |
| # resident memory is a high-water mark that ratchets to the pod's worst-ever write | |
| # and is never returned, so a memory target reads the largest write a pod ever did | |
| # rather than what it is doing now, and replicas ratchet up without scaling back in. | |
| # Memory is a floor to provision under 'resources', not a signal to scale on. | |
| # targetMemoryUtilizationPercentage: 80 | |
| targetCPUUtilizationPercentage: 60 | |
| # targetMemoryUtilizationPercentage: 80 |
Context Used: CLAUDE.md (source)
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!
|
@greptileai values.yaml comments are the chart's documented interface, printed verbatim by |
|
Agreed — |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
TLDR
Problem this solves:
helm createscaffoldhpa_tests.yaml's second suite sat after---and never ranHow it solves it:
targetCPUUtilizationPercentage: 60and say why in a commentRelevant issues
Linear ticket
Resolves LIT-5181
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This changes a chart default, so the proof is
helm templateandhelm lintrather than a live proxy; there is no request path to exercise. Base isd3d30353aa, branch isd3b2897fe0. The base copy comes fromgit archive d3d30353aa helm/litellm-helm | tar -x -C /tmp/lit5181-base, so the working tree is never touched.Both renders pin the two generated secrets, because the chart mints a random postgres password and master key on every invocation and the churn would otherwise swamp the diff:
A. Default values, which is what protects everyone who never turned autoscaling on
Of 634 rendered lines, eight differ. Seven are the
helm.sh/chartlabel following the version bump. The eighth istemplates/migrations-job.yaml:19, which annotates with{{ toYaml .Values | sha256sum }}, a hash of the entire values tree, so it moves on any values edit whether or not the Job cares about the key that changed. That Job is already apre-install/pre-upgrade(or ArgoCD PreSync) hook that re-runs on every upgrade regardless, so the annotation drives no behavior. NoHorizontalPodAutoscalerrenders on either side. I am not claiming the default render is identical, because it is not; the claim is that the only delta outside the HPA is a chart label and a values-tree checksum on a hook that re-runs anyway.B.
--set autoscaling.enabled=true, which is the change itselfplus the same seven chart-label lines and the same migrations-job checksum line as above.
C. Mutation check on the new assertion. Reverting only
values.yamlto 80 and leaving the test file alone:D. The exact commands CI runs, from
.github/workflows/helm_unit_test.yml:42-43, atd3b2897fe0:96 is 90 before this PR, plus 5 new tests, plus 1 that was dead and now runs.
E.
helm lint, clean on the base copy, on the branch, and on the branch with autoscaling enabled; all three report1 chart(s) linted, 0 chart(s) failedwith only the pre-existing[INFO] Chart.yaml: icon is recommended.Type
🐛 Bug Fix
Changes
helm/litellm-helm/values.yaml:202shippedtargetCPUUtilizationPercentage: 80. That is unexaminedhelm createscaffold rather than a number anyone picked. The tell is the company it keeps: the stockminReplicas: 1andmaxReplicas: 100, a commented-outtargetMemoryUtilizationPercentage: 80, and the boilerplate "such as Minikube" phrasing, which is the same provenance as the 128Mi resource example #35830 just corrected a few lines above it.60 is the documented recommendation, from the "Recommended Machine Specifications" section of https://docs.litellm.ai/docs/proxy/prod. The mechanism behind it is scale-up lag. This chart's own startupProbe is
failureThreshold: 30timesperiodSeconds: 10(values.yaml:108-114, wired through attemplates/deployment.yaml:203-211), so a freshly scheduled replica can take up to 300 seconds to report ready. A target high enough to trip only near saturation therefore adds capacity minutes after the moment it was needed, which is roughly the worst time for it to arrive.Please read this as a real change rather than a tidy-up.
autoscaling.enableddefaults to false, so anyone who never enabled autoscaling sees no HPA at all and is unaffected. But anyone who setenabled: trueand took the default gets a different HPA on their next upgrade and starts scaling out at 60 instead of 80. The blast radius is replica count, and therefore spend, rather than availability; more pods earlier is the failure mode, not fewer. That is milder than a resources change but it is not nothing.The memory target stays commented out on purpose rather than being given a value, and the comment now says why. The prisma query engine's resident memory behaves as a high-water mark: it ratchets up to the largest write that pod has ever served and is never returned. A memory-target HPA therefore reads the biggest thing a pod once did rather than what it is doing now, and replicas ratchet up without ever scaling back in. Memory is a floor to provision under
resources, not a signal to scale on.The chart goes to 1.1.2. Recent chart-touching commits are split on this, and #35830 correctly did not bump because its rendered output was byte-identical; this one does change rendered output whenever autoscaling is enabled, which is the case chart versions exist to signal.
On the tests:
hpa_tests.yamlheld two suites separated by a---document separator, and helm-unittest reads only the first YAML document per file, so the second suite had never run. Plantingequal: { path: spec.minReplicas, value: 4242 }in it, an assertion that cannot pass, still produced1 passed, 1 total. Appending a new suite the way the file's shape invites would have been silently dead too. Both suites are now one live suite, joined by five new tests: the rendered CPU target is 60 by default,spec.metricshas exactly one entry so nothing scales on memory, an explicit override still wins, a memory metric appears only whentargetMemoryUtilizationPercentageis set, and no HPA document renders at all when autoscaling is off. No existing test asserted the old value, so nothing needed updating for the value change itself.One quoting note for whoever writes the next helm-unittest assertion: a
pathcontaining an index such asspec.metrics[0]has to be quoted. Unquoted inside a flow mapping, YAML reads the[as the start of a sequence and the whole file fails to parse.helm/litellm-helm/tests/pdb_tests.yamlhas the same dead-suite defect, three suites of which two never run. Left alone here to keep this PR to one problem; it is tracked separately.Final Attestation