Share a single etcd3 client logger across all clients #111477

negz · 2022-07-27T21:47:33Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently the API server creates one etcd client per CRD. If clients aren't provided a logger they'll each create their own. These loggers can account for ~20% of API server memory consumption on a cluster with hundreds of CRDs.

Which issue(s) this PR fixes:

Fixes #111476

Special notes for your reviewer:

With this single logger and ~1,900 CRDs loaded I'm seeing the API server using from 5.5 to 6.5 GiB RSS as garbage collection sawtooths. That's a saving of at least 1GiB.

Profile no longer shows the logger:

Note that I don't really know how to make this logger actually log anything so it's hard to confirm whether this change has any performance or observability impact.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

This logger is responsible for 20% of the API server's memory usage when many CRDs are installed. See the below issue for more context. kubernetes#111476 Signed-off-by: Nic Cope <nicc@rk0n.org>

k8s-ci-robot · 2022-07-27T21:47:42Z

Hi @negz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

leilajal · 2022-07-28T16:38:36Z

/cc @apelisse @jpbetz
/triage accepted

jpbetz · 2022-07-28T22:54:11Z

sure how viable completely disabling it is; this PR is intended more to start a discussion and to demonstrate what I did in #111476 to disable the logger

re: viability: Sounds like you're already thinking in this direction-- I'm relatively certain there is information being logged that would be sorely missed by cluster administrators when diagnosing etcd issues.

Any idea what the current log level is and what volume of data was being logged in the 1k+ CRD case?

negz · 2022-07-29T02:09:35Z

Any idea what the current log level is and what volume of data was being logged in the 1k+ CRD case?

I don't see anything being logged, but I just have a toy kind cluster with the CRDs loaded and no controllers or real clients. I imagine there might be interesting logs on a real cluster.

Per #111476 (comment) this is almost certainly happening because we have one etcd client (and thus one logger) per CRD installed. I imagine we should probably share a client or pool of clients to etcd, but I'd love to get a fix for this memory usage into 1.25 and I believe the code freeze is days away.

Perhaps a reasonable quick fix for this logger issue would be to create one logger and pass it into all newly created etcd clients?

Currently the API server creates one etcd client per CRD. If clients aren't provided a logger they'll each create their own. These loggers can account for ~20% of API server memory consumption on a cluster with hundreds of CRDs. Signed-off-by: Nic Cope <nicc@rk0n.org>

negz · 2022-07-29T02:58:44Z

Just pushed a commit and edited the PR description to have clients share one logger rather than each creating their own.

staging/src/k8s.io/apiserver/pkg/storage/storagebackend/factory/etcd3.go

sttts · 2022-07-29T08:20:08Z

/ok-to-test

jpbetz · 2022-07-29T21:45:35Z

The way the initialization of the default etcd logger is recreated here to have a shared logger looks right to me.
/lgtm

negz · 2022-07-29T22:20:31Z

/test pull-kubernetes-e2e-kind-ipv6

negz · 2022-07-30T03:45:16Z

I'm happy to take a shot at that in a separate PR

Draft started in #111559

negz · 2022-08-01T18:10:37Z

I don't think I'll personally be confident enough in #111559 to get it done by the freeze tomorrow. Do folks feel like this one is ready to merge as a stop-gap?

jqmichael · 2022-08-01T20:47:07Z

Related discussion about one etcd client per API Kind.

https://kubernetes.slack.com/archives/C0EG7JC6T/p1618439560231400

lavalamp
1 year ago
Yeah that sounds like what it currently does. What it should do is make one client per database (recall that you can put different collections in different etcd databases).
@jingyih
might have a patch laying around that fixes this.

negz · 2022-08-01T20:59:33Z

What it should do is make one client per database (recall that you can put different collections in different etcd databases).

@jqmichael that sounds similar to what I've done in #111559 - I believe in this case one transport is roughly equivalent to one etcd database?

lavalamp · 2022-08-01T21:46:24Z

/lgtm
/approve

k8s-ci-robot · 2022-08-01T21:46:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, negz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [lavalamp]

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

…-upstream-release-1.22 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

…-upstream-release-1.24 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

…-upstream-release-1.23 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

Currently, the etcd client logger is not explicitly configured when a new etcd client is initialized, which means that a new one gets created for every client. Yet, this comes with a significant memory cost, which can be avoided explicitly initializing and configuring a shared logger. This change mimics a similar one implemented for the kubernetes apiserver: kubernetes/kubernetes#111477 The following reports the top 3 nodes from the heap pprof profile of an idle process after initializing 1K etcd clients through `kvstore.NewClient`. * Without the patch: Showing nodes accounting for 268.62MB, 88.75% of 302.68MB total Dropped 124 nodes (cum <= 1.51MB) Showing top 3 nodes out of 60 flat flat% sum% cum cum% 224.33MB 74.11% 74.11% 224.33MB 74.11% go.uber.org/zap/zapcore.newCounters 32.45MB 10.72% 84.83% 32.45MB 10.72% google.golang.org/grpc/internal/transport.newBufWriter 11.85MB 3.91% 88.75% 11.85MB 3.91% bufio.NewReaderSize * With the patch: Showing nodes accounting for 65.20MB, 69.19% of 94.23MB total Showing top 3 nodes out of 137 flat flat% sum% cum cum% 43.62MB 46.29% 46.29% 43.62MB 46.29% google.golang.org/grpc/internal/transport.newBufWriter 19.08MB 20.25% 66.54% 19.08MB 20.25% bufio.NewReaderSize 2.50MB 2.65% 69.19% 3MB 3.19% time.NewTimer Co-authored-by: yougjiahe <yongjiahe@tuputech.com> Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

Disable the etcd3 client logger

0e5401c

This logger is responsible for 20% of the API server's memory usage when many CRDs are installed. See the below issue for more context. kubernetes#111476 Signed-off-by: Nic Cope <nicc@rk0n.org>

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 27, 2022

negz mentioned this pull request Jul 27, 2022

etcd3 client logger uses significant memory with many CRDs #111476

Closed

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 27, 2022

k8s-ci-robot requested review from lavalamp and smarterclayton July 27, 2022 21:48

k8s-ci-robot requested review from apelisse and jpbetz July 28, 2022 16:38

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 28, 2022

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 29, 2022

negz changed the title ~~Disable the etcd3 client logger~~ Share a single etcd3 client logger across all clients Jul 29, 2022

sttts reviewed Jul 29, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/storage/storagebackend/factory/etcd3.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned jpbetz Jul 29, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022

k8s-ci-robot assigned lavalamp Aug 1, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2022

k8s-ci-robot merged commit 59cedf4 into kubernetes:master Aug 1, 2022

k8s-ci-robot added this to the v1.25 milestone Aug 1, 2022

negz mentioned this pull request Aug 2, 2022

API server creates one etcd client per CRD #111622

Open

negz deleted the biglogs branch August 2, 2022 20:40

aojea mentioned this pull request Aug 9, 2022

Regression in client-go >= v1.24.0 metrics lead to explosion in label cardinality #111750

Closed

k8s-ci-robot added a commit that referenced this pull request Aug 10, 2022

Merge pull request #111650 from negz/automated-cherry-pick-of-#111477…

d47f137

…-upstream-release-1.22 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

k8s-ci-robot added a commit that referenced this pull request Aug 10, 2022

Merge pull request #111648 from negz/automated-cherry-pick-of-#111477…

773212d

…-upstream-release-1.24 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

k8s-ci-robot added a commit that referenced this pull request Aug 10, 2022

Merge pull request #111649 from negz/automated-cherry-pick-of-#111477…

92215fa

…-upstream-release-1.23 Automated cherry pick of #111477: Share a single etcd3 client logger across all clients

WingkaiHo mentioned this pull request Nov 18, 2022

Fix: share etcd logger accross all client fix memory leak, possibly due to cluster mesh cilium/cilium#22263

Closed

negz mentioned this pull request Dec 14, 2022

Create and reuse a single etcd client #114458

Closed

giorio94 mentioned this pull request Jun 26, 2023

kvstore: share etcd client logger to reduce memory usage cilium/cilium#26485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share a single etcd3 client logger across all clients #111477

Share a single etcd3 client logger across all clients #111477

negz commented Jul 27, 2022 •

edited

Loading

k8s-ci-robot commented Jul 27, 2022

leilajal commented Jul 28, 2022

jpbetz commented Jul 28, 2022 •

edited

Loading

negz commented Jul 29, 2022

negz commented Jul 29, 2022

sttts commented Jul 29, 2022

jpbetz commented Jul 29, 2022

negz commented Jul 29, 2022

negz commented Jul 30, 2022

negz commented Aug 1, 2022

jqmichael commented Aug 1, 2022

negz commented Aug 1, 2022

lavalamp commented Aug 1, 2022

k8s-ci-robot commented Aug 1, 2022

Share a single etcd3 client logger across all clients #111477

Share a single etcd3 client logger across all clients #111477

Conversation

negz commented Jul 27, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Jul 27, 2022

leilajal commented Jul 28, 2022

jpbetz commented Jul 28, 2022 • edited Loading

negz commented Jul 29, 2022

negz commented Jul 29, 2022

sttts commented Jul 29, 2022

jpbetz commented Jul 29, 2022

negz commented Jul 29, 2022

negz commented Jul 30, 2022

negz commented Aug 1, 2022

jqmichael commented Aug 1, 2022

negz commented Aug 1, 2022

lavalamp commented Aug 1, 2022

k8s-ci-robot commented Aug 1, 2022

negz commented Jul 27, 2022 •

edited

Loading

jpbetz commented Jul 28, 2022 •

edited

Loading