chart: allow a gateway-only resources override (MLI-8534) - #873
Open
jay-tervala wants to merge 4 commits into
Open
chart: allow a gateway-only resources override (MLI-8534)#873jay-tervala wants to merge 4 commits into
jay-tervala wants to merge 4 commits into
Conversation
The top-level `resources` block is rendered into all three model-engine deployments — gateway, cacher and endpoint-builder. Their memory profiles are not comparable: the cacher and endpoint-builder run a handful of pods each and sit near 1GiB, while the gateway runs the whole fleet and holds async-task results in memory while proxying them inline. With one shared block the only options are under-declaring the gateway or over-reserving for the other two. On ml-serving-new the gateway currently declares no memory request at all, so the scheduler treats those nodes as memory-empty while pods sit at a ~29GiB median. Adds an optional `gateway.resources`, alongside the existing gateway.* tuning keys. When unset the rendered output is unchanged, so this is a no-op for every existing consumer. No values are changed here; the sizing lands separately in the deploy values. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jay-tervala
marked this pull request as ready for review
August 25, 2026 19:51
| unrelated to the cacher and endpoint-builder that share .Values.resources. | ||
| Falls back to the shared block when gateway.resources is unset. */}} | ||
| {{- if .Values.gateway.resources }} | ||
| {{- toYaml .Values.gateway.resources | nindent 12 }} |
Collaborator
There was a problem hiding this comment.
Does this do deep-merge gateway.resources with .Values.resources? Does a memory-only override replace also cpu and ephemeral storage?
| # task results in memory while it proxies them. Sizing all three off one block means | ||
| # either under-declaring the gateway or over-reserving for the other two. | ||
| # Leave empty to inherit `resources` unchanged. | ||
| resources: {} |
Collaborator
There was a problem hiding this comment.
Prod enables the gateway VPA in Initial mode with memory controlled and capped at 8Gi. Does this conflict with 32GB and what's the behavior?
…rces overridable Review feedback on two real defects. 1. gateway.resources replaced the shared block wholesale, so a memory-only override silently dropped the shared cpu and ephemeral-storage requests. Now deep-merges over .Values.resources: keys set win, keys absent fall through. deepCopy is required because mergeOverwrite mutates its first argument and .Values.resources is also rendered into the cacher and endpoint-builder — without it, setting gateway.resources would corrupt their output. Verified: with a memory-only override the gateway renders cpu + ephemeral-storage + memory, and both other deployments are unchanged. 2. The gateway VPA hardcoded controlledResources: ["cpu", "memory"], and its minAllowed/maxAllowed come from the shared autoscaling.vertical.* values. In updateMode: Initial the VPA rewrites requests at pod creation and clamps to maxAllowed.memory, so it would silently override any memory pinned via gateway.resources. Adds gateway.vpaControlledResources, defaulting to ["cpu", "memory"] so behaviour is unchanged; set to ["cpu"] alongside a static memory request. Defaults still render byte-identical across all three deployments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lorenzo-norcini-scale
self-requested a review
August 25, 2026 21:16
lorenzo-norcini-scale
approved these changes
Aug 25, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linear: MLI-8534
Problem
.Values.resourcesis rendered into all three model-engine deployments:Their memory profiles aren't comparable. On
ml-serving-newright now:The gateway holds async-task results in memory while proxying them inline (#868's rate-limit comments describe the same payload path). The other two don't.
With one shared block the only options are under-declaring the gateway or over-reserving for the other two. Today it's the former: the gateway declares no memory request at all, so the scheduler treats those nodes as memory-empty and packs up to 3 gateway pods onto a 120.3 GiB node. On 2026-08-25 that produced 46 OOMKills across the fleet, node-level rather than container-scoped — so the node OOM killer picks its own victim, which may be an unrelated workload.
Change
Adds an optional
gateway.resources, alongside the existinggateway.*tuning keys from #867/#868. When unset, the gateway falls back to.Values.resourcesexactly as today.Backwards compatibility
Verified with
helm templateagainst the live prod values:gateway.resourcesunset — gateway, cacher and builder all render{cpu: 2, ephemeral-storage: 256Mi}. Byte-identical to current output, so this is a no-op for every existing consumer including SGP.gateway.resourcesset — gateway picks it up; cacher and builder are untouched.helm lintclean.Not in this PR
No values are changed. The sizing (a memory request that matches reality, and whether a limit ships at all) lands separately in the deploy values, and is still under discussion — see MLI-8534 for why a limit is harder than it looks while the gateway's steady state sits at ~29 GiB and the HPA is pinned at
maxReplicas: 100.One question for review
I bumped
Chart.yamlto 0.2.10, since the templates changed.Worth flagging that the published
0.2.9inpublic.ecr.aws/b2z8n5q1does not match0.2.9at currentmain— #868'ssidecarCPURequestandreadinessProbeTimeoutSecondsare in source but not in the published tarball, under the same version string. So a bump here means the next publish also ships those. That seemed right to me, but happy to drop the bump if you'd rather sequence it separately.🤖 Generated with Claude Code
Greptile Summary
The chart adds gateway-specific resource overrides while retaining shared resource defaults, and makes the gateway VPA’s controlled resources configurable.
gateway.resourcesover the shared resource block without mutating values used by the cacher or endpoint builder.0.2.10.Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR Shared["resources"] --> Merge["Deep merge"] GatewayOverride["gateway.resources"] --> Merge Merge --> Gateway["Gateway resources"] Shared --> Cacher["Cacher resources"] Shared --> Builder["Endpoint-builder resources"] VPAConfig["gateway.vpaControlledResources"] --> GatewayVPA["Gateway VPA policy"]Reviews (3): Last reviewed commit: "chore: empty commit to retrigger CI" | Re-trigger Greptile
Context used: