Add client evictor & investigastion docs by mjudeikis · Pull Request #4138 · kcp-dev/kcp

mjudeikis · 2026-05-21T06:55:46Z

Summary

Whats in this PR:

pkg/pproflabels/labels.go — pprof label helper, wired into kubequota controllers
staging/.../apimachinery/pkg/client/constructor.go — Evict + global EvictCluster registry
pkg/server/clientcache_evictor.go + pkg/server/server.go — LogicalCluster delete → EvictCluster
Makefile — fix prometheus target so UGET_PRINT_PATH=absolute works when binary cached (needed for ./hack/run-with-prometheus.sh ./bin/kcp start --debug-socket-path=/tmp/kcp-debug.sock to work locally on mac)
docs/content/developers/investigations/memory-leak-attribution.md — full investigation guide

Impact:

Targeted bug (clientCache retention): 213 MB → 3.7 MB cum (98% recovery)
Total leak per workspace: 745 KB → 562 KB (-183 KB)
Goroutine leak class from #3350: already mostly fixed, wiring confirms attribution works

Next investigations identified (separate PRs):

200 MB cum on structToUnstructured — likely OpenAPI v3 controller's per-cluster spec cache
99 MB on json.objectInterface — JSON-decoded objects still pinned by something
pkg/index/index.go DeleteLogicalCluster incomplete (5 of 8 per-cluster maps not cleaned)

What Type of PR Is This?

/kind bug

Related Issue(s)

Fixes #4071 (one of few fixes, more will come)

Release Notes

Add client evictor to release memory on LogicalCluster deletes

Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: SAP <mangirdas.judeikis@sap.com>

kcp-ci-bot · 2026-05-21T06:55:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

mjudeikis · 2026-05-21T07:17:49Z

/test all

Copilot

Pull request overview

This PR introduces mechanisms and tooling to better attribute and reduce memory/goroutine retention across logical cluster lifecycles, and adds a new quickstart scenario for workspace-churn/performance testing.

Changes:

Add a global per-cluster client-cache eviction fan-out (EvictCluster) and wire it to LogicalCluster deletions to release cached per-workspace client state.
Introduce pkg/pproflabels and apply labels to kubequota controller/admission per-cluster initialization paths for improved pprof attribution.
Add a new quickstart “workspaces” scenario (plus flags/tests) and add investigation documentation for leak attribution.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
staging/src/github.com/kcp-dev/cli/pkg/quickstart/scenarios/workspaces.go	New quickstart scenario that creates a random tree of workspaces under an org workspace.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/scenarios/workspaces_test.go	Unit tests for workspaces scenario validation and deterministic tree generation.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/scenarios/scenarios_test.go	Adds coverage that “workspaces” is a valid scenario in the registry.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/scenarios/scenario.go	Extends `Scenario` with `Validate` and registers the new “workspaces” scenario.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/scenarios/api_provider.go	Adds scenario-specific `Validate` for workspace-name DNS-1123 constraints.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/plugin/quickstart_test.go	Updates test mocks to satisfy the new `Scenario.Validate` interface method.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/plugin/options.go	Adds workspaces scenario tuning flags and defers prefix validation to `Scenario.Validate`.
staging/src/github.com/kcp-dev/cli/pkg/quickstart/cmd/cmd.go	Updates CLI help text to describe the new “workspaces” scenario.
staging/src/github.com/kcp-dev/apimachinery/pkg/client/constructor.go	Adds `Evict` support, global evictor registry, and `EvictCluster` fan-out tied to caches created via `NewCache`.
pkg/server/server.go	Wires LogicalCluster informer delete events to client-cache eviction.
pkg/server/clientcache_evictor.go	Implements the LogicalCluster delete handler that calls `apiclient.EvictCluster`.
pkg/reconciler/kubequota/kubequota_controller.go	Wraps per-cluster startup in pprof labels to keep leaked goroutines attributable.
pkg/pproflabels/labels.go	New helper package for attaching stable pprof labels (controller, logicalcluster).
pkg/admission/kubequota/kubequota_admission.go	Wraps per-cluster quota admission delegate creation in pprof labels for attribution.
docs/content/developers/investigations/memory-leak-attribution.md	Adds a leak attribution guide using pprof labels and workspace churn.

