Repository navigation
client-go/codegen: cache only the group clients in use per LC - #4526
Conversation
kcp-ci-bot
added
release-note
|
/retest
unsure if related. |
There was a problem hiding this comment.
🟡 Changes recommended
Automatically created rate limiters are now shared across logical clusters, allowing a busy workspace to exhaust unrelated workspaces’ request budgets; per-cluster isolation must be restored and affected generated clients updated.
1 open issue
6 files not reviewed
staging/src/github.com/kcp-dev/client-go/apiextensions/client/clientset.go: Generated filestaging/src/github.com/kcp-dev/client-go/kubernetes/clientset.go: Generated filestaging/src/github.com/kcp-dev/code-generator/examples/pkg/kcp/clients/clientset/versioned/clientset.go: Generated filestaging/src/github.com/kcp-dev/code-generator/examples/pkg/kcpexisting/clients/clientset/versioned/clientset.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/clientset.go: Generated filetest/e2e/fixtures/wildwest/client/clientset/versioned/cluster/clientset.go: Generated file
What changed in this PR
Reduces per-workspace client memory usage by caching only API-group clients in use rather than complete clientsets, addressing #4401.
Changes:
- Returns lightweight scoped wrappers that delegate to existing per-group client caches.
- Replaces full-clientset caches with dedicated discovery-client caches.
- Updates the generator and regenerates clientsets, examples, and the Wildwest fixture.
| File | Description |
|---|---|
test/e2e/fixtures/wildwest/client/clientset/versioned/cluster/clientset.go |
Applies lazy group-client delegation to the Wildwest fixture. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/clientset.go |
Adds scoped wrappers and discovery caching for SDK clients. |
staging/src/github.com/kcp-dev/code-generator/examples/pkg/kcpexisting/clients/clientset/versioned/clientset.go |
Updates the existing-client generation example. |
staging/src/github.com/kcp-dev/code-generator/examples/pkg/kcp/clients/clientset/versioned/clientset.go |
Updates the standard generation example. |
staging/src/github.com/kcp-dev/code-generator/cmd/cluster-client-gen/generators/generator_for_clientset.go |
Generates scoped accessors, typed imports, and discovery caches. |
staging/src/github.com/kcp-dev/client-go/kubernetes/clientset.go |
Uses lazy per-group clients for Kubernetes APIs. |
staging/src/github.com/kcp-dev/client-go/apiextensions/client/clientset.go |
Uses lazy per-group clients for API extensions. |
Review effort: Balanced
|
/retest |
neolit123
changed the title
kcp-ci-bot
added
the
do-not-merge/work-in-progress
8e68bae to
cb9aed7
Compare
neolit123
changed the title
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
|
@mjudeikis-bot revliew this please |
| }) | ||
| } | ||
|
|
||
| //nolint:paralleltest |
There was a problem hiding this comment.
any reason these dont have parallel test?
There was a problem hiding this comment.
copied from constructor_test.go. can be dropped and add t.Parallel()
mjudeikis-bot
left a comment
mjudeikis-bot
left a comment
There was a problem hiding this comment.
Two Notes, no Critical or Warning findings. The per-logical-cluster rate-limit regression that Copilot raised is already addressed by ClusterRateLimiter on this head, so I did not re-raise it.
Not anchored
- Note: the generated
scopedClientsetwrapper (the core behaviour of this PR) has no test. Nothing checks thatCluster(p).RbacV1()creates only the rbac group cache entry forp, or that it returns the same client asClusterClientset.RbacV1().Cluster(p). The existing e2e tests exercise the wrapper but would pass equally with the old full-clientset cache. A small test instaging/src/github.com/kcp-dev/client-go/kubernetesor in the code-generator examples would pin the memory property the PR claims.
Checked
- Read beyond the diff: all of
apimachinery/pkg/client/constructor.go(clientCache.Cluster,Evict, the weak evictor registry),pkg/server/clientcache_evictor.gocallers ofEvictCluster, and the typed group client constructors (apiextensions/v1 NewForConfigAndClient,setConfigDefaults). - Correctness:
NewForConfigAndClientnow builds the discovery cache from&configShallowCopy. The old code usedc, so the sharedClusterRateLimiterreaches the discovery cache and the group caches. The group config copies keep the limiter pointer, andclientCache.Clusterswaps in the per-cluster limiter.New(c)adds anHTTPClientForcall that panics only on a config thatNewForConfigOrDiewould already have rejected. No problem found. - Concurrency:
ClusterRateLimiterguards its map with a mutex, and the evictor registration follows the existing weak-ref pattern. The one issue is the evict/re-create ordering (inline Note). - kcp specifics: logical-cluster scoping is preserved through the group caches, and the wildcard still panics in
Cluster(). Per-cluster limiters match the previous behaviour, because the old cache was built from the unmodifiedcand each cluster got its own limiter. Shard and cache-server paths are not touched. - API and storage: no CRD or stored-format changes. The generated clientsets in client-go, sdk, code-generator examples and the e2e fixtures all show the same pattern. The
ClusterInterfacereturn typeclient.Interfaceis unchanged. - Security: no request parsing, credentials or RBAC changes.
- Tests:
rate_limiter_test.gocovers per-cluster isolation, sharing across caches, eviction and user-limiter passthrough.TestClusterRateLimiter_EvictClusterpasses even if a laterCluster()call re-creates the entry (inline Note). The wrapper itself has no unit test (Not anchored Note). - Commands run:
go build ./...at the repo root: ok.go build ./...andgo vet ./...instaging/.../sdkandstaging/.../code-generator: clean.go buildinstaging/.../client-go: only themainpackage error from a rootpackage mainfile, which this PR does not touch.go test -race ./pkg/client/...inapimachinery: ok.go test ./cmd/...incode-generator: no test files.- I did not re-run
make codegento confirm the generated output matches the generator.
- e2e: not run. Running it needs a full kcp server plus multi-shard setup, which I did not attempt here, so the memory claim in the PR body (~90MB to ~2MB) is unverified by me.
| limiter, ok := l.limiters[clusterPath] | ||
| if !ok { | ||
| limiter = flowcontrol.NewTokenBucketRateLimiter(l.qps, l.burst) | ||
| l.limiters[clusterPath] = limiter |
There was a problem hiding this comment.
Note: Cluster() re-creates a limiter after Evict, leaking one entry per deleted workspace that is touched later
clientCache deliberately tombstones evicted paths (c.evicted) so that a late Cluster() for a deleted workspace does not re-cache state. ClusterRateLimiter has no tombstone. After EvictCluster(p) deletes limiters[p], a late in-flight clientCache.Cluster(p) (the evicted branch still builds an instance) calls limiter.Cluster(p) and inserts a new entry that is never removed. The cost is small, a token bucket per touched deleted workspace, but it is the same leak class the evictor exists to prevent, and TestClusterRateLimiter_EvictCluster does not catch it. Fix: keep an evicted set like clientCache and return an uncached limiter for evicted paths.
limiter, ok := l.limiters[clusterPath]
if !ok {
limiter = flowcontrol.NewTokenBucketRateLimiter(l.qps, l.burst)
if _, evicted := l.evicted[clusterPath]; !evicted {
l.limiters[clusterPath] = limiter
}
}
return limiter
mjudeikis
left a comment
mjudeikis
left a comment
There was a problem hiding this comment.
Overall this is close, just 1 comment from my bot AND lets add public interface
| "github.com/kcp-dev/logicalcluster/v3" | ||
| ) | ||
|
|
||
| type ClusterRateLimiter struct { |
There was a problem hiding this comment.
I think we need to add explisit interface here:
type ClusterAwareRateLimiter interface {
flowcontrol.RateLimiter
Cluster(clusterPath logicalcluster.Path) flowcontrol.RateLimiter
}
| } | ||
|
|
||
| cfg := SetCluster(rest.CopyConfig(c.cfg), clusterPath) | ||
| if limiter, ok := cfg.RateLimiter.(*ClusterRateLimiter); ok { |
There was a problem hiding this comment.
These will become cast to interface cfg.RateLimiter.(ClusterAwareRateLimiter)
cb9aed7 to
5f0eff9
Compare
|
need to rebase to main and re-run codegen
|
1eef066 to
723f619
Compare
| "github.com/kcp-dev/logicalcluster/v3" | ||
| ) | ||
|
|
||
| func TestClusterDelegatesToGroupClients(t *testing.T) { |
43a7850 to
0500b75
Compare
| defer l.lock.Unlock() | ||
| limiter, ok := l.limiters[clusterPath] | ||
| if !ok { | ||
| limiter = flowcontrol.NewTokenBucketRateLimiter(l.qps, l.burst) |
There was a problem hiding this comment.
Doesn't that change the meaning of the QPS and burst since it applies to each workspace client by itself and make it technically unbounded?
There was a problem hiding this comment.
it does. But sharing one bucket IMO is worse. Once workspace will take down all clients quota :/
There was a problem hiding this comment.
one bucket per WS makes more sense to me, but LMK if you think that this needs to change.
There was a problem hiding this comment.
WOnder how this would look like with P&F shape:
- Global bucket
- LC buckets
- Per APIGroups buckets.
Basically hierarchy of bucketing
There was a problem hiding this comment.
this current one only tracks LCs with Cluster(...), but the implementors could track multi-LC that way (so global in some sense). maybe we can add another interface that works for groups too. i will see if that's doable in some reasonable form.
There was a problem hiding this comment.
is one idea (latest force push here)
so if the user implements also
type GroupAwareRateLimiter interface {
flowcontrol.RateLimiter
Group(groupVersion schema.GroupVersion) flowcontrol.RateLimiter
}
then from Cluster() we can call Group() too if that iface is implemented. then the implementor could choose how to orchestrate it on t he global/lc/group level.
cfg := SetCluster(rest.CopyConfig(c.cfg), clusterPath)
if limiter, ok := cfg.RateLimiter.(ClusterAwareRateLimiter); ok {
cfg.RateLimiter = limiter.Cluster(clusterPath)
if groupLimiter, ok := cfg.RateLimiter.(GroupAwareRateLimiter); ok {
groupVersion, err := c.groupVersion()
if err != nil {
var result R
return result, err
}
cfg.RateLimiter = groupLimiter.Group(groupVersion)
}
}
this test shows the example
https://github.com/kcp-dev/kcp/compare/c49933fa3e838c8322cc25b48ea97d4758681261..9719335771d08cba49ca133709436317afdc17e8#diff-0d12ea246682986f9e4e269d72a8a781cab8f856a1f1bd7205280e1e0d12df2fR157
EDIT: overall maybe not the best solution but it allows the low memory + also allows for implementors to rate per group.
There was a problem hiding this comment.
one bucket per WS makes more sense to me, but LMK if you think that this needs to change.
I don't mind either way, I just wanted to mention it. Because technically that would make qps/burst unbounded.
There was a problem hiding this comment.
I think its fine to merge this as is, but we need to think bigger picture - P&F cluster aware
0500b75 to
c49933f
Compare
ClusterClientset.Cluster() now returns a lightweight wrapper that delegates to the per-group cluster clients instead of caching a full k8s clientset. WSs now only retain clients for the API groups they actually use. When no RateLimiter is supplied, the ClusterClientset now uses a ClusterRateLimiter that hands out a token bucket per LC, shared by all group clients of that LC, so one busy WS cannot exhaust the request budget of the others. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>
c49933f to
9719335
Compare
kcp-ci-bot
added
size/XXL
|
LGTM label has been added. DetailsGit tree hash: bd4b3e17655228068b1d34b436d2f19ab0f0f8a1 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mjudeikis The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
Summary
ClusterClientset.Cluster() now returns a lightweight wrapper that delegates to the per-group cluster clients instead of caching a full k8s clientset. (EDIT: i also added an interface for Group). WSs now only retain clients for the API groups they actually use.
some stats for 3000 WS, one group only (rbacv1)
before: ~90mb
after: ~2mb (98% reducation)
if all WS uses all groups (50/60 or so), the reducation is 0%.
What Type of PR Is This?
/kind feature
Related Issue(s)
Fixes #4401
NOTE: the ticket proposed two options. i tried both. this one ended up being simpler and less diff but scales poorly if all WS use many API groups. the other one was about sharing the same clientset for all WS, big diff, and IMO that introduced some potential for rate limiting problems. despite that, if it's still preferred i can go that route instead.
Release Notes