fix(gateway2): stop deep-copying RouteOptions per route rule (control-plane OOM) - #11264
Closed
wkrause13 wants to merge 16 commits into
Closed
fix(gateway2): stop deep-copying RouteOptions per route rule (control-plane OOM)#11264wkrause13 wants to merge 16 commits into
wkrause13 wants to merge 16 commits into
Conversation
…g per route GetRouteOptionForRouteRule deep-cloned the first RouteOption attachment for every translated route via ShallowMergeRouteOptions' dst==nil branch. When many routes reference the same RouteOption (esp. ones carrying large transformation templates), each route received its own deep copy of identical config, which dominated translation heap (~31% / 4.3GB of a user's 14GB heap). Add ShallowCopyRouteOptions, which copies only the top-level RouteOptions fields and shares the immutable sub-messages by pointer. This is consistent with the existing dst!=nil merge branch, which already shares src's fields. Each route still gets a distinct top-level message, so route plugins that reassign top-level fields (urlrewrite, headermodifier, mirror) remain isolated; they must not mutate the shared sub-messages in place. Memory now scales with the number of unique RouteOptions rather than the number of routes.
…ute rule The shallow-copy merge alone is not enough to bound translation heap: GetRouteOptionForRouteRule reads RouteOptions through a cached controller-runtime client, which deep-copies every matching object on every Get/List. Each route rule therefore still retained its own private copy of the (potentially multi-MB) options tree via the shared sub-message pointers, so memory kept scaling with the number of translated routes rather than the number of unique RouteOptions. Pass client.UnsafeDisableDeepCopy on the targetRef List, and fetch extensionRef attachments through the same client with the same option (previously they went through the generic GatewayQueries ref resolver, which both deep-copied and quietly defaulted an empty route namespace to "default"). All routes that attach the same RouteOption now share one copy: the object in the informer cache. This makes the read-only contract load-bearing: nothing in translation may mutate nested messages reachable from merged route options. That contract already effectively existed (the merge has always pointer-shared the 2nd and later attachment sources into the merged options), and an audit of every consumer of route options in OSS found only top-level field writes. It is now documented on RouteOptionQueries and enforced by tests: - query contract tests pin that lookups pass UnsafeDisableDeepCopy, that merged options share sub-messages with the client-returned objects, and that the merge never writes into them - a routeoptions plugin test runs the full attachment+merge+override flow and verifies the client-returned RouteOptions are never mutated - benchmarks quantify the per-route seeding cost: 203,676 B and 2,015 allocs per route with the old deep clone vs 384 B and 1 alloc with the shallow copy, for a 1,500-message transformation template Companion change required in solo-projects before this is consumed by enterprise: the portal plugin mutates transformation templates reached through merged route options in place and must copy-on-write first. Issue: solo-io/solo-projects#8802 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Issues linked to changelog: |
|
Visit the preview URL for this PR (updated for commit 2e14c25): https://gloo-edge--pr11264-will-routeoptions-oo-kdy5vnw3.web.app (expires Tue, 23 Jun 2026 01:58:32 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 77c2b86e287749579b7ff9cadb81e099042ef677 |
TranslateTransformation resolved the template -> staged -> settings escapeCharacters inheritance by assigning the result into the input TransformationTemplate. The input is nested in route/vhost options whose sub-messages can be shared across every route referencing the same RouteOption (solo-io/solo-projects#8802), so the write-back could leak the resolved value across routes and mask later changes to the Settings-level default. Resolve into a local and pass it down instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er filters applyRequestFilter/applyResponseFilter wrote the filter's headers into the route options' existing HeaderManipulation in place. That message can be shared with every route referencing the same RouteOption (solo-io/solo-projects#8802) when parent filters are re-applied to delegated child routes, whose options are already populated from RouteOptions by the routeoptions plugin. Copy the message before writing the filter's fields into it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Instead of sharing the merged options' sub-messages directly with the objects in the informer cache, the RouteOption query now deep-copies each unique RouteOption exactly once per query lifetime (one translation pass) and feeds the merge from that interned copy. All routes referencing the same RouteOption still share one copy — memory still scales with unique RouteOptions, not routes — but the informer cache becomes structurally unreachable from translation output: an in-place mutation by a translation plugin can at worst contaminate one pass's output (self-healing on the next sync) instead of persistently corrupting the cache for every consumer. Intern entries are keyed by name and replaced when the cached object's resourceVersion moves, so a lookup can never be served stale options and the map stays bounded even if a query outlives its pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ERNING The per-pass RouteOption interning memory optimization is now opt-in and off by default. Set GG_ROUTE_OPTION_INTERNING truthy on the control plane to enable it; otherwise translation behaves exactly as before, deep-copying each attached RouteOption per route rule (no UnsafeDisableDeepCopy lookups, ShallowMergeRouteOptions seed). Follows the gloo env-flag convention: a constant in projects/gloo/constants and a package-level var read once at init (the UseDetailedUnmarshalling precedent), captured into the query at NewQuery so a translation pass cannot flip mid-run. Only the query memory model is gated. The transformation escapeCharacters write-back fix and the headermodifier clone-before-mutate fix stay unconditional: they are correctness fixes that are safe whether or not interning is enabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
wkrause13
force-pushed
the
will/routeoptions-oom-8802
branch
from
June 14, 2026 14:22
9d86d24 to
3e6eb5e
Compare
added 10 commits
June 16, 2026 01:41
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.
Description
Fixes the dominant memory consumer behind the gloo control-plane OOMs: the Kubernetes Gateway API translation deep-copied the attached RouteOption's entire options tree — including multi-MB transformation templates — once per route rule, per listener, per translation cycle. With many routes referencing the same RouteOption, both allocation churn and retained heap scaled with
routes × options-sizeinstead ofunique RouteOptions × options-size. User heap profiles show this exact chain (translateGatewayHTTPRouteRule → GetRouteOptionForRouteRule → ShallowMergeRouteOptions → RouteOptions.Clone → TransformationTemplate.Clone) holding 57–58% of a 14.9 GB live heap.Design: one interned copy per unique RouteOption per translation pass (opt-in)
Feature flag: the optimization is gated behind the
GG_ROUTE_OPTION_INTERNINGenv var on the control plane and is off by default. With it unset, translation behaves exactly as before this PR — each attached RouteOption is deep-copied per route rule (noUnsafeDisableDeepCopy,ShallowMergeRouteOptionsseed). The two plugin correctness fixes below (commits 3–4) are not gated; they ship unconditionally because they are safe whether or not interning is enabled. Everything else in this section describes behavior when the flag is enabled.When enabled, the RouteOption query looks objects up with
client.UnsafeDisableDeepCopy(no per-rule copy out of the informer cache), deep-copies each unique RouteOption exactly once per query lifetime into an intern map, and feeds the merge from that interned copy. Every route referencing the same RouteOption shares the one interned copy's sub-messages, so memory scales with unique RouteOptions, not routes — and the informer cache is structurally unreachable from translation output: a translation plugin that mutates nested options in place can at worst contaminate one pass's output (self-healing on the next sync), never the cache that every other consumer reads.The merged options remain a distinct top-level message per route, so plugins keep reassigning top-level fields safely. Intern entries are keyed by name and replaced when the cached object's
resourceVersionmoves, so a lookup can never be served stale options, and the map stays bounded even if a query were ever to outlive its pass. The query's per-pass lifetime is documented at the sites that decide it (K8sGatewayExtensions.CreatePluginRegistry,buildProxy).An earlier revision of this PR shared the cache objects' sub-messages directly into the merged options (no interned copy). That kept the same asymptotics but made the informer cache reachable from — and corruptible by — translation output, turning "no plugin may ever mutate nested options" into a whole-program invariant. The audit below showed that invariant was already violated in-tree, so the design was revised to interning; it costs one clone per unique RouteOption per pass (~74 at production scale, vs ~2,000+ per-route clones before the fix).
Commits
fix(gateway2): share RouteOptions sub-messages instead of deep-cloning per route(cherry-picked from @puertomontt's Fix/routeoptions-shallow-copy-oom #11247, authorship preserved): addsShallowCopyRouteOptionsand uses it to seed the merged options.fix(gateway2): stop deep-copying RouteOptions out of the cache per route rule:client.UnsafeDisableDeepCopyon the targetRef List and the extensionRef Get; extensionRef lookup becomes a direct typedGet(same-namespace local ref; NotFound semantics preserved); drops the now-impossibleErrTypesNotEqualcase.fix: stop writing resolved escapeCharacters back into the input template: pre-existing OSS bug surfaced by the audit —TranslateTransformationresolved the template → staged → SettingsescapeCharactersinheritance by assigning the result into the input template. With shared options this write-back would have leaked resolved values across routes and masked later Settings changes (and under the earlier revision, persistently written them into the informer cache). It now resolves into a local passed down totranslateTransformationTemplate.fix(gateway2): clone HeaderManipulation before applying header modifier filters: second pre-existing mutator —applyRequestFilter/applyResponseFilterwrote into the route options' existingHeaderManipulationin place, reachable with shared sub-messages when parent filters are re-applied to delegated child routes.fix(gateway2): intern one RouteOption copy per translation pass: the interning described above, plus the lifecycle documentation.feat(gateway2): gate RouteOption interning behind GG_ROUTE_OPTION_INTERNING: puts the memory model (commits 1, 2, 5) behind the env flag, off by default; commits 3–4 stay unconditional. Follows the gloo env-flag convention — constant inprojects/gloo/constants, a package-level var read once at init (theUseDetailedUnmarshallingprecedent), captured per query inNewQueryso a translation pass can't flip mid-run. Newquery_sharing_test.gospecs pin the default-off path (noUnsafeDisableDeepCopy, per-route deep copy); the interning specs now set the flag explicitly.Memory measurements
Three images from the same toolchain, reproducer from the issue (1 RouteOption with staged transformations, 2-listener Gateway, N HTTPRoutes via extensionRef) in kind, capturing
/debug/pprof/heap?gc=1(forced GC ⇒ retained heap only) per phase:main@ 86ea356)translateGatewayHTTPRouteRuleGetRouteOptionForRouteRuleShallowMergeRouteOptions(merge clone)RouteOption.DeepCopyObject(cached-client copies)With #11247 alone, the merge clone disappears but
RouteOption.DeepCopyObject(the cached client copying per rule) stays, and retained growth is unchanged — each route retains its own private copy via the shared pointers. With both, all routes referencing the same RouteOption share one copy.Micro-benchmark (
merge_benchmark_test.go), per-route cost of seeding merged options from a RouteOption carrying a 1,500-message transformation template:Note on the e2e numbers: the retained-heap delta on
mainis capped by a separate pre-existing path — solo-kit's in-memory Proxy client deep-clones the whole translated Proxy for the persistence/debug snapshot, which re-expands shared options per route. The 1.20.x user profiles show translation output retained directly through the merge frames, so the retained-heap win on the LTS line should be substantially larger than −40%. The whole-Proxy snapshot clone is a worthwhile follow-up.Mutation safety (the concern raised on the issue)
The in-place suggestion floated earlier on the issue was correctly called out as unsafe. The audit of every consumer of merged route options (this repo and solo-projects) found that "nothing downstream mutates nested options" was not a safe bet — it was already false in three places:
escapeCharacterswrite-back (OSS, fixed in commit 3 — fires for any RouteOption carrying a transformation template, i.e. exactly the workload this PR targets);HeaderManipulationwrites (OSS, fixed in commit 4 — reachable via delegation);With the flag off (default), the per-route deep copy keeps the informer cache protected exactly as on
maintoday, so none of these mutators can reach it. With the flag on, interning means a future mutator of this class contaminates one pass's output for the routes sharing that RouteOption — a visible, self-healing bug — instead of silently corrupting the informer cache for every consumer until resync. The two known OSS mutators (commits 3–4) are fixed unconditionally regardless of the flag. The remaining (much weaker) rule for plugin authors, relevant only when interning is enabled: don't mutate nested messages reachable from merged route options; reassign top-level fields instead.Enforcement, not just documentation:
UnsafeDisableDeepCopy(pinned by recording-client specs);Concurrency: the query is per-pass and route plugins run sequentially within a pass;
-raceover the gateway2 translator tree, routeoptions, proxy_syncer, and glooutils is clean.Minor deliberate behavior changes (neither observable with real-world resources):
"default"(HTTPRoutes are namespaced; two tests relied on the quirk and now set a namespace).ErrTypesNotEqualcan no longer occur, so all attachment-lookup errors now set theResolvedRefscondition.Testing steps
projects/gloo/pkg/utils,routeoptions+query,headermodifier,transformation(the 9 specs that shell out to an envoy binary for config validation don't run on the dev machine — unchanged set, covered by CI),httproute,listener, fullprojects/gateway2/...(incl. the 50 golden-file translator specs covering all delegation RouteOptions merge cases), Edge translator (projects/gateway/pkg/translator, sharesmerge.go).x-served-by/x-cache-statusinjected, inja extractor reading request headers, headerManipulation stripping headers), zero errors in gloo logs. Also verified interning freshness end-to-end: patching the shared RouteOption propagates to all referencing routes on the next sync (interned copy replaced on resourceVersion change).Notes for reviewers
controller-runtime≥ 0.17 on the target branch (forUnsafeDisableDeepCopyas aGetOption) and the portal companion on the matching enterprise LTS branch; commits 3–4 should be backported with it.Checklist:
🤖 Generated with Claude Code