Comments suppressed due to low confidence (1)

staging/src/github.com/kcp-dev/apimachinery/pkg/client/constructor.go:63

The package-level evictors slice strongly references every cache ever created via NewCache (because NewCache auto-calls RegisterEvictor). If consumers create many short-lived clientsets/caches (common in tests or CLIs), this global registry will grow monotonically and prevent those caches from being garbage-collected. Consider adding an UnregisterEvictor/ResetEvictorsForTest hook, or avoiding auto-registration in NewCache in favor of explicit registration by long-lived processes (like the kcp server).

var (
	evictorsMu sync.RWMutex
	evictors   []Evictor
)

// RegisterEvictor adds e to the set of caches notified by EvictCluster.
// NewCache calls this automatically.
func RegisterEvictor(e Evictor) {
	evictorsMu.Lock()
	defer evictorsMu.Unlock()
	evictors = append(evictors, e)
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mjudeikis · 2026-05-21T09:59:28Z

 // Cache is a client factory that caches previous results.
 type Cache[R any] interface {
 	ClusterOrDie(clusterPath logicalcluster.Path) R
 	Cluster(clusterPath logicalcluster.Path) (R, error)
+	// Evict drops the cached client for clusterPath, if any. Used to release
+	// per-cluster client state (REST clients, codec factories, parsed
+	// schemas) when a logical cluster is deleted. Safe to call concurrently
+	// with Cluster / ClusterOrDie. No-op if the path is not cached.
+	Evict(clusterPath logicalcluster.Path)
+}


I would like to make this breaking so downstream clients MUST provide eviction, else it just yet-another-leak

But is this something of interest for downstream? kcp-internal only has this problem because we have the shard-cluster relation and the internal view.
External clients should only see the clusters.

And since they only see the clusters they won't have this problem.

I mean, the idea was that one could make this external, but I opted out of this. So we get compile errors :)

mjudeikis · 2026-05-21T10:15:41Z

/retest

ntnn · 2026-05-21T11:00:44Z

/test pull-kcp-test-e2e-multiple-runs

ntnn · 2026-05-26T08:08:55Z

+// RegisterEvictor adds e to the set of caches notified by EvictCluster.
+// NewCache calls this automatically.
+func RegisterEvictor(e Evictor) {
+	evictorsMu.Lock()
+	defer evictorsMu.Unlock()
+	evictors = append(evictors, e)
+}


That also means that evictors are never deregistered.
If clients are repeatedly build, each with their own cache, this would lead to them never being GC'd because they are always referenced here.

ntnn · 2026-05-26T08:15:05Z

+	if c.evictGen[clusterPath] != genAtStart {
+		// An Evict raced with this build. Hand the freshly-built client to the
+		// caller so its in-flight request can complete, but do not cache it —
+		// the cluster has been signalled as gone.
+		return instance, nil
+	}


We're only skipping caching if the client is built during an eviction.
If a client is requested after the cluster has been evicted we'd still be caching the (dead) client and hand it back to the caller.

and this is fine, we might be in the deletion flow. Client will just give error and reconciler will retry

Ah the then dead client is fine because it's cache is empty?

ntnn · 2026-05-26T08:20:21Z

 		go s.CacheKcpSharedInformerFactory.Apis().V1alpha2().APIExports().Informer().Run(hookContext.Done())
 		go s.CacheKcpSharedInformerFactory.Cache().V1alpha1().CachedResources().Informer().Run(hookContext.Done())
 		go s.CacheKcpSharedInformerFactory.Cache().V1alpha1().CachedResourceEndpointSlices().Informer().Run(hookContext.Done())
+		installClientCacheEvictor(hookCtx, s.KcpSharedInformerFactory.Core().V1alpha1().LogicalClusters())


As in the other PR, I think this should be installed after the wait below to ensure the informer is synced before kicking off the reconciler.

- registry holds caches via weak.Pointer so dropped caches can be GC'd - once a path is evicted the cache never re-caches for it - install client cache evictor after informer sync - drop named returns in kubequota controller/admission Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: SAP <mangirdas.judeikis@sap.com>

