Skip to content

fix(kubernetes): cache unstructured resources and disable deep copy#7822

Merged
arkodg merged 5 commits intoenvoyproxy:mainfrom
aponysus:perf-7626-unstructured-cache-deepcopy
Jan 24, 2026
Merged

fix(kubernetes): cache unstructured resources and disable deep copy#7822
arkodg merged 5 commits intoenvoyproxy:mainfrom
aponysus:perf-7626-unstructured-cache-deepcopy

Conversation

@aponysus
Copy link
Copy Markdown
Contributor

@aponysus aponysus commented Dec 26, 2025

What type of PR is this?
fix(kubernetes)

What this PR does / why we need it:
This PR improves performance when listing unstructured extension resources by:

  • enabling cache-backed reads for unstructured resources via client.CacheOptions{Unstructured: true} on the manager client, and
  • using client.UnsafeDisableDeepCopy on the unstructured List() call to avoid per-object deep copies.

Safety: the listed objects are treated as read-only in this path, so UnsafeDisableDeepCopy is safe here.
Scope: I intentionally did not apply client.UnsafeDisableDeepCopy to the unstructured List in internal/provider/kubernetes/controller.go because processExtensionServerPolicies mutates the listed objects (e.g., deletes status and later writes policy.Object["status"]). That path would require per-item copying to remain safe.

Which issue(s) this PR fixes:
Fixes #7626

Release Notes: No

@aponysus aponysus requested a review from a team as a code owner December 26, 2025 04:47
@codecov
Copy link
Copy Markdown

codecov bot commented Dec 26, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.53%. Comparing base (626a08a) to head (82047e3).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
internal/provider/kubernetes/filters.go 0.00% 0 Missing and 2 partials ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #7822   +/-   ##
=======================================
  Coverage   73.52%   73.53%           
=======================================
  Files         237      237           
  Lines       35653    35653           
=======================================
+ Hits        26214    26216    +2     
+ Misses       7576     7574    -2     
  Partials     1863     1863

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jukie
Copy link
Copy Markdown
Contributor

jukie commented Dec 26, 2025

Did you take a pprof to confirm and if so could you include the results please?

@aponysus
Copy link
Copy Markdown
Contributor Author

aponysus commented Dec 26, 2025

Did you take a pprof to confirm and if so could you include the results please?

I tried to validate this with the project’s make benchmark workflow (kind + nighthawk) per the docs.

I was able to get the benchmark harness to create the proxy LB service (envoy-benchmark-test-benchmark-0520098c, VIP 172.25.0.201:8081), but the run consistently times out on the HTTP readiness checks. Debugging indicates the service endpoint becomes stale / has no backing pod IP (e.g. Endpoints shows 10.244.0.10:8081 but there is no pod with that IP), so requests never get a response and the benchmark aborts/cleans up.

Given that, I don’t currently have a reliable way to produce meaningful before/after benchmark numbers in my environment.

If you have a preferred/reliable benchmark setup (or can run make benchmark on your side), I’m happy to re-run with any specific configuration you recommend. Alternatively, since the helm install for benchmarks enables pprof (config.envoyGateway.admin.enablePprof=true), I can capture CPU/heap profiles from the control-plane during a workload you recommend and post those results instead.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Dec 26, 2025 •
edited
Loading

hey @aponysus, looks like there's also a WIP for this issue #7704, since it was raised earlier, prefer prioritizing that PR in.
It did have a stale help-wanted tag, which wasnt removed and caused confusion, sorry about that

@aponysus
Copy link
Copy Markdown
Contributor Author

aponysus commented Dec 27, 2025

hey @aponysus, looks like there's also a WIP for this issue #7704, since it was raised earlier, prefer prioritizing that PR in. It did have a stale help-wanted tag, which wasnt removed and caused confusion, sorry about that

Thanks, I see #7704, but it doesn’t include the filters.go change from #7626 (using client.UnsafeDisableDeepCopy on the unstructured List() call).

My PR includes both pieces from the issue:

  1. enable cache-backed reads for unstructured resources (without clobbering existing cache options), and
  2. apply client.UnsafeDisableDeepCopy only on the specific List() call in filters.go (read-only path).

Happy to close this PR if you’d prefer to consolidate into #7704. I can cherry-pick my commits onto #7704 or open a small follow-up PR that only adds the filters.go change. What do you prefer?

@zirain zirain force-pushed the perf-7626-unstructured-cache-deepcopy branch from fdd47ea to ebb0b64 Compare January 14, 2026 02:29
@netlify
Copy link
Copy Markdown

netlify bot commented Jan 14, 2026 •
edited
Loading

✅ Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit 82047e3
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/6972accc712ee10008f31011
😎 Deploy Preview https://deploy-preview-7822--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented Jan 14, 2026

hey @aponysus wanna rebase on top of #7704 ?

@Inode1 Inode1 mentioned this pull request Jan 14, 2026
Closed
@Inode1
Copy link
Copy Markdown
Contributor

Inode1 commented Jan 22, 2026

@aponysus are you working on this?

@aponysus
Copy link
Copy Markdown
Contributor Author

aponysus commented Jan 22, 2026

@arkodg and @Inode1 Sorry, I missed this. Yes, I'll rebase on #7704

@aponysus
perf(kubernetes): cache unstructured resources and disable deep copy
4e19a51 
Signed-off-by: aponysus <me@joshuasorrell.com>
@aponysus
fix(kubernetes): format imports
91e6a16 
Signed-off-by: aponysus <me@joshuasorrell.com>
@aponysus
fix(kubernetes): preserve client cache options when enabling unstruct…
593b4dc 
…ured cache

Signed-off-by: aponysus <me@joshuasorrell.com>
@aponysus aponysus force-pushed the perf-7626-unstructured-cache-deepcopy branch from ebb0b64 to 593b4dc Compare January 22, 2026 14:22
@aponysus
Copy link
Copy Markdown
Contributor Author

aponysus commented Jan 22, 2026

Rebasing onto upstream/main (includes merged #7704) as requested. No functional change from previous description above.

@aponysus
fix(kubernetes): remove redundant setting of client.Options.Cache - a…
6843d70 
…lready set in kubernetes.go line 89 from PR 7704.

Signed-off-by: aponysus <me@joshuasorrell.com>
@aponysus
Copy link
Copy Markdown
Contributor Author

aponysus commented Jan 22, 2026

Since #7704 added client.Options.Cache.Unstructured=true, I removed that from my change. Now this PR is isolated to applying client.UnsafeDisableDeepCopy only on the specific List() call in filters.go, which was specified in #7626

jukie
jukie approved these changes Jan 22, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jukie jukie left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@zirain
Copy link
Copy Markdown
Member

zirain commented Jan 23, 2026

/retest

@arkodg arkodg merged commit 283f745 into envoyproxy:main Jan 24, 2026
53 of 59 checks passed
SadmiB pushed a commit to SadmiB/gateway that referenced this pull request Jan 30, 2026
@aponysus @SadmiB
fix(kubernetes): disable deep copy for unstructured resources (envoyp…
51ac1ff 
…roxy#7822)

Signed-off-by: aponysus <me@joshuasorrell.com>
Signed-off-by: Sadmi Bouhafs <sadmibouhafs@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arkodg arkodg arkodg approved these changes

@jukie jukie jukie approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

perf: use UnsafeDisableDeepCopy and cache for list Unstructured Resources (Extension Server)

5 participants

@aponysus @jukie @arkodg @Inode1 @zirain