ntnn · 2026-05-26T11:39:44Z

+	c.Lock()
+	defer c.Unlock()
+	delete(c.clientsByClusterPath, clusterPath)
+	c.evicted[clusterPath] = struct{}{}


Oh... I don't know if this is a good idea. Technically someone could exhaust a shard by creating and deleting workspaces until the shard is restarted.
Maybe we just eat the possibility of storing dead clients.

Or we ttlcache it :D but that'd be a follow up

100k churned workspaces ≈ ~6MB,

good luck doing this, for 6MB you will need 90h of create/delete :D I think this is fine :)

ntnn

/lgtm
/approve

under duress and protest :D

kcp-ci-bot · 2026-05-28T08:53:07Z

LGTM label has been added.

Details

Git tree hash: 23fed95a1190b39c1b6c87848dd164bc67fc1a68

xrstf · 2026-05-28T09:00:32Z

/approve

kcp-ci-bot · 2026-05-28T09:00:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ntnn, xrstf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ntnn,xrstf]
~~pkg/admission/OWNERS~~ [xrstf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ntnn · 2026-05-28T09:38:22Z

/test pull-kcp-test-e2e

seems like ci infra failure?

xrstf · 2026-05-28T10:43:54Z

/retest

* wire in labels to pprof * add workspace quickstart into cli for ease of testing * Add client evictor to recover memory Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: SAP <mangirdas.judeikis@sap.com> * move to ctx logger * update doc to use new quickstart * address reviews * add generation counter * address review feedback - registry holds caches via weak.Pointer so dropped caches can be GC'd - once a path is evicted the cache never re-caches for it - install client cache evictor after informer sync - drop named returns in kubequota controller/admission Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: SAP <mangirdas.judeikis@sap.com> --------- Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>

mjudeikis added 3 commits May 21, 2026 08:33

wire in labels to pprof

6e2ceda

add workspace quickstart into cli for ease of testing

a860dfd

Add client evictor to recover memory

e2f3a23

Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: SAP <mangirdas.judeikis@sap.com>

kcp-ci-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels May 21, 2026

kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 21, 2026

move to ctx logger

a4511da

mjudeikis marked this pull request as ready for review May 21, 2026 08:40

kcp-ci-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2026

update doc to use new quickstart

c7925eb

mjudeikis requested a review from Copilot May 21, 2026 09:04

Copilot started reviewing on behalf of mjudeikis May 21, 2026 09:05

View session

Copilot AI reviewed May 21, 2026

View reviewed changes

address reviews

2856649

ntnn reviewed May 21, 2026

View reviewed changes

Comment thread staging/src/github.com/kcp-dev/apimachinery/pkg/client/constructor.go Resolved

add generation counter

60fd063

mjudeikis added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 25, 2026

ntnn reviewed May 26, 2026

View reviewed changes

Comment thread pkg/reconciler/kubequota/kubequota_controller.go Resolved

ntnn reviewed May 26, 2026

View reviewed changes

Comment thread pkg/admission/kubequota/kubequota_admission.go Resolved

ntnn reviewed May 26, 2026

View reviewed changes

ntnn mentioned this pull request May 28, 2026

evictor wiring server->clients->cache #4164

Closed

ntnn approved these changes May 28, 2026

View reviewed changes

kcp-ci-bot assigned ntnn May 28, 2026

kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label May 28, 2026

kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2026

kcp-ci-bot merged commit 7573468 into kcp-dev:main May 28, 2026
14 checks passed

Conversation

mjudeikis commented May 21, 2026

Summary

What Type of PR Is This?

Related Issue(s)

Release Notes

Uh oh!

kcp-ci-bot commented May 21, 2026

Uh oh!

mjudeikis commented May 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mjudeikis commented May 21, 2026

Uh oh!

ntnn commented May 21, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntnn left a comment

Choose a reason for hiding this comment

Uh oh!

kcp-ci-bot commented May 28, 2026

Uh oh!

xrstf commented May 28, 2026

Uh oh!

kcp-ci-bot commented May 28, 2026

Uh oh!

ntnn commented May 28, 2026

Uh oh!

xrstf commented May 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants